Event 'discovery_document_loaded' is triggered twice, possibly causing nonce validation to fail

[amchavan]

To reproduce

The issue can be verified very simply by editing the sample app in this repo, adding a couple of lines at the beginning of AppComponent.constructor() to log the event

   this.oauthService.events
      .pipe(filter((e) => e.type === 'discovery_document_loaded'))
      .subscribe((e) => console.log( e.type, e['info'] ));

and adding a call to oauthService.initCodeFlow() to AppComponent.configureCodeFlow():

    this.oauthService.configure(authCodeFlowConfig);
    this.oauthService.initCodeFlow();   // ADDED
    this.oauthService.loadDiscoveryDocumentAndTryLogin().then((_) => { ... }

The net effect is, the user cannot login and is redirected to the login page over and over.

Analysis

In oauth-service.ts the discovery_document_loaded event gets published twice, once by loadJwks(), and then again by its caller, loadDiscoveryDocument().

This is probably inefficient but harmless, unless you call initCodeFlow() before invoking loadDiscoveryDocumentAndTryLogin(), as suggested in the "Logging in" section of the README.md file.
The code flow init method (see lines 2706-2708 in oauth-service.ts) subscribes to the discovery_document_loaded event, invoking initCodeFlowInternal() when upon reception.

The nonce is created in the course of the execution of this last method.

If the event is thrown twice:

• (first event received) A nonce is created
• The login URL is created, including that nonce
• The browser is redirected to the login page
• (second event received) A new nonce overwrites the previous one in the session storage
• The browser is redirected to the app; the URL includes the previous nonce
• Nonce validation fails: nonce in the session storage does not match what came back from the Identity Provider

I suggest that the discovery_document_loaded event gets published only once.

[amchavan]

See also #1125 and #810, which triggered the creation of this ticket.

[amchavan]

Removed from problem description:

, or initCodeFlow() is removed: interestingly, calling that method is mentioned in the documentation, but both example applications avoid calling it.

Method initCodeFlow() is indeed called by the sample app, not directly, but by way of initLoginFlow().

While the documentation seems to imply that initCodeFlow() should be called before loadDiscoveryDocumentAndTryLogin(), the samples invert that order, avoiding nonce corruption.

[jeroenheijmans]

With "the documentation" you mean above-linked README, right? I think it might technically suggest that it's an "either / or" choice, but the way it says so might not be immediately obvious especially to non-native speakers:

After this, you can initialize the code flow using:

this.oauthService.initCodeFlow();

There is also a convenience method initLoginFlow which initializes either the code flow or the implicit flow depending on your configuration.

this.oauthService.initLoginFlow();

The "there is also" part implies that it's an alternative.

It might be good to clarify that point?

[amchavan]

It's pretty clear (to me, at least) that initLoginFlow() is an alternative to initCodeFlow().
But right after that, the README continues:

Also -- as shown in the readme -- you have to execute the following code...

...which (to me, at least) implies: "after you have initialized the code flow, you'll need to configure the OAuth2 client code and load the discovery document". That's what got me.

Minor note: it may be worth it to remove that "-- as shown in the readme --" fragment, since we are in the README file.

Minor suggestion: it may be worth it to combine configuration and initialization in a single method, something like

public configureAndInitLoginFlow( config: AuthConfig ) { ... }

[manfredsteyer]

will be fixed in next version.