From cc0e6b0d30b4db91214088a45a11609404263a72 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 24 Jun 2020 11:30:27 +0200 Subject: [PATCH] auth-backend: leave it to the providers to figure out the backstage identity --- .../core-api/src/apis/definitions/auth.ts | 2 +- .../layout/Sidebar/Settings/UserProfile.tsx | 6 ++- .../src/lib/OAuthProvider.test.ts | 13 +++--- plugins/auth-backend/src/lib/OAuthProvider.ts | 40 ++++++++++--------- .../src/lib/PassportStrategyHelper.ts | 4 -- .../src/providers/github/provider.ts | 8 ++-- .../src/providers/google/provider.ts | 25 +++++++++--- plugins/auth-backend/src/providers/types.ts | 21 +++++----- 8 files changed, 67 insertions(+), 52 deletions(-) diff --git a/packages/core-api/src/apis/definitions/auth.ts b/packages/core-api/src/apis/definitions/auth.ts index ccd40e66ce..d5c0e9c0a4 100644 --- a/packages/core-api/src/apis/definitions/auth.ts +++ b/packages/core-api/src/apis/definitions/auth.ts @@ -173,7 +173,7 @@ export type ProfileInfo = { /** * Email ID. */ - email: string; + email?: string; /** * Display name that can be presented to the user. diff --git a/packages/core/src/layout/Sidebar/Settings/UserProfile.tsx b/packages/core/src/layout/Sidebar/Settings/UserProfile.tsx index 101ae7010b..3b854801c9 100644 --- a/packages/core/src/layout/Sidebar/Settings/UserProfile.tsx +++ b/packages/core/src/layout/Sidebar/Settings/UserProfile.tsx @@ -34,14 +34,16 @@ export const UserProfile: FC<{ open: boolean; setOpen: Function }> = ({ }) => { const ref = useRef(); // for scrolling down when collapse item opens const classes = useStyles(); - const profile = useApi(identityApiRef).getProfile(); + const identityApi = useApi(identityApiRef); const handleClick = () => { setOpen(!open); setTimeout(() => ref.current?.scrollIntoView({ behavior: 'smooth' }), 300); }; - const displayName = profile.displayName ?? profile.email; + const userId = identityApi.getUserId(); + const profile = identityApi.getProfile(); + const displayName = profile.displayName ?? userId; const SignInAvatar = () => ( {displayName[0]} diff --git a/plugins/auth-backend/src/lib/OAuthProvider.test.ts b/plugins/auth-backend/src/lib/OAuthProvider.test.ts index f92875fd48..f67a91dfcd 100644 --- a/plugins/auth-backend/src/lib/OAuthProvider.test.ts +++ b/plugins/auth-backend/src/lib/OAuthProvider.test.ts @@ -23,13 +23,9 @@ import { verifyNonce, OAuthProvider, } from './OAuthProvider'; -import { - WebMessageResponse, - OAuthProviderHandlers, - OAuthResponse, -} from '../providers/types'; +import { WebMessageResponse, OAuthProviderHandlers } from '../providers/types'; -const mockResponseData: OAuthResponse = { +const mockResponseData = { providerInfo: { accessToken: 'ACCESS_TOKEN', idToken: 'ID_TOKEN', @@ -39,6 +35,9 @@ const mockResponseData: OAuthResponse = { profile: { email: 'foo@bar.com', }, + backstageIdentity: { + id: 'foo', + }, }; describe('OAuthProvider Utils', () => { @@ -350,7 +349,7 @@ describe('OAuthProvider', () => { expect(mockResponse.send).toHaveBeenCalledWith({ ...mockResponseData, backstageIdentity: { - id: mockResponseData.profile.email, + id: mockResponseData.backstageIdentity.id, idToken: 'my-id-token', }, }); diff --git a/plugins/auth-backend/src/lib/OAuthProvider.ts b/plugins/auth-backend/src/lib/OAuthProvider.ts index dac31af451..b7179659f9 100644 --- a/plugins/auth-backend/src/lib/OAuthProvider.ts +++ b/plugins/auth-backend/src/lib/OAuthProvider.ts @@ -18,10 +18,10 @@ import express from 'express'; import crypto from 'crypto'; import { URL } from 'url'; import { - AuthResponse, AuthProviderRouteHandlers, OAuthProviderHandlers, WebMessageResponse, + BackstageIdentity, } from '../providers/types'; import { InputError } from '@backstage/backend-common'; import { TokenIssuer } from '../identity'; @@ -147,19 +147,12 @@ export class OAuthProvider implements AuthProviderRouteHandlers { this.setRefreshTokenCookie(res, refreshToken); } - const id = response.profile.email; - const idToken = await this.options.tokenIssuer.issueToken({ - claims: { sub: id }, - }); - const fullResponse: AuthResponse = { - ...response, - backstageIdentity: { id, idToken }, - }; + await this.populateIdentity(response.backstageIdentity); // post message back to popup if successful return postMessageResponse(res, this.options.appOrigin, { type: 'authorization_response', - response: fullResponse, + response, }); } catch (error) { // post error message back to popup if failure @@ -213,21 +206,30 @@ export class OAuthProvider implements AuthProviderRouteHandlers { // get new access_token const response = await this.providerHandlers.refresh(refreshToken, scope); - const id = response.profile.email; - const idToken = await this.options.tokenIssuer.issueToken({ - claims: { sub: id }, - }); - const fullResponse: AuthResponse = { - ...response, - backstageIdentity: { id, idToken }, - }; + await this.populateIdentity(response.backstageIdentity); - res.send(fullResponse); + res.send(response); } catch (error) { res.status(401).send(`${error.message}`); } } + /** + * If the response from the OAuth provider includes a Backstage identity, we + * make sure it's populated with all the information we can derive from the user ID. + */ + private async populateIdentity(identity?: BackstageIdentity) { + if (!identity) { + return; + } + + if (!identity.idToken) { + identity.idToken = await this.options.tokenIssuer.issueToken({ + claims: { sub: identity.id }, + }); + } + } + private setNonceCookie = (res: express.Response, nonce: string) => { res.cookie(`${this.options.providerId}-nonce`, nonce, { maxAge: TEN_MINUTES_MS, diff --git a/plugins/auth-backend/src/lib/PassportStrategyHelper.ts b/plugins/auth-backend/src/lib/PassportStrategyHelper.ts index 46a386f419..1167946560 100644 --- a/plugins/auth-backend/src/lib/PassportStrategyHelper.ts +++ b/plugins/auth-backend/src/lib/PassportStrategyHelper.ts @@ -57,10 +57,6 @@ export const makeProfileInfo = ( } } - if (!email) { - throw new Error('No email received in profile info'); - } - return { email, picture, diff --git a/plugins/auth-backend/src/providers/github/provider.ts b/plugins/auth-backend/src/providers/github/provider.ts index 3ae9858bb1..01652a6d7e 100644 --- a/plugins/auth-backend/src/providers/github/provider.ts +++ b/plugins/auth-backend/src/providers/github/provider.ts @@ -72,15 +72,13 @@ export class GithubAuthProvider implements OAuthProviderHandlers { return await executeRedirectStrategy(req, this._strategy, options); } - async handler(req: express.Request): Promise<{ response: OAuthResponse }> { - const result = await executeFrameHandlerStrategy( + async handler(req: express.Request) { + const { response } = await executeFrameHandlerStrategy( req, this._strategy, ); - return { - response: result.response, - }; + return { response }; } } diff --git a/plugins/auth-backend/src/providers/google/provider.ts b/plugins/auth-backend/src/providers/google/provider.ts index f12b248021..04705b66c1 100644 --- a/plugins/auth-backend/src/providers/google/provider.ts +++ b/plugins/auth-backend/src/providers/google/provider.ts @@ -97,14 +97,14 @@ export class GoogleAuthProvider implements OAuthProviderHandlers { async handler( req: express.Request, ): Promise<{ response: OAuthResponse; refreshToken: string }> { - const result = await executeFrameHandlerStrategy< + const { response, privateInfo } = await executeFrameHandlerStrategy< OAuthResponse, PrivateInfo >(req, this._strategy); return { - response: result.response, - refreshToken: result.privateInfo.refreshToken, + response: await this.populateIdentity(response), + refreshToken: privateInfo.refreshToken, }; } @@ -121,7 +121,7 @@ export class GoogleAuthProvider implements OAuthProviderHandlers { params.id_token, ); - return { + return this.populateIdentity({ providerInfo: { accessToken, idToken: params.id_token, @@ -129,7 +129,22 @@ export class GoogleAuthProvider implements OAuthProviderHandlers { scope: params.scope, }, profile, - }; + }); + } + + private async populateIdentity( + response: OAuthResponse, + ): Promise { + const { profile } = response; + + if (!profile.email) { + throw new Error('Google profile contained no email'); + } + + // TODO(Rugvip): Hardcoded to the local part of the email for now + const id = profile.email.split('@')[0]; + + return { ...response, backstageIdentity: { id } }; } } diff --git a/plugins/auth-backend/src/providers/types.ts b/plugins/auth-backend/src/providers/types.ts index 81773bf9dc..56d4db79fa 100644 --- a/plugins/auth-backend/src/providers/types.ts +++ b/plugins/auth-backend/src/providers/types.ts @@ -100,14 +100,20 @@ export interface OAuthProviderHandlers { */ handler( req: express.Request, - ): Promise<{ response: OAuthResponse; refreshToken?: string }>; + ): Promise<{ + response: AuthResponse; + refreshToken?: string; + }>; /** * (Optional) Given a refresh token and scope fetches a new access token from the auth provider. * @param {string} refreshToken * @param {string} scope */ - refresh?(refreshToken: string, scope: string): Promise; + refresh?( + refreshToken: string, + scope: string, + ): Promise>; /** * (Optional) Sign out of the auth provider. @@ -192,13 +198,10 @@ export type AuthProviderFactory = ( export type AuthResponse = { providerInfo: ProviderInfo; profile: ProfileInfo; - backstageIdentity: BackstageIdentity; + backstageIdentity?: BackstageIdentity; }; -export type OAuthResponse = Omit< - AuthResponse, - 'backstageIdentity' ->; +export type OAuthResponse = AuthResponse; export type BackstageIdentity = { /** @@ -209,7 +212,7 @@ export type BackstageIdentity = { /** * An ID token that can be used to authenticate the user within Backstage. */ - idToken: string; + idToken?: string; }; export type OAuthProviderInfo = { @@ -279,7 +282,7 @@ export type ProfileInfo = { /** * Email ID of the signed in user. */ - email: string; + email?: string; /** * Display name that can be presented to the signed in user. */