Feature/197 access token provided #273
Conversation
…tions with Authorization Code Grant Type
lib/auth.js
Outdated
| if (!authObject) { | ||
| throw new Error('authObject are required. see readme.'); | ||
| } else if (grant_type && grant_type == GRANT_TYPE_AUTHORIZATION_CODE) { |
There was a problem hiding this comment.
one thought here is that, since you are providing grant_type as a default, the additional check here is not needed since it will always have one. I would also prefer that since you only check for the error here, that this be included in the else if clause, rather than opening a new block to avoid nested if/else
There was a problem hiding this comment.
Thanks for the comment.
I have removed the null check for the grant_type.
If I change the condition without nested if like the following
else if (grant_type == GRANT_TYPE_AUTHORIZATION_CODE && !authObject.access_token)
then the validation would fail asking for the client_id to be present if grant_type=authorization_code and access_token is provided.
Otherwise I have to add an exclusion of the grant_type!=authorization_code in the other conditions.
lib/auth.js
Outdated
| ) { | ||
| if (this.options?.eventHandlers?.onConnectionError) { | ||
| this.options.eventHandlers.onConnectionError(error, remainingAttempts); | ||
| if (!(this.grant_type && this.grant_type === GRANT_TYPE_AUTHORIZATION_CODE)) { |
There was a problem hiding this comment.
I have a preference for "positive" checks, since they are easier to read. in this case, we could check if it is a specific grant type (without null check since we set default) and if so, return early before moving to default logic
There was a problem hiding this comment.
Thanks for the comment. Fixed it.
lib/auth.js
Outdated
| * @param {string[]} [authObject.scope] Array of scopes used for requests | ||
| * @param {object} options options for the SDK as a whole, for example collection of handler functions, or retry settings | ||
| */ | ||
| constructor(authObject, options) { | ||
| constructor(authObject, options, grant_type = GRANT_TYPE_CLIENT_CREDENTIALS) { |
There was a problem hiding this comment.
Hey @fazbyte in docs above you say grant type is on authObject, but you pass as a param? I think the authObject object is correct for this value to be honest. you can always set the default in the first line in case missing or use object.assign to set defaults and overwrite with provided values in authObject
There was a problem hiding this comment.
@DougMidgley , grant_type is not part of authObject. When the user logs in with the custom app, we then call the v2/token endpoint and after we get the response, we pass the response.data to the sfmc-sdk without changing it and pass the additional parameter grant_type='authorization_code'. This way it works with what you have already created and within the custom app where the user logs in. Let me know if this makes sense or what other change could be made.
There was a problem hiding this comment.
@DougMidgley I have completely removed the grant_type parameter now. However if authObject doesn't contain access_token, then it will kickoff the validations for client_id/client_secret. Let me know your feedback
There was a problem hiding this comment.
thank you @fazbyte - knowing Doug's schedule, he will review this next week. Right, @DougMidgley ? ;-)
|
hey @DougMidgley & @fazbyte - is this moving PR anywhere? |
|
@fazbyte - one of the test classes is failing - not sure of the cause, but maybe pulling from dev may fix it, since it doesnt look related to your code. |
|
@DougMidgley , I am a newbie to contributing to open source projects, so I don't understand fully on what you mean by pulling from dev. Do you want me to merge my code to the develop branch and push it again ? Or Since I have forked it, do I need to sync my fork ? Could you explain a bit more on what do you mean by full support for web/public app? |
PR details
What is the purpose of this pull request? (put an "X" next to an item)
What changes did you make? (Give an overview)
Allow for passing an existing access token, for example in web app context