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

Inconsistent behavior of OidcSecurityService.userData$ Observable, if autoUserinfo is false #1006

Closed
stefanocke opened this issue Mar 10, 2021 · 9 comments · Fixed by #1008 or #1014
Closed
Labels

Comments

@stefanocke
Copy link

stefanocke commented Mar 10, 2021

Describe the bug
If autoUserinfo is true, everything is as excpected:

  • intially, after checkAuth(), the userData$ fires and contains the content from userinfo endpoint
  • if the user session has been terminated (outside of the apllication, for instance directly at the identity provider), the token refresh will fail with "login_required" and the userData$ will fire null
  • if the user session has been changed (outside of the application, for instance directly at the identity provider), and in the config renewUserInfoAfterTokenRenew is true, the userinfo endpoint will be asked after token refresh and the userData$ fires the new user info

If autoUserInfo is false, the behavior of userData$ Observable becomes inconsistent:

  • intially, after checkAuth(), the userData$ fires and contains the claims from the ID token
  • if the user session has been terminated (outside of the apllication, for instance directly at the identity provider), the token refresh will fail with "login_required" and userData$ does not fire at all
  • if the user session has been changed (outside of the application, for instance directly at the identity provider), userData$ does not fire at all after token refresh. This is independent renewUserInfoAfterTokenRenew.

To Reproduce
Steps to reproduce the behavior:

  • I used code flow with pkce and silentRenew for this.
  • Please vary autoUserinfo and renewUserInfoAfterTokenRenew to reproduce and observe OidcSecurityService.userData$

Expected behavior
If autoUserInfo is false, userData$ should fire in the same situations as it does with autoUserInfo = true:

  • OidcSecurityService.userData$ should fire null, if token refresh fails (for instance with "login_required" error)
  • OidcSecurityService.userData$ should fire after token refresh and use the data from the new ID token.

If you dont't agree with this, it should at least be documented, what behavior to expect from userData$, if autoUserInfo is false.

@damienbod
Copy link
Owner

Hi @stefanocke Thanks for the issue. Yes this needs to be improved. The event should fire after each refresh. Just need to think about the other times the event fires.

@damienbod
Copy link
Owner

@stefanocke thanks for reporting. Will release a fix in version 11.6.3 Event will fire now in a refresh if the autoUserInfo is false and the renewUserInfoAfterTokenRenew is true.

@stefanocke
Copy link
Author

stefanocke commented Mar 12, 2021

@damienbod , thanks a lot for looking into this issue. Have you also considered to fire null / call resetUserDataInStore in the error case (token refresh fails, for example with "login_required")?

@damienbod
Copy link
Owner

damienbod commented Mar 12, 2021

need to think about this as well. Not sure of how to solve this yet , trying to reproduce the null case

Will keep this open until the null issue is solved.

this issue is related

#972

Thanks

@damienbod
Copy link
Owner

@stefanocke I think this is fixed now with the release 11.6.3. Can you re-open if the null case can be re-reproduced?

I this this witht he httpconfig examples and small changes to this

Greetings Damein and thanks for the issue.

@stefanocke
Copy link
Author

@damienbod thank you again.
Unfortunately I cannot fully understand your last comment. Did you try to say that you could not reproduce the null case with httpconfig example?
If so, I will of course check again.

@damienbod
Copy link
Owner

Hi @stefanocke maybe it's just me not understanding :)

I odn't see what I should do here because when the login_error happens, the lib is usually setup to handle this and reset so the user data is also reset. I don't see why adding an extra event would help help.

Good/bad?

Greetings Damien

@damienbod damienbod reopened this Mar 12, 2021
@stefanocke
Copy link
Author

stefanocke commented Mar 12, 2021

Hi @damienbod , let me try to give you some more details, since I think there was something missing in my description:

At first, I think you cannot reproduce my case, since you have startCheckSession set tor true in your example. So, the session check might be the one, that resets the user. However: my auth server does not suppert OIDC Session Management Specification. So, in my case startCheckSession is false.
That means, in case of logout (in a different OAuth client or in the auth server), the session check will not kick in in my case. Instead, the user will be kept logged in until the next token refresh.
Then, the "login_error" happens.

And then, we have the inconsistency that I described:

  • With autoUserinfo = true, the userData$ observable fires the value null. The stacktrace of this is:
(anonymous) (app.component.ts:16)
__tryOrUnsub (Subscriber.js:183)
...
next (BehaviorSubject.js:30)
resetUserDataInStore (angular-auth-oidc-client.js:1621)
resetAuthorizationData (angular-auth-oidc-client.js:1691)
codeFlowCallbackSilentRenewIframe (angular-auth-oidc-client.js:2692)
silentRenewEventHandler (angular-auth-oidc-client.js:2726)

@damienbod
Copy link
Owner

Ah thanks, yes this is no longer required as the user data is always set now.

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