From 33524a870a39ee82d520175d375fd653e2f5b29e Mon Sep 17 00:00:00 2001 From: Ruben Vallejo Date: Thu, 26 Oct 2023 11:25:49 -0400 Subject: [PATCH] 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;