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

Access token not renewed if expired #1601

Open
klues opened this issue Jul 25, 2024 · 9 comments
Open

Access token not renewed if expired #1601

klues opened this issue Jul 25, 2024 · 9 comments
Labels
bug Something isn't working

Comments

@klues
Copy link
Contributor

klues commented Jul 25, 2024

Silent renew of the access token is only done if it expires soon, not if it's already expired. It's possible to manually trigger renewal for expired access tokens by

userManager.events.addAccessTokenExpired(function() {
     userManager.signinSilent();
 })

but I wonder, why it's not done by default? Any specific reason for this, or could it be added by default?

@pamapa
Copy link
Member

pamapa commented Aug 20, 2024

userManager.getUser() should trigger an renew...

@klues
Copy link
Contributor Author

klues commented Aug 21, 2024

OK, but it doesn't do it, if the access token is already expired, only if it's expiring soon. That's my question if this should be changed?!

@pamapa
Copy link
Member

pamapa commented Aug 21, 2024

OK, but it doesn't do it, if the access token is already expired, only if it's expiring soon. That's my question if this should be changed?!

When a look at the code, it does (see "if it's negative, it will still fire" below):

public load(container: User): void {
const logger = this._logger.create("load");
// only register events if there's an access token and it has an expiration
if (container.access_token && container.expires_in !== undefined) {
const duration = container.expires_in;
logger.debug("access token present, remaining duration:", duration);
if (duration > 0) {
// only register expiring if we still have time
let expiring = duration - this._expiringNotificationTimeInSeconds;
if (expiring <= 0) {
expiring = 1;
}
logger.debug("registering expiring timer, raising in", expiring, "seconds");
this._expiringTimer.init(expiring);
}
else {
logger.debug("canceling existing expiring timer because we're past expiration.");
this._expiringTimer.cancel();
}
// if it's negative, it will still fire
const expired = duration + 1;
logger.debug("registering expired timer, raising in", expired, "seconds");
this._expiredTimer.init(expired);
}
else {
this._expiringTimer.cancel();
this._expiredTimer.cancel();
}
}

You may need to enable logging to console at debugging level to see what is going on with your application:

import { Log } from "oidc-client-ts";
Log.setLogger(console);
Log.setLevel(Log.DEBUG)

@klues
Copy link
Contributor Author

klues commented Aug 22, 2024

Yes, the event AccessTokenExpired is fired, but userManager.getUser() doesn't renew the tokens, if the access token is already expired. Therefore it's mandatory to add the code snippet I've posted to manually run userManager.signinSilent if the access token is expired.

Edit:
What I'm doing in detail:

  1. creating a new user manager with some config that was/is already authenticated, manger = new UserManager(config)
  2. register to event AccessTokenExpired, manager.addAccessTokenExpired(callback)
  3. get the user from manager, manager.getUser(), this returns an object with property expires_at with a timestamp 16 hours ago. It also calls the callback I've passed at (2), but there is no automatic renew of the access token. I have to add userManager.signinSilent() to the callback from (2), then it works as expected and the access token is renewed.

However if at (3) the access token isn't already expired, but expires soon, there is no need to register for any events and the access token is renewed automatically.

If setting the log level to DEBUG the only output I get is [UserManager] getUser: user loaded.

@pamapa
Copy link
Member

pamapa commented Aug 22, 2024

Yes, the event AccessTokenExpired is fired, but userManager.getUser() doesn't renew the tokens, if the access token is already expired. Therefore it's mandatory to add the code snippet I've posted to manually run userManager.signinSilent if the access token is expired.

Have you enabled automaticSilentRenew (is default true since v2.0)? This code is expected to run when "access token expired" happened:

protected _tokenExpiring: AccessTokenCallback = async () => {
const logger = this._logger.create("_tokenExpiring");
try {
await this._userManager.signinSilent();
logger.debug("silent token renewal successful");
}
catch (err) {
if (err instanceof ErrorTimeout) {
// no response from authority server, e.g. IFrame timeout, ...
logger.warn("ErrorTimeout from signinSilent:", err, "retry in 5s");
this._retryTimer.init(5);
return;
}
logger.error("Error from signinSilent:", err);
await this._userManager.events._raiseSilentRenewError(err as Error);
}

@klues
Copy link
Contributor Author

klues commented Aug 22, 2024

Yes, automaticSilentRenew is enabled.
I assume that the problem is that it's only listening to AccessTokenExpiring and not also AccessTokenExpired here:

this._userManager.events.addAccessTokenExpiring(this._tokenExpiring);

@pamapa
Copy link
Member

pamapa commented Aug 23, 2024

I assume that the problem is that it's only listening to AccessTokenExpiring and not also AccessTokenExpired here

I agree. This code of only doing addAccessTokenExpiring is very old.
I guess we can simply extend that and do that as well addAccessTokenExpired after the first register and also we need to remove it again. Can you try this out on your side?

@pamapa pamapa added bug Something isn't working and removed question Further information is requested labels Aug 23, 2024
@pamapa
Copy link
Member

pamapa commented Aug 23, 2024

This is probably a better way to handle this within SilentRenewService.ts:

...
            // this will trigger loading of the user so the expiring events can be initialized
            try {
                const user = await this._userManager.getUser();
                if (user?.expired) {
                    // we only listen on token expiring, thus request a new token if the current one is expired
                    await this._tokenExpiring();
                }
            }
            catch (err) {
                // catch to suppress errors since we're in a ctor
                logger.error("getUser error", err);
            }
...

Can you test this?

@franck102
Copy link

I've been bit by this as well, one question: after a signinSilent failure (I am getting a 400 - Invalid token from Keycloak) the library doesn't do anything with the user, wouldn't it make sense to somehow clear it?

Am I responsible to do that, and what would be the best API to call? If I do nothing subsequent calls to getUser() simply return the same user, with no indication that the access and/or refresh token are not valid anymore.

After adding

userManager.events.addAccessTokenExpired(async function() {
    try {
        await userManager.signinSilent();
    }
    catch (e) {
        let user = await userManager.getUser();
        // We still have a user with an access token & a refresh token here, should I discard it and how?
   }
}


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants