Angular-auth-oidc-client: Suggestion: Error handling, events and code cleanup/refactors

[fdcastel]

I was testing my app against a down STS server and I did notice the error handling for OidcSecurityService is lacking. Some considerations:

• OidcSecurityService.authorize() doesn't provide any way for the caller to know if something wrong occurred. It has no return values and doesn't throw any exceptions.
  
  
  
  • The only thing it does in case of error is to call oidcSecurityCommon.logError().
  
  
• OidcSecurityService.handleError() is never called.
  
  
  
  • Due to this, the authConfiguration.forbidden_route setting appears to be never used.
  
  
• OidcSecurityService.runGetSigningKeys() is never called.
  
  
  
  • Due to this, OidcSecurityService.handleErrorGetSigningKeys() is also never used.
  
  
• In several places we have the following pattern:
  javascript if (this.authConfiguration.trigger_authorization_result_event) { this.onAuthorizationResult.emit(AuthorizationResult.authorized); } else { this.router.navigate([this.authConfiguration.login_route]); }
  which is somewhat redundant, since the act of .navigate() to a certain route could be dispatched from a onAuthorizationResult (or similar) subscription.

Please take the above points with a grain of salt, since I'm very new to this code base and maybe I'm missing something. In this case, please enlighten me.

Suggestions

To minimize breaking changes in the current code I propose the creation of a .events() property (very similar to Angular Router Events).

Suggested events:

• AuthenticationStart, AuthenticationSuccess, AuthenticationError, AuthenticationRefresh, AuthenticationLogout (main events)
  
  
  
  • FetchConfigurationStart, FetchConfigurationSuccess, FetchConfigurationError
    
    
    
    
    
    • maps getWellKnownEndpoints()
    
    
    
  
  
  • FetchKeysStart, FetchKeysSuccess, FetchKeysError
    
    
    
    
    
    • maps getSigningKeys()
    
    
    
  
  
  • FetchUserInfoStart, FetchUserInfoSuccess, FetchUserInfoError
    
    
    
    
    
    • maps getIdentityUserData()
    
    
    
  
  
  • RefreshTokenStart, RefreshTokenSuccess, RefreshTokenError
    
    
    
    
    
    • maps the timer from runTokenValidatation()
    
    
    
  
  

This would be a central point for all event-related notifications and the user could subscribe to it to handle whatever he/she wishes. Initially this would be given just as an _additional_ way of reacting to events (i.e. it would not change any of current events/subscriptions). This way it would not require any breaking changes to the existing code (neither introduce any new bugs 😉 ).

Then, with these events in place, we could port independent parts of code (e.g. logging, route navigation, core functionality) from the current state to _subscriptions_ of these events.

IMO this could bring large benefits to the code readability and maintenance.

And, of course, I'm willing to make PRs for all this, if agreed. 😄

Best regards!

[damienbod]

@fdcastel I plan to go through this at the weekend, I have a very busy week. I'll merge the first PR as well.

Sorry for the slow answer and thanks for this.

Greetings Damien

[fdcastel]

I updated the original message with new suggested events (adds AuthenticationRefresh, AuthenticationLogout and RefreshToken* events).

Also renamed *End to *Success events.

[damienbod]

https://github.com/damienbod/angular-auth-oidc-client/issues/79

[fdcastel]

@damienbod May I make an attempt on this one or are you looking for to work on it?

[damienbod]

@fdcastel Tip top thanks, I don't have the time at the moment. We already have some events and only used the routes when configured.

If we keep the PRs small, it will make it easier to test

Greetings and thanks Damien

[fdcastel]

@damienbod What about

let parentdoc = window.parent && window.parent.document;
let existsparent = parentdoc && parentdoc.getElementById('myiFrameForSilentRenew');
let exists = window.document.getElementById('myiFrameForSilentRenew');

this.sessionIframe = existsparent || exists;
if (!this.sessionIframe) {
    throw new Error('Session IFRAME not found.');
}

this.oidcSecurityCommon.logDebug('startRenew for URL:' + url);
this.sessionIframe.src = url;

instead of

let existsparent = undefined;
try {
  let parentdoc = window.parent.document;
  if (!parentdoc) {
      throw new Error('Unaccessible');
  }

  existsparent =  parentdoc.getElementById('myiFrameForSilentRenew');
} catch (e) {
    // not accessible
}
let exists = window.document.getElementById('myiFrameForSilentRenew');
if (existsparent) {
    this.sessionIframe = existsparent;
} else if (exists) {
    this.sessionIframe = exists;
}

this.oidcSecurityCommon.logDebug('startRenew for URL:' + url);
this.sessionIframe.src = url;

Please advise. There is a reason for that try .. catch to be (other than to guard against null/undefined object references)?

[damienbod]

closing this, please re-open is required