From e5fa0184dca40346319731978fe06e236154d734 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 22 Nov 2024 13:17:00 +0100 Subject: [PATCH] core-app-api: try to refresh session if scopes are missing Signed-off-by: Patrik Oldsberg --- .changeset/dry-needles-taste.md | 6 + .../RefreshingAuthSessionManager.test.ts | 103 ++++++++++++++++++ .../RefreshingAuthSessionManager.ts | 7 +- 3 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 .changeset/dry-needles-taste.md diff --git a/.changeset/dry-needles-taste.md b/.changeset/dry-needles-taste.md new file mode 100644 index 0000000000..472bc0edb3 --- /dev/null +++ b/.changeset/dry-needles-taste.md @@ -0,0 +1,6 @@ +--- +'@backstage/core-app-api': patch +'@backstage/plugin-app': patch +--- + +The OAuth 2 client implementations will now attempt to refresh the session when the existing session doesn't have the required scopes. The previous behavior was to only try to refresh the session of it was missing, and otherwise directly request a new session. This fixes an issue where some auth providers will not return access tokens with certain scopes unless explicitly requested, leading to an auth popup even if the underlying session already had been granted the requested scopes. 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 e26934f393..88000638e6 100644 --- a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.test.ts +++ b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.test.ts @@ -165,4 +165,107 @@ describe('RefreshingAuthSessionManager', () => { expect(removeSession).toHaveBeenCalled(); expect(await manager.getSession({ optional: true })).toBe(undefined); }); + + it('should handle two simultaneous session refreshes with same scopes', async () => { + const createSession = jest.fn(); + const refreshSession = jest.fn(async (scopes?: Set) => ({ + scopes: scopes ?? new Set(), + expired: false, + })); + const manager = new RefreshingAuthSessionManager({ + connector: { createSession, refreshSession }, + ...defaultOptions, + } as any); + + const sessionPromise1 = manager.getSession({ scopes: new Set(['a']) }); + const sessionPromise2 = manager.getSession({ scopes: new Set(['a']) }); + + const [session1, session2] = await Promise.all([ + sessionPromise1, + sessionPromise2, + ]); + + expect(session1).toEqual({ scopes: new Set(['a']), expired: false }); + expect(session2).toEqual({ scopes: new Set(['a']), expired: false }); + expect(refreshSession).toHaveBeenCalledTimes(1); + }); + + it('should handle two simultaneous session refreshes with different scopes', async () => { + const createSession = jest.fn(); + const refreshSession = jest.fn(async (scopes?: Set) => ({ + scopes: scopes ?? new Set(), + expired: false, + })); + const manager = new RefreshingAuthSessionManager({ + connector: { createSession, refreshSession }, + ...defaultOptions, + } as any); + + const sessionPromise1 = manager.getSession({ scopes: new Set(['a']) }); + const sessionPromise2 = manager.getSession({ scopes: new Set(['b']) }); + + const [session1, session2] = await Promise.all([ + sessionPromise1, + sessionPromise2, + ]); + + expect(session1).toEqual({ scopes: new Set(['a']), expired: false }); + expect(session2).toEqual({ scopes: new Set(['a', 'b']), expired: false }); + expect(refreshSession).toHaveBeenCalledTimes(2); + }); + + it('should handle multiple simultaneous session refreshes with different scopes', async () => { + const createSession = jest.fn(); + const refreshSession = jest.fn(async (scopes?: Set) => ({ + scopes: scopes ?? new Set(), + expired: false, + })); + const manager = new RefreshingAuthSessionManager({ + connector: { createSession, refreshSession }, + ...defaultOptions, + } as any); + + const sessionPromise1 = manager.getSession({ scopes: new Set(['a']) }); + const sessionPromise2 = manager.getSession({ scopes: new Set(['a', 'b']) }); + const sessionPromise3 = manager.getSession({ scopes: new Set(['b', 'c']) }); + const sessionPromise4 = manager.getSession({ scopes: new Set(['a', 'c']) }); + + const [session1, session2, session3, session4] = await Promise.all([ + sessionPromise1, + sessionPromise2, + sessionPromise3, + sessionPromise4, + ]); + + expect(session1).toEqual({ scopes: new Set(['a']), expired: false }); + expect(session2).toEqual({ scopes: new Set(['a', 'b']), expired: false }); + expect(session3).toEqual({ + scopes: new Set(['a', 'b', 'c']), + expired: false, + }); + expect(session4).toEqual({ + scopes: new Set(['a', 'b', 'c']), + expired: false, + }); + expect(refreshSession).toHaveBeenCalledTimes(3); + }); + + it("should fall back to create a new session if refresh doesn't provide the correct scopes", async () => { + const createSession = jest + .fn() + .mockResolvedValue({ scopes: new Set(['c']), expired: false }); + const refreshSession = jest + .fn() + .mockResolvedValue({ scopes: new Set(['b']), expired: false }); + const manager = new RefreshingAuthSessionManager({ + connector: { createSession, refreshSession }, + ...defaultOptions, + } as any); + + const session = await manager.getSession({ scopes: new Set(['a']) }); + + expect(session).toEqual({ scopes: new Set(['c']), expired: false }); + expect(refreshSession).toHaveBeenCalledTimes(1); + expect(createSession).toHaveBeenCalledTimes(1); + }); }); diff --git a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts index c92fe9fb20..414743ba9d 100644 --- a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts +++ b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts @@ -97,7 +97,7 @@ export class RefreshingAuthSessionManager implements SessionManager { // 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 (!this.currentSession && !options.instantPopup) { + if (!options.instantPopup) { try { const newSession = await this.collapsedSessionRefresh(options.scopes); this.currentSession = newSession; @@ -143,6 +143,11 @@ export class RefreshingAuthSessionManager implements SessionManager { try { const session = await this.refreshPromise; + if (!this.helper.sessionExistsAndHasScope(session, scopes)) { + throw new Error( + 'Refreshed session did not receive the required scopes', + ); + } this.stateTracker.setIsSignedIn(true); return session; } finally {