From aec40d321d5d6a773428815763959a0d02818faa Mon Sep 17 00:00:00 2001 From: Marcus Eide Date: Mon, 24 Jan 2022 10:55:42 +0100 Subject: [PATCH 1/6] Add support for callbackUrl when setting cookie configuration Signed-off-by: Marcus Eide --- .../src/lib/oauth/OAuthAdapter.ts | 25 ++++++++++++------- plugins/auth-backend/src/lib/oauth/helpers.ts | 24 ++++++++++++++++++ .../src/providers/github/provider.ts | 1 + 3 files changed, 41 insertions(+), 9 deletions(-) diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts index ce4b52ef4f..ad1b6a316f 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts @@ -35,7 +35,7 @@ import { NotAllowedError, } from '@backstage/errors'; import { TokenIssuer } from '../../identity/types'; -import { readState, verifyNonce } from './helpers'; +import { getCookieConfig, readState, verifyNonce } from './helpers'; import { postMessageResponse, ensuresXRequestedWith } from '../flow'; import { OAuthHandlers, @@ -58,25 +58,32 @@ export type Options = { appOrigin: string; tokenIssuer: TokenIssuer; isOriginAllowed: (origin: string) => boolean; + callbackUrl?: string; }; - export class OAuthAdapter implements AuthProviderRouteHandlers { static fromConfig( config: AuthProviderConfig, handlers: OAuthHandlers, options: Pick< Options, - 'providerId' | 'persistScopes' | 'disableRefresh' | 'tokenIssuer' + | 'providerId' + | 'persistScopes' + | 'disableRefresh' + | 'tokenIssuer' + | 'callbackUrl' >, ): OAuthAdapter { const { origin: appOrigin } = new URL(config.appUrl); - const secure = config.baseUrl.startsWith('https://'); - const url = new URL(config.baseUrl); - const cookiePath = `${url.pathname}/${options.providerId}`; + const authUrl = new URL(options.callbackUrl ?? config.baseUrl); + const { cookieDomain, cookiePath, secure } = getCookieConfig( + authUrl, + options.providerId, + ); + return new OAuthAdapter(handlers, { ...options, appOrigin, - cookieDomain: url.hostname, + cookieDomain, cookiePath, secure, isOriginAllowed: config.isOriginAllowed, @@ -263,7 +270,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { secure: this.options.secure, sameSite: 'lax', domain: this.options.cookieDomain, - path: `${this.options.cookiePath}/handler`, + path: this.options.cookiePath, httpOnly: true, }); }; @@ -274,7 +281,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { secure: this.options.secure, sameSite: 'lax', domain: this.options.cookieDomain, - path: `${this.options.cookiePath}/handler`, + path: this.options.cookiePath, httpOnly: true, }); }; diff --git a/plugins/auth-backend/src/lib/oauth/helpers.ts b/plugins/auth-backend/src/lib/oauth/helpers.ts index 2bf8fa2520..635c1eb39b 100644 --- a/plugins/auth-backend/src/lib/oauth/helpers.ts +++ b/plugins/auth-backend/src/lib/oauth/helpers.ts @@ -57,3 +57,27 @@ export const verifyNonce = (req: express.Request, providerId: string) => { throw new Error('Invalid nonce'); } }; + +export const getCookieConfig = (authUrl: URL, providerId: string) => { + const { hostname: cookieDomain, pathname, protocol } = authUrl; + const secure = protocol === 'https:'; + + // If the provider supports callbackUrls, the pathname will + // contain the complete path to the frame handler. + // The regex captures the leading path including the provider. + // + // Example match: /api/auth/provider/handler/frame + // Example result: /api/auth/provider + const matcher = pathname.match( + new RegExp(`(?^\/.*?\/${providerId})\/handler\/frame$`), + )?.groups; + + // If there's no match we can manually construct the path + const cookiePath = matcher?.path ?? `${pathname}/${providerId}`; + + return { + cookieDomain, + cookiePath, + secure, + }; +}; diff --git a/plugins/auth-backend/src/providers/github/provider.ts b/plugins/auth-backend/src/providers/github/provider.ts index 220ec4fcce..a1d2ebb062 100644 --- a/plugins/auth-backend/src/providers/github/provider.ts +++ b/plugins/auth-backend/src/providers/github/provider.ts @@ -313,6 +313,7 @@ export const createGithubProvider = ( persistScopes: true, providerId, tokenIssuer, + callbackUrl, }); }); }; From 2bef25550d9d29b27315c5857665c331ec57e75a Mon Sep 17 00:00:00 2001 From: Marcus Eide Date: Mon, 24 Jan 2022 10:59:56 +0100 Subject: [PATCH 2/6] Add tests Signed-off-by: Marcus Eide --- .../src/lib/oauth/OAuthAdapter.test.ts | 81 +++++++++++++++++++ .../src/lib/oauth/helpers.test.ts | 34 +++++++- 2 files changed, 114 insertions(+), 1 deletion(-) diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts index a7463bb7dc..5d5d7e3a87 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts @@ -347,4 +347,85 @@ describe('OAuthAdapter', () => { }, }); }); + + it('sets the correct cookie configuration using the base url', async () => { + const config = { + baseUrl: 'http://domain.org/auth', + appUrl: 'http://domain.org', + isOriginAllowed: () => false, + }; + + const oauthProvider = OAuthAdapter.fromConfig( + config, + providerInstance, + oAuthProviderOptions, + ); + + const mockRequest = { + query: { + scope: 'user', + env: 'development', + }, + } as unknown as express.Request; + + const mockResponse = { + cookie: jest.fn().mockReturnThis(), + end: jest.fn().mockReturnThis(), + setHeader: jest.fn().mockReturnThis(), + statusCode: jest.fn().mockReturnThis(), + } as unknown as express.Response; + + await oauthProvider.start(mockRequest, mockResponse); + + expect(mockResponse.cookie).toBeCalledTimes(1); + expect(mockResponse.cookie).toBeCalledWith( + `${oAuthProviderOptions.providerId}-nonce`, + expect.any(String), + expect.objectContaining({ + domain: 'domain.org', + path: '/auth/test-provider', + secure: false, + }), + ); + }); + + it('sets the correct cookie configuration using a callbackUrl', async () => { + const config = { + baseUrl: 'http://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', + }); + + const mockRequest = { + query: { + scope: 'user', + env: 'development', + }, + } as unknown as express.Request; + + const mockResponse = { + cookie: jest.fn().mockReturnThis(), + end: jest.fn().mockReturnThis(), + setHeader: jest.fn().mockReturnThis(), + statusCode: jest.fn().mockReturnThis(), + } as unknown as express.Response; + + await oauthProvider.start(mockRequest, mockResponse); + + expect(mockResponse.cookie).toBeCalledTimes(1); + expect(mockResponse.cookie).toBeCalledWith( + `${oAuthProviderOptions.providerId}-nonce`, + expect.any(String), + expect.objectContaining({ + domain: 'authdomain.org', + path: '/auth/test-provider', + secure: true, + }), + ); + }); }); diff --git a/plugins/auth-backend/src/lib/oauth/helpers.test.ts b/plugins/auth-backend/src/lib/oauth/helpers.test.ts index 8af1d2f2ad..9145f73370 100644 --- a/plugins/auth-backend/src/lib/oauth/helpers.test.ts +++ b/plugins/auth-backend/src/lib/oauth/helpers.test.ts @@ -15,7 +15,12 @@ */ import express from 'express'; -import { verifyNonce, encodeState, readState } from './helpers'; +import { + verifyNonce, + encodeState, + readState, + getCookieConfig, +} from './helpers'; describe('OAuthProvider Utils', () => { describe('encodeState', () => { @@ -104,4 +109,31 @@ describe('OAuthProvider Utils', () => { }).not.toThrow(); }); }); + + describe('getCookieConfig', () => { + it('should set the correct domain and path for a base url', () => { + const mockAuthUrl = new URL('http://domain.org/auth'); + expect(getCookieConfig(mockAuthUrl, 'test-provider')).toMatchObject({ + cookieDomain: 'domain.org', + cookiePath: '/auth/test-provider', + }); + }); + + it('should set the correct domain and path for a url containing a frame handler', () => { + const mockAuthUrl = new URL( + 'http://domain.org/auth/test-provider/handler/frame', + ); + expect(getCookieConfig(mockAuthUrl, 'test-provider')).toMatchObject({ + cookieDomain: 'domain.org', + cookiePath: '/auth/test-provider', + }); + }); + + it('should set the secure flag if url is using https', () => { + const mockAuthUrl = new URL('https://domain.org/auth'); + expect(getCookieConfig(mockAuthUrl, 'test-provider')).toMatchObject({ + secure: true, + }); + }); + }); }); From 129dc2e099b1dcea5f2394dd8827d1d53945bbe4 Mon Sep 17 00:00:00 2001 From: Marcus Eide Date: Mon, 24 Jan 2022 11:04:17 +0100 Subject: [PATCH 3/6] Update api-reports Signed-off-by: Marcus Eide --- plugins/auth-backend/api-report.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/plugins/auth-backend/api-report.md b/plugins/auth-backend/api-report.md index 07a09597ee..3490ea317b 100644 --- a/plugins/auth-backend/api-report.md +++ b/plugins/auth-backend/api-report.md @@ -478,7 +478,11 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { handlers: OAuthHandlers, options: Pick< Options, - 'providerId' | 'persistScopes' | 'disableRefresh' | 'tokenIssuer' + | 'providerId' + | 'persistScopes' + | 'disableRefresh' + | 'tokenIssuer' + | 'callbackUrl' >, ): OAuthAdapter; // (undocumented) From 033493a8af8e8bc963ff99ed9e6ffbab57737e70 Mon Sep 17 00:00:00 2001 From: Marcus Eide Date: Mon, 24 Jan 2022 11:06:13 +0100 Subject: [PATCH 4/6] Add changeset Signed-off-by: Marcus Eide --- .changeset/quiet-rivers-wave.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 .changeset/quiet-rivers-wave.md diff --git a/.changeset/quiet-rivers-wave.md b/.changeset/quiet-rivers-wave.md new file mode 100644 index 0000000000..26b0b462d6 --- /dev/null +++ b/.changeset/quiet-rivers-wave.md @@ -0,0 +1,13 @@ +--- +'@backstage/plugin-auth-backend': patch +--- + +Supports callbackUrls when setting cookie configuration. +Cookie paths will no longer be scoped to include the handler of the provider: + +```diff +cookie = { +- path=`${pathname}/${provider}/handler` ++ path=`${pathname}/${provider}` +} +``` From 71a2fbaa58c0b6a7d496605b0a7b767dc416fcc0 Mon Sep 17 00:00:00 2001 From: Marcus Eide Date: Mon, 24 Jan 2022 14:22:54 +0100 Subject: [PATCH 5/6] Simplify the cookiePath logic Signed-off-by: Marcus Eide --- .../auth-backend/src/lib/oauth/helpers.test.ts | 2 ++ plugins/auth-backend/src/lib/oauth/helpers.ts | 16 +++++----------- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/plugins/auth-backend/src/lib/oauth/helpers.test.ts b/plugins/auth-backend/src/lib/oauth/helpers.test.ts index 9145f73370..a98b684141 100644 --- a/plugins/auth-backend/src/lib/oauth/helpers.test.ts +++ b/plugins/auth-backend/src/lib/oauth/helpers.test.ts @@ -116,6 +116,7 @@ describe('OAuthProvider Utils', () => { expect(getCookieConfig(mockAuthUrl, 'test-provider')).toMatchObject({ cookieDomain: 'domain.org', cookiePath: '/auth/test-provider', + secure: false, }); }); @@ -126,6 +127,7 @@ describe('OAuthProvider Utils', () => { expect(getCookieConfig(mockAuthUrl, 'test-provider')).toMatchObject({ cookieDomain: 'domain.org', cookiePath: '/auth/test-provider', + secure: false, }); }); diff --git a/plugins/auth-backend/src/lib/oauth/helpers.ts b/plugins/auth-backend/src/lib/oauth/helpers.ts index 635c1eb39b..878083e679 100644 --- a/plugins/auth-backend/src/lib/oauth/helpers.ts +++ b/plugins/auth-backend/src/lib/oauth/helpers.ts @@ -63,17 +63,11 @@ export const getCookieConfig = (authUrl: URL, providerId: string) => { const secure = protocol === 'https:'; // If the provider supports callbackUrls, the pathname will - // contain the complete path to the frame handler. - // The regex captures the leading path including the provider. - // - // Example match: /api/auth/provider/handler/frame - // Example result: /api/auth/provider - const matcher = pathname.match( - new RegExp(`(?^\/.*?\/${providerId})\/handler\/frame$`), - )?.groups; - - // If there's no match we can manually construct the path - const cookiePath = matcher?.path ?? `${pathname}/${providerId}`; + // contain the complete path to the frame handler so we need + // to slice off the trailing part of the path. + const cookiePath = pathname.endsWith(`${providerId}/handler/frame`) + ? pathname.slice(0, -'/handler/frame'.length) + : `${pathname}/${providerId}`; return { cookieDomain, From 3b6e41009ba660494ecbf4a8200601f97e0d3e13 Mon Sep 17 00:00:00 2001 From: Marcus Eide Date: Mon, 24 Jan 2022 14:24:39 +0100 Subject: [PATCH 6/6] Revert the cookiePath to include the handler suffix Signed-off-by: Marcus Eide --- .changeset/quiet-rivers-wave.md | 10 +--------- .../auth-backend/src/lib/oauth/OAuthAdapter.test.ts | 4 ++-- plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts | 4 ++-- 3 files changed, 5 insertions(+), 13 deletions(-) diff --git a/.changeset/quiet-rivers-wave.md b/.changeset/quiet-rivers-wave.md index 26b0b462d6..3ac6ec37fd 100644 --- a/.changeset/quiet-rivers-wave.md +++ b/.changeset/quiet-rivers-wave.md @@ -2,12 +2,4 @@ '@backstage/plugin-auth-backend': patch --- -Supports callbackUrls when setting cookie configuration. -Cookie paths will no longer be scoped to include the handler of the provider: - -```diff -cookie = { -- path=`${pathname}/${provider}/handler` -+ path=`${pathname}/${provider}` -} -``` +Running the `auth-backend` on multiple domains, perhaps different domains depending on the `auth.environment`, was previously not possible as the `domain` name of the cookie was taken from `backend.baseUrl`. This prevented any cookies to be set in the start of the auth flow as the domain of the cookie would not match the domain of the callbackUrl configured in the OAuth app. This change checks if a provider supports custom `callbackUrl`'s to be configured in the application configuration and uses the domain from that, allowing the `domain`'s to match and the cookie to be set. diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts index 5d5d7e3a87..a85dd49fba 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts @@ -383,7 +383,7 @@ describe('OAuthAdapter', () => { expect.any(String), expect.objectContaining({ domain: 'domain.org', - path: '/auth/test-provider', + path: '/auth/test-provider/handler', secure: false, }), ); @@ -423,7 +423,7 @@ describe('OAuthAdapter', () => { expect.any(String), expect.objectContaining({ domain: 'authdomain.org', - path: '/auth/test-provider', + path: '/auth/test-provider/handler', secure: true, }), ); diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts index ad1b6a316f..564d5c20de 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts @@ -270,7 +270,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { secure: this.options.secure, sameSite: 'lax', domain: this.options.cookieDomain, - path: this.options.cookiePath, + path: `${this.options.cookiePath}/handler`, httpOnly: true, }); }; @@ -281,7 +281,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { secure: this.options.secure, sameSite: 'lax', domain: this.options.cookieDomain, - path: this.options.cookiePath, + path: `${this.options.cookiePath}/handler`, httpOnly: true, }); };