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

Added 'Simulate cross-tab token renew' in test app #1106

Closed
wants to merge 6 commits into from

Conversation

denysoblohin-okta
Copy link
Contributor

Internal ref: https://oktainc.atlassian.net/browse/OKTA-456240
Added button 'Simulate cross-tab token renew' in test app that opens 20 iframes that tries to autorenew tokens simultaneously.
Practically system allows to run 15 simultaneous renew requests, next ones will result in HTTP 429.
See video

cross-tab-test-app.mov

test/apps/app/src/testApp.ts Outdated Show resolved Hide resolved
test/apps/app/src/testApp.ts Outdated Show resolved Hide resolved
@denysoblohin-okta
Copy link
Contributor Author

Added config in UI for tabs count and cross-tab sync storage:
Screenshot 2022-02-17 at 12 09 01

Improved output in iframes to show auth state and renew state (Wating for renew, Expired, Renewed, Updated from other tab) with colors:
Screenshot 2022-02-17 at 12 08 15

@denysoblohin-okta denysoblohin-okta force-pushed the od-cross-tabs-test-app-OKTA-456240 branch from c41d5de to f4e6eec Compare February 17, 2022 10:32
@denysoblohin-okta
Copy link
Contributor Author

denysoblohin-okta commented Feb 17, 2022

Noticed the side effect after #1087: there can be situations during cross-tab token renewal when renew fails in one tab with HTTP 429 but can be retried successfully.
See screenshot above - AuthApiError / Expired / Not authenticated vs AuthApiError / Renewed / Authenticated. In second case there are 2 attempts to renew token.

Explanation:

In TokenManager.renew() HTTP 429 error (AuthApiError) is caught here:

.catch(err => {
// If renew fails, remove token from storage and emit error
this.remove(key);

Expired token is removed from storage and triggers EVENT_REMOVED.
var tokenStorage = this.storage.getStorage();
var removedToken = tokenStorage[key];
delete tokenStorage[key];
this.storage.setStorage(tokenStorage);
this.emitRemoved(key, removedToken);

AuthStateManager listens to this event and calls updateAuthState() here:
sdk.tokenManager.on(EVENT_REMOVED, (key, token) => {
this._setLogOptions({ event: EVENT_REMOVED, key, token });
this.updateAuthState();
});

In updateAuthState() call to isAuthenticated() will be performed:
this._sdk.isAuthenticated()

In isAuthenticated() token can be renewed if autoRenew options is set:
let { accessToken, idToken } = this.tokenManager.getTokensSync();
const { autoRenew, autoRemove } = this.tokenManager.getOptions();
if (accessToken && this.tokenManager.hasExpired(accessToken)) {
accessToken = undefined;
if (autoRenew) {
try {
accessToken = await this.tokenManager.renew('accessToken') as AccessToken;

Note two things:

  1. we are renewing accessToken and idToken with one API call (TokenManager.renew(key) calls sdk.token.renewTokens())
  2. localStorage is shared across tabs

When TokenManager.renew() is called for idToken and renew fails, ID token is removed from storage - but it could be valid token just renewed from another tab, see (2), we don't check and compare actual token value we're removing.

During isAuthenticated() accessToken is checked first for expiration and renew can be triggered here because we've just removed ID token but not Access token, see (1)

Proposals?

  • when removing token from storage, check that value is same (has not been updated from another tab)
  • ignore autoRenew and autoRemove in isAuthenticated() if we caught error from renewTokens()

eng-prod-CI-bot-okta pushed a commit that referenced this pull request Feb 18, 2022
OKTA-456240
<<<Jenkins Check-In of Tested SHA: 7cea0c5 for eng_productivity_ci_bot_okta@okta.com>>>
Artifact: okta-auth-js
Files changed count: 5
PR Link: "#1106"
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.

3 participants