From 04c50e297d8a0e4fb0ad78fe9f5912ce1794f543 Mon Sep 17 00:00:00 2001 From: Marcus Eide Date: Fri, 16 Sep 2022 09:57:44 +0200 Subject: [PATCH] Resolve cookie configuration per request Signed-off-by: Marcus Eide --- .changeset/kind-penguins-report.md | 4 +- plugins/auth-backend/api-report.md | 1 - .../src/lib/oauth/OAuthAdapter.test.ts | 164 ++++++++++++++++-- .../src/lib/oauth/OAuthAdapter.ts | 74 ++++---- 4 files changed, 200 insertions(+), 43 deletions(-) diff --git a/.changeset/kind-penguins-report.md b/.changeset/kind-penguins-report.md index 9f2401491e..4023ea5976 100644 --- a/.changeset/kind-penguins-report.md +++ b/.changeset/kind-penguins-report.md @@ -5,4 +5,6 @@ CookieConfigurer can optionally return the `SameSite` cookie attribute. CookieConfigurer now requires an additional argument `appOrigin` - the origin URL of the app - which is used to calculate the `SameSite` attribute. defaultCookieConfigurer returns the `SameSite` attribute which defaults to `Lax`. In cases where an auth-backend is running on a different domain than the App, `SameSite=None` is used - but only for secure contexts. This is so that cookies can be included in third-party requests. -OAuthAdapterOptions has been modified to require additional arguments, `baseUrl`, `cookieConfigurer` and `cookieConfig`. + +OAuthAdapterOptions has been modified to require additional arguments, `baseUrl`, and `cookieConfigurer`. +OAuthAdapter now resolves cookie configuration using its supplied CookieConfigurer for each request to make sure that the proper attributes always are set. diff --git a/plugins/auth-backend/api-report.md b/plugins/auth-backend/api-report.md index 8cb7e35e9b..f4e8415266 100644 --- a/plugins/auth-backend/api-report.md +++ b/plugins/auth-backend/api-report.md @@ -285,7 +285,6 @@ export type OAuthAdapterOptions = { persistScopes?: boolean; appOrigin: string; baseUrl: string; - cookieConfig: ReturnType; cookieConfigurer: CookieConfigurer; isOriginAllowed: (origin: string) => boolean; callbackUrl: string; diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts index d59a712205..6521519808 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts @@ -63,7 +63,7 @@ describe('OAuthAdapter', () => { } const providerInstance = new MyAuthProvider(); const mockCookieConfig: ReturnType = { - domain: 'example.com', + domain: 'domain.org', path: '/auth/test-provider', secure: false, }; @@ -72,15 +72,14 @@ describe('OAuthAdapter', () => { const oAuthProviderOptions = { providerId: 'test-provider', appOrigin: 'http://localhost:3000', - cookieConfig: mockCookieConfig, + baseUrl: 'http://domain.org/auth', cookieConfigurer: mockCookieConfigurer, - baseUrl: 'http://example.com:7007', tokenIssuer: { issueToken: async () => 'my-id-token', listPublicKeys: async () => ({ keys: [] }), }, isOriginAllowed: () => false, - callbackUrl: 'http://example.com:7007/auth/test-provider/frame/handler', + callbackUrl: 'http://domain.org/auth/test-provider/handler/frame', }; it('sets the correct headers in start', async () => { @@ -108,7 +107,14 @@ describe('OAuthAdapter', () => { expect(mockResponse.cookie).toHaveBeenCalledWith( `${oAuthProviderOptions.providerId}-nonce`, expect.any(String), - expect.objectContaining({ maxAge: TEN_MINUTES_MS }), + expect.objectContaining({ + httpOnly: true, + path: '/auth/test-provider/handler', + maxAge: TEN_MINUTES_MS, + domain: 'domain.org', + sameSite: 'lax', + secure: false, + }), ); // redirect checks expect(mockResponse.setHeader).toHaveBeenCalledTimes(2); @@ -150,6 +156,7 @@ describe('OAuthAdapter', () => { httpOnly: true, path: '/auth/test-provider', maxAge: THOUSAND_DAYS_MS, + domain: 'domain.org', secure: false, sameSite: 'lax', }), @@ -218,14 +225,17 @@ describe('OAuthAdapter', () => { } as unknown as express.Response; await oauthProvider.frameHandler(mockHandleReq, mockHandleRes); - expect(mockCookieConfigurer).toHaveBeenCalledTimes(1); expect(mockHandleRes.cookie).toHaveBeenCalledTimes(1); expect(mockHandleRes.cookie).toHaveBeenCalledWith( 'test-provider-granted-scope', 'user', expect.objectContaining({ + httpOnly: true, path: '/auth/test-provider', maxAge: THOUSAND_DAYS_MS, + domain: 'domain.org', + secure: false, + sameSite: 'lax', }), ); @@ -260,6 +270,7 @@ describe('OAuthAdapter', () => { const mockRequest = { header: () => 'XMLHttpRequest', + get: jest.fn(), } as unknown as express.Request; const mockResponse = { @@ -269,6 +280,7 @@ describe('OAuthAdapter', () => { } as unknown as express.Response; await oauthProvider.logout(mockRequest, mockResponse); + expect(mockRequest.get).toHaveBeenCalledTimes(1); expect(mockResponse.cookie).toHaveBeenCalledTimes(1); expect(mockResponse.cookie).toHaveBeenCalledWith( expect.stringContaining('test-provider-refresh-token'), @@ -312,7 +324,46 @@ describe('OAuthAdapter', () => { }); }); - it('sets the correct cookie configuration using an unsecure callbackUrl', async () => { + it('sets new access-token when old cookie exists', async () => { + const oauthProvider = new OAuthAdapter(providerInstance, { + ...oAuthProviderOptions, + isOriginAllowed: () => false, + }); + + const mockRequest = { + header: () => 'XMLHttpRequest', + cookies: { + 'test-provider-refresh-token': 'old-token', + }, + query: {}, + get: jest.fn(), + } as unknown as express.Request; + + const mockResponse = { + json: jest.fn().mockReturnThis(), + status: jest.fn().mockReturnThis(), + cookie: jest.fn().mockReturnThis(), + } as unknown as express.Response; + + await oauthProvider.refresh(mockRequest, mockResponse); + expect(mockRequest.get).toHaveBeenCalledTimes(1); + expect(mockCookieConfigurer).toHaveBeenCalledTimes(1); + expect(mockResponse.cookie).toHaveBeenCalledTimes(1); + expect(mockResponse.cookie).toHaveBeenCalledWith( + 'test-provider-refresh-token', + 'token', + expect.objectContaining({ + httpOnly: true, + path: '/auth/test-provider', + maxAge: THOUSAND_DAYS_MS, + domain: 'domain.org', + secure: false, + sameSite: 'lax', + }), + ); + }); + + it('sets the correct nonce cookie configuration', async () => { const config = { baseUrl: 'http://domain.org/auth', appUrl: 'http://domain.org', @@ -321,7 +372,6 @@ describe('OAuthAdapter', () => { const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, { ...oAuthProviderOptions, - callbackUrl: 'http://authdomain.org/auth/test-provider/handler/frame', }); const mockRequest = { @@ -346,7 +396,8 @@ describe('OAuthAdapter', () => { expect.any(String), expect.objectContaining({ httpOnly: true, - domain: 'authdomain.org', + domain: 'domain.org', + maxAge: TEN_MINUTES_MS, path: '/auth/test-provider/handler', secure: false, sameSite: 'lax', @@ -361,6 +412,54 @@ describe('OAuthAdapter', () => { isOriginAllowed: () => false, }; + const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, { + ...oAuthProviderOptions, + callbackUrl: 'https://domain.org/auth/test-provider/handler/frame', + }); + + const state = { + nonce: 'nonce', + env: 'development', + }; + + const mockRequest = { + cookies: { + 'test-provider-nonce': 'nonce', + }, + query: { + state: encodeState(state), + }, + } as unknown as express.Request; + + const mockResponse = { + cookie: jest.fn().mockReturnThis(), + setHeader: jest.fn().mockReturnThis(), + end: jest.fn().mockReturnThis(), + } as unknown as express.Response; + + await oauthProvider.frameHandler(mockRequest, mockResponse); + expect(mockCookieConfigurer).not.toHaveBeenCalled(); + expect(mockResponse.cookie).toHaveBeenCalledTimes(1); + expect(mockResponse.cookie).toHaveBeenCalledWith( + expect.stringContaining('test-provider-refresh-token'), + expect.stringContaining('token'), + expect.objectContaining({ + httpOnly: true, + domain: 'domain.org', + path: '/auth/test-provider', + secure: true, + sameSite: 'lax', + }), + ); + }); + + it('sets the correct cookie configuration when on different domains and secure', async () => { + const config = { + baseUrl: 'https://domain.org/auth', + appUrl: 'http://domain.org', + isOriginAllowed: () => false, + }; + const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, { ...oAuthProviderOptions, callbackUrl: 'https://authdomain.org/auth/test-provider/handler/frame', @@ -402,7 +501,7 @@ describe('OAuthAdapter', () => { ); }); - it('sets the correct cookie configuration using state', async () => { + it('sets the correct cookie configuration using origin from state', async () => { const config = { baseUrl: 'https://domain.org/auth', appUrl: 'http://domain.org', @@ -450,4 +549,49 @@ describe('OAuthAdapter', () => { }), ); }); + + it('sets the correct cookie configuration using origin from header', async () => { + const config = { + baseUrl: 'https://domain.org/auth', + appUrl: 'http://domain.org', + isOriginAllowed: () => false, + }; + + const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, { + ...oAuthProviderOptions, + callbackUrl: 'https://domain.org/auth/test-provider/handler/frame', + }); + + const mockRequest = { + header: () => 'XMLHttpRequest', + cookies: { + 'test-provider-refresh-token': 'old-token', + }, + query: {}, + get: jest.fn().mockReturnValue('http://other.domain'), + } as unknown as express.Request; + + const mockResponse = { + json: jest.fn().mockReturnThis(), + status: jest.fn().mockReturnThis(), + cookie: jest.fn().mockReturnThis(), + } as unknown as express.Response; + + await oauthProvider.refresh(mockRequest, mockResponse); + expect(mockRequest.get).toHaveBeenCalledTimes(1); + expect(mockCookieConfigurer).not.toHaveBeenCalled(); + expect(mockResponse.cookie).toHaveBeenCalledTimes(1); + expect(mockResponse.cookie).toHaveBeenCalledWith( + 'test-provider-refresh-token', + 'token', + expect.objectContaining({ + httpOnly: true, + path: '/auth/test-provider', + maxAge: THOUSAND_DAYS_MS, + domain: 'domain.org', + secure: true, + sameSite: 'none', + }), + ); + }); }); diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts index 64dc4f70e3..43fcc5e20b 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts @@ -51,7 +51,6 @@ export type OAuthAdapterOptions = { persistScopes?: boolean; appOrigin: string; baseUrl: string; - cookieConfig: ReturnType; cookieConfigurer: CookieConfigurer; isOriginAllowed: (origin: string) => boolean; callbackUrl: string; @@ -71,24 +70,17 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { const { origin: appOrigin } = new URL(appUrl); const cookieConfigurer = config.cookieConfigurer ?? defaultCookieConfigurer; - const cookieConfig = cookieConfigurer({ - providerId: options.providerId, - baseUrl: config.baseUrl, - callbackUrl: options.callbackUrl, - appOrigin, - }); return new OAuthAdapter(handlers, { ...options, appOrigin, baseUrl, - cookieConfig, cookieConfigurer, isOriginAllowed, }); } - private baseCookieOptions: CookieOptions; + private readonly baseCookieOptions: CookieOptions; constructor( private readonly handlers: OAuthHandlers, @@ -97,7 +89,6 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { this.baseCookieOptions = { httpOnly: true, sameSite: 'lax', - ...this.options.cookieConfig, }; } @@ -111,9 +102,11 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { throw new InputError('No env provided in request query parameters'); } + const cookieConfig = this.getCookieConfig(); + const nonce = crypto.randomBytes(16).toString('base64'); // set a nonce cookie before redirecting to oauth provider - this.setNonceCookie(res, nonce); + this.setNonceCookie(res, nonce, cookieConfig); const state: OAuthState = { nonce, env, origin }; @@ -154,33 +147,23 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { } } - // Update cookie options to reflect any changes for the appOrigin - const updatedCookieOptions = this.options.cookieConfigurer({ - providerId: this.options.providerId, - baseUrl: this.options.baseUrl, - callbackUrl: this.options.callbackUrl, - appOrigin, - }); - this.baseCookieOptions = { - ...this.baseCookieOptions, - ...updatedCookieOptions, - }; - // verify nonce cookie and state cookie on callback verifyNonce(req, this.options.providerId); const { response, refreshToken } = await this.handlers.handler(req); + const cookieConfig = this.getCookieConfig(appOrigin); + // 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); + this.setGrantedScopeCookie(res, state.scope, cookieConfig); response.providerInfo.scope = state.scope; } if (refreshToken) { // set new refresh token - this.setRefreshTokenCookie(res, refreshToken); + this.setRefreshTokenCookie(res, refreshToken, cookieConfig); } const identity = await this.populateIdentity(response.backstageIdentity); @@ -208,7 +191,9 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { } // remove refresh token cookie if it is set - this.removeRefreshTokenCookie(res); + const origin = req.get('origin'); + const cookieConfig = this.getCookieConfig(origin); + this.removeRefreshTokenCookie(res, cookieConfig); res.status(200).end(); } @@ -248,7 +233,9 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { ); if (newRefreshToken && newRefreshToken !== refreshToken) { - this.setRefreshTokenCookie(res, newRefreshToken); + const origin = req.get('origin'); + const cookieConfig = this.getCookieConfig(origin); + this.setRefreshTokenCookie(res, newRefreshToken, cookieConfig); } res.status(200).json({ ...response, backstageIdentity }); @@ -274,18 +261,28 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { return prepareBackstageIdentityResponse(identity); } - private setNonceCookie = (res: express.Response, nonce: string) => { + private setNonceCookie = ( + res: express.Response, + nonce: string, + cookieConfig: ReturnType, + ) => { res.cookie(`${this.options.providerId}-nonce`, nonce, { maxAge: TEN_MINUTES_MS, ...this.baseCookieOptions, - path: `${this.options.cookieConfig.path}/handler`, + ...cookieConfig, + path: `${cookieConfig.path}/handler`, }); }; - private setGrantedScopeCookie = (res: express.Response, scope: string) => { + private setGrantedScopeCookie = ( + res: express.Response, + scope: string, + cookieConfig: ReturnType, + ) => { res.cookie(`${this.options.providerId}-granted-scope`, scope, { maxAge: THOUSAND_DAYS_MS, ...this.baseCookieOptions, + ...cookieConfig, }); }; @@ -296,17 +293,32 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { private setRefreshTokenCookie = ( res: express.Response, refreshToken: string, + cookieConfig: ReturnType, ) => { res.cookie(`${this.options.providerId}-refresh-token`, refreshToken, { maxAge: THOUSAND_DAYS_MS, ...this.baseCookieOptions, + ...cookieConfig, }); }; - private removeRefreshTokenCookie = (res: express.Response) => { + private removeRefreshTokenCookie = ( + res: express.Response, + cookieConfig: ReturnType, + ) => { res.cookie(`${this.options.providerId}-refresh-token`, '', { maxAge: 0, ...this.baseCookieOptions, + ...cookieConfig, + }); + }; + + private getCookieConfig = (origin?: string) => { + return this.options.cookieConfigurer({ + providerId: this.options.providerId, + baseUrl: this.options.baseUrl, + callbackUrl: this.options.callbackUrl, + appOrigin: origin ?? this.options.appOrigin, }); }; }