From 7a6373e08096aab4aa308c97916b10a10bdc5daa Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 20 Aug 2021 14:36:36 +0200 Subject: [PATCH] auth-backend: new GithubOAuthResult type + refresh test & fixes Signed-off-by: Patrik Oldsberg --- .../src/providers/github/index.ts | 2 +- .../src/providers/github/provider.test.ts | 71 ++++++++++++++----- .../src/providers/github/provider.ts | 59 ++++++++------- 3 files changed, 91 insertions(+), 41 deletions(-) diff --git a/plugins/auth-backend/src/providers/github/index.ts b/plugins/auth-backend/src/providers/github/index.ts index 3e6aff4c24..a855b13b83 100644 --- a/plugins/auth-backend/src/providers/github/index.ts +++ b/plugins/auth-backend/src/providers/github/index.ts @@ -15,4 +15,4 @@ */ export { createGithubProvider } from './provider'; -export type { GithubProviderOptions } from './provider'; +export type { GithubOAuthResult, GithubProviderOptions } from './provider'; diff --git a/plugins/auth-backend/src/providers/github/provider.test.ts b/plugins/auth-backend/src/providers/github/provider.test.ts index 57c3770b23..f67c41b461 100644 --- a/plugins/auth-backend/src/providers/github/provider.test.ts +++ b/plugins/auth-backend/src/providers/github/provider.test.ts @@ -18,9 +18,12 @@ import { Profile as PassportProfile } from 'passport'; import { getVoidLogger } from '@backstage/backend-common'; import { TokenIssuer } from '../../identity/types'; import { CatalogIdentityClient } from '../../lib/catalog'; -import { GithubAuthProvider, githubDefaultSignInResolver } from './provider'; +import { + GithubAuthProvider, + GithubOAuthResult, + githubDefaultSignInResolver, +} from './provider'; import * as helpers from '../../lib/passport/PassportStrategyHelper'; -import { OAuthResult } from '../../lib/oauth'; import { makeProfileInfo } from '../../lib/passport/PassportStrategyHelper'; const mockFrameHandler = jest.spyOn( @@ -28,15 +31,17 @@ const mockFrameHandler = jest.spyOn( 'executeFrameHandlerStrategy', ) as unknown as jest.MockedFunction< () => Promise<{ - result: Omit & { params: { scope: string } }; + result: GithubOAuthResult; privateInfo: { refreshToken?: string }; }> >; describe('GithubAuthProvider', () => { - const tokenIssuer = { - issueToken: jest.fn(), + const tokenIssuer: TokenIssuer = { listPublicKeys: jest.fn(), + async issueToken(params) { + return `token-for-${params.claims.sub}`; + }, }; const catalogIdentityClient = { findUser: jest.fn(), @@ -84,11 +89,10 @@ describe('GithubAuthProvider', () => { const expected = { backstageIdentity: { id: 'jimmymarkum', + token: 'token-for-jimmymarkum', }, providerInfo: { accessToken: '19xasczxcm9n7gacn9jdgm19me', - expiresInSeconds: undefined, - idToken: undefined, scope: 'read:scope', }, profile: { @@ -130,11 +134,10 @@ describe('GithubAuthProvider', () => { const expected = { backstageIdentity: { id: 'jimmymarkum', + token: 'token-for-jimmymarkum', }, providerInfo: { accessToken: '19xasczxcm9n7gacn9jdgm19me', - expiresInSeconds: undefined, - idToken: undefined, scope: 'read:scope', }, profile: { @@ -174,11 +177,10 @@ describe('GithubAuthProvider', () => { const expected = { backstageIdentity: { id: 'jimmymarkum', + token: 'token-for-jimmymarkum', }, providerInfo: { accessToken: '19xasczxcm9n7gacn9jdgm19me', - expiresInSeconds: undefined, - idToken: undefined, scope: 'read:scope', }, profile: { @@ -202,11 +204,11 @@ describe('GithubAuthProvider', () => { const fullProfile = { id: 'ipd12039', username: 'daveboyle', - provider: 'gitlab', + provider: 'github', displayName: 'Dave Boyle', emails: [ { - value: 'daveboyle@gitlab.org', + value: 'daveboyle@github.org', }, ], }; @@ -218,17 +220,16 @@ describe('GithubAuthProvider', () => { const expected = { backstageIdentity: { id: 'daveboyle', + token: 'token-for-daveboyle', }, providerInfo: { accessToken: 'ajakljsdoiahoawxbrouawucmbawe.awkxjemaneasdxwe.sodijxqeqwexeqwxe', scope: 'read:user', - expiresInSeconds: undefined, - idToken: undefined, }, profile: { displayName: 'Dave Boyle', - email: 'daveboyle@gitlab.org', + email: 'daveboyle@github.org', }, }; @@ -239,5 +240,43 @@ describe('GithubAuthProvider', () => { const { response } = await provider.handler({} as any); expect(response).toEqual(expected); }); + + it('should forward a refresh token', async () => { + mockFrameHandler.mockResolvedValueOnce({ + result: { + fullProfile: { + id: 'ipd12039', + provider: 'github', + displayName: 'Dave Boyle', + }, + accessToken: 'a.b.c', + params: { + scope: 'read:user', + expires_in: '123', + }, + }, + privateInfo: { refreshToken: 'refresh-me' }, + }); + + const response = await provider.handler({} as any); + + expect(response).toEqual({ + response: { + backstageIdentity: { + id: 'ipd12039', + token: 'token-for-ipd12039', + }, + providerInfo: { + accessToken: 'a.b.c', + scope: 'read:user', + expiresInSeconds: 123, + }, + profile: { + displayName: 'Dave Boyle', + }, + }, + refreshToken: 'refresh-me', + }); + }); }); }); diff --git a/plugins/auth-backend/src/providers/github/provider.ts b/plugins/auth-backend/src/providers/github/provider.ts index f69233c03e..b261ea7be9 100644 --- a/plugins/auth-backend/src/providers/github/provider.ts +++ b/plugins/auth-backend/src/providers/github/provider.ts @@ -16,6 +16,7 @@ import express from 'express'; import { Logger } from 'winston'; +import { Profile as PassportProfile } from 'passport'; import { Strategy as GithubStrategy } from 'passport-github2'; import { executeFetchUserProfileStrategy, @@ -38,7 +39,6 @@ import { OAuthEnvironmentHandler, OAuthStartRequest, encodeState, - OAuthResult, OAuthRefreshRequest, OAuthResponse, } from '../../lib/oauth'; @@ -49,12 +49,23 @@ type PrivateInfo = { refreshToken?: string; }; +export type GithubOAuthResult = { + fullProfile: PassportProfile; + params: { + scope: string; + expires_in?: string; + refresh_token_expires_in?: string; + }; + accessToken: string; + refreshToken?: string; +}; + export type GithubAuthProviderOptions = OAuthProviderOptions & { tokenUrl?: string; userProfileUrl?: string; authorizationUrl?: string; - signInResolver?: SignInResolver; - authHandler: AuthHandler; + signInResolver?: SignInResolver; + authHandler: AuthHandler; tokenIssuer: TokenIssuer; catalogIdentityClient: CatalogIdentityClient; logger: Logger; @@ -62,8 +73,8 @@ export type GithubAuthProviderOptions = OAuthProviderOptions & { export class GithubAuthProvider implements OAuthHandlers { private readonly _strategy: GithubStrategy; - private readonly signInResolver?: SignInResolver; - private readonly authHandler: AuthHandler; + private readonly signInResolver?: SignInResolver; + private readonly authHandler: AuthHandler; private readonly tokenIssuer: TokenIssuer; private readonly catalogIdentityClient: CatalogIdentityClient; private readonly logger: Logger; @@ -88,7 +99,7 @@ export class GithubAuthProvider implements OAuthHandlers { refreshToken: any, params: any, fullProfile: any, - done: PassportDoneCallback, + done: PassportDoneCallback, ) => { done(undefined, { fullProfile, params, accessToken }, { refreshToken }); }, @@ -104,7 +115,7 @@ export class GithubAuthProvider implements OAuthHandlers { async handler(req: express.Request) { const { result, privateInfo } = await executeFrameHandlerStrategy< - OAuthResult, + GithubOAuthResult, PrivateInfo >(req, this._strategy); @@ -132,14 +143,16 @@ export class GithubAuthProvider implements OAuthHandlers { }); } - private async handleResult(result: OAuthResult) { + private async handleResult(result: GithubOAuthResult) { const { profile } = await this.authHandler(result); + const expiresInStr = result.params.expires_in; const response: OAuthResponse = { providerInfo: { accessToken: result.accessToken, scope: result.params.scope, - expiresInSeconds: result.params.expires_in, + expiresInSeconds: + expiresInStr === undefined ? undefined : Number(expiresInStr), }, profile, }; @@ -162,27 +175,25 @@ export class GithubAuthProvider implements OAuthHandlers { } } -export const githubDefaultSignInResolver: SignInResolver = async ( - info, - ctx, -) => { - const { fullProfile } = info.result; +export const githubDefaultSignInResolver: SignInResolver = + async (info, ctx) => { + const { fullProfile } = info.result; - const userId = fullProfile.username || fullProfile.id; + const userId = fullProfile.username || fullProfile.id; - const token = await ctx.tokenIssuer.issueToken({ - claims: { sub: userId, ent: [`user:default/${userId}`] }, - }); + const token = await ctx.tokenIssuer.issueToken({ + claims: { sub: userId, ent: [`user:default/${userId}`] }, + }); - return { id: userId, token }; -}; + return { id: userId, token }; + }; export type GithubProviderOptions = { /** * The profile transformation function used to verify and convert the auth response * into the profile that will be presented to the user. */ - authHandler?: AuthHandler; + authHandler?: AuthHandler; /** * Configure sign-in for this provider, without it the provider can not be used to sign users in. @@ -194,7 +205,7 @@ export type GithubProviderOptions = { * the catalog for a single user entity that has a matching `google.com/email` annotation. */ signIn?: { - resolver?: SignInResolver; + resolver?: SignInResolver; }; }; @@ -231,7 +242,7 @@ export const createGithubProvider = ( tokenIssuer, }); - const authHandler: AuthHandler = options?.authHandler + const authHandler: AuthHandler = options?.authHandler ? options.authHandler : async ({ fullProfile }) => ({ profile: makeProfileInfo(fullProfile), @@ -240,7 +251,7 @@ export const createGithubProvider = ( const signInResolverFn = options?.signIn?.resolver ?? githubDefaultSignInResolver; - const signInResolver: SignInResolver = info => + const signInResolver: SignInResolver = info => signInResolverFn(info, { catalogIdentityClient, tokenIssuer,