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

Code Flow erroring out due to multipe expiry events #632

Closed
Jejuni opened this issue Sep 19, 2019 · 6 comments
Closed

Code Flow erroring out due to multipe expiry events #632

Jejuni opened this issue Sep 19, 2019 · 6 comments
Labels
bug For tagging faulty or unexpected behavior. pr-welcome We'd welcome a PR to solve the issue.

Comments

@Jejuni
Copy link

Jejuni commented Sep 19, 2019

Describe the bug
When OAuthModule is set up, on initial app load, the code flow behaves as expected. Redirection occurs as expected, tokens are stored in sessionStorage (as per default) and refresh token usage works as expected as well (at 75% life of token, refresh token is retrieved from storage and sent to IdP to get new tokens).
When the user now hits F5, the flow initiates, but the "token_expires" event fires twice at the same time with info "id_token". This retrieves new tokens twice, also meaning that 2 refresh tokens get generated. Only one of these is valid thereafter, though.
Once the token expire again the "token_expires" event only fires once. However, at that point it's a gamble whether or not the valid refresh token was saved to storage. If the valid token was saved everything works as expected. If not the IdP rejects the call saying the refresh token was invalid.

This ALSO happens with implicit flow. After F5'ing the token_expires event fires twice. But since implicit flow just logs the user back in there is no problem with invalid data. It is, however, a wasted call.

To Reproduce
Set up a standard Identity Server configuration with localhost:5000:

new Client
{
	ClientId = "angular",
	ClientName = "Angular Client",
	AllowedGrantTypes = GrantTypes.Code,
	RequirePkce = true,
	RequireClientSecret = false,

	Description = "Code Client for the angular",

	RequireConsent = false,

	RedirectUris =            new List<string> { "http://localhost:4200" },
	PostLogoutRedirectUris =  new List<string> { "http://localhost:4200" },
	AllowedCorsOrigins =      new List<string> { "http://localhost:4200" },

	AllowedScopes =
	{
		IdentityServerConstants.StandardScopes.OpenId
	},

	// necesarry for refresh token
	AllowOfflineAccess = true,
}

Setup a very simple Angular app with ng new:
app.module

...
  imports: [
    BrowserModule,
    HttpClientModule,
    OAuthModule.forRoot()
  ]
...

Switch the implementation of app.component to:

import { Component, OnInit } from '@angular/core';
import { AuthConfig, OAuthService, JwksValidationHandler } from '../lib/public_api';
import { from, of } from 'rxjs';
import { switchMap, mapTo, catchError } from 'rxjs/operators';

@Component({
  selector: 'app-root',
  templateUrl: './app.component.html',
  styleUrls: ['./app.component.scss']
})
export class AppComponent implements OnInit {
  title = 'oauth-test';

  ngOnInit(): void {
    this.oauthService.events.subscribe(x => console.log(x));

    const authConfig = new AuthConfig();

    authConfig.requireHttps = false;
    authConfig.issuer = 'http://localhost:5000';
    authConfig.redirectUri = 'http://localhost:4200';
    authConfig.clientId = 'angular';
    authConfig.responseType = 'code';
    authConfig.scope = 'openid offline_access';

    // timeout set to really low value to spend less time waiting for the error to occur
    authConfig.timeoutFactor = 0.05;

    this.oauthService.configure(authConfig);
    this.oauthService.tokenValidationHandler = new JwksValidationHandler();
    this.oauthService.setupAutomaticSilentRefresh();

    from(this.oauthService.loadDiscoveryDocumentAndLogin()).subscribe();
  }

  constructor(private oauthService: OAuthService) {
  }
}

Expected behavior
The "token_expires" event only ever fires once for the actual situation of a token expiring.

Desktop (please complete the following information):

  • OS: Windows
  • Browser Chrome (happens on all browsers)
  • Version 76, happens on all versions

EDIT 1:
This problem also happens with the sample application provided with the repository.
Simply set the timeoutFactor on the config to some low value (0.01) and you can see the event being emitted twice. Also of note is that for an actual error to occur the identity server needs to be set to RefreshTokenUsage = OneTime (which generates a new refresh token every time). I believe the sample apps setting is set to ReUse, which circumvents the problem.

@jeroenheijmans
Copy link
Collaborator

Thanks for the extensive details! I recall seeing similar issues (or at least: ones with similar symptoms), but can't find them at the moment...

@jeroenheijmans jeroenheijmans added the bug For tagging faulty or unexpected behavior. label Sep 24, 2019
@mehmeterdemsoy
Copy link

I see same behaviour in our application. We think the problem is in de setupSilentRefreshEventListener method. During refresh, tryLogin().. happens asynchronously so window.addEventListener('message', this.silentRefreshPostMessageEventListener); gets called before the asynchronous method finishes and replaces the tokens. Calling this method fires the second "token_expires" event.


setupSilentRefreshEventListener() {
    this.removeSilentRefreshEventListener();
    this.silentRefreshPostMessageEventListener = (/**
     * @param {?} e
     * @return {?}
     */
    (e) => {
        /** @type {?} */
        const message = this.processMessageEventMessage(e);
        this.tryLogin({
            customHashFragment: message,
            preventClearHashAfterLogin: true,
            onLoginError: (/**
             * @param {?} err
             * @return {?}
             */
            err => {
                this.eventsSubject.next(new OAuthErrorEvent('silent_refresh_error', err));
            }),
            onTokenReceived: (/**
             * @return {?}
             */
            () => {
                this.eventsSubject.next(new OAuthSuccessEvent('silently_refreshed'));
            })
        }).catch((/**
         * @param {?} err
         * @return {?}
         */
        err => this.debug('tryLogin during silent refresh failed', err)));
    });
    window.addEventListener('message', this.silentRefreshPostMessageEventListener);
}

@manfredsteyer manfredsteyer added the pr-welcome We'd welcome a PR to solve the issue. label Oct 31, 2019
@manfredsteyer
Copy link
Owner

I'm not sure, if this is the reason b/c setupSilentRefreshEventListener is used for implicit flow and the bug seems to occour for code flow.

@Jejuni Did you find a solution or do you have a proposal for a solution.

I'll look into this when making the next version ready, which will be soon b/c of Angular 9.

@Jejuni
Copy link
Author

Jejuni commented Nov 2, 2019

Hey,

the bug happens with both implicitFlow as well as code flow. The initial request is sent twice. This doesn't overly affect implicitFlow as it's just incurs an additional trip to the IdP.

Sadly we haven't implemented any satisfactory solution as of now.
Our workaround is to set the refreshTokenUsage to "ReUse" so that only 1 refresh token gets generated and we don't have to deal with potentially invalid refresh tokens.

@manfredsteyer
Copy link
Owner

I see. I'll find a solution for the next major version which lands very soon (as Angular 9 is already RC).

I make this a priority! It's important for security and to align with current best practices.

Also thanks to @BioPhoton for pointing me to this!

@manfredsteyer
Copy link
Owner

Will be solved with Angular 9 which lands soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For tagging faulty or unexpected behavior. pr-welcome We'd welcome a PR to solve the issue.
Projects
None yet
Development

No branches or pull requests

4 participants