From 8e9e5fe72f5d07f45c95929186ead16631e817ba Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 20 Aug 2021 14:08:56 +0200 Subject: [PATCH 01/10] auth-backend: allow for optional refresh token usage in OAuthAdapter Signed-off-by: Patrik Oldsberg --- plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts index df9fb43cbe..1f424febf5 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts @@ -140,11 +140,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { response.providerInfo.scope = grantedScopes; } - if (!this.options.disableRefresh) { - if (!refreshToken) { - throw new InputError('Missing refresh token'); - } - + if (refreshToken && !this.options.disableRefresh) { // set new refresh token this.setRefreshTokenCookie(res, refreshToken); } @@ -174,10 +170,9 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { return; } - if (!this.options.disableRefresh) { - // remove refresh token cookie before logout - this.removeRefreshTokenCookie(res); - } + // remove refresh token cookie if it is set + this.removeRefreshTokenCookie(res); + res.status(200).send('logout!'); } From 5f6f33a5edb913f6fbb790d8ce5eed31b6afa5cf Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 20 Aug 2021 14:09:40 +0200 Subject: [PATCH 02/10] auth-backend: fall back to username and id as displayName for profiles Signed-off-by: Patrik Oldsberg --- .../auth-backend/src/lib/passport/PassportStrategyHelper.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/plugins/auth-backend/src/lib/passport/PassportStrategyHelper.ts b/plugins/auth-backend/src/lib/passport/PassportStrategyHelper.ts index a0c07943c1..67c2678f8b 100644 --- a/plugins/auth-backend/src/lib/passport/PassportStrategyHelper.ts +++ b/plugins/auth-backend/src/lib/passport/PassportStrategyHelper.ts @@ -30,8 +30,6 @@ export const makeProfileInfo = ( profile: passport.Profile, idToken?: string, ): ProfileInfo => { - let { displayName } = profile; - let email: string | undefined = undefined; if (profile.emails && profile.emails.length > 0) { const [firstEmail] = profile.emails; @@ -44,6 +42,9 @@ export const makeProfileInfo = ( picture = firstPhoto.value; } + let displayName: string | undefined = + profile.displayName ?? profile.username ?? profile.id; + if ((!email || !picture || !displayName) && idToken) { try { const decoded: Record = jwtDecoder(idToken); From 913256d197066a28060c78248804edf7ded88a7f Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 20 Aug 2021 14:10:27 +0200 Subject: [PATCH 03/10] auth-backend: migrate GitHub handler to sign-in resolver + add refresh support Signed-off-by: Patrik Oldsberg --- .../src/providers/github/provider.test.ts | 27 ++- .../src/providers/github/provider.ts | 190 +++++++++++++++--- 2 files changed, 184 insertions(+), 33 deletions(-) diff --git a/plugins/auth-backend/src/providers/github/provider.test.ts b/plugins/auth-backend/src/providers/github/provider.test.ts index c1270a71dc..57c3770b23 100644 --- a/plugins/auth-backend/src/providers/github/provider.test.ts +++ b/plugins/auth-backend/src/providers/github/provider.test.ts @@ -15,9 +15,13 @@ */ import { Profile as PassportProfile } from 'passport'; -import { GithubAuthProvider } from './provider'; +import { getVoidLogger } from '@backstage/backend-common'; +import { TokenIssuer } from '../../identity/types'; +import { CatalogIdentityClient } from '../../lib/catalog'; +import { GithubAuthProvider, 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( helpers, @@ -25,11 +29,28 @@ const mockFrameHandler = jest.spyOn( ) as unknown as jest.MockedFunction< () => Promise<{ result: Omit & { params: { scope: string } }; + privateInfo: { refreshToken?: string }; }> >; describe('GithubAuthProvider', () => { + const tokenIssuer = { + issueToken: jest.fn(), + listPublicKeys: jest.fn(), + }; + const catalogIdentityClient = { + findUser: jest.fn(), + }; + const provider = new GithubAuthProvider({ + logger: getVoidLogger(), + catalogIdentityClient: + catalogIdentityClient as unknown as CatalogIdentityClient, + tokenIssuer: tokenIssuer as unknown as TokenIssuer, + signInResolver: githubDefaultSignInResolver, + authHandler: async ({ fullProfile }) => ({ + profile: makeProfileInfo(fullProfile), + }), callbackUrl: 'mock', clientId: 'mock', clientSecret: 'mock', @@ -80,6 +101,7 @@ describe('GithubAuthProvider', () => { mockFrameHandler.mockResolvedValueOnce({ result: { fullProfile, accessToken, params }, + privateInfo: {}, }); const { response } = await provider.handler({} as any); expect(response).toEqual(expected); @@ -124,6 +146,7 @@ describe('GithubAuthProvider', () => { mockFrameHandler.mockResolvedValueOnce({ result: { fullProfile, accessToken, params }, + privateInfo: {}, }); const { response } = await provider.handler({} as any); expect(response).toEqual(expected); @@ -167,6 +190,7 @@ describe('GithubAuthProvider', () => { mockFrameHandler.mockResolvedValueOnce({ result: { fullProfile, accessToken, params }, + privateInfo: {}, }); const { response } = await provider.handler({} as any); expect(response).toEqual(expected); @@ -210,6 +234,7 @@ describe('GithubAuthProvider', () => { mockFrameHandler.mockResolvedValueOnce({ result: { fullProfile, accessToken, params }, + privateInfo: {}, }); 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 62f2534601..f69233c03e 100644 --- a/plugins/auth-backend/src/providers/github/provider.ts +++ b/plugins/auth-backend/src/providers/github/provider.ts @@ -15,14 +15,22 @@ */ import express from 'express'; +import { Logger } from 'winston'; import { Strategy as GithubStrategy } from 'passport-github2'; import { + executeFetchUserProfileStrategy, executeFrameHandlerStrategy, executeRedirectStrategy, + executeRefreshTokenStrategy, makeProfileInfo, PassportDoneCallback, } from '../../lib/passport'; -import { RedirectInfo, AuthProviderFactory } from '../types'; +import { + RedirectInfo, + AuthProviderFactory, + AuthHandler, + SignInResolver, +} from '../types'; import { OAuthAdapter, OAuthProviderOptions, @@ -31,18 +39,41 @@ import { OAuthStartRequest, encodeState, OAuthResult, + OAuthRefreshRequest, + OAuthResponse, } from '../../lib/oauth'; +import { CatalogIdentityClient } from '../../lib/catalog'; +import { TokenIssuer } from '../../identity'; + +type PrivateInfo = { + refreshToken?: string; +}; export type GithubAuthProviderOptions = OAuthProviderOptions & { tokenUrl?: string; userProfileUrl?: string; authorizationUrl?: string; + signInResolver?: SignInResolver; + authHandler: AuthHandler; + tokenIssuer: TokenIssuer; + catalogIdentityClient: CatalogIdentityClient; + logger: Logger; }; export class GithubAuthProvider implements OAuthHandlers { private readonly _strategy: GithubStrategy; + private readonly signInResolver?: SignInResolver; + private readonly authHandler: AuthHandler; + private readonly tokenIssuer: TokenIssuer; + private readonly catalogIdentityClient: CatalogIdentityClient; + private readonly logger: Logger; constructor(options: GithubAuthProviderOptions) { + this.signInResolver = options.signInResolver; + this.authHandler = options.authHandler; + this.tokenIssuer = options.tokenIssuer; + this.catalogIdentityClient = options.catalogIdentityClient; + this.logger = options.logger; this._strategy = new GithubStrategy( { clientID: options.clientId, @@ -54,12 +85,12 @@ export class GithubAuthProvider implements OAuthHandlers { }, ( accessToken: any, - _refreshToken: any, + refreshToken: any, params: any, fullProfile: any, - done: PassportDoneCallback, + done: PassportDoneCallback, ) => { - done(undefined, { fullProfile, params, accessToken }); + done(undefined, { fullProfile, params, accessToken }, { refreshToken }); }, ); } @@ -72,42 +103,112 @@ export class GithubAuthProvider implements OAuthHandlers { } async handler(req: express.Request) { - 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, - ); + const { result, privateInfo } = await executeFrameHandlerStrategy< + OAuthResult, + PrivateInfo + >(req, this._strategy); return { - response: { - profile, - providerInfo: { - accessToken, - scope: params.scope, - expiresInSeconds: params.expires_in, - }, - backstageIdentity: { - id: fullProfile.username || fullProfile.id, - }, - }, + response: await this.handleResult(result), + refreshToken: privateInfo.refreshToken, }; } + + async refresh(req: OAuthRefreshRequest): Promise { + const { accessToken, params } = await executeRefreshTokenStrategy( + this._strategy, + req.refreshToken, + req.scope, + ); + const fullProfile = await executeFetchUserProfileStrategy( + this._strategy, + accessToken, + ); + return this.handleResult({ + fullProfile, + params, + accessToken, + refreshToken: req.refreshToken, + }); + } + + private async handleResult(result: OAuthResult) { + const { profile } = await this.authHandler(result); + + const response: OAuthResponse = { + providerInfo: { + accessToken: result.accessToken, + scope: result.params.scope, + expiresInSeconds: result.params.expires_in, + }, + profile, + }; + + if (this.signInResolver) { + response.backstageIdentity = await this.signInResolver( + { + result, + profile, + }, + { + tokenIssuer: this.tokenIssuer, + catalogIdentityClient: this.catalogIdentityClient, + logger: this.logger, + }, + ); + } + + return response; + } } -export type GithubProviderOptions = {}; +export const githubDefaultSignInResolver: SignInResolver = async ( + info, + ctx, +) => { + const { fullProfile } = info.result; + + const userId = fullProfile.username || fullProfile.id; + + const token = await ctx.tokenIssuer.issueToken({ + claims: { sub: userId, ent: [`user:default/${userId}`] }, + }); + + 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; + + /** + * Configure sign-in for this provider, without it the provider can not be used to sign users in. + */ + /** + * Maps an auth result to a Backstage identity for the user. + * + * Set to `'email'` to use the default email-based sign in resolver, which will search + * the catalog for a single user entity that has a matching `google.com/email` annotation. + */ + signIn?: { + resolver?: SignInResolver; + }; +}; export const createGithubProvider = ( - _options?: GithubProviderOptions, + options?: GithubProviderOptions, ): AuthProviderFactory => { - return ({ providerId, globalConfig, config, tokenIssuer }) => + return ({ + providerId, + globalConfig, + config, + tokenIssuer, + catalogApi, + logger, + }) => OAuthEnvironmentHandler.mapConfig(config, envConfig => { const clientId = envConfig.getString('clientId'); const clientSecret = envConfig.getString('clientSecret'); @@ -125,6 +226,27 @@ export const createGithubProvider = ( : undefined; const callbackUrl = `${globalConfig.baseUrl}/${providerId}/handler/frame`; + const catalogIdentityClient = new CatalogIdentityClient({ + catalogApi, + tokenIssuer, + }); + + const authHandler: AuthHandler = options?.authHandler + ? options.authHandler + : async ({ fullProfile }) => ({ + profile: makeProfileInfo(fullProfile), + }); + + const signInResolverFn = + options?.signIn?.resolver ?? githubDefaultSignInResolver; + + const signInResolver: SignInResolver = info => + signInResolverFn(info, { + catalogIdentityClient, + tokenIssuer, + logger, + }); + const provider = new GithubAuthProvider({ clientId, clientSecret, @@ -132,10 +254,14 @@ export const createGithubProvider = ( tokenUrl, userProfileUrl, authorizationUrl, + signInResolver, + authHandler, + tokenIssuer, + catalogIdentityClient, + logger, }); return OAuthAdapter.fromConfig(globalConfig, provider, { - disableRefresh: true, persistScopes: true, providerId, tokenIssuer, From 7a6373e08096aab4aa308c97916b10a10bdc5daa Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 20 Aug 2021 14:36:36 +0200 Subject: [PATCH 04/10] 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, From eae4656cb826873d16bf363d3f06e998407485cd Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 20 Aug 2021 15:44:58 +0200 Subject: [PATCH 05/10] core-app-api: make AuthSessionStore implement MutableSessionManager Signed-off-by: Patrik Oldsberg --- .../lib/AuthSessionManager/AuthSessionStore.test.ts | 13 +++++++++++++ .../src/lib/AuthSessionManager/AuthSessionStore.ts | 8 ++++++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/packages/core-app-api/src/lib/AuthSessionManager/AuthSessionStore.test.ts b/packages/core-app-api/src/lib/AuthSessionManager/AuthSessionStore.test.ts index 4ceadd51ba..ded26c59e5 100644 --- a/packages/core-app-api/src/lib/AuthSessionManager/AuthSessionStore.test.ts +++ b/packages/core-app-api/src/lib/AuthSessionManager/AuthSessionStore.test.ts @@ -128,6 +128,19 @@ describe('GheAuth AuthSessionStore', () => { expect(manager.removeSession).toHaveBeenCalled(); }); + it('should set session', async () => { + const manager = new MockManager(); + const store = new AuthSessionStore({ manager, ...defaultOptions }); + + await expect(store.getSession({ optional: true })).resolves.toBe(undefined); + expect(localStorage.getItem('my-key')).toBe(null); + expect(manager.setSession).not.toHaveBeenCalled(); + store.setSession('123'); + expect(manager.setSession).toHaveBeenCalled(); + expect(localStorage.getItem('my-key')).toBe('"123"'); + await expect(store.getSession({ optional: true })).resolves.toBe('123'); + }); + it('should forward sessionState calls', () => { const manager = new MockManager(); const store = new AuthSessionStore({ manager, ...defaultOptions }); diff --git a/packages/core-app-api/src/lib/AuthSessionManager/AuthSessionStore.ts b/packages/core-app-api/src/lib/AuthSessionManager/AuthSessionStore.ts index 057a70e58d..fc15da7af8 100644 --- a/packages/core-app-api/src/lib/AuthSessionManager/AuthSessionStore.ts +++ b/packages/core-app-api/src/lib/AuthSessionManager/AuthSessionStore.ts @@ -15,7 +15,6 @@ */ import { - SessionManager, MutableSessionManager, SessionScopesFunc, SessionShouldRefreshFunc, @@ -40,7 +39,7 @@ type Options = { * * Session is serialized to JSON with special support for following types: Set. */ -export class AuthSessionStore implements SessionManager { +export class AuthSessionStore implements MutableSessionManager { private readonly manager: MutableSessionManager; private readonly storageKey: string; private readonly sessionShouldRefreshFunc: SessionShouldRefreshFunc; @@ -63,6 +62,11 @@ export class AuthSessionStore implements SessionManager { }); } + setSession(session: T | undefined): void { + this.manager.setSession(session); + this.saveSession(session); + } + async getSession(options: GetSessionOptions): Promise { const { scopes } = options; const session = this.loadSession(); From 2fbcaab09620407c5c37697a9eb417b5ad5dc3af Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 20 Aug 2021 16:09:54 +0200 Subject: [PATCH 06/10] core-app-api: added OptionalRefreshSessionManagerMux + tests Signed-off-by: Patrik Oldsberg --- .../OptionalRefreshSessionManagerMux.test.ts | 138 ++++++++++++++++++ .../OptionalRefreshSessionManagerMux.ts | 105 +++++++++++++ 2 files changed, 243 insertions(+) create mode 100644 packages/core-app-api/src/lib/AuthSessionManager/OptionalRefreshSessionManagerMux.test.ts create mode 100644 packages/core-app-api/src/lib/AuthSessionManager/OptionalRefreshSessionManagerMux.ts diff --git a/packages/core-app-api/src/lib/AuthSessionManager/OptionalRefreshSessionManagerMux.test.ts b/packages/core-app-api/src/lib/AuthSessionManager/OptionalRefreshSessionManagerMux.test.ts new file mode 100644 index 0000000000..919948f531 --- /dev/null +++ b/packages/core-app-api/src/lib/AuthSessionManager/OptionalRefreshSessionManagerMux.test.ts @@ -0,0 +1,138 @@ +/* + * Copyright 2020 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Observable, SessionState } from '@backstage/core-plugin-api'; +import { OptionalRefreshSessionManagerMux } from './OptionalRefreshSessionManagerMux'; +import { MutableSessionManager, SessionManager } from './types'; + +class MockManager implements MutableSessionManager { + constructor(public session?: string) {} + + setSession(session: string | undefined): void { + this.session = session; + } + async getSession(): Promise { + return this.session; + } + async removeSession(): Promise { + delete this.session; + } + sessionState$(): Observable { + throw new Error('Method not implemented.'); + } +} + +function trackState(manager: SessionManager) { + const states = new Array(); + manager + .sessionState$() + .subscribe(state => states.push(state === SessionState.SignedIn)); + return states; +} + +describe('OptionalRefreshSessionManagerMux', () => { + it('finds no session', async () => { + const mux = new OptionalRefreshSessionManagerMux({ + staticSessionManager: new MockManager(), + refreshingSessionManager: new MockManager(), + sessionCanRefresh: () => false, + }); + + const states = trackState(mux); + await expect(mux.getSession({})).resolves.toBe(undefined); + expect(states).toEqual([false]); + }); + + it('prioritizes a static session', async () => { + const mux = new OptionalRefreshSessionManagerMux({ + staticSessionManager: new MockManager('static'), + refreshingSessionManager: new MockManager('refreshing'), + sessionCanRefresh: () => false, + }); + + const states = trackState(mux); + await expect(mux.getSession({})).resolves.toBe('static'); + expect(states).toEqual([false, true]); + }); + + it('transfers a refreshing session to the static manager', async () => { + const staticSessionManager = new MockManager(); + const refreshingSessionManager = new MockManager('refreshing'); + const mux = new OptionalRefreshSessionManagerMux({ + staticSessionManager, + refreshingSessionManager, + sessionCanRefresh: () => false, + }); + + const states = trackState(mux); + expect(staticSessionManager.session).toBeUndefined(); + await expect(mux.getSession({})).resolves.toBe('refreshing'); + expect(staticSessionManager.session).toBe('refreshing'); + expect(states).toEqual([false, true]); + }); + + it('relies on the refreshing manager if refresh is available', async () => { + const staticSessionManager = new MockManager(); + const refreshingSessionManager = new MockManager('refreshing'); + const mux = new OptionalRefreshSessionManagerMux({ + staticSessionManager, + refreshingSessionManager, + sessionCanRefresh: () => true, + }); + + const states = trackState(mux); + await expect(mux.getSession({})).resolves.toBe('refreshing'); + expect(staticSessionManager.session).toBeUndefined(); + expect(states).toEqual([false, true]); + }); + + it('can switch between refreshing and static sessions', async () => { + let canRefresh = true; + const staticSessionManager = new MockManager(); + const refreshingSessionManager = new MockManager('refreshing'); + const mux = new OptionalRefreshSessionManagerMux({ + staticSessionManager, + refreshingSessionManager, + sessionCanRefresh: () => canRefresh, + }); + + const states = trackState(mux); + await expect(mux.getSession({})).resolves.toBe('refreshing'); + expect(staticSessionManager.session).toBeUndefined(); + canRefresh = false; + await expect(mux.getSession({})).resolves.toBe('refreshing'); + expect(staticSessionManager.session).toBe('refreshing'); + + expect(states).toEqual([false, true]); + }); + + it('removes sessions from both managers', async () => { + const staticSessionManager = new MockManager('static'); + const refreshingSessionManager = new MockManager('refreshing'); + const mux = new OptionalRefreshSessionManagerMux({ + staticSessionManager, + refreshingSessionManager, + sessionCanRefresh: () => true, + }); + + const states = trackState(mux); + await expect(mux.getSession({})).resolves.toBe('static'); + await mux.removeSession(); + expect(staticSessionManager.session).toBeUndefined(); + expect(refreshingSessionManager.session).toBeUndefined(); + expect(states).toEqual([false, true, false]); + }); +}); diff --git a/packages/core-app-api/src/lib/AuthSessionManager/OptionalRefreshSessionManagerMux.ts b/packages/core-app-api/src/lib/AuthSessionManager/OptionalRefreshSessionManagerMux.ts new file mode 100644 index 0000000000..221a7635ea --- /dev/null +++ b/packages/core-app-api/src/lib/AuthSessionManager/OptionalRefreshSessionManagerMux.ts @@ -0,0 +1,105 @@ +/* + * Copyright 2020 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Observable, SessionState } from '@backstage/core-plugin-api'; +import { + SessionManager, + MutableSessionManager, + GetSessionOptions, +} from './types'; +import { SessionStateTracker } from './SessionStateTracker'; + +type Options = { + /** + * A callback that is called to determine whether a given session supports refresh + */ + sessionCanRefresh: (session: T) => boolean; + + /** + * The session manager that is used if the a session does not support refresh. + */ + staticSessionManager: MutableSessionManager; + + /** + * The session manager that is used if the a session supports refresh. + */ + refreshingSessionManager: SessionManager; +}; + +/** + * OptionalRefreshSessionManagerMux wraps two different session managers, one for + * static session storage and another one that supports refresh. For each session + * that is retrieved is checked for whether it supports refresh. If it does, the + * refreshing session manager is used, otherwise the static session manager is used. + */ +export class OptionalRefreshSessionManagerMux implements SessionManager { + private readonly stateTracker = new SessionStateTracker(); + + private readonly sessionCanRefresh: (session: T) => boolean; + private readonly staticSessionManager: MutableSessionManager; + private readonly refreshingSessionManager: SessionManager; + + constructor(options: Options) { + this.sessionCanRefresh = options.sessionCanRefresh; + this.staticSessionManager = options.staticSessionManager; + this.refreshingSessionManager = options.refreshingSessionManager; + } + + async getSession(options: GetSessionOptions): Promise { + // First we check if there is an existing static session, using an optional request + const staticSession = await this.staticSessionManager.getSession({ + ...options, + optional: true, + }); + if (staticSession) { + this.stateTracker.setIsSignedIn(true); + return staticSession; + } + + // If there is no static session available, we ask the refresh manager to get a session + const session = await this.refreshingSessionManager.getSession(options); + + // Handling the case where the session request is optional + if (!session) { + this.stateTracker.setIsSignedIn(false); + return undefined; + } + + // Next we check if the session we received from the refreshing manager can actually + // be refreshed. If it can, we use this session without storing it in the static manager. + if (this.sessionCanRefresh(session)) { + this.stateTracker.setIsSignedIn(true); + return session; + } + + // If the session can't be refreshed, we store it in the static manager + this.staticSessionManager.setSession(session); + this.stateTracker.setIsSignedIn(true); + return session; + } + + async removeSession(): Promise { + await Promise.all([ + this.refreshingSessionManager.removeSession(), + this.staticSessionManager.removeSession(), + ]); + this.stateTracker.setIsSignedIn(false); + } + + sessionState$(): Observable { + return this.stateTracker.sessionState$(); + } +} From f9db16c4774271070bfea233267f09ba518f9ef9 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 20 Aug 2021 16:26:11 +0200 Subject: [PATCH 07/10] core-app-api: add support for session refresh in GithubAuth Signed-off-by: Patrik Oldsberg --- packages/core-app-api/api-report.md | 2 +- .../implementations/auth/github/GithubAuth.ts | 37 +++++++++++++++---- .../apis/implementations/auth/github/types.ts | 2 +- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/packages/core-app-api/api-report.md b/packages/core-app-api/api-report.md index e2c6b3a939..20d8a5a770 100644 --- a/packages/core-app-api/api-report.md +++ b/packages/core-app-api/api-report.md @@ -394,7 +394,7 @@ export type GithubSession = { providerInfo: { accessToken: string; scopes: Set; - expiresAt: Date; + expiresAt?: Date; }; profile: ProfileInfo; backstageIdentity: BackstageIdentity; diff --git a/packages/core-app-api/src/apis/implementations/auth/github/GithubAuth.ts b/packages/core-app-api/src/apis/implementations/auth/github/GithubAuth.ts index 21ebc5699a..85e3e79ad5 100644 --- a/packages/core-app-api/src/apis/implementations/auth/github/GithubAuth.ts +++ b/packages/core-app-api/src/apis/implementations/auth/github/GithubAuth.ts @@ -29,15 +29,17 @@ import { import { SessionManager } from '../../../../lib/AuthSessionManager/types'; import { AuthSessionStore, + RefreshingAuthSessionManager, StaticAuthSessionManager, } from '../../../../lib/AuthSessionManager'; import { OAuthApiCreateOptions } from '../types'; +import { OptionalRefreshSessionManagerMux } from '../../../../lib/AuthSessionManager/OptionalRefreshSessionManagerMux'; export type GithubAuthResponse = { providerInfo: { accessToken: string; scope: string; - expiresInSeconds: number; + expiresInSeconds?: number; }; profile: ProfileInfo; backstageIdentity: BackstageIdentity; @@ -68,27 +70,46 @@ class GithubAuth implements OAuthApi, SessionApi { providerInfo: { accessToken: res.providerInfo.accessToken, scopes: GithubAuth.normalizeScope(res.providerInfo.scope), - expiresAt: new Date( - Date.now() + res.providerInfo.expiresInSeconds * 1000, - ), + expiresAt: res.providerInfo.expiresInSeconds + ? new Date(Date.now() + res.providerInfo.expiresInSeconds * 1000) + : undefined, }, }; }, }); - const sessionManager = new StaticAuthSessionManager({ + const refreshingSessionManager = new RefreshingAuthSessionManager({ connector, defaultScopes: new Set(defaultScopes), sessionScopes: (session: GithubSession) => session.providerInfo.scopes, + sessionShouldRefresh: (session: GithubSession) => { + const { expiresAt } = session.providerInfo; + if (!expiresAt) { + return false; + } + const expiresInSec = (expiresAt.getTime() - Date.now()) / 1000; + return expiresInSec < 60 * 5; + }, }); - const authSessionStore = new AuthSessionStore({ - manager: sessionManager, + const staticSessionManager = new AuthSessionStore({ + manager: new StaticAuthSessionManager({ + connector, + defaultScopes: new Set(defaultScopes), + sessionScopes: (session: GithubSession) => session.providerInfo.scopes, + }), storageKey: `${provider.id}Session`, sessionScopes: (session: GithubSession) => session.providerInfo.scopes, }); - return new GithubAuth(authSessionStore); + const sessionManagerMux = new OptionalRefreshSessionManagerMux({ + refreshingSessionManager, + staticSessionManager, + sessionCanRefresh: session => + session.providerInfo.expiresAt !== undefined, + }); + + return new GithubAuth(sessionManagerMux); } constructor(private readonly sessionManager: SessionManager) {} diff --git a/packages/core-app-api/src/apis/implementations/auth/github/types.ts b/packages/core-app-api/src/apis/implementations/auth/github/types.ts index 88df25b49d..f5dae3a064 100644 --- a/packages/core-app-api/src/apis/implementations/auth/github/types.ts +++ b/packages/core-app-api/src/apis/implementations/auth/github/types.ts @@ -20,7 +20,7 @@ export type GithubSession = { providerInfo: { accessToken: string; scopes: Set; - expiresAt: Date; + expiresAt?: Date; }; profile: ProfileInfo; backstageIdentity: BackstageIdentity; From 392b36fa1f26d3e522f624e300548e91931e1750 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 20 Aug 2021 16:31:46 +0200 Subject: [PATCH 08/10] changesets: add changeset for GitHub App auth support Signed-off-by: Patrik Oldsberg --- .changeset/four-vans-sit.md | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 .changeset/four-vans-sit.md diff --git a/.changeset/four-vans-sit.md b/.changeset/four-vans-sit.md new file mode 100644 index 0000000000..4082e3062e --- /dev/null +++ b/.changeset/four-vans-sit.md @@ -0,0 +1,10 @@ +--- +'@backstage/core-app-api': patch +'@backstage/plugin-auth-backend': patch +--- + +Added support for using authenticating via GitHub Apps in addition to GitHub OAuth Apps. It used to be possible to use GitHub Apps, but they did not handle session refresh correctly. + +Note that GitHub Apps handle OAuth scope at the app installation level, meaning that the `scope` parameter for `getAccessToken` has no effect. When calling `getAccessToken` in open source plugins, one should still include the appropriate scope, but also document in the plugin README what scopes are required in the case of GitHub Apps. + +In addition, the `authHandler` and `signInResolver` options have been implemented for the GitHub provider in the auth backend. From aba260dd10a344787e939e810b79f9761b911a1f Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 20 Aug 2021 16:50:18 +0200 Subject: [PATCH 09/10] docs/auth/github: updated documentation to reflect app support Signed-off-by: Patrik Oldsberg --- docs/auth/github/provider.md | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/docs/auth/github/provider.md b/docs/auth/github/provider.md index 4b65c8c3c2..81b2e98f2c 100644 --- a/docs/auth/github/provider.md +++ b/docs/auth/github/provider.md @@ -10,11 +10,16 @@ that can authenticate users using GitHub or GitHub Enterprise OAuth. ## Create an OAuth App on GitHub -To add GitHub authentication, you must create an OAuth App from the GitHub +To add GitHub authentication, you must create either a GitHub App, or an OAuth +App from the GitHub [developer settings](https://github.com/settings/developers). The `Homepage URL` should point to Backstage's frontend, while the `Authorization callback URL` will point to the auth backend. +Note that if you're using a GitHub App, the allowed scopes are configured as +part of that app. This means you need to verify what scopes the plugins you use +require, so be sure to check the plugin READMEs for that information. + Settings for local development: - Application name: Backstage (or your custom app name) @@ -51,3 +56,11 @@ The GitHub provider is a structure with three configuration keys: To add the provider to the frontend, add the `githubAuthApi` reference and `SignInPage` component as shown in [Adding the provider to the sign-in page](../index.md#adding-the-provider-to-the-sign-in-page). + +## Difference between GitHub Apps and GitHub OAuth Apps + +GitHub Apps handle OAuth scope at the app installation level, meaning that the +`scope` parameter for the call to `getAccessToken` in the frontend has no +effect. When calling `getAccessToken` in open source plugins, one should still +include the appropriate scope, but also document in the plugin README what +scopes are required for GitHub Apps. From ef2eb3b9ac7d1c88a13e67e6230b717b8b991303 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 21 Aug 2021 11:31:25 +0200 Subject: [PATCH 10/10] auth-backend: fix sign-in provider docs Signed-off-by: Patrik Oldsberg --- plugins/auth-backend/src/providers/github/provider.ts | 9 +++------ plugins/auth-backend/src/providers/google/provider.ts | 9 +++------ plugins/auth-backend/src/providers/microsoft/provider.ts | 9 +++------ plugins/auth-backend/src/providers/okta/provider.ts | 9 +++------ 4 files changed, 12 insertions(+), 24 deletions(-) diff --git a/plugins/auth-backend/src/providers/github/provider.ts b/plugins/auth-backend/src/providers/github/provider.ts index b261ea7be9..b6b198b644 100644 --- a/plugins/auth-backend/src/providers/github/provider.ts +++ b/plugins/auth-backend/src/providers/github/provider.ts @@ -198,13 +198,10 @@ export type GithubProviderOptions = { /** * Configure sign-in for this provider, without it the provider can not be used to sign users in. */ - /** - * Maps an auth result to a Backstage identity for the user. - * - * Set to `'email'` to use the default email-based sign in resolver, which will search - * the catalog for a single user entity that has a matching `google.com/email` annotation. - */ signIn?: { + /** + * Maps an auth result to a Backstage identity for the user. + */ resolver?: SignInResolver; }; }; diff --git a/plugins/auth-backend/src/providers/google/provider.ts b/plugins/auth-backend/src/providers/google/provider.ts index e934a94c48..d85fc2de2d 100644 --- a/plugins/auth-backend/src/providers/google/provider.ts +++ b/plugins/auth-backend/src/providers/google/provider.ts @@ -240,13 +240,10 @@ export type GoogleProviderOptions = { /** * Configure sign-in for this provider, without it the provider can not be used to sign users in. */ - /** - * Maps an auth result to a Backstage identity for the user. - * - * Set to `'email'` to use the default email-based sign in resolver, which will search - * the catalog for a single user entity that has a matching `google.com/email` annotation. - */ signIn?: { + /** + * Maps an auth result to a Backstage identity for the user. + */ resolver?: SignInResolver; }; }; diff --git a/plugins/auth-backend/src/providers/microsoft/provider.ts b/plugins/auth-backend/src/providers/microsoft/provider.ts index 26009474be..2dc259fe74 100644 --- a/plugins/auth-backend/src/providers/microsoft/provider.ts +++ b/plugins/auth-backend/src/providers/microsoft/provider.ts @@ -247,13 +247,10 @@ export type MicrosoftProviderOptions = { /** * Configure sign-in for this provider, without it the provider can not be used to sign users in. */ - /** - * Maps an auth result to a Backstage identity for the user. - * - * Set to `'email'` to use the default email-based sign in resolver, which will search - * the catalog for a single user entity that has a matching `microsoft.com/email` annotation. - */ signIn?: { + /** + * Maps an auth result to a Backstage identity for the user. + */ resolver?: SignInResolver; }; }; diff --git a/plugins/auth-backend/src/providers/okta/provider.ts b/plugins/auth-backend/src/providers/okta/provider.ts index 24ee14becc..44e26bb7fe 100644 --- a/plugins/auth-backend/src/providers/okta/provider.ts +++ b/plugins/auth-backend/src/providers/okta/provider.ts @@ -250,13 +250,10 @@ export type OktaProviderOptions = { /** * Configure sign-in for this provider, without it the provider can not be used to sign users in. */ - /** - * Maps an auth result to a Backstage identity for the user. - * - * Set to `'email'` to use the default email-based sign in resolver, which will search - * the catalog for a single user entity that has a matching `okta.com/email` annotation. - */ signIn?: { + /** + * Maps an auth result to a Backstage identity for the user. + */ resolver?: SignInResolver; }; };