UserManager signoutRedirect not working

[ch-lepp]

To logout I use the signoutRedirect method of the UserManager.
That worked perfectly fine with version 2.4.0.

With version 3.0.0 the user is no longer properly logged out.

oidc-client-ts correctly redirects the user agent to the auth server with id_token_hint and post_logout_redirect_uri.
The auth server performs the logout as usual and redirects the user agent back to the application.

The difference compared to version 2.4.0 is, that the UserManager still holds a user object, accesible by the getUser method.

Causing the issue should be the removal of this line in the _signoutStart method.

Is this a bug or should I change some things in my application?

[ch-lepp]

The migration guide is not mentioning this at all btw.
It probably should...

[pamapa]

Can you verify if that is caused by #1342?

[ch-lepp]

Yes, reverting the mentioned PR solves my issue.

I think the underlying problem is the following:

I have not implemented the logout callback functionality in my application.
I do have a dedicated endpoint for login and refresh callbacks, because here the application actually has stuff to do (e.g. changing an auth code for an access token).
But after a logout I assumed no special actions need to be taken and therefor I simply redirect to the applications main page.

So the signoutRedirectCallback method as well as the _signoutEnd method never get called...

[ch-lepp]

I have to add, that I originally come from the oidc-client-js lib.
Here the signoutCallback was merely a logger.

So there was actually no real reason to use it.

Same goes for the oidc-client-ts library before the last major update.
In Version 2.4.0 the signoutCallback seems to have had a simple logging purpose...

[pamapa]

I see multiple solutions:

1. reverting Fix too early UserManager.events().unload() event before user is actually signed out #1342

• PRO: simple
• NEG: raise condition of events, but we had them since ever

2. when _signoutStart and _signoutEnd are called in sequence (without callback) we defer the removeUser else keep as before Fix too early UserManager.events().unload() event before user is actually signed out #1342

• PRO: probably keeps the event raise condition fixed and does not depend if callback is not called. To be checked first.
• NEG: remove user if called differently, once in _signoutStart else in _signoutEnd

3. keep as we have it

• PRO: simple
• NEG: if no callback or for whatever reason the call back is not called, the user remains and not event is raised

Any other solutions?

@PSanetra Would version 2. work for you?

I am thinking about reverting, because its dead simple and makes sure that the user is removed and the event is sent at any situation. Maybe version 2. would be a compromise. I no longer like version 3. though...

[PSanetra]

@pamapa Please do not revert that PR.

I am not sure what the issue is. Is there a case where _signoutEnd is never called? This seems to be generally wrong 😕

Is the client not required to call _signoutEnd somehow?

Even if there are existing clients which do not call UserManager.signoutCallback or UserManager.signoutRedirectCallback: Is it not better to fix the clients which miss this callback? Otherwise it is probably better to remove this callback handler completely?!

I would be ok to introduce a flag to the UserManager to configure if such a callback is supported by the client or not. If it is supported, the behavior, introduced by the PR should be maintained, otherwise I would be fine to fall back to the pre-PR behavior, which is prone to race conditions.

[pamapa]

@ch-lepp I guess you can easily implement that callback, _signoutEnd must really be called otherwise we have too many different code paths, thus makes everything more easy. We can adapt the migration notice for others...

[ch-lepp]

Yes, I could use the callback.
That indeed fixes the issue for me.

Thank you for your support.

[pamapa]

i have improved the migration document

[pamapa]

I am not so sure if the callback is called by all IDPs (e.g. Microsoft Entra ID) Callback comes when you have a valid session.

[pamapa]

@PSanetra What concerns me is that when for whatever reason the callback is not called by the IDP (e.g. no valid session), the client stucks in a non fixable state (the user is still stored locally and will not be removed due to the not coming callback). In order to address that we could remove user in _signoutStart, but defer the notification into the callback handling _signoutEnd`.

[PSanetra]

@pamapa I think defering the notification would also be ok. Our original issue was only caused by a race condition between that event notification handling and the actual logout. As far as I know our application does not directly depend on that internal persistent state.

Regarding the local storage state: I did not look deep into the reason why this state is necessary. Might it be possible to completely remove this persistent state? Usually the login flow will anyway work also without such a state, right? Maybe I am missing something, but I think it might be possible to remove the persistent state.

[pamapa]

I think defering the notification would also be ok. Our original issue was only caused by a race condition between that event notification handling and the actual logout. As far as I know our application does not directly depend on that internal persistent state.

So tricky, what if we only defer the notify: user is removed (good), but rest of application is not notified -> not good.

Regarding the local storage state:

Can you move that into a new issue, it is better to track it separately...

[pamapa]

I do not want to enforce the users of this library to implement a callback by default. I am going to revert to the previous 100% reliable behavior.

[WtfJoke]

Thanks for the fast release :) I was searching for an issue over at https://github.com/authts/react-oidc-context