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

Tokens don't manage to refresh within 3 seconds when oidc provider is under a high load #891

Closed
Nikitakun opened this issue Nov 3, 2020 · 6 comments · Fixed by #900
Closed
Labels

Comments

@Nikitakun
Copy link

Describe the bug
When our IdentityServer4 is under a high load, refreshing tokens might take more than 3 seconds, in which case the library would cancel the previous token refresh request and start a new one, only increasing load on the server, thus never getting the token refreshed.

Expected behavior
It would be helpful to have the token refresh interval configurable so as to set a custom interval value in case 3 seconds is not enough to process a refresh.

The TOKEN_REFRESH_INTERVALL_IN_SECONDS in OidcSecurityService could be used as the default/fallback value in case it wasn't supplied in the config.

export class OidcSecurityService {
    private TOKEN_REFRESH_INTERVALL_IN_SECONDS = 3;
   ...

Desktop (please complete the following information):

  • OS: Windows 10
  • Browser: Chrome
  • Version: 86.0.4240.111
@Expelz
Copy link

Expelz commented Nov 9, 2020

Hello @Nikitakun
Refresh session mechanism with enabled refresh token feature doesn't work as you discribed. Current implementation starts token validation every 3 seconds as you mentioned. But it doesn't mean that the token refresh request will be sent. Firstly this process periodicallyTokenCheckService.startTokenValidationPeriodically will check identity and access tokens expiration dates and only if atleast one of them is expired and the silent renew process already hasn't running it will send the token refresh request to your IS (only in this case and with these conditions).

Please check source code for more details.
If you need some help please decribe in more details your problem.

@Nikitakun
Copy link
Author

Hello @Expelz, @damienbod thanks for the feedback!

I have debugged it some more and came up with a step by step explanation on how the bug occurs:

  1. An interval starts emitting values every 3 seconds. On every emission, a switchMap operator is called, it internally unsubscribes from the inner observable subscribed to on the previous interval emission

    const periodicallyCheck$ = this.intervallService.startPeriodicTokenCheck(repeatAfterSeconds).pipe(
    switchMap(() => {

  2. Given the identity or access token has expired and no silent renew/refresh token is running, the inner refresh token request observable is returned

    return this.refreshSessionRefreshTokenService.refreshSessionWithRefreshTokens();

  3. Given the request is taking 4 seconds, we would observe the following. The interval from p.1 would emit a new value 3 seconds after the request started, calling the said switchMap operator. It would unsubscribe from Angular's http request returned in p.2 effectively aborting it. The function inside switchMap would be invoked and it would reach the shouldBeExecuted check which will fail because isSilentRenewRunning would be true. So, this function will return of(null)

    const shouldBeExecuted = userDataFromStore && !isSilentRenewRunning && idToken;
    if (!shouldBeExecuted) {
    return of(null);
    }

  4. Since of(null) immediately emits a value, the whole periodicallyCheck$ stream success handler would be called. This handler would call resetSilentRenewRunning() making isSilentRenewRunning === false next time the interval handler runs.

    .subscribe(
    () => {
    this.loggerService.logDebug('silent renew finished!');
    if (this.flowHelper.isCurrentFlowCodeFlowWithRefeshTokens()) {
    this.flowsDataService.resetSilentRenewRunning();
    }
    },

  5. 3 seconds passed, a new interval emission comes up and steps 2-5 repeat again, aborting requests and repeating them indefinitely, increasing load on the server.

That is how it works for me. I believe, it would be essential to have this 3 seconds interval configurable!

@Expelz
Copy link

Expelz commented Nov 19, 2020

@Nikitakun, you are right! There is the problem with such implementation.
I've never faced with this problem because I'm using code flow with silent renew.

I suppose that these line should be removed:

if (this.flowHelper.isCurrentFlowCodeFlowWithRefeshTokens()) {
this.flowsDataService.resetSilentRenewRunning();
}

But then we will be faced with infinite silent renew process running, so it's not an option. Need to investigate possible solutions to this problem.

Please share some ideas about it.

P.S.: I think TOKEN_REFRESH_INTERVALL_IN_SECONDS as configurable value it's only workarond to the problem.

@Nikitakun
Copy link
Author

@Expelz, I do agree that making the interval value optionally configurable would be the easiest solution, and potentially the best one, because it wouldn't fiddle with any core logic.

@damienbod, will it be possible to have this introduced in an upcoming release?

@damienbod
Copy link
Owner

@Nikitakun @Expelz Thanks for taking to time to anaylse this problem. I will add this, hopefully today.

@damienbod
Copy link
Owner

damienbod commented Nov 20, 2020

released now in 11.2.3, thanks for you help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants