core-app-api: try to refresh session if scopes are missing

Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>

.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.

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<string>) => ({
+      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<string>) => ({
+      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<string>) => ({
+      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);
+  });
 });

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

@@ -97,7 +97,7 @@ export class RefreshingAuthSessionManager<T> implements SessionManager<T> {
     // 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<T> implements SessionManager<T> {
 
     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 {