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

TokenManager does not refresh access token successfully #559

Open
jelhan opened this issue Dec 10, 2020 · 17 comments
Open

TokenManager does not refresh access token successfully #559

jelhan opened this issue Dec 10, 2020 · 17 comments

Comments

@jelhan
Copy link

jelhan commented Dec 10, 2020

I'm facing some issues with refresh of tokens in TokenManager.

I fetch an access token and add it to TokenManager. On refresh I see token manager trying to fetch an ID token additionally.

If scopes used to fetch the access token do not contain 'openid', it fails with an error complaining about missing 'openid' scope.

If scopes used to fetch the access token do contain 'openid', I see an endless loop of refreshes.

Here is a simplified reproduction:

import { OktaAuth } from '@okta/okta-auth-js';

const config = {
  clientId: 'secret',
  issuer: `https://examples.com/oauth2/secret`,
  pkce: false,
  redirectUri: `http://examples.com`,
};

const authClient = new OktaAuth(config);

// fetch an access token
const response = await authClient.token.getWithoutPrompt({
  responseType: 'token',
  scopes: ['foo'],
});
const { accessToken } = response.tokens;

// add token to token manager
authClient.tokenManager.add('foo', accessToken);

// log renewals
authClient.tokenManager.on('renewed', function (key, newToken) {
  console.log(`Renewed token with key ${key}`, newToken);
});

// log errors
authClient.tokenManager.on('error', function(error) {
  console.error('TokenManager error', error);
});
@swiftone
Copy link
Contributor

@jelhan - Thanks for the report.

You have run afoul of the defaults, which says that a user is not "authenticated" without both an idToken and an accessToken. (see https://github.com/okta/okta-auth-js#authstatemanager )

You can work around this by changing the definition of "isAuthenticated" using the transformAuthState config option: https://github.com/okta/okta-auth-js#transformauthstate

You would want to set .isAuthenticated based solely on the presence of authState.accessToken:

const config = {
  // other config
  transformAuthState: async (oktaAuth, authState) => {
    // oktaAuth is not used in this example
    authState.isAuthenticated = !!authState.accessToken; // convert to boolean
    return authState;
  }
};

@jelhan
Copy link
Author

jelhan commented Dec 11, 2020

Thanks for the quick response.

I added the transformAuthState to my simple reproduction. But still seeing the same issue. It seems that it tries to fetch the ID token before the transformAuthState is called for the refresh.

@swiftone
Copy link
Contributor

( I spent several minutes trying to find a way to say "well that's not good" in a professional manner)

I'll consult with the rest of the team to figure out how to get this to behave as intended. Are you blocked by this, or is this an inconvenience that can wait until we have a permanent solution?

@swiftone
Copy link
Contributor

Internal ref: OKTA-352791

@jelhan
Copy link
Author

jelhan commented Dec 14, 2020

Are you blocked by this, or is this an inconvenience that can wait until we have a permanent solution?

We wanted to use TokenManager for several features:

  • Persist access tokens in local storage.
  • Share access tokens across multiple instances (browser windows) running in parallel.
  • Refresh access tokens.

We need to deliver this functionality within the next weeks. If this bug is not resoled we would be required to implement it ourselves, which is a lot of additional work. We would appreciate if there is any chance for a hot fix or a work-a-round.

@shuowu
Copy link
Contributor

shuowu commented Dec 14, 2020

@jelhan The features you want should be supported in @okta/oka-auth-js@>=4.1. As we introduced AuthState since v4.1, we do suggest using it to get the latest info about your app's latest authState.

Here is a sample to follow: https://github.com/okta/okta-auth-js/tree/master/samples/generated/webpack-spa

@jelhan
Copy link
Author

jelhan commented Dec 14, 2020

@shuowu Thanks for your reply. But to be honest I can't follow.

If I didn't missed something in Your documentation AuthStateManager does not allow to add tokens to be managed by the library. We are planning to use it together with TokenManager. I don't see how it can replace it.

Could you please provide required changes to the minimal reproduction given above?

@shuowu
Copy link
Contributor

shuowu commented Dec 14, 2020

@jelhan Basically, the TokenManager handles store and maintain the renewal process. AuthStateManager subscribes to the emitted events from tokenManager to evaluate the latest authState.

For the renew process, you can call tokenManager.renew to start the renew process. And subscribe to the authState change with authStateManager.subscribe

Here are some code links from the linked sample in the last comment:

subscribe to authState change: https://github.com/okta/okta-auth-js/blob/master/samples/generated/webpack-spa/src/index.js#L63
renew token: https://github.com/okta/okta-auth-js/blob/master/samples/generated/webpack-spa/src/index.js#L161

@jelhan
Copy link
Author

jelhan commented Dec 14, 2020

@shuowu I'm very sorry but I can't follow. I provide a simplified reproduction in this bug report. It can be summarized as the following:

  1. Assume the user has a valid Okta session cookie.
  2. Fetch an access token using token.getWithoutPrompt() method:
    await authClient.token.getWithoutPrompt({
      responseType: 'token',
      scopes: ['foo'],
    });
  3. Add that access token to TokenManager:
    const { accessToken } = response.tokens;
    authClient.tokenManager.add('foo', accessToken);
    I also tried to use tokenManager.setTokens({ accessToken }) instead. But I was facing the same bug.
  4. Wait until Okta Auth JS tries to renew the token.

I now even tried to disable automatically renewal tokenManager: { autoRenew: false } and trigger it manually tokenManager.renew('foo'). But I see the same error.

I'm using v4.4.0 of @okta/okta-auth-js, which is the latest release accordingly to GitHub.

Just to be clear: I was not able yet add all to use the renew logic of TokenManager. It's always throwing. I'm very confused why such a basic workflow fails. 😕

Could this be caused by using an incompatible Okta server version? I haven't found a compatibility matrix in the documentation. Or is it maybe related to not using PKCE? Please note that we set pkce to false in the Okta configuration.

@shuowu
Copy link
Contributor

shuowu commented Dec 14, 2020

@jelhan Sorry about the confusion. Can you tried the sample app shared above to see if you can repro the issue? I tried the authentication and renewal flows in it, but failed the reproduce the issue you have. It would also be great if you can share the specific error you have, like the network log.

Here is a relevant issue about the endless renew loop which caused by customizing the system clock okta/okta-oidc-js#894

@jelhan
Copy link
Author

jelhan commented Dec 18, 2020

@jelhan Sorry about the confusion. Can you tried the sample app shared above to see if you can repro the issue? I tried the authentication and renewal flows in it, but failed the reproduce the issue you have. It would also be great if you can share the specific error you have, like the network log.

The sample app does not use the functionality we need. It's also way more complex than the simplified reproduction I have included in this GitHub issue. My reproduction has 30 lines of code. The sample app has 439 lines of code.

Are you able to reproduce my bug report using the reproduction I included in the initial comment? If not: What additional information do you need?

I also created a case in Okta help desk so that you can link this bug report to the customer. Happy to share more code or do a screen sharing session if that helps you with debugging.

Here is a relevant issue about the endless renew loop which caused by customizing the system clock okta/okta-oidc-js#894

The system clock is not customized.

@shuowu
Copy link
Contributor

shuowu commented Dec 18, 2020

@jelhan okta-auth-js provides higher-level methods to handle sign & renew process. You can use them instead of calling the lower-level methods from the token scope.

Here is a quick write-up in react (I feel it's easier to include both js and jsx together), you can move the logic out from useEffect and call it on dom ready if you are writing vanilla JS.

import { useState, useEffect } from 'react';
import { OktaAuth } from '@okta/okta-auth-js';

const oktaAuth = new OktaAuth({
  issuer: '....',
  clientId: '....',
  scopes: ['openid', 'email'],
})

function App() {
  const [authState, setAuthState] = useState({ 
    isPending: true,
    isAuthenticated: false,
    idToken: null,
    accessToken: null,
  });

  const login = () => {
    oktaAuth.signInWithRedirect();
  };

  const logout = () => {
    oktaAuth.signOut();
  }

  const renew = () => {
    oktaAuth.tokenManager.renew('accessToken')
  }

  useEffect(() => {
    // subscribe to the latest authState
    oktaAuth.authStateManager.subscribe(authState => {
      console.log('updated authState', authState)
      setAuthState(authState);
    });

    if (oktaAuth.isLoginRedirect()) {
      // handle success login redirect from okta
      // store tokens and make proper post login redirect 
      oktaAuth.handleLoginRedirect()
    } else {
      // trigger initial authState update when page load
      oktaAuth.authStateManager.updateAuthState();
    }
  }, [oktaAuth]);

  return (
    <div className="App">
      {
        authState.isAuthenticated ? 
          <button onClick={logout}>Logout</button> :
          <button onClick={login}>Login</button>
      }
      <button onClick={renew}>Renew</button>
    </div>
  );
}

export default App;

@jelhan
Copy link
Author

jelhan commented Dec 22, 2020

@shuowu That example is not matching what we want to do. Our requirements can be summarized as the following:

  • Fetch access tokens without user interaction.
  • Fetch and manage multiple access token with different scopes.
  • Share multiple access tokens between different instances.
  • Refresh all access tokens fetched during the lifetime of the app.

As far as I'm aware this is only possible by using low-level APIs like token.getWithoutPrompt().

We need to deliver this functionality within the next weeks. If this bug is not resoled we would be required to implement it ourselves, which is a lot of additional work. We would appreciate if there is any chance for a hot fix or a work-a-round.

We came to the point where we needed to evaluate this again. To do so we decided to evaluate if TokenManager is able to handle multiple access tokens in parallel. But we faced another issue with TokenManager not working well together with access token stored with keys not matching accessToken or idToken. As the renewal process of the library doesn't seem to be stable we decided to not use TokenManager and AuthStateManager but implementing token management ourselves.

Keeping this bug open as we may want to consider replacing our custom solution with the features provided by @okta/okta-auth-js when the bugs are resolved.

@aarongranick-okta
Copy link
Contributor

@jelhan Thank you for the concise and detailed description of the problem. I was able to reproduce the issue using the okta-auth-js test app. There is an assumption in TokenManager that both tokens will be used together. This assumption is obviously flawed. We will fix this issue so that TokenManager correctly supports the case of accessToken with no idToken (OKTA-352791)

In the meantime, I believe that TokenManager will satisfy most of your requirements for cross-tab storage. However the autoRenew feature will not work as expected (due to this flaw). As a workaround, you can manually renew with code like this:

import { OktaAuth, EVENT_EXPIRED } from '@okta/okta-auth-js';
const config = {
  issuer: '..',
  clientId: '',
  tokenManager: {
    autoRenew: false,
    autoRemove: false
  }
}
const sdk = new OktaAuth(config);
sdk.tokenManager.on(EVENT_EXPIRED, (key) => {
  const existingToken = await sdk.tokenManager.get(key);
  sdk.token.renew(existingToken)
    .then(newToken => {
        sdk.tokenManager.add(key, newToken);
    })
    .catch(e => {
      sdk.tokenManager.remove(key);
    });
});

Note that autoRemove should also be disabled.

@jelhan
Copy link
Author

jelhan commented Jan 15, 2021

@aarongranick-okta Thanks a lot for sharing the update.

Glad to hear that you were able to reproduce the bug. I was still worried that we do something wrong. Do you already have an ETA for the bug fix?

Thanks a lot for sharing a work-a-round as well. We already rolled out our own solution for cross-tab storage and renewal of the tokens as we needed it urgently. I think we will use that one until the bug is fixed.

@rwev
Copy link

rwev commented Mar 8, 2022

Hi all, I believe we ran into a similar issue @jelhan is experiencing. We ended up having to fork renewTokensWithRefresh to support renewing multiple application-scoped tokens issued to different clients, (in our case, various angular SPAs running on the same origin at different subroutes, and thus various OktaConfig configurations).

The problem with the existing okta-auth-js seems to be as follows:

  • {@link OktaAuth.tokenManager.renew} relies on renewTokens() source
  • {@link OktaAuth.token.renewTokens } only renews the token at key accessToken with the refresh token at key refreshToken, and relies on renewTokensWithRefresh()
  • {@link renewTokensWithRefresh} requests token renewal with the clientId from the actively injected OktaConfig, which causes requests to renew tokens issued to other applications to fail with error:

"error": "invalid_grant",
"error_description": "The grant was issued to another client. Please make sure the 'client_id' matches the one used at the authorize request."

Here is the function that works for our use case, which adapts the clientId to match that of the token being renewed, instead of the current injection context.

export async function renewTokensWithRefreshOverrideId(
  oktaAuth: OktaAuth,
  accessTokenToRefresh: AccessToken,
  refreshTokenObject: RefreshToken,
): Promise<TokenResponse> {
  const clientId: string = (accessTokenToRefresh.claims as any).cid
  const tokenParams = {
    clientId,
  }

  const tokenResponse = await postRefreshToken(oktaAuth, tokenParams, refreshTokenObject)
  const urls = getOAuthUrls(oktaAuth, {})
  return await handleOAuthResponse(oktaAuth, tokenParams, tokenResponse, urls)
}

@rwev
Copy link

rwev commented Dec 9, 2022

Bump, any updates on this?

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

No branches or pull requests

5 participants