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

fix issues with refresh token #832

Closed
wants to merge 10 commits into from

Conversation

aarongranick-okta
Copy link
Contributor

@aarongranick-okta aarongranick-okta commented Jun 21, 2021

  • prevents non-refresh token from being stored as a refresh token
  • does not try to renew the refresh token
  • catches renew with refresh token failure and removes relevant token
  • fix some incorrect typings
  • add unit tests

#738
#747
#784

OKTA-399459
OKTA-400565
OKTA-397735
OKTA-403154

@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2021

Codecov Report

Merging #832 (5673490) into master (578fb28) will decrease coverage by 0.24%.
The diff coverage is 88.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #832      +/-   ##
==========================================
- Coverage   91.70%   91.45%   -0.25%     
==========================================
  Files         114      116       +2     
  Lines        3268     3313      +45     
  Branches      679      688       +9     
==========================================
+ Hits         2997     3030      +33     
- Misses        271      283      +12     
Impacted Files Coverage Δ
lib/OktaAuth.ts 88.25% <ø> (ø)
lib/types/Token.ts 58.33% <ø> (-41.67%) ⬇️
lib/TokenManager.ts 94.39% <80.00%> (-0.17%) ⬇️
lib/oidc/util/refreshToken.ts 80.95% <80.95%> (ø)
lib/oidc/renewToken.ts 85.71% <83.33%> (-8.04%) ⬇️
lib/errors/AuthApiError.ts 100.00% <100.00%> (ø)
lib/errors/index.ts 100.00% <100.00%> (ø)
lib/oidc/renewTokens.ts 100.00% <100.00%> (ø)
lib/oidc/renewTokensWithRefresh.ts 100.00% <100.00%> (ø)
lib/oidc/util/index.ts 100.00% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 578fb28...5673490. Read the comment docs.

@@ -359,19 +353,15 @@ export class TokenManager {
this.emitRemoved(key, removedToken);
}

// TODO: these methods are redundant and can be removed in the next major version
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add a jira to track it.

return this.sdk.token.renew(token);
}
validateToken(token: Token) {
Copy link
Contributor

Choose a reason for hiding this comment

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

also remove this one in 6.0?

Copy link
Contributor

@shuowu shuowu left a comment

Choose a reason for hiding this comment

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

Missing changelog

- do not try to renew the refresh token
- catch refresh token failure and remove token
- prevent invalid refresh tokens from being stored
@aarongranick-okta aarongranick-okta force-pushed the ag-OKTA-403154-refresh-token branch from f94f167 to 802a128 Compare June 24, 2021 18:56

- [#832](https://github.com/okta/okta-auth-js/pull/832) Fixes issues with refresh tokens

## 5.1.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding all those changelogs, I'll just close my PR #835

@@ -45,7 +45,7 @@ Feature: Multi-Factor Authentication with Password and SMS
When She selects SMS from the list
And She inputs a invalid phone number
And She selects "Receive a Code"
Then she should see a message "Unable to initiate factor enrollment: Invalid Phone Number."
Copy link
Contributor

Choose a reason for hiding this comment

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

probably search the error message in the code base, looks like still missing one e2e test case and some unit tests (probably not necessary).

eng-prod-CI-bot-okta pushed a commit that referenced this pull request Jun 25, 2021
- do not try to renew the refresh token
- catch refresh token failure and remove token
- prevent invalid refresh tokens from being stored
fix: support rotating refresh tokens
Update string for phone enroll error

OKTA-329744
update error msg

OKTA-403154
<<<Jenkins Check-In of Tested SHA: 5673490 for eng_productivity_ci_bot_okta@okta.com>>>
Artifact: okta-auth-js
Files changed count: 31
PR Link: "#832"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants