From 2c5aa0a5c868c20599f0cf7cb26986ee3f8bdc7f Mon Sep 17 00:00:00 2001 From: Ruben Vallejo Date: Tue, 17 Oct 2023 10:55:43 -0400 Subject: [PATCH 1/4] Add PinnipedStrategyFactory with Cache, on-demand issuer query. Need to incorporate cache expiration Signed-off-by: Ruben Vallejo --- .../package.json | 3 +- .../src/authenticator.test.ts | 331 ++++++++++++++++-- .../src/authenticator.ts | 95 ++++- yarn.lock | 1 + 4 files changed, 388 insertions(+), 42 deletions(-) diff --git a/plugins/auth-backend-module-pinniped-provider/package.json b/plugins/auth-backend-module-pinniped-provider/package.json index 087a90cdbb..5541d008da 100644 --- a/plugins/auth-backend-module-pinniped-provider/package.json +++ b/plugins/auth-backend-module-pinniped-provider/package.json @@ -25,14 +25,15 @@ "dependencies": { "@backstage/backend-common": "workspace:^", "@backstage/backend-plugin-api": "workspace:^", + "@backstage/config": "workspace:^", "@backstage/plugin-auth-node": "workspace:^", + "luxon": "^3.4.3", "openid-client": "^5.4.3" }, "devDependencies": { "@backstage/backend-defaults": "workspace:^", "@backstage/backend-test-utils": "workspace:^", "@backstage/cli": "workspace:^", - "@backstage/config": "workspace:^", "@backstage/plugin-auth-backend": "workspace:^", "cookie-parser": "^1.4.6", "express": "^4.18.2", diff --git a/plugins/auth-backend-module-pinniped-provider/src/authenticator.test.ts b/plugins/auth-backend-module-pinniped-provider/src/authenticator.test.ts index f908c1ea13..8b4c092e3a 100644 --- a/plugins/auth-backend-module-pinniped-provider/src/authenticator.test.ts +++ b/plugins/auth-backend-module-pinniped-provider/src/authenticator.test.ts @@ -21,7 +21,7 @@ import { decodeOAuthState, encodeOAuthState, } from '@backstage/plugin-auth-node'; -import { pinnipedAuthenticator } from './authenticator'; +import { pinnipedAuthenticator, getIssuer } from './authenticator'; import { setupServer } from 'msw/node'; import { setupRequestMockHandlers } from '@backstage/backend-test-utils'; import { ConfigReader } from '@backstage/config'; @@ -30,7 +30,7 @@ import { rest } from 'msw'; import express from 'express'; describe('pinnipedAuthenticator', () => { - let implementation: any; + let authCtx: any; let oauthState: OAuthState; let idToken: string; let publicKey: JWK; @@ -128,7 +128,7 @@ describe('pinnipedAuthenticator', () => { }), ); - implementation = pinnipedAuthenticator.initialize({ + authCtx = pinnipedAuthenticator.initialize({ callbackUrl: 'https://backstage.test/callback', config: new ConfigReader({ federationDomain: 'https://federationDomain.test', @@ -143,6 +143,27 @@ describe('pinnipedAuthenticator', () => { }; }); + describe('#initialize', () => { + it('always returns a PinnipedStrategyFactory', async () => { + const pinnipedStrategyFactory = pinnipedAuthenticator.initialize({ + callbackUrl: 'https://backstage.test/callback', + config: new ConfigReader({ + federationDomain: 'https://federationDomain.test', + clientId: 'clientId', + clientSecret: 'clientSecret', + }), + }); + + const { providerStrategy, client } = + await pinnipedStrategyFactory.getStrategy(); + + expect(providerStrategy).toBeDefined(); + expect(client.issuer.authorization_endpoint).toMatch( + 'https://pinniped.test/oauth2/authorize', + ); + }); + }); + describe('#start', () => { let fakeSession: Record; let startRequest: OAuthAuthenticatorStartInput; @@ -162,7 +183,7 @@ describe('pinnipedAuthenticator', () => { it('redirects to authorization endpoint returned from OIDC metadata endpoint', async () => { const startResponse = await pinnipedAuthenticator.start( startRequest, - implementation, + authCtx, ); const url = new URL(startResponse.url); @@ -174,7 +195,7 @@ describe('pinnipedAuthenticator', () => { it('initiates authorization code grant', async () => { const startResponse = await pinnipedAuthenticator.start( startRequest, - implementation, + authCtx, ); const { searchParams } = new URL(startResponse.url); @@ -185,7 +206,7 @@ describe('pinnipedAuthenticator', () => { startRequest.req.query = { audience: 'test-cluster' }; const startResponse = await pinnipedAuthenticator.start( startRequest, - implementation, + authCtx, ); const { searchParams } = new URL(startResponse.url); const stateParam = searchParams.get('state'); @@ -201,7 +222,7 @@ describe('pinnipedAuthenticator', () => { it('passes client ID from config', async () => { const startResponse = await pinnipedAuthenticator.start( startRequest, - implementation, + authCtx, ); const { searchParams } = new URL(startResponse.url); @@ -211,7 +232,7 @@ describe('pinnipedAuthenticator', () => { it('passes callback URL from config', async () => { const startResponse = await pinnipedAuthenticator.start( startRequest, - implementation, + authCtx, ); const { searchParams } = new URL(startResponse.url); @@ -223,7 +244,7 @@ describe('pinnipedAuthenticator', () => { it('generates PKCE challenge', async () => { const startResponse = await pinnipedAuthenticator.start( startRequest, - implementation, + authCtx, ); const { searchParams } = new URL(startResponse.url); @@ -232,14 +253,14 @@ describe('pinnipedAuthenticator', () => { }); it('stores PKCE verifier in session', async () => { - await pinnipedAuthenticator.start(startRequest, implementation); + await pinnipedAuthenticator.start(startRequest, authCtx); expect(fakeSession['oidc:pinniped.test'].code_verifier).toBeDefined(); }); it('requests sufficient scopes for token exchange by default', async () => { const startResponse = await pinnipedAuthenticator.start( startRequest, - implementation, + authCtx, ); const { searchParams } = new URL(startResponse.url); const scopes = searchParams.get('scope')?.split(' ') ?? []; @@ -257,7 +278,7 @@ describe('pinnipedAuthenticator', () => { it('encodes OAuth state in query param', async () => { const startResponse = await pinnipedAuthenticator.start( startRequest, - implementation, + authCtx, ); const { searchParams } = new URL(startResponse.url); const stateParam = searchParams.get('state'); @@ -276,10 +297,94 @@ describe('pinnipedAuthenticator', () => { url: 'test', }, } as unknown as OAuthAuthenticatorStartInput, - implementation, + authCtx, ), ).rejects.toThrow('authentication requires session support'); }); + + it('refreshes oidc metadata after a failed fetch', async () => { + mswServer.use( + rest.get( + 'https://federationDomain.test/.well-known/openid-configuration', + (_req, res, _ctx) => res.networkError('Timeout'), + ), + ); + + const authCtxCreatedWhileSupervisorUnavailable = + pinnipedAuthenticator.initialize({ + callbackUrl: 'https://backstage.test/callback', + config: new ConfigReader({ + federationDomain: 'https://federationDomain.test', + clientId: 'clientId', + clientSecret: 'clientSecret', + }), + }); + + mswServer.use( + rest.get( + 'https://federationDomain.test/.well-known/openid-configuration', + (_req, res, ctx) => + res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(issuerMetadata), + ), + ), + ); + + const response = await pinnipedAuthenticator.start( + startRequest, + authCtxCreatedWhileSupervisorUnavailable, + ); + + expect(response.url).toMatch('https://pinniped.test/oauth2/authorize'); + }); + + it('only refreshes metadata after a failure', async () => { + mswServer.use( + rest.get( + 'https://federationDomain.test/.well-known/openid-configuration', + (_req, res, _ctx) => res.networkError('Timeout'), + ), + ); + + const authCtxCreatedWhileSupervisorUnavailable = + pinnipedAuthenticator.initialize({ + callbackUrl: 'https://backstage.test/callback', + config: new ConfigReader({ + federationDomain: 'https://federationDomain.test', + clientId: 'clientId', + clientSecret: 'clientSecret', + }), + }); + + let supervisorCalls: number = 0; + + mswServer.use( + rest.get( + 'https://federationDomain.test/.well-known/openid-configuration', + (_req, res, ctx) => { + supervisorCalls += 1; + return res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(issuerMetadata), + ); + }, + ), + ); + + await pinnipedAuthenticator.start( + startRequest, + authCtxCreatedWhileSupervisorUnavailable, + ); + await pinnipedAuthenticator.start( + startRequest, + authCtxCreatedWhileSupervisorUnavailable, + ); + + expect(supervisorCalls).toEqual(1); + }); }); describe('#authenticate', () => { @@ -304,7 +409,7 @@ describe('pinnipedAuthenticator', () => { it('exchanges authorization code for access token', async () => { const handlerResponse = await pinnipedAuthenticator.authenticate( handlerRequest, - implementation, + authCtx, ); const accessToken = handlerResponse.session.accessToken; @@ -314,7 +419,7 @@ describe('pinnipedAuthenticator', () => { it('exchanges authorization code for refresh token', async () => { const handlerResponse = await pinnipedAuthenticator.authenticate( handlerRequest, - implementation, + authCtx, ); const refreshToken = handlerResponse.session.refreshToken; @@ -324,7 +429,7 @@ describe('pinnipedAuthenticator', () => { it('returns granted scope', async () => { const handlerResponse = await pinnipedAuthenticator.authenticate( handlerRequest, - implementation, + authCtx, ); const responseScope = handlerResponse.session.scope; @@ -349,7 +454,7 @@ describe('pinnipedAuthenticator', () => { const handlerResponse = await pinnipedAuthenticator.authenticate( handlerRequest, - implementation, + authCtx, ); expect(handlerResponse.session.idToken).toEqual(clusterScopedIdToken); @@ -413,7 +518,7 @@ describe('pinnipedAuthenticator', () => { }; await expect( - pinnipedAuthenticator.authenticate(handlerRequest, implementation), + pinnipedAuthenticator.authenticate(handlerRequest, authCtx), ).rejects.toThrow( `Failed to get cluster specific ID token for "test_cluster": Error: RFC8693 token exchange failed with error: NetworkError: Connection timed out`, ); @@ -422,7 +527,7 @@ describe('pinnipedAuthenticator', () => { it('fails without authorization code', async () => { handlerRequest.req.url = 'https://test.com'; return expect( - pinnipedAuthenticator.authenticate(handlerRequest, implementation), + pinnipedAuthenticator.authenticate(handlerRequest, authCtx), ).rejects.toThrow('Unexpected redirect'); }); @@ -440,7 +545,7 @@ describe('pinnipedAuthenticator', () => { }, } as unknown as express.Request, }, - implementation, + authCtx, ), ).rejects.toThrow( 'Authentication rejected, state missing from the response', @@ -456,10 +561,105 @@ describe('pinnipedAuthenticator', () => { url: 'https://test.com', } as unknown as express.Request, }, - implementation, + authCtx, ), ).rejects.toThrow('authentication requires session support'); }); + + it('refreshes oidc metadata after a failed fetch', async () => { + mswServer.use( + rest.get( + 'https://federationDomain.test/.well-known/openid-configuration', + (_req, res, _ctx) => res.networkError('Timeout'), + ), + ); + + const authCtxCreatedWhileSupervisorUnavailable = + pinnipedAuthenticator.initialize({ + callbackUrl: 'https://backstage.test/callback', + config: new ConfigReader({ + federationDomain: 'https://federationDomain.test', + clientId: 'clientId', + clientSecret: 'clientSecret', + }), + }); + + mswServer.use( + rest.get( + 'https://federationDomain.test/.well-known/openid-configuration', + (_req, res, ctx) => + res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(issuerMetadata), + ), + ), + ); + + const response = await pinnipedAuthenticator.authenticate( + handlerRequest, + authCtxCreatedWhileSupervisorUnavailable, + ); + expect(response.session.accessToken).toEqual('accessToken'); + }); + + it('only refreshes metadata after a failure', async () => { + mswServer.use( + rest.get( + 'https://federationDomain.test/.well-known/openid-configuration', + (_req, res, _ctx) => res.networkError('Timeout'), + ), + ); + + const authCtxCreatedWhileSupervisorUnavailable = + pinnipedAuthenticator.initialize({ + callbackUrl: 'https://backstage.test/callback', + config: new ConfigReader({ + federationDomain: 'https://federationDomain.test', + clientId: 'clientId', + clientSecret: 'clientSecret', + }), + }); + + let supervisorCalls: number = 0; + + mswServer.use( + rest.get( + 'https://federationDomain.test/.well-known/openid-configuration', + (_req, res, ctx) => { + supervisorCalls += 1; + return res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(issuerMetadata), + ); + }, + ), + ); + + await pinnipedAuthenticator.authenticate( + handlerRequest, + authCtxCreatedWhileSupervisorUnavailable, + ); + await pinnipedAuthenticator.authenticate( + { + req: { + method: 'GET', + url: `https://test?code=authorization_code&state=${encodeOAuthState( + oauthState, + )}`, + session: { + 'oidc:pinniped.test': { + state: encodeOAuthState(oauthState), + }, + }, + } as unknown as express.Request, + }, + authCtxCreatedWhileSupervisorUnavailable, + ); + + expect(supervisorCalls).toEqual(1); + }); }); describe('#refresh', () => { @@ -476,7 +676,7 @@ describe('pinnipedAuthenticator', () => { it('gets new refresh token', async () => { const refreshResponse = await pinnipedAuthenticator.refresh( refreshRequest, - implementation, + authCtx, ); expect(refreshResponse.session.refreshToken).toBe('refreshToken'); @@ -485,7 +685,7 @@ describe('pinnipedAuthenticator', () => { it('gets access token', async () => { const refreshResponse = await pinnipedAuthenticator.refresh( refreshRequest, - implementation, + authCtx, ); expect(refreshResponse.session.accessToken).toBe('accessToken'); @@ -494,10 +694,93 @@ describe('pinnipedAuthenticator', () => { it('gets id token', async () => { const refreshResponse = await pinnipedAuthenticator.refresh( refreshRequest, - implementation, + authCtx, ); expect(refreshResponse.session.idToken).toBe(idToken); }); + + it('refreshes oidc metadata after a failed fetch', async () => { + mswServer.use( + rest.get( + 'https://federationDomain.test/.well-known/openid-configuration', + (_req, res, _ctx) => res.networkError('Timeout'), + ), + ); + + const authCtxCreatedWhileSupervisorUnavailable = + pinnipedAuthenticator.initialize({ + callbackUrl: 'https://backstage.test/callback', + config: new ConfigReader({ + federationDomain: 'https://federationDomain.test', + clientId: 'clientId', + clientSecret: 'clientSecret', + }), + }); + + mswServer.use( + rest.get( + 'https://federationDomain.test/.well-known/openid-configuration', + (_req, res, ctx) => + res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(issuerMetadata), + ), + ), + ); + + const response = await pinnipedAuthenticator.refresh( + refreshRequest, + authCtxCreatedWhileSupervisorUnavailable, + ); + expect(response.session.accessToken).toEqual('accessToken'); + }); + + it('only refreshes metadata after a failure', async () => { + mswServer.use( + rest.get( + 'https://federationDomain.test/.well-known/openid-configuration', + (_req, res, _ctx) => res.networkError('Timeout'), + ), + ); + + const authCtxCreatedWhileSupervisorUnavailable = + pinnipedAuthenticator.initialize({ + callbackUrl: 'https://backstage.test/callback', + config: new ConfigReader({ + federationDomain: 'https://federationDomain.test', + clientId: 'clientId', + clientSecret: 'clientSecret', + }), + }); + + let supervisorCalls: number = 0; + + mswServer.use( + rest.get( + 'https://federationDomain.test/.well-known/openid-configuration', + (_req, res, ctx) => { + supervisorCalls += 1; + return res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(issuerMetadata), + ); + }, + ), + ); + + await pinnipedAuthenticator.refresh( + refreshRequest, + authCtxCreatedWhileSupervisorUnavailable, + ); + await pinnipedAuthenticator.refresh( + refreshRequest, + authCtxCreatedWhileSupervisorUnavailable, + ); + + expect(supervisorCalls).toEqual(1); + }); }); }); diff --git a/plugins/auth-backend-module-pinniped-provider/src/authenticator.ts b/plugins/auth-backend-module-pinniped-provider/src/authenticator.ts index 83c4204ad0..91aca3140e 100644 --- a/plugins/auth-backend-module-pinniped-provider/src/authenticator.ts +++ b/plugins/auth-backend-module-pinniped-provider/src/authenticator.ts @@ -13,6 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +import { Config } from '@backstage/config'; import { PassportDoneCallback } from '@backstage/plugin-auth-node'; import { createOAuthAuthenticator, @@ -24,7 +25,9 @@ import { Issuer, TokenSet, Strategy as OidcStrategy, + BaseClient, } from 'openid-client'; +import { DateTime } from 'luxon'; const rfc8693TokenExchange = async ({ subject_token, @@ -53,22 +56,74 @@ const rfc8693TokenExchange = async ({ }); }; -/** @public */ -export const pinnipedAuthenticator = createOAuthAuthenticator({ - defaultProfileTransform: async (_r, _c) => ({ profile: {} }), - async initialize({ callbackUrl, config }) { +class PinnipedStrategyFactory { + private readonly callbackUrl: string; + private readonly config: Config; + private strategyPromise: Promise<{ + providerStrategy: OidcStrategy<{ tokenset: TokenSet }, BaseClient>; + client: BaseClient; + }>; + + private cachedPromise?: Promise<{ + providerStrategy: OidcStrategy<{ tokenset: TokenSet }, BaseClient>; + client: BaseClient; + }>; + private cachedPromiseExpiry?: Date; + + constructor(callbackUrl: string, config: Config) { + this.callbackUrl = callbackUrl; + this.config = config; + this.strategyPromise = this.buildStrategy(); + } + + // need some sort of unit test to capture that a cached strategy will retry if expiration date is expired + public async getStrategy(): Promise<{ + providerStrategy: OidcStrategy<{ tokenset: TokenSet }, BaseClient>; + client: BaseClient; + }> { + if (this.cachedPromise) { + if ( + this.cachedPromiseExpiry && + DateTime.fromJSDate(this.cachedPromiseExpiry) > DateTime.local() + ) { + return this.cachedPromise; + } + // cachedPromise has expired, generate new strategy + delete this.cachedPromise; + } + + this.cachedPromise = this.strategyPromise; + this.cachedPromiseExpiry = DateTime.utc() + .plus({ seconds: 3600 }) + .toJSDate(); + try { + // if we fail to generate a strategy, retry and overwrite strategy + await this.strategyPromise; + } catch (error) { + this.strategyPromise = this.buildStrategy(); + delete this.cachedPromise; + delete this.cachedPromiseExpiry; + } + + return this.strategyPromise; + } + + private async buildStrategy(): Promise<{ + providerStrategy: OidcStrategy<{ tokenset: TokenSet }, BaseClient>; + client: BaseClient; + }> { const issuer = await Issuer.discover( - `${config.getString( + `${this.config.getString( 'federationDomain', )}/.well-known/openid-configuration`, ); const client = new issuer.Client({ - access_type: 'offline', // this option must be passed to provider to receive a refresh token - client_id: config.getString('clientId'), - client_secret: config.getString('clientSecret'), - redirect_uris: [callbackUrl], + access_type: 'offline', + client_id: this.config.getString('clientId'), + client_secret: this.config.getString('clientSecret'), + redirect_uris: [this.callbackUrl], response_types: ['code'], - scope: config.getOptionalString('scope') || '', + scope: this.config.getOptionalString('scope') || '', id_token_signed_response_alg: 'ES256', }); const providerStrategy = new OidcStrategy( @@ -88,12 +143,18 @@ export const pinnipedAuthenticator = createOAuthAuthenticator({ done(undefined, { tokenset }, {}); }, ); - return { providerStrategy, client }; - }, + } +} - async start(input, ctx) { - const { providerStrategy } = await ctx; +/** @public */ +export const pinnipedAuthenticator = createOAuthAuthenticator({ + defaultProfileTransform: async (_r, _c) => ({ profile: {} }), + initialize({ callbackUrl, config }) { + return new PinnipedStrategyFactory(callbackUrl, config); + }, + async start(input, ctx): Promise<{ url: string; status?: number }> { + const { providerStrategy } = await ctx.getStrategy(); const stringifiedAudience = input.req.query?.audience as string; const decodedState = decodeOAuthState(input.state); const state = { ...decodedState, audience: stringifiedAudience }; @@ -117,7 +178,7 @@ export const pinnipedAuthenticator = createOAuthAuthenticator({ }, async authenticate(input, ctx) { - const { providerStrategy } = await ctx; + const { providerStrategy } = await ctx.getStrategy(); const { req } = input; const { searchParams } = new URL(req.url, 'https://pinniped.com'); const stateParam = searchParams.get('state'); @@ -132,7 +193,7 @@ export const pinnipedAuthenticator = createOAuthAuthenticator({ ? rfc8693TokenExchange({ subject_token: user.tokenset.access_token, target_audience: audience, - ctx, + ctx: ctx.getStrategy(), }).catch(err => reject( new Error( @@ -172,7 +233,7 @@ export const pinnipedAuthenticator = createOAuthAuthenticator({ }, async refresh(input, ctx) { - const { client } = await ctx; + const { client } = await ctx.getStrategy(); const tokenset = await client.refresh(input.refreshToken); return new Promise((resolve, reject) => { diff --git a/yarn.lock b/yarn.lock index 956c7a7895..fef1b6c95a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5038,6 +5038,7 @@ __metadata: express-promise-router: ^4.1.1 express-session: ^1.17.3 jose: ^4.14.6 + luxon: ^3.4.3 msw: ^1.3.0 openid-client: ^5.4.3 passport: ^0.6.0 From 33524a870a39ee82d520175d375fd653e2f5b29e Mon Sep 17 00:00:00 2001 From: Ruben Vallejo Date: Thu, 26 Oct 2023 11:25:49 -0400 Subject: [PATCH 2/4] Add oidc metadata cache with passing unit tests Signed-off-by: Ruben Vallejo --- .../src/authenticator.test.ts | 149 +++++++++++++++++- .../src/authenticator.ts | 15 +- 2 files changed, 150 insertions(+), 14 deletions(-) diff --git a/plugins/auth-backend-module-pinniped-provider/src/authenticator.test.ts b/plugins/auth-backend-module-pinniped-provider/src/authenticator.test.ts index 8b4c092e3a..b9703df619 100644 --- a/plugins/auth-backend-module-pinniped-provider/src/authenticator.test.ts +++ b/plugins/auth-backend-module-pinniped-provider/src/authenticator.test.ts @@ -21,13 +21,14 @@ import { decodeOAuthState, encodeOAuthState, } from '@backstage/plugin-auth-node'; -import { pinnipedAuthenticator, getIssuer } from './authenticator'; +import { pinnipedAuthenticator } from './authenticator'; import { setupServer } from 'msw/node'; import { setupRequestMockHandlers } from '@backstage/backend-test-utils'; import { ConfigReader } from '@backstage/config'; import { JWK, SignJWT, exportJWK, generateKeyPair } from 'jose'; import { rest } from 'msw'; import express from 'express'; +import { DateTime } from 'luxon'; describe('pinnipedAuthenticator', () => { let authCtx: any; @@ -85,6 +86,7 @@ describe('pinnipedAuthenticator', () => { beforeEach(() => { jest.clearAllMocks(); + jest.restoreAllMocks(); mswServer.use( rest.get( @@ -340,7 +342,7 @@ describe('pinnipedAuthenticator', () => { expect(response.url).toMatch('https://pinniped.test/oauth2/authorize'); }); - it('only refreshes metadata after a failure', async () => { + it('caches oidc metadata after a success', async () => { mswServer.use( rest.get( 'https://federationDomain.test/.well-known/openid-configuration', @@ -385,6 +387,45 @@ describe('pinnipedAuthenticator', () => { expect(supervisorCalls).toEqual(1); }); + + it('refreshes oidc metadata when current one in cache expires', async () => { + let supervisorCalls: number = 0; + const fixedTime = DateTime.local(); + jest.spyOn(DateTime, 'local').mockImplementation(() => fixedTime); + + mswServer.use( + rest.get( + 'https://federationDomain.test/.well-known/openid-configuration', + (_req, res, ctx) => { + supervisorCalls += 1; + return res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(issuerMetadata), + ); + }, + ), + ); + + authCtx = pinnipedAuthenticator.initialize({ + callbackUrl: 'https://backstage.test/callback', + config: new ConfigReader({ + federationDomain: 'https://federationDomain.test', + clientId: 'clientId', + clientSecret: 'clientSecret', + }), + }); + + await pinnipedAuthenticator.start(startRequest, authCtx); + + jest + .spyOn(DateTime, 'local') + .mockImplementation(() => fixedTime.plus({ seconds: 60000 })); + + await pinnipedAuthenticator.start(startRequest, authCtx); + + expect(supervisorCalls).toEqual(2); + }); }); describe('#authenticate', () => { @@ -603,7 +644,7 @@ describe('pinnipedAuthenticator', () => { expect(response.session.accessToken).toEqual('accessToken'); }); - it('only refreshes metadata after a failure', async () => { + it('refreshes metadata after a failure', async () => { mswServer.use( rest.get( 'https://federationDomain.test/.well-known/openid-configuration', @@ -621,13 +662,13 @@ describe('pinnipedAuthenticator', () => { }), }); - let supervisorCalls: number = 0; + let metadataCalls: number = 0; mswServer.use( rest.get( 'https://federationDomain.test/.well-known/openid-configuration', (_req, res, ctx) => { - supervisorCalls += 1; + metadataCalls += 1; return res( ctx.status(200), ctx.set('Content-Type', 'application/json'), @@ -641,6 +682,7 @@ describe('pinnipedAuthenticator', () => { handlerRequest, authCtxCreatedWhileSupervisorUnavailable, ); + await pinnipedAuthenticator.authenticate( { req: { @@ -658,7 +700,61 @@ describe('pinnipedAuthenticator', () => { authCtxCreatedWhileSupervisorUnavailable, ); - expect(supervisorCalls).toEqual(1); + expect(metadataCalls).toEqual(1); + }); + + it('refreshes oidc metadata when current one in cache expires', async () => { + let supervisorCalls: number = 0; + const fixedTime = DateTime.local(); + jest.spyOn(DateTime, 'local').mockImplementation(() => fixedTime); + + mswServer.use( + rest.get( + 'https://federationDomain.test/.well-known/openid-configuration', + (_req, res, ctx) => { + supervisorCalls += 1; + return res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(issuerMetadata), + ); + }, + ), + ); + + authCtx = pinnipedAuthenticator.initialize({ + callbackUrl: 'https://backstage.test/callback', + config: new ConfigReader({ + federationDomain: 'https://federationDomain.test', + clientId: 'clientId', + clientSecret: 'clientSecret', + }), + }); + + await pinnipedAuthenticator.authenticate(handlerRequest, authCtx); + + jest + .spyOn(DateTime, 'local') + .mockImplementation(() => fixedTime.plus({ seconds: 60000 })); + + await pinnipedAuthenticator.authenticate( + { + req: { + method: 'GET', + url: `https://test?code=authorization_code&state=${encodeOAuthState( + oauthState, + )}`, + session: { + 'oidc:pinniped.test': { + state: encodeOAuthState(oauthState), + }, + }, + } as unknown as express.Request, + }, + authCtx, + ); + + expect(supervisorCalls).toEqual(2); }); }); @@ -737,7 +833,7 @@ describe('pinnipedAuthenticator', () => { expect(response.session.accessToken).toEqual('accessToken'); }); - it('only refreshes metadata after a failure', async () => { + it('caches oidc metadata after a success', async () => { mswServer.use( rest.get( 'https://federationDomain.test/.well-known/openid-configuration', @@ -782,5 +878,44 @@ describe('pinnipedAuthenticator', () => { expect(supervisorCalls).toEqual(1); }); + + it('refreshes oidc metadata when current one in cache expires', async () => { + let supervisorCalls: number = 0; + const fixedTime = DateTime.local(); + jest.spyOn(DateTime, 'local').mockImplementation(() => fixedTime); + + mswServer.use( + rest.get( + 'https://federationDomain.test/.well-known/openid-configuration', + (_req, res, ctx) => { + supervisorCalls += 1; + return res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(issuerMetadata), + ); + }, + ), + ); + + authCtx = pinnipedAuthenticator.initialize({ + callbackUrl: 'https://backstage.test/callback', + config: new ConfigReader({ + federationDomain: 'https://federationDomain.test', + clientId: 'clientId', + clientSecret: 'clientSecret', + }), + }); + + await pinnipedAuthenticator.refresh(refreshRequest, authCtx); + + jest + .spyOn(DateTime, 'local') + .mockImplementation(() => fixedTime.plus({ seconds: 60000 })); + + await pinnipedAuthenticator.refresh(refreshRequest, authCtx); + + expect(supervisorCalls).toEqual(2); + }); }); }); diff --git a/plugins/auth-backend-module-pinniped-provider/src/authenticator.ts b/plugins/auth-backend-module-pinniped-provider/src/authenticator.ts index 91aca3140e..6aae150188 100644 --- a/plugins/auth-backend-module-pinniped-provider/src/authenticator.ts +++ b/plugins/auth-backend-module-pinniped-provider/src/authenticator.ts @@ -76,7 +76,6 @@ class PinnipedStrategyFactory { this.strategyPromise = this.buildStrategy(); } - // need some sort of unit test to capture that a cached strategy will retry if expiration date is expired public async getStrategy(): Promise<{ providerStrategy: OidcStrategy<{ tokenset: TokenSet }, BaseClient>; client: BaseClient; @@ -88,18 +87,20 @@ class PinnipedStrategyFactory { ) { return this.cachedPromise; } - // cachedPromise has expired, generate new strategy + // cachedPromise has expired, remove promise from cache and regenerate strategy + this.strategyPromise = this.buildStrategy(); delete this.cachedPromise; } - this.cachedPromise = this.strategyPromise; - this.cachedPromiseExpiry = DateTime.utc() - .plus({ seconds: 3600 }) - .toJSDate(); try { - // if we fail to generate a strategy, retry and overwrite strategy + // if strategy is generated successfully, save it to cache await this.strategyPromise; + this.cachedPromise = this.strategyPromise; + this.cachedPromiseExpiry = DateTime.utc() + .plus({ seconds: 3600 }) + .toJSDate(); } catch (error) { + // if we fail to generate a strategy, retry and overwrite strategy this.strategyPromise = this.buildStrategy(); delete this.cachedPromise; delete this.cachedPromiseExpiry; From a8f6afda4a26e8242104ea850d5b698b6c7bc980 Mon Sep 17 00:00:00 2001 From: Ruben Vallejo Date: Mon, 30 Oct 2023 13:42:52 -0400 Subject: [PATCH 3/4] PR-chore: changeset,api-report Signed-off-by: Ruben Vallejo --- .changeset/metal-hornets-drum.md | 5 + .../api-report.md | 16 ++- .../src/authenticator.test.ts | 128 +++--------------- .../src/authenticator.ts | 9 +- .../src/index.ts | 2 +- 5 files changed, 42 insertions(+), 118 deletions(-) create mode 100644 .changeset/metal-hornets-drum.md diff --git a/.changeset/metal-hornets-drum.md b/.changeset/metal-hornets-drum.md new file mode 100644 index 0000000000..c7ee859fe8 --- /dev/null +++ b/.changeset/metal-hornets-drum.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-auth-backend-module-pinniped-provider': patch +--- + +Introduced PinnipedStrategyCache to act as a metadata cache for the PinnipedAuthenticator. diff --git a/plugins/auth-backend-module-pinniped-provider/api-report.md b/plugins/auth-backend-module-pinniped-provider/api-report.md index b9b993bd1e..430099713e 100644 --- a/plugins/auth-backend-module-pinniped-provider/api-report.md +++ b/plugins/auth-backend-module-pinniped-provider/api-report.md @@ -5,6 +5,7 @@ ```ts import { BackendFeature } from '@backstage/backend-plugin-api'; import { BaseClient } from 'openid-client'; +import { Config } from '@backstage/config'; import { OAuthAuthenticator } from '@backstage/plugin-auth-node'; import { Strategy } from 'openid-client'; import { TokenSet } from 'openid-client'; @@ -14,7 +15,15 @@ export const authModulePinnipedProvider: () => BackendFeature; // @public (undocumented) export const pinnipedAuthenticator: OAuthAuthenticator< - Promise<{ + PinnipedStrategyCache, + unknown +>; + +// @public (undocumented) +export class PinnipedStrategyCache { + constructor(callbackUrl: string, config: Config); + // (undocumented) + getStrategy(): Promise<{ providerStrategy: Strategy< { tokenset: TokenSet; @@ -22,7 +31,6 @@ export const pinnipedAuthenticator: OAuthAuthenticator< BaseClient >; client: BaseClient; - }>, - unknown ->; + }>; +} ``` diff --git a/plugins/auth-backend-module-pinniped-provider/src/authenticator.test.ts b/plugins/auth-backend-module-pinniped-provider/src/authenticator.test.ts index b9703df619..2ecd54e80a 100644 --- a/plugins/auth-backend-module-pinniped-provider/src/authenticator.test.ts +++ b/plugins/auth-backend-module-pinniped-provider/src/authenticator.test.ts @@ -146,18 +146,8 @@ describe('pinnipedAuthenticator', () => { }); describe('#initialize', () => { - it('always returns a PinnipedStrategyFactory', async () => { - const pinnipedStrategyFactory = pinnipedAuthenticator.initialize({ - callbackUrl: 'https://backstage.test/callback', - config: new ConfigReader({ - federationDomain: 'https://federationDomain.test', - clientId: 'clientId', - clientSecret: 'clientSecret', - }), - }); - - const { providerStrategy, client } = - await pinnipedStrategyFactory.getStrategy(); + it('always returns a PinnipedStrategyCache', async () => { + const { providerStrategy, client } = await authCtx.getStrategy(); expect(providerStrategy).toBeDefined(); expect(client.issuer.authorization_endpoint).toMatch( @@ -343,24 +333,8 @@ describe('pinnipedAuthenticator', () => { }); it('caches oidc metadata after a success', async () => { - mswServer.use( - rest.get( - 'https://federationDomain.test/.well-known/openid-configuration', - (_req, res, _ctx) => res.networkError('Timeout'), - ), - ); - - const authCtxCreatedWhileSupervisorUnavailable = - pinnipedAuthenticator.initialize({ - callbackUrl: 'https://backstage.test/callback', - config: new ConfigReader({ - federationDomain: 'https://federationDomain.test', - clientId: 'clientId', - clientSecret: 'clientSecret', - }), - }); - - let supervisorCalls: number = 0; + // we start with 1 because the supervisor was called once already when we initialize. + let supervisorCalls: number = 1; mswServer.use( rest.get( @@ -376,20 +350,15 @@ describe('pinnipedAuthenticator', () => { ), ); - await pinnipedAuthenticator.start( - startRequest, - authCtxCreatedWhileSupervisorUnavailable, - ); - await pinnipedAuthenticator.start( - startRequest, - authCtxCreatedWhileSupervisorUnavailable, - ); + await pinnipedAuthenticator.start(startRequest, authCtx); + await pinnipedAuthenticator.start(startRequest, authCtx); expect(supervisorCalls).toEqual(1); }); it('refreshes oidc metadata when current one in cache expires', async () => { - let supervisorCalls: number = 0; + // we start with 1 because the supervisor was called once already when we initialize. + let supervisorCalls: number = 1; const fixedTime = DateTime.local(); jest.spyOn(DateTime, 'local').mockImplementation(() => fixedTime); @@ -407,15 +376,6 @@ describe('pinnipedAuthenticator', () => { ), ); - authCtx = pinnipedAuthenticator.initialize({ - callbackUrl: 'https://backstage.test/callback', - config: new ConfigReader({ - federationDomain: 'https://federationDomain.test', - clientId: 'clientId', - clientSecret: 'clientSecret', - }), - }); - await pinnipedAuthenticator.start(startRequest, authCtx); jest @@ -644,31 +604,14 @@ describe('pinnipedAuthenticator', () => { expect(response.session.accessToken).toEqual('accessToken'); }); - it('refreshes metadata after a failure', async () => { - mswServer.use( - rest.get( - 'https://federationDomain.test/.well-known/openid-configuration', - (_req, res, _ctx) => res.networkError('Timeout'), - ), - ); - - const authCtxCreatedWhileSupervisorUnavailable = - pinnipedAuthenticator.initialize({ - callbackUrl: 'https://backstage.test/callback', - config: new ConfigReader({ - federationDomain: 'https://federationDomain.test', - clientId: 'clientId', - clientSecret: 'clientSecret', - }), - }); - - let metadataCalls: number = 0; + it('caches oidc metadata after a success', async () => { + let supervisorCalls: number = 1; mswServer.use( rest.get( 'https://federationDomain.test/.well-known/openid-configuration', (_req, res, ctx) => { - metadataCalls += 1; + supervisorCalls += 1; return res( ctx.status(200), ctx.set('Content-Type', 'application/json'), @@ -678,10 +621,7 @@ describe('pinnipedAuthenticator', () => { ), ); - await pinnipedAuthenticator.authenticate( - handlerRequest, - authCtxCreatedWhileSupervisorUnavailable, - ); + await pinnipedAuthenticator.authenticate(handlerRequest, authCtx); await pinnipedAuthenticator.authenticate( { @@ -697,10 +637,10 @@ describe('pinnipedAuthenticator', () => { }, } as unknown as express.Request, }, - authCtxCreatedWhileSupervisorUnavailable, + authCtx, ); - expect(metadataCalls).toEqual(1); + expect(supervisorCalls).toEqual(1); }); it('refreshes oidc metadata when current one in cache expires', async () => { @@ -834,24 +774,7 @@ describe('pinnipedAuthenticator', () => { }); it('caches oidc metadata after a success', async () => { - mswServer.use( - rest.get( - 'https://federationDomain.test/.well-known/openid-configuration', - (_req, res, _ctx) => res.networkError('Timeout'), - ), - ); - - const authCtxCreatedWhileSupervisorUnavailable = - pinnipedAuthenticator.initialize({ - callbackUrl: 'https://backstage.test/callback', - config: new ConfigReader({ - federationDomain: 'https://federationDomain.test', - clientId: 'clientId', - clientSecret: 'clientSecret', - }), - }); - - let supervisorCalls: number = 0; + let supervisorCalls: number = 1; mswServer.use( rest.get( @@ -867,20 +790,14 @@ describe('pinnipedAuthenticator', () => { ), ); - await pinnipedAuthenticator.refresh( - refreshRequest, - authCtxCreatedWhileSupervisorUnavailable, - ); - await pinnipedAuthenticator.refresh( - refreshRequest, - authCtxCreatedWhileSupervisorUnavailable, - ); + await pinnipedAuthenticator.refresh(refreshRequest, authCtx); + await pinnipedAuthenticator.refresh(refreshRequest, authCtx); expect(supervisorCalls).toEqual(1); }); it('refreshes oidc metadata when current one in cache expires', async () => { - let supervisorCalls: number = 0; + let supervisorCalls: number = 1; const fixedTime = DateTime.local(); jest.spyOn(DateTime, 'local').mockImplementation(() => fixedTime); @@ -898,15 +815,6 @@ describe('pinnipedAuthenticator', () => { ), ); - authCtx = pinnipedAuthenticator.initialize({ - callbackUrl: 'https://backstage.test/callback', - config: new ConfigReader({ - federationDomain: 'https://federationDomain.test', - clientId: 'clientId', - clientSecret: 'clientSecret', - }), - }); - await pinnipedAuthenticator.refresh(refreshRequest, authCtx); jest diff --git a/plugins/auth-backend-module-pinniped-provider/src/authenticator.ts b/plugins/auth-backend-module-pinniped-provider/src/authenticator.ts index 6aae150188..e07eaba759 100644 --- a/plugins/auth-backend-module-pinniped-provider/src/authenticator.ts +++ b/plugins/auth-backend-module-pinniped-provider/src/authenticator.ts @@ -56,7 +56,10 @@ const rfc8693TokenExchange = async ({ }); }; -class PinnipedStrategyFactory { +const OIDC_METADATA_TTL_SECONDS = 3600; + +/** @public */ +export class PinnipedStrategyCache { private readonly callbackUrl: string; private readonly config: Config; private strategyPromise: Promise<{ @@ -97,7 +100,7 @@ class PinnipedStrategyFactory { await this.strategyPromise; this.cachedPromise = this.strategyPromise; this.cachedPromiseExpiry = DateTime.utc() - .plus({ seconds: 3600 }) + .plus({ seconds: OIDC_METADATA_TTL_SECONDS }) .toJSDate(); } catch (error) { // if we fail to generate a strategy, retry and overwrite strategy @@ -152,7 +155,7 @@ class PinnipedStrategyFactory { export const pinnipedAuthenticator = createOAuthAuthenticator({ defaultProfileTransform: async (_r, _c) => ({ profile: {} }), initialize({ callbackUrl, config }) { - return new PinnipedStrategyFactory(callbackUrl, config); + return new PinnipedStrategyCache(callbackUrl, config); }, async start(input, ctx): Promise<{ url: string; status?: number }> { const { providerStrategy } = await ctx.getStrategy(); diff --git a/plugins/auth-backend-module-pinniped-provider/src/index.ts b/plugins/auth-backend-module-pinniped-provider/src/index.ts index 9a2ca6727e..df4cd58603 100644 --- a/plugins/auth-backend-module-pinniped-provider/src/index.ts +++ b/plugins/auth-backend-module-pinniped-provider/src/index.ts @@ -20,5 +20,5 @@ * @packageDocumentation */ -export { pinnipedAuthenticator } from './authenticator'; +export { pinnipedAuthenticator, PinnipedStrategyCache } from './authenticator'; export { authModulePinnipedProvider } from './module'; From e6731f550629c3b4cc14a243a580405f3a5bfe2c Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 14 Nov 2023 11:49:19 +0100 Subject: [PATCH 4/4] Update .changeset/metal-hornets-drum.md Signed-off-by: Patrik Oldsberg --- .changeset/metal-hornets-drum.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/metal-hornets-drum.md b/.changeset/metal-hornets-drum.md index c7ee859fe8..04a7302f8a 100644 --- a/.changeset/metal-hornets-drum.md +++ b/.changeset/metal-hornets-drum.md @@ -2,4 +2,4 @@ '@backstage/plugin-auth-backend-module-pinniped-provider': patch --- -Introduced PinnipedStrategyCache to act as a metadata cache for the PinnipedAuthenticator. +Introduced metadata cache for the `pinniped` provider.