UserManager.events().unload() event is triggered too early on UserManager.signoutRedirect()

[PSanetra]

We observed the UserManager.events().unload() event was fired too early, before the user was actually signed out. Our application reacted with a redirect to the login page to this event, but as there was the possibilty of a race condition between login redirect and actual signout, the signout might not yet have happened and therefore resulted in an unsuccessful signout.

The problematic code is located in UserManager._signoutStart(), which is called from UserManager.signoutRedirect():

oidc-client-ts/src/UserManager.ts

Lines 623 to 624 in 4a8d6d9

	await this.removeUser();
	logger.debug("user removed, creating signout request");

I would expect this code instead to be located in UserManager._signoutEnd() where it should be guaranteed that the user was successfully signed out.

I will create a PR to fix this issue.

[pamapa]

I see your point, makes sense, thanks for taking care...

[pamapa]

We observed the UserManager.events().unload() event was fired too early, before the user was actually signed out. Our application reacted with a redirect to the login page to this event, but as there was the possibilty of a race condition between login redirect and actual signout, the signout might not yet have happened and therefore resulted in an unsuccessful signout.

If you would not react with redirect, you would not have that issue at all. Why do you react with a redirect to login page with UserManager.events().unload() at all? For the normal signout you could do your redirect in the signout callback...

[PSanetra]

Why do you react with a redirect to login page

As far I know I need to react to the user unload event also for other reasons like session timeout which might lead to a user unload event. In all these cases I want to show the login page to the user as the actual application is only intended for users which are logged in.

[PSanetra]

@pamapa Actually our application does not depend directly on that event, but it depends on reacting to AuthState.isAuthenticated, which is returned from the useAuth() hook in the react-oidc-context which is also maintained by the authts organization:
https://github.com/authts/react-oidc-context/blob/4deb6591910ef9bed996ea4cf0faa37d579305f5/src/reducer.ts#L28-L33

How do you suggest to react to a change of the isAuthenticated state when the user is actually still authenticated?

[pamapa]

How do you suggest to react to a change of the isAuthenticated state when the user is actually still authenticated?

Locally he is no longer as we have remove the user object locally, but on the IDP the user still has for a very short time a valid session. But that session is now useless, as the local tokens are gone.

Can you post the code of what you are doing in the !isAuthenticated case?

[PSanetra]

@pamapa It looks like this:

  const { isAuthenticated, isLoading, signinRedirect } = useAuth();
  const login = () =>
    signinRedirect({
      scope: 'openid',
      redirect_uri: myRedirectUri
    });

  // automatically sign-in
  React.useEffect(() => {
    if (!isAuthenticated && !isLoading) {
      void login();
    }
  }, [isAuthenticated, isLoading]);

(I removed a little clutter, but this is generally the logic.)

The same code is also triggered when the user opens the page but is not yet logged in.

[pamapa]

Have you see https://github.com/authts/react-oidc-context?tab=readme-ov-file#automatic-sign-in, there is more to add...

[PSanetra]

@pamapa I think essentially we are doing exactly that what is suggested in that section. Do you see any major differences?

[pamapa]

In your code it do not see checking for active navigator: auth.activeNavigator and retry preventing...

const auth = useAuth();
    const [hasTriedSignin, setHasTriedSignin] = React.useState(false);

    // automatically sign-in
    React.useEffect(() => {
        if (!hasAuthParams() &&
            !auth.isAuthenticated && !auth.activeNavigator && !auth.isLoading &&
            !hasTriedSignin
        ) {
            auth.signinRedirect();
            setHasTriedSignin(true);
        }
    }, [auth, hasTriedSignin]);
```

[PSanetra]

Yes sorry. That activeNavigator was there in the actual code, but I removed it here as I thought it was not important for this case.

Regarding retry preventing: I am not sure why it would make a difference as the issue is that signinRedirect() is getting called too early, but the only thing hasTriedSignin does, is to prevent to calling signingRedirect() a second time after it has already been called. In my case the first time is already a problem.

[PSanetra]

fyi: For us using react-oidc-context the bug seems to be only reproducible when signoutRedirect() is called from a react useEffect() context. When signoutRedirect() is called from a click-event-handler, the bug does not seem to be reproducible. I think in the click-event-handler case the bug is just not triggered, because of some react internals. (Maybe rerenderings are generally deferred in click-event-handler contexts.)

[pamapa]

yi: For us using react-oidc-context the bug seems to be only reproducible when signoutRedirect() is called from a react useEffect() context. When signoutRedirect() is called from a click-event-handler, the bug does not seem to be reproducible. I think in the click-event-handler case the bug is just not triggered, because of some react internals. (Maybe rerenderings are generally deferred in click-event-handler contexts.)

Good to hear, i just temporary added automatically sign-in to one of my apps and it works too (called from on-click handler).
Also notice that you can defer in react operations with queueMicrotask or force immediate execution with ReactDOM.flushSync.
I will add a notice in react-oidc-context...

[vxsx]

For us this bug started to happen when we started to manually pass UserManager instance to the react-oidc-context's AuthProvider and using the same instance to signoutRedirect in the onClick handler (before that since the onClick triggered a redux action we had to create a new instance of user manager with same config passed to AuthProvider - and signoutRedirect from there.

It seems like it did work because a different UserManager instance was removing the user and not the one used in the react component (i'm guessing)

We were using roughly the same code as provided by react-oidc-context readme

We currently implemented a workaround that checks if we started the signout or not, but i would've expected for activeNavigator to already be 'signoutRedirect' when the user is removed and isAuthenticated set to false

  const auth = useAuth();
  const isLogginOut = useIsLoggingOut(); // exact details aren't important here

  useEffect(() => {
    if (
      !hasAuthParams() &&
      !auth.isAuthenticated &&
      !auth.activeNavigator &&
      !auth.isLoading &&
      !auth.error
    ) {
        // if we don't check here for logging out this triggers immediately after logout button click
        if (!isLogginOut) {
          auth.signinRedirect({
            redirect_uri: `${window.location.href}`,
          });
        }
      }
    }
  }, [
    auth,
    auth.isAuthenticated,
    auth.activeNavigator,
    auth.isLoading,
    auth.signinRedirect,
    auth.error,
    isLogginOut,
  ]);