From 65fa16dee469792b1c6e890ed9c8f7291c2f5b58 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 14 Feb 2021 17:38:07 +0100 Subject: [PATCH] auth-backend: refactor profile transform to happen within handlers instead of verify callback --- plugins/auth-backend/src/lib/oauth/index.ts | 1 + plugins/auth-backend/src/lib/oauth/types.ts | 12 ++ .../passport/PassportStrategyHelper.test.ts | 2 +- .../lib/passport/PassportStrategyHelper.ts | 10 +- .../src/providers/auth0/provider.ts | 37 +++--- .../src/providers/github/provider.test.ts | 87 ++++++++------ .../src/providers/github/provider.ts | 94 +++++---------- .../src/providers/gitlab/provider.test.ts | 35 ++++-- .../src/providers/gitlab/provider.ts | 112 +++++++----------- .../src/providers/google/provider.ts | 37 +++--- .../src/providers/microsoft/provider.ts | 83 ++++++------- .../src/providers/oauth2/provider.ts | 34 +++--- .../src/providers/oidc/provider.ts | 56 +++++---- .../src/providers/okta/provider.ts | 38 +++--- .../src/providers/onelogin/provider.ts | 38 +++--- .../src/providers/saml/provider.ts | 34 +++--- 16 files changed, 351 insertions(+), 359 deletions(-) diff --git a/plugins/auth-backend/src/lib/oauth/index.ts b/plugins/auth-backend/src/lib/oauth/index.ts index 564a0e1e7c..bf3e0658e4 100644 --- a/plugins/auth-backend/src/lib/oauth/index.ts +++ b/plugins/auth-backend/src/lib/oauth/index.ts @@ -25,4 +25,5 @@ export type { OAuthState, OAuthStartRequest, OAuthRefreshRequest, + OAuthResult, } from './types'; diff --git a/plugins/auth-backend/src/lib/oauth/types.ts b/plugins/auth-backend/src/lib/oauth/types.ts index b2b7915a4e..dd477388bf 100644 --- a/plugins/auth-backend/src/lib/oauth/types.ts +++ b/plugins/auth-backend/src/lib/oauth/types.ts @@ -15,6 +15,7 @@ */ import express from 'express'; +import { Profile as PassportProfile } from 'passport'; import { AuthResponse, RedirectInfo } from '../../providers/types'; /** @@ -35,6 +36,17 @@ export type OAuthProviderOptions = { callbackUrl: string; }; +export type OAuthResult = { + fullProfile: PassportProfile; + params: { + id_token?: string; + scope: string; + expires_in: number; + }; + accessToken: string; + refreshToken?: string; +}; + export type OAuthResponse = AuthResponse; export type OAuthProviderInfo = { diff --git a/plugins/auth-backend/src/lib/passport/PassportStrategyHelper.test.ts b/plugins/auth-backend/src/lib/passport/PassportStrategyHelper.test.ts index f7a22e9ad9..c27eebbd9a 100644 --- a/plugins/auth-backend/src/lib/passport/PassportStrategyHelper.test.ts +++ b/plugins/auth-backend/src/lib/passport/PassportStrategyHelper.test.ts @@ -87,7 +87,7 @@ describe('PassportStrategyHelper', () => { expect(spyAuthenticate).toBeCalledTimes(1); await expect(frameHandlerStrategyPromise).resolves.toStrictEqual( expect.objectContaining({ - response: { accessToken: 'ACCESS_TOKEN' }, + result: { accessToken: 'ACCESS_TOKEN' }, privateInfo: { refreshToken: 'REFRESH_TOKEN' }, }), ); diff --git a/plugins/auth-backend/src/lib/passport/PassportStrategyHelper.ts b/plugins/auth-backend/src/lib/passport/PassportStrategyHelper.ts index 079f44151a..8819da0a2f 100644 --- a/plugins/auth-backend/src/lib/passport/PassportStrategyHelper.ts +++ b/plugins/auth-backend/src/lib/passport/PassportStrategyHelper.ts @@ -39,7 +39,7 @@ export const makeProfileInfo = ( } let picture: string | undefined = undefined; - if (profile.photos) { + if (profile.photos && profile.photos.length > 0) { const [firstPhoto] = profile.photos; picture = firstPhoto.value; } @@ -80,15 +80,15 @@ export const executeRedirectStrategy = async ( }); }; -export const executeFrameHandlerStrategy = async ( +export const executeFrameHandlerStrategy = async ( req: express.Request, providerStrategy: passport.Strategy, ) => { - return new Promise<{ response: T; privateInfo: PrivateInfo }>( + return new Promise<{ result: Result; privateInfo: PrivateInfo }>( (resolve, reject) => { const strategy = Object.create(providerStrategy); - strategy.success = (response: any, privateInfo: any) => { - resolve({ response, privateInfo }); + strategy.success = (result: any, privateInfo: any) => { + resolve({ result, privateInfo }); }; strategy.fail = ( info: { type: 'success' | 'error'; message?: string }, diff --git a/plugins/auth-backend/src/providers/auth0/provider.ts b/plugins/auth-backend/src/providers/auth0/provider.ts index 1244820139..2a2f743d12 100644 --- a/plugins/auth-backend/src/providers/auth0/provider.ts +++ b/plugins/auth-backend/src/providers/auth0/provider.ts @@ -26,6 +26,7 @@ import { OAuthStartRequest, encodeState, OAuthRefreshRequest, + OAuthResult, } from '../../lib/oauth'; import { executeFetchUserProfileStrategy, @@ -61,20 +62,16 @@ export class Auth0AuthProvider implements OAuthHandlers { accessToken: any, refreshToken: any, params: any, - rawProfile: passport.Profile, - done: PassportDoneCallback, + fullProfile: passport.Profile, + done: PassportDoneCallback, ) => { - const profile = makeProfileInfo(rawProfile, params.id_token); done( undefined, { - providerInfo: { - idToken: params.id_token, - accessToken, - scope: params.scope, - expiresInSeconds: params.expires_in, - }, - profile, + fullProfile, + accessToken, + refreshToken, + params, }, { refreshToken, @@ -96,13 +93,23 @@ export class Auth0AuthProvider implements OAuthHandlers { async handler( req: express.Request, ): Promise<{ response: OAuthResponse; refreshToken: string }> { - const { response, privateInfo } = await executeFrameHandlerStrategy< - OAuthResponse, + const { result, privateInfo } = await executeFrameHandlerStrategy< + OAuthResult, PrivateInfo >(req, this._strategy); + const profile = makeProfileInfo(result.fullProfile, result.params.id_token); + return { - response: await this.populateIdentity(response), + response: await this.populateIdentity({ + profile, + providerInfo: { + idToken: result.params.id_token, + accessToken: result.accessToken, + scope: result.params.scope, + expiresInSeconds: result.params.expires_in, + }, + }), refreshToken: privateInfo.refreshToken, }; } @@ -114,11 +121,11 @@ export class Auth0AuthProvider implements OAuthHandlers { req.scope, ); - const rawProfile = await executeFetchUserProfileStrategy( + const fullProfile = await executeFetchUserProfileStrategy( this._strategy, accessToken, ); - const profile = makeProfileInfo(rawProfile, params.id_token); + const profile = makeProfileInfo(fullProfile, params.id_token); return this.populateIdentity({ providerInfo: { diff --git a/plugins/auth-backend/src/providers/github/provider.test.ts b/plugins/auth-backend/src/providers/github/provider.test.ts index 61d4bb887f..6394ed9f2a 100644 --- a/plugins/auth-backend/src/providers/github/provider.test.ts +++ b/plugins/auth-backend/src/providers/github/provider.test.ts @@ -14,13 +14,31 @@ * limitations under the License. */ +import { Profile as PassportProfile } from 'passport'; import { GithubAuthProvider } from './provider'; +import * as helpers from '../../lib/passport'; +import { OAuthResult } from '../../lib/oauth'; + +const mockFrameHandler = (jest.spyOn( + helpers, + 'executeFrameHandlerStrategy', +) as unknown) as jest.MockedFunction< + () => Promise<{ + result: Omit & { params: { scope: string } }; + }> +>; describe('GithubAuthProvider', () => { + const provider = new GithubAuthProvider({ + callbackUrl: 'mock', + clientId: 'mock', + clientSecret: 'mock', + }); + describe('should transform to type OAuthResponse', () => { - it('when all fields are present, it should be able to map them', () => { + it('when all fields are present, it should be able to map them', async () => { const accessToken = '19xasczxcm9n7gacn9jdgm19me'; - const rawProfile = { + const fullProfile = { id: 'uid-123', username: 'jimmymarkum', provider: 'github', @@ -59,18 +77,17 @@ describe('GithubAuthProvider', () => { 'https://a1cf74336522e87f135f-2f21ace9a6cf0052456644b80fa06d4f.ssl.cf2.rackcdn.com/images/characters_opt/p-mystic-river-sean-penn.jpg', }, }; - expect( - GithubAuthProvider.transformOAuthResponse( - accessToken, - rawProfile, - params, - ), - ).toEqual(expected); + + mockFrameHandler.mockResolvedValueOnce({ + result: { fullProfile, accessToken, params }, + }); + const { response } = await provider.handler({} as any); + expect(response).toEqual(expected); }); - it('when "email" is missing, it should be able to create the profile without it', () => { + it('when "email" is missing, it should be able to create the profile without it', async () => { const accessToken = '19xasczxcm9n7gacn9jdgm19me'; - const rawProfile = { + const fullProfile = ({ id: 'uid-123', username: 'jimmymarkum', provider: 'github', @@ -82,7 +99,7 @@ describe('GithubAuthProvider', () => { 'https://a1cf74336522e87f135f-2f21ace9a6cf0052456644b80fa06d4f.ssl.cf2.rackcdn.com/images/characters_opt/p-mystic-river-sean-penn.jpg', }, ], - }; + } as unknown) as PassportProfile; const params = { scope: 'read:scope', @@ -105,18 +122,16 @@ describe('GithubAuthProvider', () => { }, }; - expect( - GithubAuthProvider.transformOAuthResponse( - accessToken, - rawProfile, - params, - ), - ).toEqual(expected); + mockFrameHandler.mockResolvedValueOnce({ + result: { fullProfile, accessToken, params }, + }); + const { response } = await provider.handler({} as any); + expect(response).toEqual(expected); }); - it('when "displayName" is missing, it should be able to create the profile and map "displayName" with "username"', () => { + it('when "displayName" is missing, it should be able to create the profile and map "displayName" with "username"', async () => { const accessToken = '19xasczxcm9n7gacn9jdgm19me'; - const rawProfile = { + const fullProfile = ({ id: 'uid-123', username: 'jimmymarkum', provider: 'github', @@ -128,7 +143,7 @@ describe('GithubAuthProvider', () => { 'https://a1cf74336522e87f135f-2f21ace9a6cf0052456644b80fa06d4f.ssl.cf2.rackcdn.com/images/characters_opt/p-mystic-river-sean-penn.jpg', }, ], - }; + } as unknown) as PassportProfile; const params = { scope: 'read:scope', @@ -150,19 +165,17 @@ describe('GithubAuthProvider', () => { }, }; - expect( - GithubAuthProvider.transformOAuthResponse( - accessToken, - rawProfile, - params, - ), - ).toEqual(expected); + mockFrameHandler.mockResolvedValueOnce({ + result: { fullProfile, accessToken, params }, + }); + const { response } = await provider.handler({} as any); + expect(response).toEqual(expected); }); - it('when "photos" is missing, it should be able to create the profile without it', () => { + it('when "photos" is missing, it should be able to create the profile without it', async () => { const accessToken = 'ajakljsdoiahoawxbrouawucmbawe.awkxjemaneasdxwe.sodijxqeqwexeqwxe'; - const rawProfile = { + const fullProfile = { id: 'ipd12039', username: 'daveboyle', provider: 'gitlab', @@ -195,13 +208,11 @@ describe('GithubAuthProvider', () => { }, }; - expect( - GithubAuthProvider.transformOAuthResponse( - accessToken, - rawProfile, - params, - ), - ).toEqual(expected); + mockFrameHandler.mockResolvedValueOnce({ + result: { fullProfile, accessToken, params }, + }); + const { response } = await provider.handler({} as any); + expect(response).toEqual(expected); }); }); }); diff --git a/plugins/auth-backend/src/providers/github/provider.ts b/plugins/auth-backend/src/providers/github/provider.ts index 47d17fdac2..438cbca07b 100644 --- a/plugins/auth-backend/src/providers/github/provider.ts +++ b/plugins/auth-backend/src/providers/github/provider.ts @@ -27,12 +27,11 @@ import { OAuthAdapter, OAuthProviderOptions, OAuthHandlers, - OAuthResponse, OAuthEnvironmentHandler, OAuthStartRequest, encodeState, + OAuthResult, } from '../../lib/oauth'; -import passport from 'passport'; export type GithubAuthProviderOptions = OAuthProviderOptions & { tokenUrl?: string; @@ -43,55 +42,6 @@ export type GithubAuthProviderOptions = OAuthProviderOptions & { export class GithubAuthProvider implements OAuthHandlers { private readonly _strategy: GithubStrategy; - static transformPassportProfile(rawProfile: any): passport.Profile { - const profile: passport.Profile = { - id: rawProfile.username, - username: rawProfile.username, - provider: rawProfile.provider, - displayName: rawProfile.displayName || rawProfile.username, - photos: rawProfile.photos, - emails: rawProfile.emails, - }; - - return profile; - } - - static transformOAuthResponse( - accessToken: string, - rawProfile: any, - params: any = {}, - ): OAuthResponse { - const passportProfile = GithubAuthProvider.transformPassportProfile( - rawProfile, - ); - - const profile = makeProfileInfo(passportProfile, params.id_token); - const providerInfo = { - accessToken, - scope: params.scope, - expiresInSeconds: params.expires_in, - idToken: params.id_token, - }; - - // GitHub provides an id numeric value (123) - // as a fallback - const id = passportProfile!.id; - - if (params.expires_in) { - providerInfo.expiresInSeconds = params.expires_in; - } - if (params.id_token) { - providerInfo.idToken = params.id_token; - } - return { - providerInfo, - profile, - backstageIdentity: { - id, - }, - }; - } - constructor(options: GithubAuthProviderOptions) { this._strategy = new GithubStrategy( { @@ -104,17 +54,12 @@ export class GithubAuthProvider implements OAuthHandlers { }, ( accessToken: any, - _: any, + refreshToken: any, params: any, - rawProfile: any, - done: PassportDoneCallback, + fullProfile: any, + done: PassportDoneCallback, ) => { - const oauthResponse = GithubAuthProvider.transformOAuthResponse( - accessToken, - rawProfile, - params, - ); - done(undefined, oauthResponse); + done(undefined, { fullProfile, params, accessToken, refreshToken }); }, ); } @@ -127,12 +72,33 @@ export class GithubAuthProvider implements OAuthHandlers { } async handler(req: express.Request) { - const { response } = await executeFrameHandlerStrategy( - req, - this._strategy, + const { + result: { fullProfile, accessToken, params }, + } = await executeFrameHandlerStrategy(req, this._strategy); + + const profile = makeProfileInfo( + { + ...fullProfile, + id: fullProfile.username || fullProfile.id, + displayName: + fullProfile.displayName || fullProfile.username || fullProfile.id, + }, + params.id_token, ); - return { response }; + return { + response: { + profile, + providerInfo: { + accessToken, + scope: params.scope, + expiresInSeconds: params.expires_in, + }, + backstageIdentity: { + id: fullProfile.username || fullProfile.id, + }, + }, + }; } } diff --git a/plugins/auth-backend/src/providers/gitlab/provider.test.ts b/plugins/auth-backend/src/providers/gitlab/provider.test.ts index d4d1de17ec..b57d846f23 100644 --- a/plugins/auth-backend/src/providers/gitlab/provider.test.ts +++ b/plugins/auth-backend/src/providers/gitlab/provider.test.ts @@ -15,14 +15,21 @@ */ import { GitlabAuthProvider } from './provider'; +import * as helpers from '../../lib/passport'; +import { OAuthResult } from '../../lib/oauth'; + +const mockFrameHandler = (jest.spyOn( + helpers, + 'executeFrameHandlerStrategy', +) as unknown) as jest.MockedFunction<() => Promise<{ result: OAuthResult }>>; describe('GitlabAuthProvider', () => { - it('should transform to type OAuthResponse', () => { + it('should transform to type OAuthResponse', async () => { const tests = [ { - arguments: { + result: { accessToken: '19xasczxcm9n7gacn9jdgm19me', - rawProfile: { + fullProfile: { id: 'uid-123', username: 'jimmymarkum', provider: 'gitlab', @@ -58,10 +65,10 @@ describe('GitlabAuthProvider', () => { }, }, { - arguments: { + result: { accessToken: 'ajakljsdoiahoawxbrouawucmbawe.awkxjemaneasdxwe.sodijxqeqwexeqwxe', - rawProfile: { + fullProfile: { id: 'ipd12039', username: 'daveboyle', provider: 'gitlab', @@ -74,6 +81,7 @@ describe('GitlabAuthProvider', () => { }, params: { scope: 'read_repository', + expires_in: 200, }, }, expect: { @@ -83,6 +91,7 @@ describe('GitlabAuthProvider', () => { providerInfo: { accessToken: 'ajakljsdoiahoawxbrouawucmbawe.awkxjemaneasdxwe.sodijxqeqwexeqwxe', + expiresInSeconds: 200, scope: 'read_repository', }, profile: { @@ -93,14 +102,16 @@ describe('GitlabAuthProvider', () => { }, ]; + const provider = new GitlabAuthProvider({ + clientId: 'mock', + clientSecret: 'mock', + callbackUrl: 'mock', + baseUrl: 'mock', + }); for (const test of tests) { - expect( - GitlabAuthProvider.transformOAuthResponse( - test.arguments.accessToken, - test.arguments.rawProfile, - test.arguments.params, - ), - ).toEqual(test.expect); + mockFrameHandler.mockResolvedValueOnce({ result: test.result }); + const { response } = await provider.handler({} as any); + expect(response).toEqual(test.expect); } }); }); diff --git a/plugins/auth-backend/src/providers/gitlab/provider.ts b/plugins/auth-backend/src/providers/gitlab/provider.ts index 05a1ab2efe..7c24757833 100644 --- a/plugins/auth-backend/src/providers/gitlab/provider.ts +++ b/plugins/auth-backend/src/providers/gitlab/provider.ts @@ -31,8 +31,8 @@ import { OAuthEnvironmentHandler, OAuthStartRequest, encodeState, + OAuthResult, } from '../../lib/oauth'; -import passport from 'passport'; export type GitlabAuthProviderOptions = OAuthProviderOptions & { baseUrl: string; @@ -41,64 +41,6 @@ export type GitlabAuthProviderOptions = OAuthProviderOptions & { export class GitlabAuthProvider implements OAuthHandlers { private readonly _strategy: GitlabStrategy; - static transformPassportProfile(rawProfile: any): passport.Profile { - const profile: passport.Profile = { - id: rawProfile.id, - username: rawProfile.username, - provider: rawProfile.provider, - displayName: rawProfile.displayName, - }; - - if (rawProfile.emails && rawProfile.emails.length > 0) { - profile.emails = rawProfile.emails; - } - if (rawProfile.avatarUrl) { - profile.photos = [{ value: rawProfile.avatarUrl }]; - } - - return profile; - } - - static transformOAuthResponse( - accessToken: string, - rawProfile: any, - params: any = {}, - ): OAuthResponse { - const passportProfile = GitlabAuthProvider.transformPassportProfile( - rawProfile, - ); - - const profile = makeProfileInfo(passportProfile, params.id_token); - const providerInfo = { - accessToken, - scope: params.scope, - expiresInSeconds: params.expires_in, - idToken: params.id_token, - }; - - // gitlab provides an id numeric value (123) - // as a fallback - let id = passportProfile!.id; - - if (profile.email) { - id = profile.email.split('@')[0]; - } - - if (params.expires_in) { - providerInfo.expiresInSeconds = params.expires_in; - } - if (params.id_token) { - providerInfo.idToken = params.id_token; - } - return { - providerInfo, - profile, - backstageIdentity: { - id, - }, - }; - } - constructor(options: GitlabAuthProviderOptions) { this._strategy = new GitlabStrategy( { @@ -109,17 +51,12 @@ export class GitlabAuthProvider implements OAuthHandlers { }, ( accessToken: any, - _: any, + refreshToken: any, params: any, - rawProfile: any, - done: PassportDoneCallback, + fullProfile: any, + done: PassportDoneCallback, ) => { - const oauthResponse = GitlabAuthProvider.transformOAuthResponse( - accessToken, - rawProfile, - params, - ); - done(undefined, oauthResponse); + done(undefined, { fullProfile, params, accessToken, refreshToken }); }, ); } @@ -132,10 +69,47 @@ export class GitlabAuthProvider implements OAuthHandlers { } async handler(req: express.Request): Promise<{ response: OAuthResponse }> { - return await executeFrameHandlerStrategy( + const { result } = await executeFrameHandlerStrategy( req, this._strategy, ); + const { accessToken, params } = result; + const fullProfile = result.fullProfile as OAuthResult['fullProfile'] & { + avatarUrl?: string; + }; + + const profile = makeProfileInfo( + { + ...fullProfile, + photos: [ + ...(fullProfile.photos ?? []), + ...(fullProfile.avatarUrl ? [{ value: fullProfile.avatarUrl }] : []), + ], + }, + params.id_token, + ); + + // gitlab provides an id numeric value (123) + // as a fallback + let id = fullProfile.id; + if (profile.email) { + id = profile.email.split('@')[0]; + } + + return { + response: { + profile, + providerInfo: { + accessToken, + scope: params.scope, + expiresInSeconds: params.expires_in, + idToken: params.id_token, + }, + backstageIdentity: { + id, + }, + }, + }; } } diff --git a/plugins/auth-backend/src/providers/google/provider.ts b/plugins/auth-backend/src/providers/google/provider.ts index 12d882cbb0..a00644a90d 100644 --- a/plugins/auth-backend/src/providers/google/provider.ts +++ b/plugins/auth-backend/src/providers/google/provider.ts @@ -28,6 +28,7 @@ import { OAuthRefreshRequest, OAuthResponse, OAuthStartRequest, + OAuthResult, } from '../../lib/oauth'; import { executeFetchUserProfileStrategy, @@ -74,20 +75,16 @@ export class GoogleAuthProvider implements OAuthHandlers { accessToken: any, refreshToken: any, params: any, - rawProfile: passport.Profile, - done: PassportDoneCallback, + fullProfile: passport.Profile, + done: PassportDoneCallback, ) => { - const profile = makeProfileInfo(rawProfile, params.id_token); done( undefined, { - providerInfo: { - idToken: params.id_token, - accessToken, - scope: params.scope, - expiresInSeconds: params.expires_in, - }, - profile, + fullProfile, + params, + accessToken, + refreshToken, }, { refreshToken, @@ -109,13 +106,23 @@ export class GoogleAuthProvider implements OAuthHandlers { async handler( req: express.Request, ): Promise<{ response: OAuthResponse; refreshToken: string }> { - const { response, privateInfo } = await executeFrameHandlerStrategy< - OAuthResponse, + const { result, privateInfo } = await executeFrameHandlerStrategy< + OAuthResult, PrivateInfo >(req, this._strategy); + const profile = makeProfileInfo(result.fullProfile, result.params.id_token); + return { - response: await this.populateIdentity(response), + response: await this.populateIdentity({ + providerInfo: { + idToken: result.params.id_token, + accessToken: result.accessToken, + scope: result.params.scope, + expiresInSeconds: result.params.expires_in, + }, + profile, + }), refreshToken: privateInfo.refreshToken, }; } @@ -127,11 +134,11 @@ export class GoogleAuthProvider implements OAuthHandlers { req.scope, ); - const rawProfile = await executeFetchUserProfileStrategy( + const fullProfile = await executeFetchUserProfileStrategy( this._strategy, accessToken, ); - const profile = makeProfileInfo(rawProfile, params.id_token); + const profile = makeProfileInfo(fullProfile, params.id_token); return this.populateIdentity({ providerInfo: { diff --git a/plugins/auth-backend/src/providers/microsoft/provider.ts b/plugins/auth-backend/src/providers/microsoft/provider.ts index 7b110e5f1a..032af46804 100644 --- a/plugins/auth-backend/src/providers/microsoft/provider.ts +++ b/plugins/auth-backend/src/providers/microsoft/provider.ts @@ -38,6 +38,7 @@ import { OAuthStartRequest, encodeState, OAuthRefreshRequest, + OAuthResult, } from '../../lib/oauth'; import got from 'got'; @@ -54,32 +55,6 @@ export type MicrosoftAuthProviderOptions = OAuthProviderOptions & { export class MicrosoftAuthProvider implements OAuthHandlers { private readonly _strategy: MicrosoftStrategy; - static transformAuthResponse( - accessToken: string, - params: any, - rawProfile: any, - photoURL: any, - ): OAuthResponse { - let passportProfile: passport.Profile = rawProfile; - passportProfile = { - ...passportProfile, - photos: [{ value: photoURL }], - }; - - const profile = makeProfileInfo(passportProfile, params.id_token); - const providerInfo = { - idToken: params.id_token, - accessToken, - scope: params.scope, - expiresInSeconds: params.expires_in, - }; - - return { - providerInfo, - profile, - }; - } - constructor(options: MicrosoftAuthProviderOptions) { this._strategy = new MicrosoftStrategy( { @@ -94,22 +69,10 @@ export class MicrosoftAuthProvider implements OAuthHandlers { accessToken: any, refreshToken: any, params: any, - rawProfile: passport.Profile, - done: PassportDoneCallback, + fullProfile: passport.Profile, + done: PassportDoneCallback, ) => { - this.getUserPhoto(accessToken) - .then(photoURL => { - const authResponse = MicrosoftAuthProvider.transformAuthResponse( - accessToken, - params, - rawProfile, - photoURL, - ); - done(undefined, authResponse, { refreshToken }); - }) - .catch(error => { - throw new Error(`Error processing auth response: ${error}`); - }); + done(undefined, { fullProfile, accessToken, refreshToken, params }); }, ); } @@ -124,15 +87,37 @@ export class MicrosoftAuthProvider implements OAuthHandlers { async handler( req: express.Request, ): Promise<{ response: OAuthResponse; refreshToken: string }> { - const { response, privateInfo } = await executeFrameHandlerStrategy< - OAuthResponse, + const { result, privateInfo } = await executeFrameHandlerStrategy< + OAuthResult, PrivateInfo >(req, this._strategy); - return { - response: await this.populateIdentity(response), - refreshToken: privateInfo.refreshToken, - }; + try { + const photoUrl = await this.getUserPhoto(result.accessToken); + + const profile = makeProfileInfo( + { + ...result.fullProfile, + photos: photoUrl ? [{ value: photoUrl }] : undefined, + }, + result.params.id_token, + ); + + return { + response: await this.populateIdentity({ + profile, + providerInfo: { + idToken: result.params.id_token, + accessToken: result.accessToken, + scope: result.params.scope, + expiresInSeconds: result.params.expires_in, + }, + }), + refreshToken: privateInfo.refreshToken, + }; + } catch (error) { + throw new Error(`Error processing auth response: ${error}`); + } } async refresh(req: OAuthRefreshRequest): Promise { @@ -142,11 +127,11 @@ export class MicrosoftAuthProvider implements OAuthHandlers { req.scope, ); - const rawProfile = await executeFetchUserProfileStrategy( + const fullProfile = await executeFetchUserProfileStrategy( this._strategy, accessToken, ); - const profile = makeProfileInfo(rawProfile, params.id_token); + const profile = makeProfileInfo(fullProfile, params.id_token); const photo = await this.getUserPhoto(accessToken); if (photo) { profile.picture = photo; diff --git a/plugins/auth-backend/src/providers/oauth2/provider.ts b/plugins/auth-backend/src/providers/oauth2/provider.ts index a5d3f96fdf..fb347b836c 100644 --- a/plugins/auth-backend/src/providers/oauth2/provider.ts +++ b/plugins/auth-backend/src/providers/oauth2/provider.ts @@ -26,6 +26,7 @@ import { OAuthStartRequest, encodeState, OAuthRefreshRequest, + OAuthResult, } from '../../lib/oauth'; import { executeFetchUserProfileStrategy, @@ -65,21 +66,16 @@ export class OAuth2AuthProvider implements OAuthHandlers { accessToken: any, refreshToken: any, params: any, - rawProfile: passport.Profile, - done: PassportDoneCallback, + fullProfile: passport.Profile, + done: PassportDoneCallback, ) => { - const profile = makeProfileInfo(rawProfile, params.id_token); - done( undefined, { - providerInfo: { - idToken: params.id_token, - accessToken, - scope: params.scope, - expiresInSeconds: params.expires_in, - }, - profile, + fullProfile, + accessToken, + refreshToken, + params, }, { refreshToken, @@ -101,13 +97,23 @@ export class OAuth2AuthProvider implements OAuthHandlers { async handler( req: express.Request, ): Promise<{ response: OAuthResponse; refreshToken: string }> { - const { response, privateInfo } = await executeFrameHandlerStrategy< - OAuthResponse, + const { result, privateInfo } = await executeFrameHandlerStrategy< + OAuthResult, PrivateInfo >(req, this._strategy); + const profile = makeProfileInfo(result.fullProfile, result.params.id_token); + return { - response: await this.populateIdentity(response), + response: await this.populateIdentity({ + profile, + providerInfo: { + idToken: result.params.id_token, + accessToken: result.accessToken, + scope: result.params.scope, + expiresInSeconds: result.params.expires_in, + }, + }), refreshToken: privateInfo.refreshToken, }; } diff --git a/plugins/auth-backend/src/providers/oidc/provider.ts b/plugins/auth-backend/src/providers/oidc/provider.ts index b1bedd0f54..0ec8d0bbf7 100644 --- a/plugins/auth-backend/src/providers/oidc/provider.ts +++ b/plugins/auth-backend/src/providers/oidc/provider.ts @@ -37,10 +37,10 @@ import { executeRedirectStrategy, PassportDoneCallback, } from '../../lib/passport'; -import { RedirectInfo, AuthProviderFactory, ProfileInfo } from '../types'; +import { RedirectInfo, AuthProviderFactory } from '../types'; type PrivateInfo = { - refreshToken: string; + refreshToken?: string; }; type OidcImpl = { @@ -48,6 +48,11 @@ type OidcImpl = { client: Client; }; +type AuthResult = { + tokenset: TokenSet; + userinfo: UserinfoResponse; +}; + export type Options = OAuthProviderOptions & { metadataUrl: string; tokenSignedResponseAlg?: string; @@ -72,15 +77,30 @@ export class OidcAuthProvider implements OAuthHandlers { async handler( req: express.Request, - ): Promise<{ response: OAuthResponse; refreshToken: string }> { + ): Promise<{ response: OAuthResponse; refreshToken?: string }> { const { strategy } = await this.implementation; - const { response, privateInfo } = await executeFrameHandlerStrategy< - OAuthResponse, - PrivateInfo - >(req, strategy); + const { + result: { userinfo, tokenset }, + privateInfo, + } = await executeFrameHandlerStrategy( + req, + strategy, + ); return { - response: await this.populateIdentity(response), + response: await this.populateIdentity({ + profile: { + displayName: userinfo.name, + email: userinfo.email, + picture: userinfo.picture, + }, + providerInfo: { + idToken: tokenset.id_token, + accessToken: tokenset.access_token || '', + scope: tokenset.scope || '', + expiresInSeconds: tokenset.expires_in, + }, + }), refreshToken: privateInfo.refreshToken, }; } @@ -123,27 +143,13 @@ export class OidcAuthProvider implements OAuthHandlers { ( tokenset: TokenSet, userinfo: UserinfoResponse, - done: PassportDoneCallback, + done: PassportDoneCallback, ) => { - const profile: ProfileInfo = { - displayName: userinfo.name, - email: userinfo.email, - picture: userinfo.picture, - }; - done( undefined, + { tokenset, userinfo }, { - providerInfo: { - idToken: tokenset.id_token || '', - accessToken: tokenset.access_token || '', - scope: tokenset.scope || '', - expiresInSeconds: tokenset.expires_in, - }, - profile, - }, - { - refreshToken: tokenset.refresh_token || '', + refreshToken: tokenset.refresh_token, }, ); }, diff --git a/plugins/auth-backend/src/providers/okta/provider.ts b/plugins/auth-backend/src/providers/okta/provider.ts index fbca780571..6320230e67 100644 --- a/plugins/auth-backend/src/providers/okta/provider.ts +++ b/plugins/auth-backend/src/providers/okta/provider.ts @@ -23,6 +23,7 @@ import { OAuthStartRequest, encodeState, OAuthRefreshRequest, + OAuthResult, } from '../../lib/oauth'; import { Strategy as OktaStrategy } from 'passport-okta-oauth'; import passport from 'passport'; @@ -80,21 +81,16 @@ export class OktaAuthProvider implements OAuthHandlers { accessToken: any, refreshToken: any, params: any, - rawProfile: passport.Profile, - done: PassportDoneCallback, + fullProfile: passport.Profile, + done: PassportDoneCallback, ) => { - const profile = makeProfileInfo(rawProfile, params.id_token); - done( undefined, { - providerInfo: { - idToken: params.id_token, - accessToken, - scope: params.scope, - expiresInSeconds: params.expires_in, - }, - profile, + accessToken, + refreshToken, + params, + fullProfile, }, { refreshToken, @@ -116,13 +112,23 @@ export class OktaAuthProvider implements OAuthHandlers { async handler( req: express.Request, ): Promise<{ response: OAuthResponse; refreshToken: string }> { - const { response, privateInfo } = await executeFrameHandlerStrategy< - OAuthResponse, + const { result, privateInfo } = await executeFrameHandlerStrategy< + OAuthResult, PrivateInfo >(req, this._strategy); + const profile = makeProfileInfo(result.fullProfile, result.params.id_token); + return { - response: await this.populateIdentity(response), + response: await this.populateIdentity({ + profile, + providerInfo: { + idToken: result.params.id_token, + accessToken: result.accessToken, + scope: result.params.scope, + expiresInSeconds: result.params.expires_in, + }, + }), refreshToken: privateInfo.refreshToken, }; } @@ -134,11 +140,11 @@ export class OktaAuthProvider implements OAuthHandlers { req.scope, ); - const rawProfile = await executeFetchUserProfileStrategy( + const fullProfile = await executeFetchUserProfileStrategy( this._strategy, accessToken, ); - const profile = makeProfileInfo(rawProfile, params.id_token); + const profile = makeProfileInfo(fullProfile, params.id_token); return this.populateIdentity({ providerInfo: { diff --git a/plugins/auth-backend/src/providers/onelogin/provider.ts b/plugins/auth-backend/src/providers/onelogin/provider.ts index 5549db06c6..5cb0f84415 100644 --- a/plugins/auth-backend/src/providers/onelogin/provider.ts +++ b/plugins/auth-backend/src/providers/onelogin/provider.ts @@ -25,6 +25,7 @@ import { OAuthStartRequest, encodeState, OAuthRefreshRequest, + OAuthResult, } from '../../lib/oauth'; import passport from 'passport'; import { @@ -61,21 +62,16 @@ export class OneLoginProvider implements OAuthHandlers { accessToken: any, refreshToken: any, params: any, - rawProfile: passport.Profile, - done: PassportDoneCallback, + fullProfile: passport.Profile, + done: PassportDoneCallback, ) => { - const profile = makeProfileInfo(rawProfile, params.id_token); - done( undefined, { - providerInfo: { - idToken: params.id_token, - accessToken, - scope: params.scope, - expiresInSeconds: params.expires_in, - }, - profile, + accessToken, + refreshToken, + params, + fullProfile, }, { refreshToken, @@ -96,13 +92,23 @@ export class OneLoginProvider implements OAuthHandlers { async handler( req: express.Request, ): Promise<{ response: OAuthResponse; refreshToken: string }> { - const { response, privateInfo } = await executeFrameHandlerStrategy< - OAuthResponse, + const { result, privateInfo } = await executeFrameHandlerStrategy< + OAuthResult, PrivateInfo >(req, this._strategy); + const profile = makeProfileInfo(result.fullProfile, result.params.id_token); + return { - response: await this.populateIdentity(response), + response: await this.populateIdentity({ + profile, + providerInfo: { + idToken: result.params.id_token, + accessToken: result.accessToken, + scope: result.params.scope, + expiresInSeconds: result.params.expires_in, + }, + }), refreshToken: privateInfo.refreshToken, }; } @@ -114,11 +120,11 @@ export class OneLoginProvider implements OAuthHandlers { req.scope, ); - const rawProfile = await executeFetchUserProfileStrategy( + const fullProfile = await executeFetchUserProfileStrategy( this._strategy, accessToken, ); - const profile = makeProfileInfo(rawProfile, params.id_token); + const profile = makeProfileInfo(fullProfile, params.id_token); return this.populateIdentity({ providerInfo: { diff --git a/plugins/auth-backend/src/providers/saml/provider.ts b/plugins/auth-backend/src/providers/saml/provider.ts index caeb86aa6b..c3a91b3b77 100644 --- a/plugins/auth-backend/src/providers/saml/provider.ts +++ b/plugins/auth-backend/src/providers/saml/provider.ts @@ -26,17 +26,12 @@ import { executeRedirectStrategy, PassportDoneCallback, } from '../../lib/passport'; -import { - AuthProviderRouteHandlers, - ProfileInfo, - AuthProviderFactory, -} from '../types'; +import { AuthProviderRouteHandlers, AuthProviderFactory } from '../types'; import { postMessageResponse } from '../../lib/flow'; import { TokenIssuer } from '../../identity'; type SamlInfo = { - userId: string; - profile: ProfileInfo; + fullProfile: any; }; type Options = SamlConfig & { @@ -53,7 +48,7 @@ export class SamlAuthProvider implements AuthProviderRouteHandlers { this.appUrl = options.appUrl; this.tokenIssuer = options.tokenIssuer; this.strategy = new SamlStrategy({ ...options }, (( - profile: SamlProfile, + fullProfile: SamlProfile, done: PassportDoneCallback, ) => { // TODO: There's plenty more validation and profile handling to do here, @@ -61,13 +56,7 @@ export class SamlAuthProvider implements AuthProviderRouteHandlers { // for non-oauth auth flows. // TODO: This flow doesn't issue an identity token that can be used to validate // the identity of the user in other backends, which we need in some form. - done(undefined, { - userId: profile.nameID!, - profile: { - email: profile.email!, - displayName: profile.displayName as string, - }, - }); + done(undefined, { fullProfile }); }) as VerifyWithoutRequest); } @@ -81,11 +70,13 @@ export class SamlAuthProvider implements AuthProviderRouteHandlers { res: express.Response, ): Promise { try { - const { - response: { userId, profile }, - } = await executeFrameHandlerStrategy(req, this.strategy); + const { result } = await executeFrameHandlerStrategy( + req, + this.strategy, + ); + + const id = result.fullProfile.nameID; - const id = userId; const idToken = await this.tokenIssuer.issueToken({ claims: { sub: id }, }); @@ -93,8 +84,11 @@ export class SamlAuthProvider implements AuthProviderRouteHandlers { return postMessageResponse(res, this.appUrl, { type: 'authorization_response', response: { + profile: { + email: result.fullProfile.email, + displayName: result.fullProfile.displayName, + }, providerInfo: {}, - profile, backstageIdentity: { id, idToken }, }, });