fix: TokenManager.renew(key) should renew only requested token #656

denysoblohin-okta · 2021-03-11T21:16:17Z

Current implementation of TokenManager.renew refreshes access token and id token together if user not specified responseType in OktaAuth config (as in #559 - responseType was used only in getWithoutPrompt and not in OktaAuth contructor).

okta-auth-js/lib/oidc/renewTokens.ts

Line 41 in 5e9af2f

const { responseType } = getDefaultTokenParams(sdk);

If user uses multiple access tokens with multiple keys (as in issue #559), TokenManager.renew will always use key of first access token in token storage to update any access token with new value:

okta-auth-js/lib/TokenManager.ts

Line 338 in 5e9af2f

    
           emitRenewed(tokenMgmtRef, accessTokenKey, accessToken, oldTokenStorage[accessTokenKey]),

okta-auth-js/lib/TokenManager.ts

Line 187 in 5e9af2f

function getKeyByType(storage, type: TokenType): string {

This will lead to a problem when expired access token will stay in storage. And on resetExpireEventTimeoutAll timer to update will fire again

okta-auth-js/lib/TokenManager.ts

Line 139 in 5e9af2f

for(var key in tokenStorage) {

Internal ref: OKTA-352791
Issue: #559

shuowu · 2021-03-12T17:14:21Z

lib/TokenManager.ts

-  })
-    .then(function(freshTokens) {
-      // store and emit events for freshTokens
+  tokenMgmtRef.renewPromise[key] = sdk.token.renew(token)


sdk.token.renew (oidc/renewToken) only handles renew in the hidden iframe way. This change should still keep the renew with refresh approach.

codecov-io · 2021-03-13T02:18:34Z

Codecov Report

Merging #656 (e5a8d01) into master (a81cd6d) will increase coverage by 0.15%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##           master     #656      +/-   ##
==========================================
+ Coverage   92.72%   92.88%   +0.15%     
==========================================
  Files          67       67              
  Lines        2447     2599     +152     
  Branches      517      559      +42     
==========================================
+ Hits         2269     2414     +145     
- Misses        178      185       +7

Impacted Files	Coverage Δ
lib/TokenManager.ts	`96.25% <89.28%> (-1.48%)`	⬇️
lib/browser/browser.ts	`93.29% <100.00%> (+2.38%)`	⬆️
lib/oidc/index.ts	`100.00% <100.00%> (ø)`
lib/OktaAuthBase.ts	`88.88% <0.00%> (-1.12%)`	⬇️
lib/oidc/getWithPopup.ts	`100.00% <0.00%> (ø)`
lib/oidc/getWithRedirect.ts	`100.00% <0.00%> (ø)`
lib/oidc/getWithoutPrompt.ts	`100.00% <0.00%> (ø)`
lib/oidc/util/defaultTokenParams.ts	`100.00% <0.00%> (ø)`
... and 1 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 a81cd6d...e5a8d01. Read the comment docs.

shuowu · 2021-03-16T14:27:45Z

lib/oidc/renewToken.ts

-    issuer
-  })
+
+  // If we have a refresh token, renew using that, otherwise getWithoutPrompt


I think it would make sense to add a helper renew function in TokenManager to make the flows check (it's actually looks like a manager function).

Also, as we expose the renew functions from the top-level (browser.ts), behavior change in the lower-level functions looks like a breaking change to me (changes the public API authClient.token.renew, which should use getWithoutPrompt only).

Moved renew token with refresh token flow to TokenManager

denysoblohin-okta · 2021-03-22T07:09:06Z

@shuowu What do you think on the latest change?
I've moved renew token (with or without refresh token) logic to helper function _renewToken in TokenManager.
I've exposed renewTokensWithRefresh to Token API, to be able to use it in TokenManager (maybe should add _ prefix?).

shuowu-okta · 2021-03-22T16:35:43Z

lib/browser/browser.ts

@@ -50,6 +50,7 @@ import {
  revokeToken,
  renewToken,
  renewTokens,
+  renewTokensWithRefresh,


Looks like this method does not exist anymore, remove?

shuowu-okta · 2021-03-22T16:35:52Z

lib/browser/browser.ts

@@ -213,6 +214,7 @@ class OktaAuthBrowser extends OktaAuthBase implements OktaAuth, SignoutAPI {
      decode: decodeToken,
      revoke: revokeToken.bind(null, this),
      renew: renewToken.bind(null, this),
+      renewTokensWithRefresh: renewTokensWithRefresh.bind(null, this),


shuowu-okta · 2021-03-22T16:35:57Z

lib/oidc/index.ts

@@ -16,6 +16,7 @@ export * from './util';
 export { decodeToken } from './decodeToken';
 export { revokeToken } from './revokeToken';
 export { renewToken } from './renewToken';
+export { renewTokensWithRefresh } from './renewTokensWithRefresh';


shuowu-okta · 2021-03-22T16:36:04Z

lib/types/api.ts

@@ -148,6 +148,7 @@ export interface TokenAPI extends BaseTokenAPI {
  revoke(token: RevocableToken): Promise<object>;
  renew(token: Token): Promise<Token>;
  renewTokens(): Promise<Tokens>;
+  renewTokensWithRefresh(refreshTokenObject: RefreshToken): Promise<Tokens>;


shuowu-okta · 2021-03-22T16:39:57Z

We don't need to add renewTokensWithRefresh to the top level api, otherwise LGTM.

support renew token with refresh token

This reverts commit 20a8e11.

shuowu · 2021-03-22T17:38:39Z

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog

+## 4.8.1


Let's make the change as 4.9.0 (minor), since it adds new top level API.

denysoblohin-okta force-pushed the fix-renew-by-key-OKTA-352791 branch from d875717 to b8a788d Compare March 12, 2021 14:26

denysoblohin-okta changed the title ~~fix: TokenManager.renew(key) should not renew all tokens~~ fix: TokenManager.renew(key) should renew only requested token Mar 12, 2021

denysoblohin-okta marked this pull request as ready for review March 12, 2021 15:20

denysoblohin-okta requested review from shuowu, swiftone and aarongranick-okta March 12, 2021 15:20

shuowu reviewed Mar 12, 2021

View reviewed changes

denysoblohin-okta requested a review from shuowu March 16, 2021 00:10

shuowu reviewed Mar 16, 2021

View reviewed changes

denysoblohin-okta force-pushed the fix-renew-by-key-OKTA-352791 branch from a8c7da5 to e5a8d01 Compare March 17, 2021 17:00

shuowu-okta reviewed Mar 22, 2021

View reviewed changes

shuowu-okta approved these changes Mar 22, 2021

View reviewed changes

denysoblohin-okta added 6 commits March 22, 2021 19:27

fix: TokenManager.renew(key) should not renew all tokens

66155a1

fix lint

4a24ee4

support renew token with refresh token

Revert "fix lint"

fa70cf8

This reverts commit 20a8e11.

renew token with refresh moved to manager

06252bd

_renewToken

104e4fb

changelog

e88635e

denysoblohin-okta force-pushed the fix-renew-by-key-OKTA-352791 branch from 5e6f48d to e88635e Compare March 22, 2021 17:30

shuowu reviewed Mar 22, 2021

View reviewed changes

changelog

9d9bc46

eng-prod-CI-bot-okta closed this Mar 22, 2021

Tennyx mentioned this pull request May 20, 2021

Post 4.9.0 update, refresh token not being rotated in local storage on tokenManager.renew() #747

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: TokenManager.renew(key) should renew only requested token #656

fix: TokenManager.renew(key) should renew only requested token #656

denysoblohin-okta commented Mar 11, 2021 •

edited

Loading

shuowu Mar 12, 2021

codecov-io commented Mar 13, 2021 •

edited

Loading

shuowu Mar 16, 2021

denysoblohin-okta Mar 17, 2021

denysoblohin-okta commented Mar 22, 2021

shuowu-okta Mar 22, 2021

shuowu-okta Mar 22, 2021

shuowu-okta Mar 22, 2021

shuowu-okta Mar 22, 2021

shuowu-okta commented Mar 22, 2021

shuowu Mar 22, 2021

fix: TokenManager.renew(key) should renew only requested token #656

fix: TokenManager.renew(key) should renew only requested token #656

Conversation

denysoblohin-okta commented Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Mar 13, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denysoblohin-okta commented Mar 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuowu-okta commented Mar 22, 2021

Choose a reason for hiding this comment

denysoblohin-okta commented Mar 11, 2021 •

edited

Loading

codecov-io commented Mar 13, 2021 •

edited

Loading