From 599f2929f2311fbefb2d62886882809d45e8d9ea Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Fri, 14 Jan 2022 21:58:51 +0100 Subject: [PATCH 1/4] Hang identity api methods until implementation is set. Signed-off-by: Eric Peterson --- .../IdentityApi/AppIdentityProxy.ts | 46 +++++++++---------- .../src/apis/system/ApiProvider.tsx | 18 ++++++++ packages/core-app-api/src/app/AppManager.tsx | 25 +++++++++- 3 files changed, 63 insertions(+), 26 deletions(-) diff --git a/packages/core-app-api/src/apis/implementations/IdentityApi/AppIdentityProxy.ts b/packages/core-app-api/src/apis/implementations/IdentityApi/AppIdentityProxy.ts index 5677299c83..cf69ead361 100644 --- a/packages/core-app-api/src/apis/implementations/IdentityApi/AppIdentityProxy.ts +++ b/packages/core-app-api/src/apis/implementations/IdentityApi/AppIdentityProxy.ts @@ -46,11 +46,20 @@ type CompatibilityIdentityApi = IdentityApi & { * and sign-in page. */ export class AppIdentityProxy implements IdentityApi { - private target?: CompatibilityIdentityApi; +private target?: CompatibilityIdentityApi; +private waitForTarget: Promise; + private resolveTarget: (api: IdentityApi) => void = () => {}; + + constructor() { + this.waitForTarget = new Promise(resolve => { + this.resolveTarget = resolve; + }); + } // This is called by the app manager once the sign-in page provides us with an implementation setTarget(identityApi: CompatibilityIdentityApi) { this.target = identityApi; + this.resolveTarget(identityApi); } getUserId(): string { @@ -76,17 +85,11 @@ export class AppIdentityProxy implements IdentityApi { } async getProfileInfo(): Promise { - if (!this.target) { - throw mkError('getProfileInfo'); - } - return this.target.getProfileInfo(); + return this.waitForTarget.then(target => target.getProfileInfo()); } async getBackstageIdentity(): Promise { - if (!this.target) { - throw mkError('getBackstageIdentity'); - } - const identity = await this.target.getBackstageIdentity(); + const identity = await this.waitForTarget.then(target => target.getBackstageIdentity()); if (!identity.userEntityRef.match(/^.*:.*\/.*$/)) { // eslint-disable-next-line no-console console.warn( @@ -99,28 +102,21 @@ export class AppIdentityProxy implements IdentityApi { } async getCredentials(): Promise<{ token?: string | undefined }> { - if (!this.target) { - throw mkError('getCredentials'); - } - return this.target.getCredentials(); + return this.waitForTarget.then(target => target.getCredentials()); } async getIdToken(): Promise { - if (!this.target) { - throw mkError('getIdToken'); - } - if (!this.target.getIdToken) { - throw new Error('IdentityApi does not implement getIdToken'); - } - logDeprecation('getIdToken'); - return this.target.getIdToken(); + return this.waitForTarget.then((target: CompatibilityIdentityApi) => { + if (!target.getIdToken) { + throw new Error('IdentityApi does not implement getIdToken'); + } + logDeprecation('getIdToken'); + return target.getIdToken() + }); } async signOut(): Promise { - if (!this.target) { - throw mkError('signOut'); - } - await this.target.signOut(); + await this.waitForTarget.then(target => target.signOut()); location.reload(); } } diff --git a/packages/core-app-api/src/apis/system/ApiProvider.tsx b/packages/core-app-api/src/apis/system/ApiProvider.tsx index c75f883a51..681c741afb 100644 --- a/packages/core-app-api/src/apis/system/ApiProvider.tsx +++ b/packages/core-app-api/src/apis/system/ApiProvider.tsx @@ -53,6 +53,24 @@ export const ApiProvider = (props: PropsWithChildren) => { ); }; +/** + * Same as above, but does not inherit APIs from API providers further up in + * the react tree. + * + * Private to (and only used in) this package. + */ +export const IsolatedApiProvider = ( + props: PropsWithChildren, +) => { + const { apis, children } = props; + return ( + + ); +}; + ApiProvider.propTypes = { apis: PropTypes.shape({ get: PropTypes.func.isRequired }).isRequired, children: PropTypes.node, diff --git a/packages/core-app-api/src/app/AppManager.tsx b/packages/core-app-api/src/app/AppManager.tsx index 0262ece8f4..484a63396d 100644 --- a/packages/core-app-api/src/app/AppManager.tsx +++ b/packages/core-app-api/src/app/AppManager.tsx @@ -79,6 +79,7 @@ import { AppThemeProvider } from './AppThemeProvider'; import { defaultConfigLoader } from './defaultConfigLoader'; import { ApiRegistry } from '../apis/system/ApiRegistry'; import { resolveRouteBindings } from './resolveRouteBindings'; +import { IsolatedApiProvider } from '../apis/system/ApiProvider'; type CompatiblePlugin = | BackstagePlugin @@ -335,7 +336,14 @@ export class AppManager implements BackstageApp { const [identityApi, setIdentityApi] = useState(); if (!identityApi) { - return ; + // Encapsulate the sign in page component in an API provider which + // contains all APIs except the identity API. Provides fast feedback in + // case the identity API is accidentally required on the sign-in page. + return ( + + + + ); } this.appIdentityProxy.setTarget(identityApi); @@ -473,6 +481,21 @@ export class AppManager implements BackstageApp { return this.apiHolder; } + private getIdentitylessApiHolder() { + const registrySansIdentity = new ApiFactoryRegistry(); + + // Assume all APIs in the current registry are valid. + this.apiFactoryRegistry.getAllApis().forEach(apiRef => { + const factory = this.apiFactoryRegistry.get(apiRef); + // Add all API factories except identity! + if (factory && apiRef !== identityApiRef) { + registrySansIdentity.register('default', factory); + } + }); + + return new ApiResolver(registrySansIdentity); + } + private verifyPlugins(plugins: Iterable) { const pluginIds = new Set(); From 76aebab211655eaf1a79c74ace04ea09edf0095d Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Sat, 15 Jan 2022 17:18:34 +0100 Subject: [PATCH 2/4] Test isolated ApiProvider behavior for SignInPage component. Signed-off-by: Eric Peterson --- .../core-app-api/src/app/AppManager.test.tsx | 105 ++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/packages/core-app-api/src/app/AppManager.test.tsx b/packages/core-app-api/src/app/AppManager.test.tsx index 467145acfc..718a90dabe 100644 --- a/packages/core-app-api/src/app/AppManager.test.tsx +++ b/packages/core-app-api/src/app/AppManager.test.tsx @@ -34,6 +34,9 @@ import { createSubRouteRef, createRoutableExtension, analyticsApiRef, + useApi, + identityApiRef, + fetchApiRef, } from '@backstage/core-plugin-api'; import { AppManager } from './AppManager'; import { AppComponents, AppIcons } from './types'; @@ -569,4 +572,106 @@ describe('Integration Test', () => { ), ]); }); + + it('explodes if identityApi is directly used in a given SignInPage', async () => { + // Sign-in page that uses APIs it oughtn't to directly! + const OffendingSignInPage = () => { + try { + useApi(identityApiRef); + return <>No problem.; + } catch (e) { + return <>{(e as any).message}; + } + }; + + const app = new AppManager({ + themes: [ + { + id: 'light', + title: 'Light Theme', + variant: 'light', + Provider: ({ children }) => <>{children}, + }, + ], + icons, + components: { + ...components, + SignInPage: OffendingSignInPage, + }, + configLoader: async () => [], + }); + + const Provider = app.getProvider(); + const Router = app.getRouter(); + const { getByText } = await renderWithEffects( + + + + } /> + + + , + ); + + expect( + getByText('No implementation available for apiRef{core.identity}'), + ).toBeInTheDocument(); + }); + + it('explodes if identityApi is indirectly used in a given SignInPage', async () => { + // Set up fetchApi with a dependency on identityApi. + const apis = [ + createApiFactory({ + api: fetchApiRef, + deps: { identityApiRef }, + factory: () => ({} as any), + }), + ]; + + // Sign-in page that uses APIs it oughtn't to, but indirectly! + const OffendingSignInPage = () => { + try { + useApi(fetchApiRef); + return <>No problem.; + } catch (e) { + return <>{(e as any).message}; + } + }; + + const app = new AppManager({ + apis, + themes: [ + { + id: 'light', + title: 'Light Theme', + variant: 'light', + Provider: ({ children }) => <>{children}, + }, + ], + icons, + components: { + ...components, + SignInPage: OffendingSignInPage, + }, + configLoader: async () => [], + }); + + const Provider = app.getProvider(); + const Router = app.getRouter(); + const { getByText } = await renderWithEffects( + + + + } /> + + + , + ); + + expect( + getByText( + 'No API factory available for dependency apiRef{core.identity} of dependent apiRef{core.fetch}', + ), + ).toBeInTheDocument(); + }); }); From f959c227877f5c4d1fc72037f64bbcb1cb8bac3f Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Sat, 15 Jan 2022 17:28:49 +0100 Subject: [PATCH 3/4] Changeset. Signed-off-by: Eric Peterson --- .changeset/varldens-starkaste-bjorn.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/varldens-starkaste-bjorn.md diff --git a/.changeset/varldens-starkaste-bjorn.md b/.changeset/varldens-starkaste-bjorn.md new file mode 100644 index 0000000000..5b147395fb --- /dev/null +++ b/.changeset/varldens-starkaste-bjorn.md @@ -0,0 +1,7 @@ +--- +'@backstage/core-app-api': patch +--- + +Asynchronous methods on the identity API can now reliably be called at any time, including early in the bootstrap process or prior to successful sign-in. + +Previously in such situations, a `Tried to access IdentityApi before app was loaded` error would be thrown. Now, those methods will wait and resolve eventually (as soon as a concrete identity API is provided). From 6c59abc74e2148c9e794e990cfd298528fbbc821 Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Wed, 19 Jan 2022 10:31:05 +0100 Subject: [PATCH 4/4] Revert app manager protections and let identity API hang. Signed-off-by: Eric Peterson --- .../IdentityApi/AppIdentityProxy.ts | 16 +-- .../src/apis/system/ApiProvider.tsx | 18 --- .../core-app-api/src/app/AppManager.test.tsx | 105 ------------------ packages/core-app-api/src/app/AppManager.tsx | 25 +---- 4 files changed, 10 insertions(+), 154 deletions(-) diff --git a/packages/core-app-api/src/apis/implementations/IdentityApi/AppIdentityProxy.ts b/packages/core-app-api/src/apis/implementations/IdentityApi/AppIdentityProxy.ts index cf69ead361..3ef3a1f6d3 100644 --- a/packages/core-app-api/src/apis/implementations/IdentityApi/AppIdentityProxy.ts +++ b/packages/core-app-api/src/apis/implementations/IdentityApi/AppIdentityProxy.ts @@ -46,12 +46,12 @@ type CompatibilityIdentityApi = IdentityApi & { * and sign-in page. */ export class AppIdentityProxy implements IdentityApi { -private target?: CompatibilityIdentityApi; -private waitForTarget: Promise; - private resolveTarget: (api: IdentityApi) => void = () => {}; + private target?: CompatibilityIdentityApi; + private waitForTarget: Promise; + private resolveTarget: (api: CompatibilityIdentityApi) => void = () => {}; constructor() { - this.waitForTarget = new Promise(resolve => { + this.waitForTarget = new Promise(resolve => { this.resolveTarget = resolve; }); } @@ -89,7 +89,9 @@ private waitForTarget: Promise; } async getBackstageIdentity(): Promise { - const identity = await this.waitForTarget.then(target => target.getBackstageIdentity()); + const identity = await this.waitForTarget.then(target => + target.getBackstageIdentity(), + ); if (!identity.userEntityRef.match(/^.*:.*\/.*$/)) { // eslint-disable-next-line no-console console.warn( @@ -106,12 +108,12 @@ private waitForTarget: Promise; } async getIdToken(): Promise { - return this.waitForTarget.then((target: CompatibilityIdentityApi) => { + return this.waitForTarget.then(target => { if (!target.getIdToken) { throw new Error('IdentityApi does not implement getIdToken'); } logDeprecation('getIdToken'); - return target.getIdToken() + return target.getIdToken(); }); } diff --git a/packages/core-app-api/src/apis/system/ApiProvider.tsx b/packages/core-app-api/src/apis/system/ApiProvider.tsx index 681c741afb..c75f883a51 100644 --- a/packages/core-app-api/src/apis/system/ApiProvider.tsx +++ b/packages/core-app-api/src/apis/system/ApiProvider.tsx @@ -53,24 +53,6 @@ export const ApiProvider = (props: PropsWithChildren) => { ); }; -/** - * Same as above, but does not inherit APIs from API providers further up in - * the react tree. - * - * Private to (and only used in) this package. - */ -export const IsolatedApiProvider = ( - props: PropsWithChildren, -) => { - const { apis, children } = props; - return ( - - ); -}; - ApiProvider.propTypes = { apis: PropTypes.shape({ get: PropTypes.func.isRequired }).isRequired, children: PropTypes.node, diff --git a/packages/core-app-api/src/app/AppManager.test.tsx b/packages/core-app-api/src/app/AppManager.test.tsx index 718a90dabe..467145acfc 100644 --- a/packages/core-app-api/src/app/AppManager.test.tsx +++ b/packages/core-app-api/src/app/AppManager.test.tsx @@ -34,9 +34,6 @@ import { createSubRouteRef, createRoutableExtension, analyticsApiRef, - useApi, - identityApiRef, - fetchApiRef, } from '@backstage/core-plugin-api'; import { AppManager } from './AppManager'; import { AppComponents, AppIcons } from './types'; @@ -572,106 +569,4 @@ describe('Integration Test', () => { ), ]); }); - - it('explodes if identityApi is directly used in a given SignInPage', async () => { - // Sign-in page that uses APIs it oughtn't to directly! - const OffendingSignInPage = () => { - try { - useApi(identityApiRef); - return <>No problem.; - } catch (e) { - return <>{(e as any).message}; - } - }; - - const app = new AppManager({ - themes: [ - { - id: 'light', - title: 'Light Theme', - variant: 'light', - Provider: ({ children }) => <>{children}, - }, - ], - icons, - components: { - ...components, - SignInPage: OffendingSignInPage, - }, - configLoader: async () => [], - }); - - const Provider = app.getProvider(); - const Router = app.getRouter(); - const { getByText } = await renderWithEffects( - - - - } /> - - - , - ); - - expect( - getByText('No implementation available for apiRef{core.identity}'), - ).toBeInTheDocument(); - }); - - it('explodes if identityApi is indirectly used in a given SignInPage', async () => { - // Set up fetchApi with a dependency on identityApi. - const apis = [ - createApiFactory({ - api: fetchApiRef, - deps: { identityApiRef }, - factory: () => ({} as any), - }), - ]; - - // Sign-in page that uses APIs it oughtn't to, but indirectly! - const OffendingSignInPage = () => { - try { - useApi(fetchApiRef); - return <>No problem.; - } catch (e) { - return <>{(e as any).message}; - } - }; - - const app = new AppManager({ - apis, - themes: [ - { - id: 'light', - title: 'Light Theme', - variant: 'light', - Provider: ({ children }) => <>{children}, - }, - ], - icons, - components: { - ...components, - SignInPage: OffendingSignInPage, - }, - configLoader: async () => [], - }); - - const Provider = app.getProvider(); - const Router = app.getRouter(); - const { getByText } = await renderWithEffects( - - - - } /> - - - , - ); - - expect( - getByText( - 'No API factory available for dependency apiRef{core.identity} of dependent apiRef{core.fetch}', - ), - ).toBeInTheDocument(); - }); }); diff --git a/packages/core-app-api/src/app/AppManager.tsx b/packages/core-app-api/src/app/AppManager.tsx index 484a63396d..0262ece8f4 100644 --- a/packages/core-app-api/src/app/AppManager.tsx +++ b/packages/core-app-api/src/app/AppManager.tsx @@ -79,7 +79,6 @@ import { AppThemeProvider } from './AppThemeProvider'; import { defaultConfigLoader } from './defaultConfigLoader'; import { ApiRegistry } from '../apis/system/ApiRegistry'; import { resolveRouteBindings } from './resolveRouteBindings'; -import { IsolatedApiProvider } from '../apis/system/ApiProvider'; type CompatiblePlugin = | BackstagePlugin @@ -336,14 +335,7 @@ export class AppManager implements BackstageApp { const [identityApi, setIdentityApi] = useState(); if (!identityApi) { - // Encapsulate the sign in page component in an API provider which - // contains all APIs except the identity API. Provides fast feedback in - // case the identity API is accidentally required on the sign-in page. - return ( - - - - ); + return ; } this.appIdentityProxy.setTarget(identityApi); @@ -481,21 +473,6 @@ export class AppManager implements BackstageApp { return this.apiHolder; } - private getIdentitylessApiHolder() { - const registrySansIdentity = new ApiFactoryRegistry(); - - // Assume all APIs in the current registry are valid. - this.apiFactoryRegistry.getAllApis().forEach(apiRef => { - const factory = this.apiFactoryRegistry.get(apiRef); - // Add all API factories except identity! - if (factory && apiRef !== identityApiRef) { - registrySansIdentity.register('default', factory); - } - }); - - return new ApiResolver(registrySansIdentity); - } - private verifyPlugins(plugins: Iterable) { const pluginIds = new Set();