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 {