search

IdentityModel / oidc-client-js

OpenID Connect (OIDC) and OAuth2 protocol support for browser-based JavaScript applications
Apache License 2.0
2.43k stars 842 forks source link

AccessTokenExpired event should trigger a silentRenew in case of code pkce/refresh_token flow #787

Open Elias-Serneels opened 5 years ago

Elias-Serneels commented 5 years ago

Hi, Setup: We are using a code flow with pkce which puts refresh_tokens directly in the front-end.

Using this setup oidc-client-js will attempt to get a new access_token based on the current refresh_token as soon as the access_token is about to expire. However, oidc-client-js does NOT perform this same action if the access_token has already expired.

This means that if a user is unable to refresh the access_token in time and then connects to the internet again oidc-client-js will not attempt to refresh the current session (even after a page refresh) which results in an expired access_token remaining in storage until somehow a manual "login" trigger is called from somewhere else.

This seems to defeat to purpose of using code flow with pkce and the refresh_tokens it provides which should have a very long lifetime.

To work around this problem for now I have implemented the following code during startup which adds a function that calls signinSilent on the "AccessTokenExpired" event. This triggers the refresh_token refresh mechanism and establishes a "new" session. This allows us to re-establish the session after a page refresh.

userManager.events.addAccessTokenExpired(() => userManager.signinSilent);

Do you have any plans to add support for something similar in the library itself? To summarize: I would like to see the UserManager try to refresh an access_token (with the current valid refresh_token) after it expired instead of only during the "expiring" duration.

Above approach does not re-establish a session automatically when the internet connection comes back online but I believe that might be out of scope for this library.

Please let me know what you think of this suggestion, if you have any questions regarding our set-up or use case feel free to ask.

Thanks, Elias

brockallen commented 5 years ago

I see what you're saying. Given that that event didn't previously do that and the library didn't used to support refresh tokens, I think automatically adding this would be a breaking change (at least semantically).

I'll leave this open for future consideration.

NunoCruzSW commented 4 years ago

another option is a method that gets a new token if the current is expired, refreshing it case of expired. while the refresh token is valid there no problem to get a new access token, the problem is if the refresh token expires to, not sure if we need to try automatically renew the refresh token in case of app inactivity the could do the lifetime extend based on activity

sorenhoyer commented 4 years ago

@brockallen as you say this could break things for some of us that are still stuck with implicit flow (thanks to current B2C CORS limitations in our case). For us it works perfectly as it is currently, where silent renew is triggered on accessTokenExpires event if automaticSilentRenew is set to true, and then we can logout the user in accessTokenExpired if signInSilent did not succeed in accessTokenExpires event prior to that.

Now that I see you added it to the 2.0.0 milestone, could it perhaps be implemented with a flag, or take the scope info consideration, to avoid breaking things?

Thanks btw, for a great package!