From b943eb065e9cd912cbb901950d0c30d262d080f8 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 16 Jun 2020 00:10:15 +0200 Subject: [PATCH 1/5] packages/core-api: move config loading logic in AppProvider into separate hook --- packages/core-api/src/app/App.tsx | 66 ++++++++++++++++++++----------- 1 file changed, 43 insertions(+), 23 deletions(-) diff --git a/packages/core-api/src/app/App.tsx b/packages/core-api/src/app/App.tsx index 1923adba0d..6da2b5f1b9 100644 --- a/packages/core-api/src/app/App.tsx +++ b/packages/core-api/src/app/App.tsx @@ -45,6 +45,41 @@ type FullAppOptions = { configLoader?: AppConfigLoader; }; +function useConfigLoader( + configLoader: AppConfigLoader | undefined, + components: AppComponents, + appThemeApi: AppThemeApi, +): { api: ConfigApi } | { node: JSX.Element } { + // Keeping this synchronous when a config loader isn't set simplifies tests a lot + const hasConfig = Boolean(configLoader); + const config = useAsync(configLoader || (() => Promise.resolve([]))); + + let noConfigNode = undefined; + + if (hasConfig && config.loading) { + const { Progress } = components; + noConfigNode = ; + } else if (config.error) { + const { BootErrorPage } = components; + noConfigNode = ; + } + + // Before the config is loaded we can't use a router, so exit early + if (noConfigNode) { + return { + node: ( + + {noConfigNode} + + ), + }; + } + + const configReader = ConfigReader.fromConfigs(config.value ?? []); + + return { api: configReader }; +} + export class PrivateAppImpl implements BackstageApp { private apis?: ApiHolder = undefined; private readonly icons: SystemIcons; @@ -151,32 +186,17 @@ export class PrivateAppImpl implements BackstageApp { [], ); - // Keeping this synchronous when a config loader isn't set simplifies tests a lot - const hasConfig = Boolean(this.configLoader); - const config = useAsync(this.configLoader || (() => Promise.resolve([]))); + const loadedConfig = useConfigLoader( + this.configLoader, + this.components, + appThemeApi, + ); - let noConfigNode = undefined; - - if (hasConfig && config.loading) { - const { Progress } = this.components; - noConfigNode = ; - } else if (config.error) { - const { BootErrorPage } = this.components; - noConfigNode = ( - - ); + if ('node' in loadedConfig) { + return loadedConfig.node; } + const configReader = loadedConfig.api; - // Before the config is loaded we can't use a router, so exit early - if (noConfigNode) { - return ( - - {noConfigNode} - - ); - } - - const configReader = ConfigReader.fromConfigs(config.value ?? []); const appApis = ApiRegistry.from([ [appThemeApiRef, AppThemeSelector.createWithStorage(this.themes)], [configApiRef, configReader], From 9a3799515167dbdd3af389d60b1fc7fc4f7c25aa Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 16 Jun 2020 00:46:39 +0200 Subject: [PATCH 2/5] packages/core-api: split AppComponent into AppRouter and AppRoutes --- packages/app/src/App.tsx | 11 ++-- .../default-app/packages/app/src/App.tsx | 7 ++- packages/core-api/src/app/App.tsx | 55 ++++++++++++------- packages/core-api/src/app/types.ts | 21 ++++--- packages/dev-utils/src/devApp/render.tsx | 15 +++-- .../test-utils/src/testUtils/appWrappers.tsx | 9 ++- 6 files changed, 76 insertions(+), 42 deletions(-) diff --git a/packages/app/src/App.tsx b/packages/app/src/App.tsx index ed0518485a..ba7fb4a8a5 100644 --- a/packages/app/src/App.tsx +++ b/packages/app/src/App.tsx @@ -27,15 +27,18 @@ const app = createApp({ }); const AppProvider = app.getProvider(); -const AppComponent = app.getRootComponent(); +const AppRouter = app.getRouter(); +const AppRoutes = app.getRoutes(); const App: FC<{}> = () => ( - - - + + + + + ); diff --git a/packages/cli/templates/default-app/packages/app/src/App.tsx b/packages/cli/templates/default-app/packages/app/src/App.tsx index 140d1c1660..1b58a3e5df 100644 --- a/packages/cli/templates/default-app/packages/app/src/App.tsx +++ b/packages/cli/templates/default-app/packages/app/src/App.tsx @@ -26,13 +26,16 @@ const app = createApp({ }); const AppProvider = app.getProvider(); -const AppComponent = app.getRootComponent(); +const AppRouter = app.getRouter(); +const AppRoutes = app.getRoutes(); const App: FC<{}> = () => { useStyles(); return ( - + + + ); }; diff --git a/packages/core-api/src/app/App.tsx b/packages/core-api/src/app/App.tsx index 6da2b5f1b9..8d625a3a0d 100644 --- a/packages/core-api/src/app/App.tsx +++ b/packages/core-api/src/app/App.tsx @@ -19,7 +19,11 @@ import { AppContextProvider } from './AppContext'; import { BackstageApp, AppComponents, AppConfigLoader, Apis } from './types'; import { BackstagePlugin } from '../plugin'; import { FeatureFlagsRegistryItem } from './FeatureFlags'; -import { featureFlagsApiRef } from '../apis/definitions'; +import { + featureFlagsApiRef, + AppThemeApi, + ConfigApi, +} from '../apis/definitions'; import { AppThemeProvider } from './AppThemeProvider'; import { IconComponent, SystemIcons, SystemIconKey } from '../icons'; @@ -32,6 +36,7 @@ import { appThemeApiRef, configApiRef, ConfigReader, + useApi, } from '../apis'; import { ApiAggregator } from '../apis/ApiAggregator'; import { useAsync } from 'react-use'; @@ -114,7 +119,7 @@ export class PrivateAppImpl implements BackstageApp { return this.icons[key]; } - getRootComponent(): ComponentType<{}> { + getRoutes(): ComponentType<{}> { const routes = new Array(); const registeredFeatureFlags = new Array(); @@ -195,26 +200,42 @@ export class PrivateAppImpl implements BackstageApp { if ('node' in loadedConfig) { return loadedConfig.node; } - const configReader = loadedConfig.api; + const configApi = loadedConfig.api; const appApis = ApiRegistry.from([ - [appThemeApiRef, AppThemeSelector.createWithStorage(this.themes)], - [configApiRef, configReader], + [appThemeApiRef, appThemeApi], + [configApiRef, configApi], ]); if (!this.apis) { if ('get' in this.apisOrFactory) { this.apis = this.apisOrFactory; } else { - this.apis = this.apisOrFactory(configReader); + this.apis = this.apisOrFactory(configApi); } } const apis = new ApiAggregator(this.apis, appApis); - const { Router } = this.components; + return ( + + + {children} + + + ); + }; + return Provider; + } + + getRouter(): ComponentType<{}> { + const { Router: RouterComponent } = this.components; + + const AppRouter: FC<{}> = ({ children }) => { + const configApi = useApi(configApiRef); + let { pathname } = new URL( - configReader.getString('app.baseUrl') ?? '/', + configApi.getString('app.baseUrl') ?? '/', 'http://dummy.dev', // baseUrl can be specified as just a path ); if (pathname.endsWith('/')) { @@ -222,20 +243,14 @@ export class PrivateAppImpl implements BackstageApp { } return ( - - - - - - {children}} /> - - - - - + + + {children}} /> + + ); }; - return Provider; + return AppRouter; } verify() { diff --git a/packages/core-api/src/app/types.ts b/packages/core-api/src/app/types.ts index e30c79dd73..97809ca899 100644 --- a/packages/core-api/src/app/types.ts +++ b/packages/core-api/src/app/types.ts @@ -117,14 +117,19 @@ export type BackstageApp = { getSystemIcon(key: SystemIconKey): IconComponent; /** - * Creates a root component for this app, including the App chrome - * and routes to all plugins. - */ - getRootComponent(): ComponentType<{}>; - - /** - * Provider component that should wrap the App's RootComponent and - * any other components that need to be within the app context. + * Provider component that should wrap the Router created with getRouter() + * and any other components that need to be within the app context. */ getProvider(): ComponentType<{}>; + + /** + * Router component that should wrap the App Routes create with getRoutes() + * and any other components that should only be available while signed in. + */ + getRouter(): ComponentType<{}>; + + /** + * Routes component that contains all routes for plugin pages in the app. + */ + getRoutes(): ComponentType<{}>; }; diff --git a/packages/dev-utils/src/devApp/render.tsx b/packages/dev-utils/src/devApp/render.tsx index 8900ecd54b..0e28a5bf8b 100644 --- a/packages/dev-utils/src/devApp/render.tsx +++ b/packages/dev-utils/src/devApp/render.tsx @@ -82,8 +82,10 @@ class DevAppBuilder { apis: this.setupApiRegistry(this.factories), plugins: this.plugins, }); + const AppProvider = app.getProvider(); - const AppComponent = app.getRootComponent(); + const AppRouter = app.getRouter(); + const AppRoutes = app.getRoutes(); const sidebar = this.setupSidebar(this.plugins); @@ -93,10 +95,13 @@ class DevAppBuilder { {this.rootChildren} - - {sidebar} - - + + + + {sidebar} + + + ); }; diff --git a/packages/test-utils/src/testUtils/appWrappers.tsx b/packages/test-utils/src/testUtils/appWrappers.tsx index c474678fd1..15814d918d 100644 --- a/packages/test-utils/src/testUtils/appWrappers.tsx +++ b/packages/test-utils/src/testUtils/appWrappers.tsx @@ -89,12 +89,15 @@ export function wrapInTestApp( } const AppProvider = app.getProvider(); + const AppRouter = app.getRouter(); return ( - {/* The path of * here is needed to be set as a catch all, so it will render the wrapper element - * and work with nested routes if they exist too */} - } /> + + {/* The path of * here is needed to be set as a catch all, so it will render the wrapper element + * and work with nested routes if they exist too */} + } /> + ); } From 2ed4db0f00f263747db6a8c5cd7a1af390213efa Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 16 Jun 2020 11:21:32 +0200 Subject: [PATCH 3/5] packages/core-api: add IdentityApi to app and hook up to optional sign-in page component --- .../src/apis/definitions/IdentityApi.ts | 2 +- packages/core-api/src/app/App.tsx | 79 +++++++++++++++++-- packages/core-api/src/app/AppIdentity.ts | 61 ++++++++++++++ packages/core-api/src/app/types.ts | 34 ++++++++ 4 files changed, 169 insertions(+), 7 deletions(-) create mode 100644 packages/core-api/src/app/AppIdentity.ts diff --git a/packages/core-api/src/apis/definitions/IdentityApi.ts b/packages/core-api/src/apis/definitions/IdentityApi.ts index bbfab0ef35..4c9ccfdbf6 100644 --- a/packages/core-api/src/apis/definitions/IdentityApi.ts +++ b/packages/core-api/src/apis/definitions/IdentityApi.ts @@ -40,7 +40,7 @@ export type IdentityApi = { // TODO: getProfile(): Promise - We want this to be async when added, but needs more work. }; -export const identifyApiRef = createApiRef({ +export const identityApiRef = createApiRef({ id: 'core.identity', description: 'Provides access to the identity of the signed in user', }); diff --git a/packages/core-api/src/app/App.tsx b/packages/core-api/src/app/App.tsx index 8d625a3a0d..163a546943 100644 --- a/packages/core-api/src/app/App.tsx +++ b/packages/core-api/src/app/App.tsx @@ -13,16 +13,31 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import React, { ComponentType, FC, useMemo } from 'react'; +import React, { + ComponentType, + FC, + useMemo, + useCallback, + useState, + ReactElement, +} from 'react'; import { Route, Routes, Navigate } from 'react-router-dom'; import { AppContextProvider } from './AppContext'; -import { BackstageApp, AppComponents, AppConfigLoader, Apis } from './types'; +import { + BackstageApp, + AppComponents, + AppConfigLoader, + Apis, + SignInResult, + SignInPageProps, +} from './types'; import { BackstagePlugin } from '../plugin'; import { FeatureFlagsRegistryItem } from './FeatureFlags'; import { featureFlagsApiRef, AppThemeApi, ConfigApi, + identityApiRef, } from '../apis/definitions'; import { AppThemeProvider } from './AppThemeProvider'; @@ -40,6 +55,7 @@ import { } from '../apis'; import { ApiAggregator } from '../apis/ApiAggregator'; import { useAsync } from 'react-use'; +import { AppIdentity } from './AppIdentity'; type FullAppOptions = { apis: Apis; @@ -93,6 +109,8 @@ export class PrivateAppImpl implements BackstageApp { private readonly themes: AppTheme[]; private readonly configLoader?: AppConfigLoader; + private readonly identityApi = new AppIdentity(); + private apisOrFactory: Apis; constructor(options: FullAppOptions) { @@ -205,6 +223,7 @@ export class PrivateAppImpl implements BackstageApp { const appApis = ApiRegistry.from([ [appThemeApiRef, appThemeApi], [configApiRef, configApi], + [identityApiRef, this.identityApi], ]); if (!this.apis) { @@ -229,7 +248,35 @@ export class PrivateAppImpl implements BackstageApp { } getRouter(): ComponentType<{}> { - const { Router: RouterComponent } = this.components; + const { + Router: RouterComponent, + SignInPage: SignInPageComponent, + } = this.components; + + // This wraps the sign-in page and waits for sign-in to be completed before rendering the app + const SignInPageWrapper: FC<{ + component: ComponentType; + children: ReactElement; + }> = ({ component: Component, children }) => { + const [done, setDone] = useState(false); + + const onResult = useCallback( + (result: SignInResult) => { + if (done) { + throw new Error('Identity result callback was called twice'); + } + setDone(true); + this.identityApi.setSignInResult(result); + }, + [done], + ); + + if (done) { + return children; + } + + return ; + }; const AppRouter: FC<{}> = ({ children }) => { const configApi = useApi(configApiRef); @@ -242,14 +289,34 @@ export class PrivateAppImpl implements BackstageApp { pathname = pathname.replace(/\/$/, ''); } + // If the app hasn't configured a sign-in page, we just continue as guest. + if (!SignInPageComponent) { + this.identityApi.setSignInResult({ + userId: 'guest', + idToken: undefined, + logout: async () => {}, + }); + + return ( + + + {children}} /> + + + ); + } + return ( - - {children}} /> - + + + {children}} /> + + ); }; + return AppRouter; } diff --git a/packages/core-api/src/app/AppIdentity.ts b/packages/core-api/src/app/AppIdentity.ts new file mode 100644 index 0000000000..a0f933710b --- /dev/null +++ b/packages/core-api/src/app/AppIdentity.ts @@ -0,0 +1,61 @@ +/* + * Copyright 2020 Spotify AB + * + * 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 { IdentityApi } from '../apis'; +import { SignInResult } from './types'; + +/** + * Implementation of the connection between the App-wide IdentityApi + * and sign-in page. + */ +export class AppIdentity implements IdentityApi { + private userId?: string; + private idToken?: string; + private logoutFunc?: () => Promise; + + getUserId(): string { + if (!this.userId) { + throw new Error( + 'Tried to access IdentityApi userId before app was loaded', + ); + } + return this.userId; + } + + getIdToken(): string { + if (!this.idToken) { + throw new Error( + 'Tried to access IdentityApi idToken before app was loaded', + ); + } + return this.idToken; + } + + async logout(): Promise { + if (!this.logoutFunc) { + throw new Error( + 'Tried to access IdentityApi logoutFunc before app was loaded', + ); + } + await this.logoutFunc; + } + + setSignInResult(result: SignInResult) { + this.userId = result.userId; + this.idToken = result.idToken; + this.logoutFunc = result.logout; + } +} diff --git a/packages/core-api/src/app/types.ts b/packages/core-api/src/app/types.ts index 97809ca899..099438a58c 100644 --- a/packages/core-api/src/app/types.ts +++ b/packages/core-api/src/app/types.ts @@ -25,11 +25,45 @@ export type BootErrorPageProps = { step: 'load-config'; error: Error; }; + +export type SignInResult = { + /** + * User ID that will be returned by the IdentityApi + */ + userId: string; + /** + * ID token that will be returned by the IdentityApi + */ + idToken: string | undefined; + /** + * Logout handler that will be called if the user requests a logout. + */ + logout: () => Promise; +}; + +export type SignInPageProps = { + /** + * Set the sign-in result for the app. This should only be called once. + */ + onResult(result: SignInResult): void; +}; + export type AppComponents = { NotFoundErrorPage: ComponentType<{}>; BootErrorPage: ComponentType; Progress: ComponentType<{}>; Router: ComponentType<{}>; + + /** + * An optional sign-in page that will be rendered instead of the AppRouter at startup. + * + * If a sign-in page is set, it will always be shown before the app, and it is up + * to the sign-in page to handle e.g. saving of login methods for subsequent visits. + * + * The sign-in page will be displayed until it has passed up a result to the parent, + * and which point the AppRouter and all of its children will be rendered instead. + */ + SignInPage?: ComponentType; }; /** From 09c16fb5542b86593b5ee6e8c27c911e383de881 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 16 Jun 2020 14:14:05 +0200 Subject: [PATCH 4/5] packages/core-api: make idToken and logout optional in SignInResult --- packages/core-api/src/app/types.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core-api/src/app/types.ts b/packages/core-api/src/app/types.ts index 099438a58c..a152ef9dfb 100644 --- a/packages/core-api/src/app/types.ts +++ b/packages/core-api/src/app/types.ts @@ -34,11 +34,11 @@ export type SignInResult = { /** * ID token that will be returned by the IdentityApi */ - idToken: string | undefined; + idToken?: string; /** * Logout handler that will be called if the user requests a logout. */ - logout: () => Promise; + logout?: () => Promise; }; export type SignInPageProps = { From ee7f6f2b2e3afeecdf6f4a1d49ec9b99776b78aa Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 16 Jun 2020 15:06:26 +0200 Subject: [PATCH 5/5] packages/core-api: add logout to IdentityApi and properly handle multiple sign-in attempts --- .../src/apis/definitions/IdentityApi.ts | 5 +++++ packages/core-api/src/app/AppIdentity.ts | 21 +++++++++++++------ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/packages/core-api/src/apis/definitions/IdentityApi.ts b/packages/core-api/src/apis/definitions/IdentityApi.ts index 4c9ccfdbf6..5da9ed4814 100644 --- a/packages/core-api/src/apis/definitions/IdentityApi.ts +++ b/packages/core-api/src/apis/definitions/IdentityApi.ts @@ -38,6 +38,11 @@ export type IdentityApi = { getIdToken(): string | undefined; // TODO: getProfile(): Promise - We want this to be async when added, but needs more work. + + /** + * Log out the current user + */ + logout(): Promise; }; export const identityApiRef = createApiRef({ diff --git a/packages/core-api/src/app/AppIdentity.ts b/packages/core-api/src/app/AppIdentity.ts index a0f933710b..77de445a20 100644 --- a/packages/core-api/src/app/AppIdentity.ts +++ b/packages/core-api/src/app/AppIdentity.ts @@ -22,21 +22,22 @@ import { SignInResult } from './types'; * and sign-in page. */ export class AppIdentity implements IdentityApi { + private hasIdentity = false; private userId?: string; private idToken?: string; private logoutFunc?: () => Promise; getUserId(): string { - if (!this.userId) { + if (!this.hasIdentity) { throw new Error( 'Tried to access IdentityApi userId before app was loaded', ); } - return this.userId; + return this.userId!; } - getIdToken(): string { - if (!this.idToken) { + getIdToken(): string | undefined { + if (!this.hasIdentity) { throw new Error( 'Tried to access IdentityApi idToken before app was loaded', ); @@ -45,15 +46,23 @@ export class AppIdentity implements IdentityApi { } async logout(): Promise { - if (!this.logoutFunc) { + if (!this.hasIdentity) { throw new Error( 'Tried to access IdentityApi logoutFunc before app was loaded', ); } - await this.logoutFunc; + await this.logoutFunc?.(); + location.reload(); } setSignInResult(result: SignInResult) { + if (this.hasIdentity) { + return; + } + if (!result.userId) { + throw new Error('Invalid sign-in result, userId not set'); + } + this.hasIdentity = true; this.userId = result.userId; this.idToken = result.idToken; this.logoutFunc = result.logout;