From ab9a6fb3215c623efd5e556a18e2a5be288c0344 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 12 Feb 2025 12:13:12 +0100 Subject: [PATCH 1/2] auth-node: add scopeAlreadyGranted field Signed-off-by: Patrik Oldsberg --- .changeset/stale-eagles-cheer.md | 5 ++ plugins/auth-node/report.api.md | 1 + .../src/oauth/CookieScopeManager.test.ts | 49 +++++++++++++++++++ .../auth-node/src/oauth/CookieScopeManager.ts | 17 +++++++ .../oauth/createOAuthRouteHandlers.test.ts | 1 + .../src/oauth/createOAuthRouteHandlers.ts | 7 ++- plugins/auth-node/src/oauth/types.ts | 5 ++ 7 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 .changeset/stale-eagles-cheer.md diff --git a/.changeset/stale-eagles-cheer.md b/.changeset/stale-eagles-cheer.md new file mode 100644 index 0000000000..0f8d0970ed --- /dev/null +++ b/.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. diff --git a/plugins/auth-node/report.api.md b/plugins/auth-node/report.api.md index e5f33b4e7e..3b467c077f 100644 --- a/plugins/auth-node/report.api.md +++ b/plugins/auth-node/report.api.md @@ -326,6 +326,7 @@ export interface OAuthAuthenticatorRefreshInput { req: Request_2; // (undocumented) scope: string; + scopeAlreadyGranted?: boolean; } // @public (undocumented) diff --git a/plugins/auth-node/src/oauth/CookieScopeManager.test.ts b/plugins/auth-node/src/oauth/CookieScopeManager.test.ts index 6cdee1168f..bc8e7a233a 100644 --- a/plugins/auth-node/src/oauth/CookieScopeManager.test.ts +++ b/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, + 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'], diff --git a/plugins/auth-node/src/oauth/CookieScopeManager.ts b/plugins/auth-node/src/oauth/CookieScopeManager.ts index 3cbf977e09..d2db6d20a9 100644 --- a/plugins/auth-node/src/oauth/CookieScopeManager.ts +++ 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): Promise; }> { 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, + requestedScope: string, +): boolean { + const granted = new Set(grantedScope); + for (const requested of splitScope(requestedScope)) { + if (!granted.has(requested)) { + return false; + } + } + return true; +} diff --git a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts index 5bc9fa2551..a943717553 100644 --- a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts +++ b/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' }, ); diff --git a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts index 72f45d8ab1..3849bc5f9e 100644 --- a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts +++ b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts @@ -315,7 +315,12 @@ export function createOAuthRouteHandlers( 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, ); diff --git a/plugins/auth-node/src/oauth/types.ts b/plugins/auth-node/src/oauth/types.ts index c4ae2ee1cf..50d81f99dc 100644 --- a/plugins/auth-node/src/oauth/types.ts +++ b/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; From b40af03894473e4a4a6687f9499a9b1ca3df19a6 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 12 Feb 2025 12:18:22 +0100 Subject: [PATCH 2/2] auth-backend-module-github-provider: reject refresh for sessions without granted scope Signed-off-by: Patrik Oldsberg --- .changeset/chilled-kings-fold.md | 7 ++++++ .../src/authenticator.test.ts | 24 ++++++++++++++++--- .../src/authenticator.ts | 7 +++++- 3 files changed, 34 insertions(+), 4 deletions(-) create mode 100644 .changeset/chilled-kings-fold.md diff --git a/.changeset/chilled-kings-fold.md b/.changeset/chilled-kings-fold.md new file mode 100644 index 0000000000..4f518e6ba3 --- /dev/null +++ b/.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. diff --git a/plugins/auth-backend-module-github-provider/src/authenticator.test.ts b/plugins/auth-backend-module-github-provider/src/authenticator.test.ts index dc5c7208ea..48a297fa41 100644 --- a/plugins/auth-backend-module-github-provider/src/authenticator.test.ts +++ b/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( diff --git a/plugins/auth-backend-module-github-provider/src/authenticator.ts b/plugins/auth-backend-module-github-provider/src/authenticator.ts index 7c94a04c30..e9dc29e4a6 100644 --- a/plugins/auth-backend-module-github-provider/src/authenticator.ts +++ b/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