Repository: authts/oidc-client-ts (ts)

Issue:
`UserManager.events().unload()` event is triggered too early on `UserManager.signoutRedirect()`
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()`:

https://github.com/authts/oidc-client-ts/blob/4a8d6d922690077d4edbc86cfe1af363c7b9414c/src/UserManager.ts#L623-L624

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.

Pull request:
Fix too early `UserManager.events().unload()` event before user is actually signed out
This PR prevents race conditions with event handlers which react to this unload event and expect the user to be signed out already.

Fixes #1341


Patch:
diff --git a/.gitignore b/.gitignore
index b8db61b2..54d944ed 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,3 +1,4 @@
+.idea
 
 # build artifacts
 *.tsbuildinfo
diff --git a/package-lock.json b/package-lock.json
index ee6e1eba..1f914d2a 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1939,9 +1939,9 @@
       "peer": true
     },
     "node_modules/@types/node": {
-      "version": "20.11.2",
-      "resolved": "https://registry.npmjs.org/@types/node/-/node-20.11.2.tgz",
-      "integrity": "sha512-cZShBaVa+UO1LjWWBPmWRR4+/eY/JR/UIEcDlVsw3okjWEu+rB7/mH6X3B/L+qJVHDLjk9QW/y2upp9wp1yDXA==",
+      "version": "20.11.0",
+      "resolved": "https://registry.npmjs.org/@types/node/-/node-20.11.0.tgz",
+      "integrity": "sha512-o9bjXmDNcF7GbM4CNQpmi+TutCgap/K3w1JyKgxAjqx41zp9qlIAVFi0IhCNsJcXolEqLWhbFbEeL0PvYm4pcQ==",
       "dev": true,
       "dependencies": {
         "undici-types": "~5.26.4"
diff --git a/src/UserManager.ts b/src/UserManager.ts
index 81af6738..204b73c3 100644
--- a/src/UserManager.ts
+++ b/src/UserManager.ts
@@ -620,9 +620,6 @@ export class UserManager {
                 args.id_token_hint = id_token;
             }
 
-            await this.removeUser();
-            logger.debug("user removed, creating signout request");
-
             const signoutRequest = await this._client.createSignoutRequest(args);
             logger.debug("got signout request");
 
@@ -638,11 +635,15 @@ export class UserManager {
             throw err;
         }
     }
+
     protected async _signoutEnd(url: string): Promise<SignoutResponse> {
         const logger = this._logger.create("_signoutEnd");
         const signoutResponse = await this._client.processSignoutResponse(url);
         logger.debug("got signout response");
 
+        await this.removeUser();
+        logger.debug("user removed");
+
         return signoutResponse;
     }
 


Test patch:
diff --git a/src/UserManager.test.ts b/src/UserManager.test.ts
index a079a7c3..73d49c0d 100644
--- a/src/UserManager.test.ts
+++ b/src/UserManager.test.ts
@@ -2,7 +2,13 @@
 // Licensed under the Apache License, Version 2.0. See LICENSE in the project root for license information.
 
 import { once } from "events";
-import { RedirectNavigator, type PopupWindow, PopupNavigator, IFrameNavigator } from "./navigators";
+import {
+    RedirectNavigator,
+    type PopupWindow,
+    PopupNavigator,
+    IFrameNavigator,
+    type NavigateResponse,
+} from "./navigators";
 import type { SigninResponse } from "./SigninResponse";
 import type { SignoutResponse } from "./SignoutResponse";
 import { UserManager, type SigninPopupArgs, type SigninRedirectArgs, type SigninSilentArgs, type SignoutSilentArgs } from "./UserManager";
@@ -33,6 +39,7 @@ describe("UserManager", () => {
             userStore: userStoreMock,
             metadata: {
                 authorization_endpoint: "http://sts/oidc/authorize",
+                end_session_endpoint:  "http://sts/oidc/logout",
                 token_endpoint: "http://sts/oidc/token",
                 revocation_endpoint: "http://sts/oidc/revoke",
             },
@@ -838,6 +845,53 @@ describe("UserManager", () => {
         });
     });
 
+    describe("signoutRedirect", () => {
+        it("should not unload user to avoid race condition between actual signout and signout event handlers", async () => {
+            // arrange
+            const navigateMock = jest.fn().mockReturnValue(Promise.resolve({
+                url: "http://localhost:8080",
+            } as NavigateResponse));
+            jest.spyOn(subject["_redirectNavigator"], "prepare").mockReturnValue(Promise.resolve({
+                navigate: navigateMock,
+                close: () => {},
+            }));
+            const user = new User({
+                access_token: "access_token",
+                token_type: "token_type",
+                profile: {} as UserProfile,
+            });
+            await subject.storeUser(user);
+
+            // act
+            await subject.signoutRedirect();
+
+            // assert
+            expect(navigateMock).toHaveBeenCalledTimes(1);
+            const storageString = await subject.settings.userStore.get(subject["_userStoreKey"]);
+            expect(storageString).not.toBeNull();
+        });
+    });
+
+    describe("signoutRedirectCallback", () => {
+        it("should unload user", async () => {
+            // arrange
+            const user = new User({
+                access_token: "access_token",
+                token_type: "token_type",
+                profile: {} as UserProfile,
+            });
+            await subject.storeUser(user);
+
+            expect(await subject.settings.userStore.get(subject["_userStoreKey"])).not.toBeNull();
+
+            // act
+            await subject.signoutRedirectCallback();
+
+            // assert
+            expect(await subject.settings.userStore.get(subject["_userStoreKey"])).toBeNull();
+        });
+    });
+
     describe("storeUser", () => {
         it("should add user to store", async () => {
             // arrange