-
Notifications
You must be signed in to change notification settings - Fork 60
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
feat: handle access token refresh - OKTA-291504 #138
Conversation
src/http.js
Outdated
.catch(error => { | ||
// Handle oauth access token expiration | ||
if (this.oauth && error && error.status === 401) { | ||
return this.oauth.introspectAccessToken() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two questions about the logic here,
- Do we need to introspect the token? Would it be simpler to just assume it's invalid, clear it from local cache, and then try to get a new access token?
- Does this logic work OK if I submit bad JWKs data? In other words, if getting an access token fails because of bad credentials, does it give a 400 or 401 response? If it's 401 it seems like we could get in a failure loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to introspect the token? Would it be simpler to just assume it's invalid, clear it from local cache, and then try to get a new access token?
Since there are other scenarios can also cause status === 401
, I added introspect here just want to make sure it retry on getting access token only when token is not active. As it's not a call will happen frequently, and it also eliminates other retry cases, so I don't really think it will add too much burden to the process. But I agree, clearCachedToken should happen as long as error happen.
Does this logic work OK if I submit bad JWKs data? In other words, if getting an access token fails because of bad credentials, does it give a 400 or 401 response? If it's 401 it seems like we could get in a failure loop?
ErrorFilter had been added for getAccessToken call (https://github.com/okta/okta-sdk-nodejs/pull/138/files#diff-3a1276f4dc4422026a36b4b8750287bfR52) to throw the error here. Actually introspectAccessToken
can also help in this case to identity current token status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved clearCachedAccessToken
call outside of introspectAccessToken
to make sure all 401 error will clear cached token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to only retry one time.
3c666ac
to
471d5be
Compare
What's in this PR:
authorizationMode
isPrivateKey
Fixes #135