diff --git a/.changeset/tame-kings-eat.md b/.changeset/tame-kings-eat.md new file mode 100644 index 0000000000..74dfc25f3d --- /dev/null +++ b/.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 diff --git a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.test.ts b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.test.ts index 9afc710bdd..d5e72a2365 100644 --- a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.test.ts +++ b/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); + }); }); diff --git a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts index 216409a528..fbb57b4a5e 100644 --- a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts +++ b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts @@ -64,6 +64,7 @@ export class RefreshingAuthSessionManager implements SessionManager { } async getSession(options: GetSessionOptions): Promise { + let alreadyTriedToRefreshSession = false; if ( this.helper.sessionExistsAndHasScope(this.currentSession, options.scopes) ) { @@ -72,6 +73,8 @@ export class RefreshingAuthSessionManager implements SessionManager { return this.currentSession!; } + alreadyTriedToRefreshSession = true; + try { const refreshedSession = await this.collapsedSessionRefresh( options.scopes, @@ -83,27 +86,31 @@ export class RefreshingAuthSessionManager implements SessionManager { } 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 implements SessionManager { 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();