Merge pull request #27905 from gusega/master

create new session if session exists and refresh fails

.changeset/tame-kings-eat.md

@@ -0,0 +1,5 @@
+---
+'@backstage/core-app-api': minor
+---
+
+if session exists and refresh fails, then create a new session if not instant popup

packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.test.ts

@@ -138,7 +138,7 @@ describe('RefreshingAuthSessionManager', () => {
     expect(refreshSession).toHaveBeenCalledWith({ scopes: new Set() });
   });
 
-  it('should forward option to instantly show auth popup and not attempt refresh', async () => {
+  it('should forward option to instantly show auth popup after attempting refresh', async () => {
     const createSession = jest.fn();
     const refreshSession = jest.fn().mockRejectedValue(new Error('NOPE'));
     const manager = new RefreshingAuthSessionManager({
@@ -152,7 +152,7 @@ describe('RefreshingAuthSessionManager', () => {
       scopes: new Set(),
       instantPopup: true,
     });
-    expect(refreshSession).toHaveBeenCalledTimes(0);
+    expect(refreshSession).toHaveBeenCalledTimes(1);
   });
 
   it('should remove session straight away', async () => {
@@ -275,4 +275,26 @@ describe('RefreshingAuthSessionManager', () => {
     expect(refreshSession).toHaveBeenCalledTimes(1);
     expect(createSession).toHaveBeenCalledTimes(1);
   });
+
+  it('should create a new session if refresh fails with existing expired session', async () => {
+    const createSession = jest.fn();
+    const refreshSession = jest.fn().mockRejectedValue(new Error('NOPE'));
+    const manager = new RefreshingAuthSessionManager({
+      connector: { createSession, refreshSession },
+      ...defaultOptions,
+    } as any);
+
+    createSession.mockResolvedValue({
+      scopes: new Set(['a']),
+      expired: true,
+    });
+    await manager.getSession({ scopes: new Set(['a']) });
+    expect(refreshSession).toHaveBeenCalledTimes(1);
+    expect(createSession).toHaveBeenCalledTimes(1);
+
+    await manager.getSession({ scopes: new Set(['a']) });
+    // call refresh session only once
+    expect(refreshSession).toHaveBeenCalledTimes(2);
+    expect(createSession).toHaveBeenCalledTimes(2);
+  });
 });

packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts

@@ -64,6 +64,7 @@ export class RefreshingAuthSessionManager<T> implements SessionManager<T> {
   }
 
   async getSession(options: GetSessionOptions): Promise<T | undefined> {
+    let alreadyTriedToRefreshSession = false;
     if (
       this.helper.sessionExistsAndHasScope(this.currentSession, options.scopes)
     ) {
@@ -72,6 +73,8 @@ export class RefreshingAuthSessionManager<T> implements SessionManager<T> {
         return this.currentSession!;
       }
 
+      alreadyTriedToRefreshSession = true;
+
       try {
         const refreshedSession = await this.collapsedSessionRefresh(
           options.scopes,
@@ -83,27 +86,31 @@ export class RefreshingAuthSessionManager<T> implements SessionManager<T> {
         }
         return refreshedSession;
       } catch (error) {
+        this.removeLocalSession();
+
         if (options.optional) {
           return undefined;
         }
-        throw error;
+        // If the refresh attempt fails we assume we don't have a session, so continue to create one
       }
     }
 
     // The user may still have a valid refresh token in their cookies. Attempt to
     // initiate a fresh session through the backend using that refresh token.
     //
-    // We skip this check if an instant login popup is requested, as we need to
-    // stay in a synchronous call stack from the user interaction. The downside
-    // is that the user will sometimes be requested to log in even if they
-    // already had an existing session.
-    if (!options.instantPopup) {
+    // We can still try to refresh even if client requested instant popup.
+    // With instant popup option, the client is responsible for providing the user login prompt modal window.
+    // If control flow executes this code and client requested instant popup, it means that
+    // must have clicked sign in on the login prompt. The browser allows asynchronous code to open a popup
+    // if it is caused by a user interaction, clicking on a sign-in button, for example.
+    if (!alreadyTriedToRefreshSession) {
       try {
         const newSession = await this.collapsedSessionRefresh(options.scopes);
         this.currentSession = newSession;
         // The session might not have the scopes requested so go back and check again
         return this.getSession(options);
       } catch {
+        this.removeLocalSession();
         // If the refresh attempt fails we assume we don't have a session, so continue to create one.
       }
     }
@@ -122,6 +129,18 @@ export class RefreshingAuthSessionManager<T> implements SessionManager<T> {
     return this.currentSession;
   }
 
+  /**
+   * Sets `undefined` to this.{@link currentSession} and tells this.{@link stateTracker}, session state tracker,
+   * that a user has signed out.
+   *
+   * Does not propagate session removal to the connector like {@link removeSession}().
+   *
+   */
+  removeLocalSession() {
+    this.currentSession = undefined;
+    this.stateTracker.setIsSignedIn(false);
+  }
+
   async removeSession() {
     this.currentSession = undefined;
     await this.connector.removeSession();