From ad15aaa285d9f2308f8a43eddd011dc8e29eeca3 Mon Sep 17 00:00:00 2001 From: headphonejames Date: Tue, 21 Feb 2023 18:45:50 -0800 Subject: [PATCH] Add onSignInStarted() and onSignInFailure() hooks. Clean up and renaming. Signed-off-by: headphonejames --- packages/core-app-api/src/app/types.ts | 10 +++++++++- .../src/layout/SignInPage/commonProvider.tsx | 16 ++++++++++------ .../src/layout/SignInPage/customProvider.tsx | 3 ++- .../src/layout/SignInPage/guestProvider.tsx | 7 +++++-- .../src/layout/SignInPage/providers.tsx | 10 ++++++++++ .../src/lib/oauth/OAuthAdapter.test.ts | 6 +++--- .../auth-backend/src/lib/oauth/OAuthAdapter.ts | 17 ++++++++++------- plugins/auth-backend/src/lib/oauth/types.ts | 2 +- plugins/auth-backend/src/service/router.ts | 1 - 9 files changed, 50 insertions(+), 22 deletions(-) diff --git a/packages/core-app-api/src/app/types.ts b/packages/core-app-api/src/app/types.ts index 75b8fb2a34..8f5e1f5b27 100644 --- a/packages/core-app-api/src/app/types.ts +++ b/packages/core-app-api/src/app/types.ts @@ -45,9 +45,17 @@ export type BootErrorPageProps = { */ export type SignInPageProps = { /** - * Set the IdentityApi on successful sign in. This should only be called once. + * Invoked when the sign-in process has started. + */ + onSignInStarted(): void; + /** + * Set the IdentityApi on successful sign-in. This should only be called once. */ onSignInSuccess(identityApi: IdentityApi): void; + /** + * Invoked when the sign-in process has failed. + */ + onSignInFailure(): void; }; /** diff --git a/packages/core-components/src/layout/SignInPage/commonProvider.tsx b/packages/core-components/src/layout/SignInPage/commonProvider.tsx index 6796e3f22d..1827981e6f 100644 --- a/packages/core-components/src/layout/SignInPage/commonProvider.tsx +++ b/packages/core-components/src/layout/SignInPage/commonProvider.tsx @@ -28,21 +28,25 @@ import { useApi, errorApiRef } from '@backstage/core-plugin-api'; import { GridItem } from './styles'; import { ForwardedError } from '@backstage/errors'; import { UserIdentity } from './UserIdentity'; -import { PROVIDER_STORAGE_KEY } from './providers'; -const Component: ProviderComponent = ({ config, onSignInSuccess }) => { - const { apiRef, title, message, id } = config as SignInProviderConfig; +const Component: ProviderComponent = ({ + config, + onSignInStarted, + onSignInSuccess, + onSignInFailure, +}) => { + const { apiRef, title, message } = config as SignInProviderConfig; const authApi = useApi(apiRef); const errorApi = useApi(errorApiRef); const handleLogin = async () => { try { - localStorage.setItem(PROVIDER_STORAGE_KEY, id); + onSignInStarted(); const identityResponse = await authApi.getBackstageIdentity({ instantPopup: true, }); if (!identityResponse) { - localStorage.removeItem(PROVIDER_STORAGE_KEY); + onSignInFailure(); throw new Error( `The ${title} provider is not configured to support sign-in`, ); @@ -58,7 +62,7 @@ const Component: ProviderComponent = ({ config, onSignInSuccess }) => { }), ); } catch (error) { - localStorage.removeItem(PROVIDER_STORAGE_KEY); + onSignInFailure(); errorApi.post(new ForwardedError('Login failed', error)); } }; diff --git a/packages/core-components/src/layout/SignInPage/customProvider.tsx b/packages/core-components/src/layout/SignInPage/customProvider.tsx index 101c74d3cd..ad02715447 100644 --- a/packages/core-components/src/layout/SignInPage/customProvider.tsx +++ b/packages/core-components/src/layout/SignInPage/customProvider.tsx @@ -61,7 +61,7 @@ const asInputRef = (renderResult: UseFormRegisterReturn) => { }; }; -const Component: ProviderComponent = ({ onSignInSuccess }) => { +const Component: ProviderComponent = ({ onSignInStarted, onSignInSuccess }) => { const classes = useFormStyles(); const { register, handleSubmit, formState } = useForm({ mode: 'onChange', @@ -70,6 +70,7 @@ const Component: ProviderComponent = ({ onSignInSuccess }) => { const { errors } = formState; const handleResult = ({ userId, idToken }: Data) => { + onSignInStarted(); onSignInSuccess( UserIdentity.fromLegacy({ userId, diff --git a/packages/core-components/src/layout/SignInPage/guestProvider.tsx b/packages/core-components/src/layout/SignInPage/guestProvider.tsx index b2370a98d5..5393c1ea89 100644 --- a/packages/core-components/src/layout/SignInPage/guestProvider.tsx +++ b/packages/core-components/src/layout/SignInPage/guestProvider.tsx @@ -22,7 +22,7 @@ import { GridItem } from './styles'; import { ProviderComponent, ProviderLoader, SignInProvider } from './types'; import { GuestUserIdentity } from './GuestUserIdentity'; -const Component: ProviderComponent = ({ onSignInSuccess }) => ( +const Component: ProviderComponent = ({ onSignInStarted, onSignInSuccess }) => ( ( diff --git a/packages/core-components/src/layout/SignInPage/providers.tsx b/packages/core-components/src/layout/SignInPage/providers.tsx index 716befda17..1f3db4da36 100644 --- a/packages/core-components/src/layout/SignInPage/providers.tsx +++ b/packages/core-components/src/layout/SignInPage/providers.tsx @@ -176,11 +176,21 @@ export const useSignInProviders = ( handleWrappedResult(result); }; + const handleSignInStarted = () => { + localStorage.setItem(PROVIDER_STORAGE_KEY, provider.config!.id); + }; + + const handleSignInFailure = () => { + localStorage.removeItem(PROVIDER_STORAGE_KEY); + }; + return ( ); }), diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts index dd45ab5441..1b6653d97e 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts @@ -178,7 +178,7 @@ describe('OAuthAdapter', () => { const state = { ...defaultState, redirectUrl: 'http://localhost:3000', - authFlow: 'redirect', + flow: 'redirect', }; const mockRequest = createEncodedQueryMockRequest(state); @@ -506,7 +506,7 @@ describe('OAuthAdapter', () => { expect(mockResponse.redirect).not.toHaveBeenCalled(); }); - it('executed a response redirect when authFlow query string is set to "redirect"', async () => { + it('executed a response redirect when flow query string is set to "redirect"', async () => { const handlers = { start: jest.fn(async (_req: { state: OAuthState }) => ({ url: '/url', @@ -531,7 +531,7 @@ describe('OAuthAdapter', () => { ...defaultState, origin: 'http://other.domain', redirectUrl: 'http://domain.org', - authFlow: 'redirect', + flow: 'redirect', }; const mockRequest = { diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts index ac55fa8ad1..78365cda0f 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts @@ -55,7 +55,6 @@ export type OAuthAdapterOptions = { providerId: string; persistScopes?: boolean; appOrigin: string; - redirectUrl?: string; baseUrl: string; cookieConfigurer: CookieConfigurer; isOriginAllowed: (origin: string) => boolean; @@ -69,7 +68,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { handlers: OAuthHandlers, options: Pick< OAuthAdapterOptions, - 'providerId' | 'persistScopes' | 'callbackUrl' | 'redirectUrl' + 'providerId' | 'persistScopes' | 'callbackUrl' >, ): OAuthAdapter { const { appUrl, baseUrl, isOriginAllowed } = config; @@ -104,7 +103,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { const env = req.query.env?.toString(); const origin = req.query.origin?.toString(); const redirectUrl = req.query.redirectUrl?.toString(); - const authFlow = req.query.authFlow?.toString(); + const flow = req.query.authFlow?.toString(); if (!env) { throw new InputError('No env provided in request query parameters'); } @@ -115,7 +114,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { // set a nonce cookie before redirecting to oauth provider this.setNonceCookie(res, nonce, cookieConfig); - const state: OAuthState = { nonce, env, origin, redirectUrl, authFlow }; + const state: OAuthState = { nonce, env, origin, redirectUrl, flow: flow }; // If scopes are persisted then we pass them through the state so that we // can set the cookie on successful auth @@ -142,7 +141,6 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { try { const state: OAuthState = readState(req.query.state?.toString() ?? ''); - const redirectUrl = state.redirectUrl ?? ''; if (state.origin) { try { @@ -181,8 +179,13 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { response: { ...response, backstageIdentity: identity }, }; - if (state.authFlow === 'redirect') { - res.redirect(redirectUrl); + if (state.flow === 'redirect') { + if (!state.redirectUrl) { + throw new InputError( + 'No redirectUrl provided in request query parameters', + ); + } + res.redirect(state.redirectUrl); } // post message back to popup if successful return postMessageResponse(res, appOrigin, responseObj); diff --git a/plugins/auth-backend/src/lib/oauth/types.ts b/plugins/auth-backend/src/lib/oauth/types.ts index 4cb7a94ffd..e960af2988 100644 --- a/plugins/auth-backend/src/lib/oauth/types.ts +++ b/plugins/auth-backend/src/lib/oauth/types.ts @@ -91,7 +91,7 @@ export type OAuthState = { origin?: string; scope?: string; redirectUrl?: string; - authFlow?: string; + flow?: string; }; /** @public */ diff --git a/plugins/auth-backend/src/service/router.ts b/plugins/auth-backend/src/service/router.ts index 9440a2c4ed..8013777415 100644 --- a/plugins/auth-backend/src/service/router.ts +++ b/plugins/auth-backend/src/service/router.ts @@ -107,7 +107,6 @@ export async function createRouter( ...providerFactories, }; const providersConfig = config.getConfig('auth.providers'); - const configuredProviders = providersConfig.keys(); const isOriginAllowed = createOriginFilter(config);