Merge pull request #28821 from backstage/rugvip/gh-auth-fix

auth: fixed refreshing of GitHub OAuth App sessions

.changeset/chilled-kings-fold.md

@@ -0,0 +1,7 @@
+---
+'@backstage/plugin-auth-backend-module-github-provider': patch
+---
+
+Fixed a bug where the requested scope was ignored when refreshing sessions for a GitHub OAuth App. This would lead to access tokens being returned that didn't have the requested scope, and in turn errors when trying to use these tokens.
+
+As part of this fix all existing sessions are being revoked in order to ensure that they receive the correct scope.

.changeset/stale-eagles-cheer.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-auth-node': patch
+---
+
+Added `scopeAlreadyGranted` property to `OAuthAuthenticatorRefreshInput`, signaling to the provider whether the requested scope has already been granted when persisting session scope.

plugins/auth-backend-module-github-provider/src/authenticator.test.ts

@@ -42,7 +42,7 @@ describe('githubAuthenticator', () => {
         accessToken: 'my-token',
         scope: 'user:read',
         tokenType: 'bearer',
-        refreshToken: 'access-token.my-token',
+        refreshToken: 'access-token-v2.my-token',
       },
     });
   });
@@ -105,9 +105,10 @@ describe('githubAuthenticator', () => {
     await expect(
       githubAuthenticator.refresh(
         {
-          refreshToken: 'access-token.my-token',
+          refreshToken: 'access-token-v2.my-token',
           req: {} as any,
           scope: 'user:read',
+          scopeAlreadyGranted: true,
         },
         {
           fetchProfile: async _input => ({ id: 'id' } as PassportProfile),
@@ -119,11 +120,28 @@ describe('githubAuthenticator', () => {
         accessToken: 'my-token',
         scope: 'user:read',
         tokenType: 'bearer',
-        refreshToken: 'access-token.my-token',
+        refreshToken: 'access-token-v2.my-token',
       },
     });
   });
 
+  it('should fail refresh if scope has not already been granted', async () => {
+    await expect(
+      githubAuthenticator.refresh(
+        {
+          refreshToken: 'access-token-v2.my-token',
+          req: {} as any,
+          scope: 'user:read',
+        },
+        {
+          fetchProfile: async _input => ({ id: 'id' } as PassportProfile),
+        } as PassportOAuthAuthenticatorHelper,
+      ),
+    ).rejects.toThrow(
+      'Refresh failed, session has not been granted the requested scope',
+    );
+  });
+
   it('should refresh with refresh token', async () => {
     const res = {};
     await expect(

plugins/auth-backend-module-github-provider/src/authenticator.ts

@@ -22,7 +22,7 @@ import {
   PassportProfile,
 } from '@backstage/plugin-auth-node';
 
-const ACCESS_TOKEN_PREFIX = 'access-token.';
+const ACCESS_TOKEN_PREFIX = 'access-token-v2.';
 
 /** @public */
 export const githubAuthenticator = createOAuthAuthenticator({
@@ -98,6 +98,11 @@ export const githubAuthenticator = createOAuthAuthenticator({
     // refresh token cookie. We use that token to fetch the user profile and
     // refresh the Backstage session when needed.
     if (input.refreshToken?.startsWith(ACCESS_TOKEN_PREFIX)) {
+      if (!input.scopeAlreadyGranted) {
+        throw new Error(
+          'Refresh failed, session has not been granted the requested scope',
+        );
+      }
       const accessToken = input.refreshToken.slice(ACCESS_TOKEN_PREFIX.length);
 
       const fullProfile = await helper

plugins/auth-node/report.api.md

@@ -326,6 +326,7 @@ export interface OAuthAuthenticatorRefreshInput {
   req: Request_2;
   // (undocumented)
   scope: string;
+  scopeAlreadyGranted?: boolean;
 }
 
 // @public (undocumented)

plugins/auth-node/src/oauth/CookieScopeManager.test.ts

@@ -193,6 +193,55 @@ describe('CookieScopeManager', () => {
     );
   });
 
+  it('should signal whether persisted scopes have already been granted when refreshing', async () => {
+    const getGrantedScopes = jest.fn();
+    const manager = CookieScopeManager.create({
+      authenticator: {
+        scopes: {
+          persist: true,
+        } as OAuthAuthenticatorScopeOptions,
+      } as OAuthAuthenticator<any, any>,
+      cookieManager: {
+        getGrantedScopes,
+      } as unknown as OAuthCookieManager,
+    });
+
+    getGrantedScopes.mockReturnValue('x y');
+    await expect(manager.refresh(makeReq('x,y'))).resolves.toEqual({
+      scope: 'x y',
+      scopeAlreadyGranted: true,
+      commit: expect.any(Function),
+    });
+
+    getGrantedScopes.mockReturnValueOnce('x y');
+    await expect(manager.refresh(makeReq('x'))).resolves.toEqual({
+      scope: 'x y',
+      scopeAlreadyGranted: true,
+      commit: expect.any(Function),
+    });
+
+    getGrantedScopes.mockReturnValueOnce('x y');
+    await expect(manager.refresh(makeReq('x,y,z'))).resolves.toEqual({
+      scope: 'x y z',
+      scopeAlreadyGranted: false,
+      commit: expect.any(Function),
+    });
+
+    getGrantedScopes.mockReturnValueOnce('');
+    await expect(manager.refresh(makeReq('x,y'))).resolves.toEqual({
+      scope: 'x y',
+      scopeAlreadyGranted: false,
+      commit: expect.any(Function),
+    });
+
+    getGrantedScopes.mockReturnValueOnce(undefined);
+    await expect(manager.refresh(makeReq('x,y'))).resolves.toEqual({
+      scope: 'x y',
+      scopeAlreadyGranted: false,
+      commit: expect.any(Function),
+    });
+  });
+
   it('should use custom scope transform', async () => {
     const manager = CookieScopeManager.create({
       additionalScopes: ['b'],

plugins/auth-node/src/oauth/CookieScopeManager.ts

@@ -138,6 +138,7 @@ export class CookieScopeManager {
 
   async refresh(req: express.Request): Promise<{
     scope: string;
+    scopeAlreadyGranted?: boolean;
     commit(result: OAuthAuthenticatorResult<any>): Promise<string>;
   }> {
     const requestScope = splitScope(req.query.scope?.toString());
@@ -147,6 +148,9 @@ export class CookieScopeManager {
 
     return {
       scope,
+      scopeAlreadyGranted: this.cookieManager
+        ? hasScopeBeenGranted(grantedScope, scope)
+        : undefined,
       commit: async result => {
         if (this.cookieManager) {
           this.cookieManager.setGrantedScopes(
@@ -162,3 +166,16 @@ export class CookieScopeManager {
     };
   }
 }
+
+function hasScopeBeenGranted(
+  grantedScope: Iterable<string>,
+  requestedScope: string,
+): boolean {
+  const granted = new Set(grantedScope);
+  for (const requested of splitScope(requestedScope)) {
+    if (!granted.has(requested)) {
+      return false;
+    }
+  }
+  return true;
+}

plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts

@@ -720,6 +720,7 @@ describe('createOAuthRouteHandlers', () => {
           req: expect.anything(),
           refreshToken: 'refresh-token',
           scope: 'persisted-scope',
+          scopeAlreadyGranted: true,
         },
         { ctx: 'authenticator' },
       );

plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts

@@ -315,7 +315,12 @@ export function createOAuthRouteHandlers<TProfile>(
         const scopeRefresh = await scopeManager.refresh(req);
 
         const result = await authenticator.refresh(
-          { req, scope: scopeRefresh.scope, refreshToken },
+          {
+            req,
+            scope: scopeRefresh.scope,
+            scopeAlreadyGranted: scopeRefresh.scopeAlreadyGranted,
+            refreshToken,
+          },
           authenticatorCtx,
         );
 

plugins/auth-node/src/oauth/types.ts

@@ -59,6 +59,11 @@ export interface OAuthAuthenticatorAuthenticateInput {
 
 /** @public */
 export interface OAuthAuthenticatorRefreshInput {
+  /**
+   * Signals whether the requested scope has already been granted for the session. Will only be set if the `scopes.persist` option is enabled.
+   */
+  scopeAlreadyGranted?: boolean;
+
   scope: string;
   refreshToken: string;
   req: Request;