From 5f1c1c2edf2b3f9c4463dc7d58afb1e8e7e843dd Mon Sep 17 00:00:00 2001 From: Brian Leathem Date: Mon, 23 Nov 2020 13:34:44 -0800 Subject: [PATCH 1/6] Fixed the OIDC token refresh --- .../src/providers/oidc/provider.ts | 51 ++++++++----------- 1 file changed, 21 insertions(+), 30 deletions(-) diff --git a/plugins/auth-backend/src/providers/oidc/provider.ts b/plugins/auth-backend/src/providers/oidc/provider.ts index 5ebb859f2f..765fd8627e 100644 --- a/plugins/auth-backend/src/providers/oidc/provider.ts +++ b/plugins/auth-backend/src/providers/oidc/provider.ts @@ -33,10 +33,8 @@ import { OAuthRefreshRequest, } from '../../lib/oauth'; import { - executeFetchUserProfileStrategy, executeFrameHandlerStrategy, executeRedirectStrategy, - executeRefreshTokenStrategy, PassportDoneCallback, } from '../../lib/passport'; import { RedirectInfo, AuthProviderFactory, ProfileInfo } from '../types'; @@ -45,20 +43,25 @@ type PrivateInfo = { refreshToken: string; }; +type OidcImpl = { + strategy: OidcStrategy; + client: Client; +}; + export type OidcAuthProviderOptions = OAuthProviderOptions & { metadataUrl: string; tokenSignedResponseAlg?: string; }; export class OidcAuthProvider implements OAuthHandlers { - readonly _strategy: Promise>; + readonly _implementation: Promise; constructor(options: OidcAuthProviderOptions) { - this._strategy = this.setupStrategy(options); + this._implementation = this.setupStrategy(options); } async start(req: OAuthStartRequest): Promise { - const strategy = await this._strategy; + const { strategy } = await this._implementation; return await executeRedirectStrategy(req, strategy, { accessType: 'offline', prompt: 'none', @@ -70,7 +73,7 @@ export class OidcAuthProvider implements OAuthHandlers { async handler( req: express.Request, ): Promise<{ response: OAuthResponse; refreshToken: string }> { - const strategy = await this._strategy; + const { strategy } = await this._implementation; const { response, privateInfo } = await executeFrameHandlerStrategy< OAuthResponse, PrivateInfo @@ -83,31 +86,19 @@ export class OidcAuthProvider implements OAuthHandlers { } async refresh(req: OAuthRefreshRequest): Promise { - const strategy = await this._strategy; - const refreshTokenResponse = await executeRefreshTokenStrategy( - strategy, - req.refreshToken, - req.scope, - ); - const { - accessToken, - params, - refreshToken: updatedRefreshToken, - } = refreshTokenResponse; - - const profile = await executeFetchUserProfileStrategy( - strategy, - accessToken, - params.id_token, - ); + const { client } = await this._implementation; + const tokenset = await client.refresh(req.refreshToken); + if (!tokenset.access_token) { + throw new Error('Refresh failed'); + } + const profile = await client.userinfo(tokenset.access_token); return this.populateIdentity({ providerInfo: { - accessToken, - refreshToken: updatedRefreshToken, - idToken: params.id_token, - expiresInSeconds: params.expires_in, - scope: params.scope, + accessToken: tokenset.access_token, + refreshToken: tokenset.refresh_token, + expiresInSeconds: tokenset.expires_at, + scope: tokenset.scope || '', }, profile, }); @@ -115,7 +106,7 @@ export class OidcAuthProvider implements OAuthHandlers { private async setupStrategy( options: OidcAuthProviderOptions, - ): Promise> { + ): Promise { const issuer = await Issuer.discover(options.metadataUrl); const client = new issuer.Client({ client_id: options.clientId, @@ -159,7 +150,7 @@ export class OidcAuthProvider implements OAuthHandlers { }, ); strategy.error = console.error; - return strategy; + return { strategy, client }; } // Use this function to grab the user profile info from the token From 636cc58dca2ddd8c19ce25e128b682295f8071d8 Mon Sep 17 00:00:00 2001 From: Brian Leathem Date: Mon, 23 Nov 2020 13:41:37 -0800 Subject: [PATCH 2/6] Fixed a TS type error in the OIDC provider test --- plugins/auth-backend/src/providers/oidc/provider.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/auth-backend/src/providers/oidc/provider.test.ts b/plugins/auth-backend/src/providers/oidc/provider.test.ts index ade69255b7..5073cd88d1 100644 --- a/plugins/auth-backend/src/providers/oidc/provider.test.ts +++ b/plugins/auth-backend/src/providers/oidc/provider.test.ts @@ -54,7 +54,7 @@ describe('OidcAuthProvider', () => { .get('/.well-known/openid-configuration') .reply(200, issuerMetadata); const provider = new OidcAuthProvider(clientMetadata); - const strategy = ((await provider._strategy) as any) as { + const strategy = ((await (provider as any)._strategy) as any) as { _client: ClientMetadata; _issuer: IssuerMetadata; }; From e3ad37e39d126a0c3dc9ccc8a22c061137ff0098 Mon Sep 17 00:00:00 2001 From: Brian Leathem Date: Mon, 23 Nov 2020 14:48:24 -0800 Subject: [PATCH 3/6] Fix broken tests --- .../src/providers/oidc/provider.test.ts | 49 ++++++++++++------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/plugins/auth-backend/src/providers/oidc/provider.test.ts b/plugins/auth-backend/src/providers/oidc/provider.test.ts index 5073cd88d1..1851d1a473 100644 --- a/plugins/auth-backend/src/providers/oidc/provider.test.ts +++ b/plugins/auth-backend/src/providers/oidc/provider.test.ts @@ -22,7 +22,7 @@ import { createOidcProvider, OidcAuthProvider } from './provider'; import { JWT, JWK } from 'jose'; import { AuthProviderFactoryOptions } from '../types'; import { Config } from '@backstage/config'; -import { OAuthAdapter } from '../../lib/oauth'; +import { OAuthAdapter, OAuthStartRequest } from '../../lib/oauth'; const issuerMetadata = { issuer: 'https://oidc.test', @@ -54,9 +54,11 @@ describe('OidcAuthProvider', () => { .get('/.well-known/openid-configuration') .reply(200, issuerMetadata); const provider = new OidcAuthProvider(clientMetadata); - const strategy = ((await (provider as any)._strategy) as any) as { - _client: ClientMetadata; - _issuer: IssuerMetadata; + const { strategy } = ((await provider._implementation) as any) as { + strategy: { + _client: ClientMetadata; + _issuer: IssuerMetadata; + }; }; // Assert that the expected request to the metadaurl was made. expect(scope.isDone()).toBeTruthy(); @@ -89,27 +91,38 @@ describe('OidcAuthProvider', () => { const provider = new OidcAuthProvider(clientMetadata); const req = { method: 'GET', - url: '/?code=test2', + url: 'https://oidc.test/?code=test2', session: ({ 'oidc:oidc.test': 'test' } as any) as Session, } as express.Request; await provider.handler(req); expect(scope.isDone()).toBeTruthy(); }); - const options = { - globalConfig: { - appUrl: 'https://oidc.test', - baseUrl: 'https://oidc.test', - }, - config: ({ - keys: jest.fn(() => ['test']), - getConfig: jest.fn(() => ({ getString: () => '' })), - } as any) as Config, - } as AuthProviderFactoryOptions; - - it('createOidcProvider', () => { + it('createOidcProvider', async () => { + const scope = nock('https://oidc.test') + .get('/.well-known/openid-configuration') + .reply(200, issuerMetadata); + const options = { + globalConfig: { + appUrl: 'https://oidc.test', + baseUrl: 'https://oidc.test', + }, + config: ({ + keys: jest.fn(() => ['test']), + getConfig: jest.fn(() => ({ + getString: (key: string) => { + const conf = { + ...clientMetadata, + metadataUrl: 'https://oidc.test/.well-known/openid-configuration', + } as any; + return conf[key] as string; + }, + })), + } as any) as Config, + } as AuthProviderFactoryOptions; const provider = createOidcProvider(options) as OAuthAdapter; - console.log(provider); expect(provider.start).toBeDefined(); + await new Promise(resolve => process.nextTick(resolve)); // advance a tick to give nock a chance to intercept the request + expect(scope.isDone()).toBeTruthy(); }); }); From 78177f24b7770e81ef414b64b29069606cac3ea8 Mon Sep 17 00:00:00 2001 From: Brian Leathem Date: Mon, 23 Nov 2020 14:54:24 -0800 Subject: [PATCH 4/6] Fixed a lint failure on the oidc provider test --- plugins/auth-backend/src/providers/oidc/provider.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/auth-backend/src/providers/oidc/provider.test.ts b/plugins/auth-backend/src/providers/oidc/provider.test.ts index 1851d1a473..3732e7b1a7 100644 --- a/plugins/auth-backend/src/providers/oidc/provider.test.ts +++ b/plugins/auth-backend/src/providers/oidc/provider.test.ts @@ -22,7 +22,7 @@ import { createOidcProvider, OidcAuthProvider } from './provider'; import { JWT, JWK } from 'jose'; import { AuthProviderFactoryOptions } from '../types'; import { Config } from '@backstage/config'; -import { OAuthAdapter, OAuthStartRequest } from '../../lib/oauth'; +import { OAuthAdapter } from '../../lib/oauth'; const issuerMetadata = { issuer: 'https://oidc.test', From cec3da2c901998a71074936a62ed9383dd314bf6 Mon Sep 17 00:00:00 2001 From: Brian Leathem Date: Mon, 23 Nov 2020 15:23:21 -0800 Subject: [PATCH 5/6] Used ts private instead of _ naming convention --- .../auth-backend/src/providers/oidc/provider.test.ts | 2 +- plugins/auth-backend/src/providers/oidc/provider.ts | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/plugins/auth-backend/src/providers/oidc/provider.test.ts b/plugins/auth-backend/src/providers/oidc/provider.test.ts index 3732e7b1a7..f2bd8dcb90 100644 --- a/plugins/auth-backend/src/providers/oidc/provider.test.ts +++ b/plugins/auth-backend/src/providers/oidc/provider.test.ts @@ -54,7 +54,7 @@ describe('OidcAuthProvider', () => { .get('/.well-known/openid-configuration') .reply(200, issuerMetadata); const provider = new OidcAuthProvider(clientMetadata); - const { strategy } = ((await provider._implementation) as any) as { + const { strategy } = ((await (provider as any).implementation) as any) as { strategy: { _client: ClientMetadata; _issuer: IssuerMetadata; diff --git a/plugins/auth-backend/src/providers/oidc/provider.ts b/plugins/auth-backend/src/providers/oidc/provider.ts index 765fd8627e..4f2054c5f6 100644 --- a/plugins/auth-backend/src/providers/oidc/provider.ts +++ b/plugins/auth-backend/src/providers/oidc/provider.ts @@ -54,14 +54,14 @@ export type OidcAuthProviderOptions = OAuthProviderOptions & { }; export class OidcAuthProvider implements OAuthHandlers { - readonly _implementation: Promise; + private readonly implementation: Promise; constructor(options: OidcAuthProviderOptions) { - this._implementation = this.setupStrategy(options); + this.implementation = this.setupStrategy(options); } async start(req: OAuthStartRequest): Promise { - const { strategy } = await this._implementation; + const { strategy } = await this.implementation; return await executeRedirectStrategy(req, strategy, { accessType: 'offline', prompt: 'none', @@ -73,7 +73,7 @@ export class OidcAuthProvider implements OAuthHandlers { async handler( req: express.Request, ): Promise<{ response: OAuthResponse; refreshToken: string }> { - const { strategy } = await this._implementation; + const { strategy } = await this.implementation; const { response, privateInfo } = await executeFrameHandlerStrategy< OAuthResponse, PrivateInfo @@ -86,7 +86,7 @@ export class OidcAuthProvider implements OAuthHandlers { } async refresh(req: OAuthRefreshRequest): Promise { - const { client } = await this._implementation; + const { client } = await this.implementation; const tokenset = await client.refresh(req.refreshToken); if (!tokenset.access_token) { throw new Error('Refresh failed'); From 9c9d12e8993bd196165578abb083c883d29e3d96 Mon Sep 17 00:00:00 2001 From: Brian Leathem Date: Tue, 24 Nov 2020 11:30:25 -0800 Subject: [PATCH 6/6] Added back the id_token when calling refresh --- plugins/auth-backend/src/providers/oidc/provider.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/plugins/auth-backend/src/providers/oidc/provider.ts b/plugins/auth-backend/src/providers/oidc/provider.ts index 4f2054c5f6..fbf38c69c4 100644 --- a/plugins/auth-backend/src/providers/oidc/provider.ts +++ b/plugins/auth-backend/src/providers/oidc/provider.ts @@ -97,7 +97,8 @@ export class OidcAuthProvider implements OAuthHandlers { providerInfo: { accessToken: tokenset.access_token, refreshToken: tokenset.refresh_token, - expiresInSeconds: tokenset.expires_at, + expiresInSeconds: tokenset.expires_in, + idToken: tokenset.id_token, scope: tokenset.scope || '', }, profile,