From 9d75a939b63ad53f55677ea91be50b8d35530280 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 1 Feb 2022 18:40:51 +0100 Subject: [PATCH] auth-backend: fix OAuthAdapter scope store, only storing on success Signed-off-by: Patrik Oldsberg --- .changeset/chilled-papayas-wonder.md | 5 + plugins/auth-backend/api-report.md | 1 + .../src/lib/oauth/OAuthAdapter.test.ts | 98 ++++++++++++++++++- .../src/lib/oauth/OAuthAdapter.ts | 76 +++++++------- plugins/auth-backend/src/lib/oauth/types.ts | 1 + 5 files changed, 141 insertions(+), 40 deletions(-) create mode 100644 .changeset/chilled-papayas-wonder.md diff --git a/.changeset/chilled-papayas-wonder.md b/.changeset/chilled-papayas-wonder.md new file mode 100644 index 0000000000..4a3c2322e1 --- /dev/null +++ b/.changeset/chilled-papayas-wonder.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-auth-backend': patch +--- + +Fixed a bug where providers that tracked the granted scopes through a cookie would not take failed authentication attempts into account. diff --git a/plugins/auth-backend/api-report.md b/plugins/auth-backend/api-report.md index 7cdc33f0e2..b6c484e066 100644 --- a/plugins/auth-backend/api-report.md +++ b/plugins/auth-backend/api-report.md @@ -592,6 +592,7 @@ export type OAuthState = { nonce: string; env: string; origin?: string; + scope?: string; }; // @public diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts index a85dd49fba..d1057b19a8 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts @@ -17,7 +17,7 @@ import express from 'express'; import { THOUSAND_DAYS_MS, TEN_MINUTES_MS, OAuthAdapter } from './OAuthAdapter'; import { encodeState } from './helpers'; -import { OAuthHandlers, OAuthResponse } from './types'; +import { OAuthHandlers, OAuthResponse, OAuthState } from './types'; const mockResponseData = { providerInfo: { @@ -148,6 +148,102 @@ describe('OAuthAdapter', () => { ); }); + it('persists scope through cookie if enabled', async () => { + const handlers = { + start: jest.fn(async (_req: { state: OAuthState }) => ({ + url: '/url', + status: 301, + })), + handler: jest.fn(async () => ({ response: mockResponseData })), + refresh: jest.fn(async () => ({ response: mockResponseData })), + }; + const oauthProvider = new OAuthAdapter(handlers, { + ...oAuthProviderOptions, + disableRefresh: false, + persistScopes: true, + }); + + // First we test the /start request, making sure state is set + const mockStartReq = { + query: { + scope: 'user', + env: 'development', + }, + } as unknown as express.Request; + const mockStartRes = { + cookie: jest.fn().mockReturnThis(), + end: jest.fn().mockReturnThis(), + setHeader: jest.fn().mockReturnThis(), + statusCode: jest.fn().mockReturnThis(), + } as unknown as express.Response; + + await oauthProvider.start(mockStartReq, mockStartRes); + + expect(handlers.start).toHaveBeenCalledTimes(1); + expect(handlers.start).toHaveBeenCalledWith({ + query: { + scope: 'user', + env: 'development', + }, + scope: 'user', + state: { + nonce: expect.any(String), + env: 'development', + origin: undefined, + scope: 'user', + }, + }); + + // Then test the /handler, making sure the granted scope cookie is set + const providedState = handlers.start.mock.calls[0][0].state; + const mockHandleReq = { + cookies: { + 'test-provider-nonce': providedState.nonce, + }, + query: { + state: encodeState(providedState), + }, + } as unknown as express.Request; + const mockHandleRes = { + cookie: jest.fn().mockReturnThis(), + setHeader: jest.fn().mockReturnThis(), + end: jest.fn().mockReturnThis(), + } as unknown as express.Response; + + await oauthProvider.frameHandler(mockHandleReq, mockHandleRes); + expect(mockHandleRes.cookie).toHaveBeenCalledTimes(1); + expect(mockHandleRes.cookie).toHaveBeenCalledWith( + 'test-provider-granted-scope', + 'user', + expect.objectContaining({ + path: '/auth/test-provider', + maxAge: THOUSAND_DAYS_MS, + }), + ); + + // Them make sure scopes are forwarded correctly during refresh + const mockRefreshReq = { + query: { scope: 'ignore-me' }, + cookies: { + 'test-provider-granted-scope': 'user', + 'test-provider-refresh-token': 'refresh-token', + }, + header: jest.fn().mockReturnValue('XMLHttpRequest'), + } as unknown as express.Request; + const mockRefreshRes = { + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + } as unknown as express.Response; + await oauthProvider.refresh(mockRefreshReq, mockRefreshRes); + expect(handlers.refresh).toHaveBeenCalledTimes(1); + expect(handlers.refresh).toHaveBeenCalledWith( + expect.objectContaining({ + scope: 'user', + refreshToken: 'refresh-token', + }), + ); + }); + it('does not set the refresh cookie if refresh is disabled', async () => { const oauthProvider = new OAuthAdapter(providerInstance, { ...oAuthProviderOptions, diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts index 564d5c20de..2e2d26db61 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import express from 'express'; +import express, { CookieOptions } from 'express'; import crypto from 'crypto'; import { URL } from 'url'; import { @@ -90,10 +90,20 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { }); } + private readonly baseCookieOptions: CookieOptions; + constructor( private readonly handlers: OAuthHandlers, private readonly options: Options, - ) {} + ) { + this.baseCookieOptions = { + httpOnly: true, + sameSite: 'lax', + secure: this.options.secure, + path: this.options.cookiePath, + domain: this.options.cookieDomain, + }; + } async start(req: express.Request, res: express.Response): Promise { // retrieve scopes from request @@ -105,15 +115,17 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { throw new InputError('No env provided in request query parameters'); } - if (this.options.persistScopes) { - this.setScopesCookie(res, scope); - } - const nonce = crypto.randomBytes(16).toString('base64'); // set a nonce cookie before redirecting to oauth provider this.setNonceCookie(res, nonce); - const state = { nonce, env, origin }; + const state: OAuthState = { nonce, env, origin }; + + // If scopes are persisted then we pass them through the state so that we + // can set the cookie on successful auth + if (this.options.persistScopes) { + state.scope = scope; + } const forwardReq = Object.assign(req, { scope, state }); const { url, status } = await this.handlers.start( @@ -151,12 +163,11 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { const { response, refreshToken } = await this.handlers.handler(req); - if (this.options.persistScopes) { - const grantedScopes = this.getScopesFromCookie( - req, - this.options.providerId, - ); - response.providerInfo.scope = grantedScopes; + // Store the scope that we have been granted for this session. This is useful if + // the provider does not return granted scopes on refresh or if they are normalized. + if (this.options.persistScopes && state.scope) { + this.setGrantedScopeCookie(res, state.scope); + response.providerInfo.scope = state.scope; } if (refreshToken && !this.options.disableRefresh) { @@ -214,8 +225,10 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { throw new InputError('Missing session cookie'); } - const scope = req.query.scope?.toString() ?? ''; - + let scope = req.query.scope?.toString() ?? ''; + if (this.options.persistScopes) { + scope = this.getGrantedScopeFromCookie(req); + } const forwardReq = Object.assign(req, { scope, refreshToken }); // get new access_token @@ -267,27 +280,20 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { private setNonceCookie = (res: express.Response, nonce: string) => { res.cookie(`${this.options.providerId}-nonce`, nonce, { maxAge: TEN_MINUTES_MS, - secure: this.options.secure, - sameSite: 'lax', - domain: this.options.cookieDomain, + ...this.baseCookieOptions, path: `${this.options.cookiePath}/handler`, - httpOnly: true, }); }; - private setScopesCookie = (res: express.Response, scope: string) => { - res.cookie(`${this.options.providerId}-scope`, scope, { - maxAge: TEN_MINUTES_MS, - secure: this.options.secure, - sameSite: 'lax', - domain: this.options.cookieDomain, - path: `${this.options.cookiePath}/handler`, - httpOnly: true, + private setGrantedScopeCookie = (res: express.Response, scope: string) => { + res.cookie(`${this.options.providerId}-granted-scope`, scope, { + maxAge: THOUSAND_DAYS_MS, + ...this.baseCookieOptions, }); }; - private getScopesFromCookie = (req: express.Request, providerId: string) => { - return req.cookies[`${providerId}-scope`]; + private getGrantedScopeFromCookie = (req: express.Request) => { + return req.cookies[`${this.options.providerId}-granted-scope`]; }; private setRefreshTokenCookie = ( @@ -296,22 +302,14 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { ) => { res.cookie(`${this.options.providerId}-refresh-token`, refreshToken, { maxAge: THOUSAND_DAYS_MS, - secure: this.options.secure, - sameSite: 'lax', - domain: this.options.cookieDomain, - path: this.options.cookiePath, - httpOnly: true, + ...this.baseCookieOptions, }); }; private removeRefreshTokenCookie = (res: express.Response) => { res.cookie(`${this.options.providerId}-refresh-token`, '', { maxAge: 0, - secure: this.options.secure, - sameSite: 'lax', - domain: this.options.cookieDomain, - path: this.options.cookiePath, - httpOnly: true, + ...this.baseCookieOptions, }); }; } diff --git a/plugins/auth-backend/src/lib/oauth/types.ts b/plugins/auth-backend/src/lib/oauth/types.ts index 9ddef007a7..6973e99569 100644 --- a/plugins/auth-backend/src/lib/oauth/types.ts +++ b/plugins/auth-backend/src/lib/oauth/types.ts @@ -87,6 +87,7 @@ export type OAuthState = { nonce: string; env: string; origin?: string; + scope?: string; }; export type OAuthStartRequest = express.Request<{}> & {