From d8e6b69baeb028abb87ce1d86013d7cea12f0a4d Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Tue, 13 Aug 2024 23:43:41 -0400 Subject: [PATCH] change api auth utility to hook Signed-off-by: Stephen Glass --- packages/app-defaults/src/defaults/apis.ts | 9 --- packages/core-app-api/api-report.md | 9 --- packages/core-app-api/package.json | 1 - .../src/apis/implementations/index.ts | 1 - packages/core-components/package.json | 1 + .../src/layout/SignInPage/SignInPage.tsx | 27 ++++--- .../src/layout/SignInPage/providers.tsx | 29 +++---- packages/core-plugin-api/api-report.md | 8 -- .../src/apis/definitions/AuthErrorApi.ts | 45 ----------- .../src/apis/definitions/index.ts | 1 - plugins/auth-react/src/hooks/index.ts | 1 + .../src/hooks/useSignInAuthError/index.tsx | 2 +- .../useSignInAuthError.test.tsx | 76 ++++++++++--------- .../useSignInAuthError/useSignInAuthError.tsx | 29 ++++--- yarn.lock | 2 +- 15 files changed, 86 insertions(+), 155 deletions(-) delete mode 100644 packages/core-plugin-api/src/apis/definitions/AuthErrorApi.ts rename packages/core-app-api/src/apis/implementations/AuthErrorApi/index.ts => plugins/auth-react/src/hooks/useSignInAuthError/index.tsx (91%) rename packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.test.ts => plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.test.tsx (53%) rename packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts => plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.tsx (59%) diff --git a/packages/app-defaults/src/defaults/apis.ts b/packages/app-defaults/src/defaults/apis.ts index 84902b9f52..4e9e1a492c 100644 --- a/packages/app-defaults/src/defaults/apis.ts +++ b/packages/app-defaults/src/defaults/apis.ts @@ -35,7 +35,6 @@ import { createFetchApi, FetchMiddlewares, VMwareCloudAuth, - SignInAuthErrorApi, } from '@backstage/core-app-api'; import { @@ -59,7 +58,6 @@ import { bitbucketServerAuthApiRef, atlassianAuthApiRef, vmwareCloudAuthApiRef, - authErrorApiRef, } from '@backstage/core-plugin-api'; import { permissionApiRef, @@ -289,11 +287,4 @@ export const apis = [ factory: ({ config, discovery, identity }) => IdentityPermissionApi.create({ config, discovery, identity }), }), - createApiFactory({ - api: authErrorApiRef, - deps: { - discovery: discoveryApiRef, - }, - factory: ({ discovery }) => SignInAuthErrorApi.create({ discovery }), - }), ]; diff --git a/packages/core-app-api/api-report.md b/packages/core-app-api/api-report.md index ae9d709c88..8b8cc991c8 100644 --- a/packages/core-app-api/api-report.md +++ b/packages/core-app-api/api-report.md @@ -16,7 +16,6 @@ import { AppConfig } from '@backstage/config'; import { AppTheme } from '@backstage/core-plugin-api'; import { AppThemeApi } from '@backstage/core-plugin-api'; import { atlassianAuthApiRef } from '@backstage/core-plugin-api'; -import { AuthErrorApi } from '@backstage/core-plugin-api'; import { AuthProviderInfo } from '@backstage/core-plugin-api'; import { AuthRequestOptions } from '@backstage/core-plugin-api'; import { BackstageIdentityApi } from '@backstage/core-plugin-api'; @@ -636,14 +635,6 @@ export class SamlAuth signOut(): Promise; } -// @public -export class SignInAuthErrorApi implements AuthErrorApi { - // (undocumented) - static create(options: { discovery: DiscoveryApi }): SignInAuthErrorApi; - // (undocumented) - getSignInAuthError(): Promise; -} - // @public export type SignInPageProps = PropsWithChildren<{ onSignInSuccess(identityApi: IdentityApi): void; diff --git a/packages/core-app-api/package.json b/packages/core-app-api/package.json index 6025fd5677..0f78b11f98 100644 --- a/packages/core-app-api/package.json +++ b/packages/core-app-api/package.json @@ -44,7 +44,6 @@ "dependencies": { "@backstage/config": "workspace:^", "@backstage/core-plugin-api": "workspace:^", - "@backstage/errors": "workspace:^", "@backstage/types": "workspace:^", "@backstage/version-bridge": "workspace:^", "@types/prop-types": "^15.7.3", diff --git a/packages/core-app-api/src/apis/implementations/index.ts b/packages/core-app-api/src/apis/implementations/index.ts index 7643dfcd82..1c79d3d164 100644 --- a/packages/core-app-api/src/apis/implementations/index.ts +++ b/packages/core-app-api/src/apis/implementations/index.ts @@ -30,4 +30,3 @@ export * from './FeatureFlagsApi'; export * from './FetchApi'; export * from './OAuthRequestApi'; export * from './StorageApi'; -export * from './AuthErrorApi'; diff --git a/packages/core-components/package.json b/packages/core-components/package.json index a2688a0037..49cecfbb21 100644 --- a/packages/core-components/package.json +++ b/packages/core-components/package.json @@ -57,6 +57,7 @@ "@backstage/config": "workspace:^", "@backstage/core-plugin-api": "workspace:^", "@backstage/errors": "workspace:^", + "@backstage/plugin-auth-react": "workspace:^", "@backstage/theme": "workspace:^", "@backstage/version-bridge": "workspace:^", "@date-io/core": "^1.3.13", diff --git a/packages/core-components/src/layout/SignInPage/SignInPage.tsx b/packages/core-components/src/layout/SignInPage/SignInPage.tsx index 932266cdc7..d7e8d1fcf6 100644 --- a/packages/core-components/src/layout/SignInPage/SignInPage.tsx +++ b/packages/core-components/src/layout/SignInPage/SignInPage.tsx @@ -19,14 +19,13 @@ import { configApiRef, SignInPageProps, useApi, - authErrorApiRef, } from '@backstage/core-plugin-api'; import { UserIdentity } from './UserIdentity'; import Button from '@material-ui/core/Button'; import Grid from '@material-ui/core/Grid'; import Typography from '@material-ui/core/Typography'; -import React, { useState } from 'react'; -import { useAsync, useMountEffect } from '@react-hookz/web'; +import React, { useEffect, useState } from 'react'; +import { useMountEffect } from '@react-hookz/web'; import { Progress } from '../../components/Progress'; import { Content } from '../Content/Content'; import { ContentHeader } from '../ContentHeader/ContentHeader'; @@ -39,6 +38,7 @@ import { IdentityProviders, SignInProviderConfig } from './types'; import { coreComponentsTranslationRef } from '../../translation'; import { useTranslationRef } from '@backstage/core-plugin-api/alpha'; import { useSearchParams } from 'react-router-dom'; +import { useSignInAuthError } from '@backstage/plugin-auth-react'; type MultiSignInPageProps = SignInPageProps & { providers: IdentityProviders; @@ -99,7 +99,7 @@ export const SingleSignInPage = ({ const classes = useStyles(); const authApi = useApi(provider.apiRef); const configApi = useApi(configApiRef); - const authErrorApi = useApi(authErrorApiRef); + const { error: signInError, checkAuthError } = useSignInAuthError(); const { t } = useTranslationRef(coreComponentsTranslationRef); const [error, setError] = useState(); @@ -160,20 +160,19 @@ export const SingleSignInPage = ({ } }; - const [_, { execute: checkAuthErrors }] = useAsync(async () => { - if (hasErrorSearchParam) { - const errorResponse = await authErrorApi.getSignInAuthError(); - if (errorResponse) { - setError(errorResponse); - } - } - }); - useMountEffect(() => { - checkAuthErrors(); + if (hasErrorSearchParam) { + checkAuthError(); + } login({ checkExisting: true }); }); + useEffect(() => { + if (signInError) { + setError(signInError); + } + }, [signInError]); + return showLoginPage ? (
diff --git a/packages/core-components/src/layout/SignInPage/providers.tsx b/packages/core-components/src/layout/SignInPage/providers.tsx index 1cf4ad54bf..8d34d8d9de 100644 --- a/packages/core-components/src/layout/SignInPage/providers.tsx +++ b/packages/core-components/src/layout/SignInPage/providers.tsx @@ -14,14 +14,19 @@ * limitations under the License. */ -import React, { useLayoutEffect, useState, useMemo, useCallback } from 'react'; +import React, { + useLayoutEffect, + useState, + useMemo, + useCallback, + useEffect, +} from 'react'; import { SignInPageProps, useApi, useApiHolder, errorApiRef, IdentityApi, - authErrorApiRef, } from '@backstage/core-plugin-api'; import { IdentityProviders, @@ -33,7 +38,8 @@ import { guestProvider } from './guestProvider'; import { customProvider } from './customProvider'; import { IdentityApiSignOutProxy } from './IdentityApiSignOutProxy'; import { useSearchParams } from 'react-router-dom'; -import { useMountEffect, useAsync } from '@react-hookz/web'; +import { useMountEffect } from '@react-hookz/web'; +import { useSignInAuthError } from '@backstage/plugin-auth-react'; const PROVIDER_STORAGE_KEY = '@backstage/core:SignInPage:provider'; @@ -89,7 +95,7 @@ export const useSignInProviders = ( ) => { const errorApi = useApi(errorApiRef); const apiHolder = useApiHolder(); - const authErrorApi = useApi(authErrorApiRef); + const { error: signInError, checkAuthError } = useSignInAuthError(); const [loading, setLoading] = useState(true); // User was redirected back to sign in page with error from auth redirect flow @@ -97,16 +103,13 @@ export const useSignInProviders = ( const errorParam = searchParams.get('error'); const hasErrorSearchParam = errorParam !== 'false' && errorParam !== null; - const [_, { execute: checkAuthErrors }] = useAsync(async () => { - if (hasErrorSearchParam) { - const errorResponse = await authErrorApi.getSignInAuthError(); - if (errorResponse) { - errorApi.post(errorResponse); - } - } - }); + useMountEffect(() => hasErrorSearchParam && checkAuthError()); - useMountEffect(checkAuthErrors); + useEffect(() => { + if (signInError) { + errorApi.post(signInError); + } + }, [errorApi, signInError]); // This decorates the result with sign out logic from this hook const handleWrappedResult = useCallback( diff --git a/packages/core-plugin-api/api-report.md b/packages/core-plugin-api/api-report.md index 22e254cc29..134b338c92 100644 --- a/packages/core-plugin-api/api-report.md +++ b/packages/core-plugin-api/api-report.md @@ -194,14 +194,6 @@ export function attachComponentData

( data: unknown, ): void; -// @public -export type AuthErrorApi = { - getSignInAuthError(): Promise; -}; - -// @public -export const authErrorApiRef: ApiRef; - // @public export type AuthProviderInfo = { id: string; diff --git a/packages/core-plugin-api/src/apis/definitions/AuthErrorApi.ts b/packages/core-plugin-api/src/apis/definitions/AuthErrorApi.ts deleted file mode 100644 index 558c1202a8..0000000000 --- a/packages/core-plugin-api/src/apis/definitions/AuthErrorApi.ts +++ /dev/null @@ -1,45 +0,0 @@ -/* - * Copyright 2024 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 { ApiRef, createApiRef } from '../system'; - -/** - * A wrapper for retrieving any errors caught in authentication redirect flow - * - * @public - */ -export type AuthErrorApi = { - /** - * Get any caught errors during the auth redirect flow - */ - getSignInAuthError(): Promise; -}; - -/** - * The {@link ApiRef} of {@link AuthErrorApi}. - * - * @remarks - * - * This is a wrapper that uses fetch to retrieve any caught errors - * in the authentication redirect flow process. This API calls a - * Backstage endpoint to read any error stored in authentication error - * cookie and returns it to the user. - * - * @public - */ -export const authErrorApiRef: ApiRef = createApiRef({ - id: 'core.auth-error', -}); diff --git a/packages/core-plugin-api/src/apis/definitions/index.ts b/packages/core-plugin-api/src/apis/definitions/index.ts index 406dc1980e..67d442587d 100644 --- a/packages/core-plugin-api/src/apis/definitions/index.ts +++ b/packages/core-plugin-api/src/apis/definitions/index.ts @@ -33,4 +33,3 @@ export * from './FetchApi'; export * from './IdentityApi'; export * from './OAuthRequestApi'; export * from './StorageApi'; -export * from './AuthErrorApi'; diff --git a/plugins/auth-react/src/hooks/index.ts b/plugins/auth-react/src/hooks/index.ts index 1257334498..0e87bab67f 100644 --- a/plugins/auth-react/src/hooks/index.ts +++ b/plugins/auth-react/src/hooks/index.ts @@ -18,3 +18,4 @@ // which hooks are public API and should be exported from the package. export * from './useCookieAuthRefresh'; +export * from './useSignInAuthError'; diff --git a/packages/core-app-api/src/apis/implementations/AuthErrorApi/index.ts b/plugins/auth-react/src/hooks/useSignInAuthError/index.tsx similarity index 91% rename from packages/core-app-api/src/apis/implementations/AuthErrorApi/index.ts rename to plugins/auth-react/src/hooks/useSignInAuthError/index.tsx index 6f3a3ae269..549b5b9f1f 100644 --- a/packages/core-app-api/src/apis/implementations/AuthErrorApi/index.ts +++ b/plugins/auth-react/src/hooks/useSignInAuthError/index.tsx @@ -14,4 +14,4 @@ * limitations under the License. */ -export { SignInAuthErrorApi } from './SignInAuthErrorApi'; +export { useSignInAuthError } from './useSignInAuthError'; diff --git a/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.test.ts b/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.test.tsx similarity index 53% rename from packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.test.ts rename to plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.test.tsx index c4d6fe83de..a5ac57ec83 100644 --- a/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.test.ts +++ b/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.test.tsx @@ -14,29 +14,20 @@ * limitations under the License. */ -import { DiscoveryApi } from '@backstage/core-plugin-api'; +import React from 'react'; +import { act, renderHook } from '@testing-library/react'; +import { discoveryApiRef } from '@backstage/core-plugin-api'; +import { TestApiProvider } from '@backstage/test-utils'; +import { useSignInAuthError } from './useSignInAuthError'; import { serializeError } from '@backstage/errors'; -import { SignInAuthErrorApi } from './SignInAuthErrorApi'; -describe('SignInAuthErrorApi', () => { - const mockDiscoveryApi = { - getBaseUrl: jest.fn(), - } as unknown as jest.Mocked; +describe('useCookieAuthRefresh', () => { + const discoveryApiMock = { + getBaseUrl: jest.fn().mockResolvedValue('http://localhost:7000/api/auth'), + }; beforeEach(() => { - jest.resetAllMocks(); - }); - - it('should create an instance of SignInAuthErrorApi', async () => { - const api = SignInAuthErrorApi.create({ - discovery: mockDiscoveryApi, - }); - - mockDiscoveryApi.getBaseUrl.mockResolvedValue( - 'http://localhost:7007/api/auth', - ); - - expect(api).toBeInstanceOf(SignInAuthErrorApi); + jest.clearAllMocks(); }); it('should return an error when the cookie returns an error object', async () => { @@ -45,50 +36,63 @@ describe('SignInAuthErrorApi', () => { message: 'This is a test error', }; const serializedError = serializeError(errorObject); - mockDiscoveryApi.getBaseUrl.mockResolvedValue( - 'http://localhost:7000/api/auth', - ); global.fetch = jest.fn().mockResolvedValue({ ok: true, json: jest.fn().mockResolvedValue(serializedError), } as unknown as Response); - const api = SignInAuthErrorApi.create({ discovery: mockDiscoveryApi }); - const error = await api.getSignInAuthError(); + const { result } = renderHook(() => useSignInAuthError(), { + wrapper: ({ children }) => ( + + {children} + + ), + }); - expect(mockDiscoveryApi.getBaseUrl).toHaveBeenCalledWith('auth'); + await act(async () => { + result.current.checkAuthError(); + }); + + expect(discoveryApiMock.getBaseUrl).toHaveBeenCalledWith('auth'); expect(fetch).toHaveBeenCalledWith( 'http://localhost:7000/api/auth/.backstage/error', { credentials: 'include', }, ); - expect(error).toBeInstanceOf(Error); - expect((error as Error).name).toEqual(errorObject.name); - expect((error as Error).message).toEqual(errorObject.message); + expect(result.current.error).toBeInstanceOf(Error); + expect((result.current.error as Error).name).toEqual(errorObject.name); + expect((result.current.error as Error).message).toEqual( + errorObject.message, + ); }); it('should return undefined when the backend does not return an error object', async () => { - mockDiscoveryApi.getBaseUrl.mockResolvedValue( - 'http://localhost:7000/api/auth', - ); - global.fetch = jest.fn().mockResolvedValue({ ok: true, json: jest.fn().mockResolvedValue(undefined), } as unknown as Response); - const api = SignInAuthErrorApi.create({ discovery: mockDiscoveryApi }); - const error = await api.getSignInAuthError(); + const { result } = renderHook(() => useSignInAuthError(), { + wrapper: ({ children }) => ( + + {children} + + ), + }); - expect(mockDiscoveryApi.getBaseUrl).toHaveBeenCalledWith('auth'); + await act(async () => { + result.current.checkAuthError(); + }); + + expect(discoveryApiMock.getBaseUrl).toHaveBeenCalledWith('auth'); expect(fetch).toHaveBeenCalledWith( 'http://localhost:7000/api/auth/.backstage/error', { credentials: 'include', }, ); - expect(error).toBeUndefined(); + expect(result.current.error).toBeUndefined(); }); }); diff --git a/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts b/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.tsx similarity index 59% rename from packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts rename to plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.tsx index 77b3c53f14..8d4af1f555 100644 --- a/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts +++ b/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.tsx @@ -14,25 +14,20 @@ * limitations under the License. */ -import { DiscoveryApi, AuthErrorApi } from '@backstage/core-plugin-api'; +import { discoveryApiRef, useApi } from '@backstage/core-plugin-api'; +import { useAsync } from '@react-hookz/web'; import { deserializeError } from '@backstage/errors'; -/** - * The default implementation of the AuthErrorApi, which simply calls auth error endpoint. - * @public - */ -export class SignInAuthErrorApi implements AuthErrorApi { - private constructor(private readonly discoveryApi: DiscoveryApi) {} +export function useSignInAuthError(): { + error: Error | undefined; + checkAuthError: () => void; +} { + const discoveryApi = useApi(discoveryApiRef); - static create(options: { discovery: DiscoveryApi }) { - const { discovery } = options; - return new SignInAuthErrorApi(discovery); - } + const [state, { execute: checkAuthError }] = useAsync(async () => { + const baseUrl = await discoveryApi.getBaseUrl('auth'); - async getSignInAuthError(): Promise { - const baseUrl = await this.discoveryApi.getBaseUrl('auth'); - - // use native fetch instead of depending on fetchApi because + // use native fetch instead of fetchApi because // we are not signed in and are calling an unauthenticated endpoint const response = await fetch(`${baseUrl}/.backstage/error`, { credentials: 'include', @@ -40,5 +35,7 @@ export class SignInAuthErrorApi implements AuthErrorApi { const data = await response.json(); return data ? deserializeError(data) : undefined; - } + }); + + return { error: state.result, checkAuthError }; } diff --git a/yarn.lock b/yarn.lock index cb41faa20f..dfd1cffd9e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4123,7 +4123,6 @@ __metadata: "@backstage/cli": "workspace:^" "@backstage/config": "workspace:^" "@backstage/core-plugin-api": "workspace:^" - "@backstage/errors": "workspace:^" "@backstage/test-utils": "workspace:^" "@backstage/types": "workspace:^" "@backstage/version-bridge": "workspace:^" @@ -4288,6 +4287,7 @@ __metadata: "@backstage/core-app-api": "workspace:^" "@backstage/core-plugin-api": "workspace:^" "@backstage/errors": "workspace:^" + "@backstage/plugin-auth-react": "workspace:^" "@backstage/test-utils": "workspace:^" "@backstage/theme": "workspace:^" "@backstage/version-bridge": "workspace:^"