Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OAuth2 support #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

edevalais-medallia
Copy link

No description provided.

Dockerfile Outdated
Comment on lines 13 to 14
ENV CONVO_INSTANCE_URL=https://<MEDALLIA_CONVERSATION_HOST>
ENV CONVO_WEBHOOK_URL=https://<MEDALLIA_CONVERSATION_HOST>/cg/mc/custom/<CHANNEL_GUID>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecate the CONVO_WEBHOOK_URL with the new API endpoints available.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need the CHANNEL_GUID. Do you want a CHANNEL_GUID and the CONVO_INSTANCE_URL (that will change to CONVO_API_GATEWAY) in two env configs?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please!

README.md Outdated
@@ -1,5 +1,4 @@
# Medallia Conversations Adapter

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave this in for formatting sync with other projects. (Style guidelines say to have a blank line after each Markdown heading.)

README.md Outdated
Comment on lines 25 to 26
* `export AUTH_TYPE_INBOUND=<It can be 'Oauth2' or 'Signature'>`
* `export SHARED_SECRET=<32_CHARACTER_STRING only for Signature>`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are fully deprecating Signature. Only show OAuth2 at this point. Also, the standard name is OAuth for capitalization correctness.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it the same for API-Token in Outbound messages?

README.md Outdated
Comment on lines 38 to 39
### Auth Configuration
For inbound conversations configuration you can setup 2 auth types:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a newline between these. Only keep the "OAuth2" type.

README.md Outdated
For inbound conversations configuration you can setup 2 auth types:
* Signature: This is used to generate signature of the body to send it to Medallia Conversations with the SHARED_SECRET key. In the converation side, under the Signed request auth type, this Secret should match. The string must be 32 characters long.
* Oauth2: It will use the Conversations OAuth server. You will need the following configuration CONVO_INSTANCE_URL, CLIENT_ID and CLIENT_SECRET.
* CONVO_INSTANCE_URL: <MEDALLIA_CONVERSATION_HOST>/oauth/token
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add https://

Comment on lines 1 to 2
// test OAuth server that supports only client_credentials grant type
// with a fixed set of client id and secret values configured in auth-settings.js
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// This implements a basic OAuth 2.0-compatible token server for use with this reference implementation.
// It only supports client_credentials grants and uses the static client_id/client_secret values that are
// configured in auth-settings.js.

Comment on lines 11 to 13
const cache = new Cache({
ttl: 3600 * 1000
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've used this Cache declaration twice now? Should we abstract to a helper function and/or at least move the default TTL value to a shared constant file?

} else {
const token = crypto.randomBytes(16).toString('hex');
const { auth } = req;
if (auth.user) cache.put(token, auth.user);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use single-line if statements. Add {} and put on multiple lines.

const { auth } = req;
if (auth.user) cache.put(token, auth.user);
console.info(`Issued new access token: ${token} for client ${auth.user || 'unknown'}`);
res.status(200).send({ access_token: token, expires_in: 3600 });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reference the DEFAULT_OAUTH_EXPIRES_SECS constant here?

}
});

// This is just to confirm the token is valid and get the client info for the token
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not clear what exactly this function does. Is this used for your internal debugging? Or is it valuable to Conversations somehow?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants