From be610c5744141dc6a358a46cecef7c990e9db253 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 13 Jan 2022 11:16:16 +0100 Subject: [PATCH] address review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- docs/auth/google/gcp-iap-auth.md | 6 +-- packages/core-components/api-report.md | 20 ++++----- .../ProxiedSignInIdentity.test.ts} | 24 +++++++---- .../ProxiedSignInIdentity.ts} | 41 +++++++++++-------- .../ProxiedSignInPage.tsx} | 19 ++++----- .../index.ts | 4 +- .../types.test.ts | 8 ++-- .../types.ts | 6 +-- packages/core-components/src/layout/index.ts | 2 +- packages/core-components/src/setupTests.ts | 1 + 10 files changed, 70 insertions(+), 61 deletions(-) rename packages/core-components/src/layout/{DelegatedSignInPage/DelegatedSignInIdentity.test.ts => ProxiedSignInPage/ProxiedSignInIdentity.test.ts} (87%) rename packages/core-components/src/layout/{DelegatedSignInPage/DelegatedSignInIdentity.ts => ProxiedSignInPage/ProxiedSignInIdentity.ts} (82%) rename packages/core-components/src/layout/{DelegatedSignInPage/DelegatedSignInPage.tsx => ProxiedSignInPage/ProxiedSignInPage.tsx} (80%) rename packages/core-components/src/layout/{DelegatedSignInPage => ProxiedSignInPage}/index.ts (82%) rename packages/core-components/src/layout/{DelegatedSignInPage => ProxiedSignInPage}/types.test.ts (81%) rename packages/core-components/src/layout/{DelegatedSignInPage => ProxiedSignInPage}/types.ts (89%) diff --git a/docs/auth/google/gcp-iap-auth.md b/docs/auth/google/gcp-iap-auth.md index 88077080da..29951951d1 100644 --- a/docs/auth/google/gcp-iap-auth.md +++ b/docs/auth/google/gcp-iap-auth.md @@ -95,7 +95,7 @@ sign-in mechanism to poll that endpoint through the IAP, on the user's behalf. ## Frontend Changes -Any Backstage app needs a `SignInPage` to be configured. Its purpose is to +All Backstage apps need a `SignInPage` to be configured. Its purpose is to establish who the user is and what their identifying credentials are, blocking rendering the rest of the UI until that's complete, and then keeping those credentials fresh. @@ -114,11 +114,11 @@ component for this purpose out of the box. Update your `createApp` call in `packages/app/src/App.tsx`, as follows. ```diff -+import { DelegatedSignInPage } from '@backstage/core-components'; ++import { ProxiedSignInPage } from '@backstage/core-components'; const app = createApp({ components: { -+ SignInPage: props => , ++ SignInPage: props => , ``` After this, your app should be ready to leverage the Identity-Aware Proxy for diff --git a/packages/core-components/api-report.md b/packages/core-components/api-report.md index bdd6f0e030..6af2746683 100644 --- a/packages/core-components/api-report.md +++ b/packages/core-components/api-report.md @@ -202,16 +202,6 @@ export type CustomProviderClassKey = 'form' | 'button'; // @public (undocumented) export function DashboardIcon(props: IconComponentProps): JSX.Element; -// @public -export const DelegatedSignInPage: ( - props: DelegatedSignInPageProps, -) => JSX.Element | null; - -// @public -export type DelegatedSignInPageProps = SignInPageProps & { - provider: string; -}; - // @public type DependencyEdge = T & { from: string; @@ -765,6 +755,16 @@ export function Progress( props: PropsWithChildren, ): JSX.Element; +// @public +export const ProxiedSignInPage: ( + props: ProxiedSignInPageProps, +) => JSX.Element | null; + +// @public +export type ProxiedSignInPageProps = SignInPageProps & { + provider: string; +}; + // Warning: (ae-missing-release-tag) "Ranker" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public diff --git a/packages/core-components/src/layout/DelegatedSignInPage/DelegatedSignInIdentity.test.ts b/packages/core-components/src/layout/ProxiedSignInPage/ProxiedSignInIdentity.test.ts similarity index 87% rename from packages/core-components/src/layout/DelegatedSignInPage/DelegatedSignInIdentity.test.ts rename to packages/core-components/src/layout/ProxiedSignInPage/ProxiedSignInIdentity.test.ts index 16ce71980a..cc23678606 100644 --- a/packages/core-components/src/layout/DelegatedSignInPage/DelegatedSignInIdentity.test.ts +++ b/packages/core-components/src/layout/ProxiedSignInPage/ProxiedSignInIdentity.test.ts @@ -15,20 +15,19 @@ */ import { setupRequestMockHandlers } from '@backstage/test-utils'; -import fetch from 'cross-fetch'; import { rest } from 'msw'; import { setupServer } from 'msw/node'; import { DEFAULTS, - DelegatedSignInIdentity, + ProxiedSignInIdentity, tokenToExpiry, -} from './DelegatedSignInIdentity'; +} from './ProxiedSignInIdentity'; const validBackstageTokenExpClaim = 1641216199; const validBackstageToken = 'eyJhbGciOiJFUzI1NiIsImtpZCI6ImMxNTMzNDRiLWZjYzktNGIwOS1iN2ZhLTU3ZmM5MDhjMjBiNiJ9.eyJpc3MiOiJodHRwOi8vbG9jYWxob3N0OjcwMDcvYXBpL2F1dGgiLCJzdWIiOiJmcmViZW4iLCJhdWQiOiJiYWNrc3RhZ2UiLCJpYXQiOjE2NDEyMTI1OTksImV4cCI6MTY0MTIxNjE5OSwiZW50IjpbInVzZXI6ZGVmYXVsdC9mcmViZW4iXX0.4nOTmPHPwhzaKTzikgUsHcszfcP-JamcojMnRfyfsKhyHCCEywe6uLFlvvmK5NbaX5Z7IIji-kg7bxKU58kwoQ'; -describe('DelegatedSignInIdentity', () => { +describe('ProxiedSignInIdentity', () => { describe('tokenToExpiry', () => { beforeEach(() => jest.useFakeTimers('modern')); afterEach(() => jest.useRealTimers()); @@ -46,9 +45,17 @@ describe('DelegatedSignInIdentity', () => { ), ); }); + + it('handles a token that has no exp', async () => { + const [a, _b, c] = validBackstageToken.split('.'); + const botched = `${a}.${btoa(JSON.stringify({}))}.${c}`; + expect(tokenToExpiry(botched)).toEqual( + new Date(new Date(Date.now() + DEFAULTS.defaultTokenExpiryMillis)), + ); + }); }); - describe('DelegatedSignInIdentity', () => { + describe('ProxiedSignInIdentity', () => { beforeEach(() => jest.useFakeTimers('modern')); afterEach(() => jest.useRealTimers()); @@ -61,7 +68,7 @@ describe('DelegatedSignInIdentity', () => { function makeToken() { const iat = Math.floor(Date.now() / 1000); - const exp = iat + 100; + const exp = iat + 3600; return { providerInfo: { stuff: 1, @@ -107,10 +114,9 @@ describe('DelegatedSignInIdentity', () => { ), ); - const identity = new DelegatedSignInIdentity({ + const identity = new ProxiedSignInIdentity({ provider: 'foo', discoveryApi: { getBaseUrl }, - fetchApi: { fetch }, }); getBaseUrl.mockResolvedValue('http://example.com/api/auth'); @@ -126,7 +132,7 @@ describe('DelegatedSignInIdentity', () => { // Use a fairly large margin (1000) since the iat and exp are clamped to // full seconds, but the "local current time" isn't jest.advanceTimersByTime( - 100 * 1000 - DEFAULTS.tokenExpiryMarginMillis - 1000, + 3600 * 1000 - DEFAULTS.tokenExpiryMarginMillis - 1000, ); await identity.getSessionAsync(); // still no need to fetch again expect(serverCalled).toBeCalledTimes(1); diff --git a/packages/core-components/src/layout/DelegatedSignInPage/DelegatedSignInIdentity.ts b/packages/core-components/src/layout/ProxiedSignInPage/ProxiedSignInIdentity.ts similarity index 82% rename from packages/core-components/src/layout/DelegatedSignInPage/DelegatedSignInIdentity.ts rename to packages/core-components/src/layout/ProxiedSignInPage/ProxiedSignInIdentity.ts index 750f9ef027..1f8737b200 100644 --- a/packages/core-components/src/layout/DelegatedSignInPage/DelegatedSignInIdentity.ts +++ b/packages/core-components/src/layout/ProxiedSignInPage/ProxiedSignInIdentity.ts @@ -17,38 +17,40 @@ import { BackstageUserIdentity, discoveryApiRef, - fetchApiRef, IdentityApi, ProfileInfo, } from '@backstage/core-plugin-api'; import { ResponseError } from '@backstage/errors'; -import { DelegatedSession, delegatedSessionSchema } from './types'; +import { ProxiedSession, proxiedSessionSchema } from './types'; export const DEFAULTS = { // The amount of time between token refreshes, if we fail to get an actual // value out of the exp claim - defaultTokenExpiryMillis: 60_000, + defaultTokenExpiryMillis: 5 * 60 * 1000, // The amount of time before the actual expiry of the Backstage token, that we // shall start trying to get a new one - tokenExpiryMarginMillis: 30_000, + tokenExpiryMarginMillis: 5 * 60 * 1000, } as const; // When the token expires, with some margin export function tokenToExpiry(jwtToken: string | undefined): Date { + const fallback = new Date(Date.now() + DEFAULTS.defaultTokenExpiryMillis); if (!jwtToken) { - return new Date(Date.now() + DEFAULTS.defaultTokenExpiryMillis); + return fallback; } const [_header, rawPayload, _signature] = jwtToken.split('.'); const payload = JSON.parse(atob(rawPayload)); + if (typeof payload.exp !== 'number') { + return fallback; + } return new Date(payload.exp * 1000 - DEFAULTS.tokenExpiryMarginMillis); } -type DelegatedSignInIdentityOptions = { +type ProxiedSignInIdentityOptions = { provider: string; discoveryApi: typeof discoveryApiRef.T; - fetchApi: typeof fetchApiRef.T; }; type State = @@ -57,12 +59,12 @@ type State = } | { type: 'fetching'; - promise: Promise; - previous: DelegatedSession | undefined; + promise: Promise; + previous: ProxiedSession | undefined; } | { type: 'active'; - session: DelegatedSession; + session: ProxiedSession; expiresAt: Date; } | { @@ -74,12 +76,12 @@ type State = * An identity API that gets the user auth information solely based on a * provider's `/refresh` endpoint. */ -export class DelegatedSignInIdentity implements IdentityApi { - private readonly options: DelegatedSignInIdentityOptions; +export class ProxiedSignInIdentity implements IdentityApi { + private readonly options: ProxiedSignInIdentityOptions; private readonly abortController: AbortController; private state: State; - constructor(options: DelegatedSignInIdentityOptions) { + constructor(options: ProxiedSignInIdentityOptions) { this.options = options; this.abortController = new AbortController(); this.state = { type: 'empty' }; @@ -133,7 +135,7 @@ export class DelegatedSignInIdentity implements IdentityApi { this.abortController.abort(); } - getSessionSync(): DelegatedSession { + getSessionSync(): ProxiedSession { if (this.state.type === 'active') { return this.state.session; } else if (this.state.type === 'fetching' && this.state.previous) { @@ -142,7 +144,7 @@ export class DelegatedSignInIdentity implements IdentityApi { throw new Error('No session available. Try reloading your browser page.'); } - async getSessionAsync(): Promise { + async getSessionAsync(): Promise { if (this.state.type === 'fetching') { return this.state.promise; } else if ( @@ -182,10 +184,13 @@ export class DelegatedSignInIdentity implements IdentityApi { return promise; } - async fetchSession(): Promise { + async fetchSession(): Promise { const baseUrl = await this.options.discoveryApi.getBaseUrl('auth'); - const response = await this.options.fetchApi.fetch( + // Note that we do not use the fetchApi here, since this all happens before + // sign-in completes so there can be no automatic token injection and + // similar. + const response = await fetch( `${baseUrl}/${this.options.provider}/refresh`, { signal: this.abortController.signal, @@ -198,6 +203,6 @@ export class DelegatedSignInIdentity implements IdentityApi { throw await ResponseError.fromResponse(response); } - return delegatedSessionSchema.parse(await response.json()); + return proxiedSessionSchema.parse(await response.json()); } } diff --git a/packages/core-components/src/layout/DelegatedSignInPage/DelegatedSignInPage.tsx b/packages/core-components/src/layout/ProxiedSignInPage/ProxiedSignInPage.tsx similarity index 80% rename from packages/core-components/src/layout/DelegatedSignInPage/DelegatedSignInPage.tsx rename to packages/core-components/src/layout/ProxiedSignInPage/ProxiedSignInPage.tsx index 0337e3122d..adcd6ced8f 100644 --- a/packages/core-components/src/layout/DelegatedSignInPage/DelegatedSignInPage.tsx +++ b/packages/core-components/src/layout/ProxiedSignInPage/ProxiedSignInPage.tsx @@ -16,7 +16,6 @@ import { discoveryApiRef, - fetchApiRef, SignInPageProps, useApi, } from '@backstage/core-plugin-api'; @@ -24,14 +23,14 @@ import React from 'react'; import useAsync from 'react-use/lib/useAsync'; import { ErrorPanel } from '../../components/ErrorPanel'; import { Progress } from '../../components/Progress'; -import { DelegatedSignInIdentity } from './DelegatedSignInIdentity'; +import { ProxiedSignInIdentity } from './ProxiedSignInIdentity'; /** - * Props for {@link DelegatedSignInPage}. + * Props for {@link ProxiedSignInPage}. * * @public */ -export type DelegatedSignInPageProps = SignInPageProps & { +export type ProxiedSignInPageProps = SignInPageProps & { /** * The provider to use, e.g. "gcp-iap" or "aws-alb". This must correspond to * a properly configured auth provider ID in the auth backend. @@ -40,9 +39,9 @@ export type DelegatedSignInPageProps = SignInPageProps & { }; /** - * A sign-in page that has no user interface of its own. Instead, it delegates - * sign-in to some reverse authenticating proxy that Backstage is deployed - * behind, and leverages its session handling. + * A sign-in page that has no user interface of its own. Instead, it relies on + * sign-in being performed by a reverse authenticating proxy that Backstage is + * deployed behind, and leverages its session handling. * * @remarks * @@ -54,15 +53,13 @@ export type DelegatedSignInPageProps = SignInPageProps & { * * @public */ -export const DelegatedSignInPage = (props: DelegatedSignInPageProps) => { +export const ProxiedSignInPage = (props: ProxiedSignInPageProps) => { const discoveryApi = useApi(discoveryApiRef); - const fetchApi = useApi(fetchApiRef); const { loading, error } = useAsync(async () => { - const identity = new DelegatedSignInIdentity({ + const identity = new ProxiedSignInIdentity({ provider: props.provider, discoveryApi, - fetchApi, }); await identity.start(); diff --git a/packages/core-components/src/layout/DelegatedSignInPage/index.ts b/packages/core-components/src/layout/ProxiedSignInPage/index.ts similarity index 82% rename from packages/core-components/src/layout/DelegatedSignInPage/index.ts rename to packages/core-components/src/layout/ProxiedSignInPage/index.ts index bf3081e630..f7bab5f46c 100644 --- a/packages/core-components/src/layout/DelegatedSignInPage/index.ts +++ b/packages/core-components/src/layout/ProxiedSignInPage/index.ts @@ -14,5 +14,5 @@ * limitations under the License. */ -export { DelegatedSignInPage } from './DelegatedSignInPage'; -export type { DelegatedSignInPageProps } from './DelegatedSignInPage'; +export { ProxiedSignInPage } from './ProxiedSignInPage'; +export type { ProxiedSignInPageProps } from './ProxiedSignInPage'; diff --git a/packages/core-components/src/layout/DelegatedSignInPage/types.test.ts b/packages/core-components/src/layout/ProxiedSignInPage/types.test.ts similarity index 81% rename from packages/core-components/src/layout/DelegatedSignInPage/types.test.ts rename to packages/core-components/src/layout/ProxiedSignInPage/types.test.ts index c3eeff4eec..5da8203cbd 100644 --- a/packages/core-components/src/layout/DelegatedSignInPage/types.test.ts +++ b/packages/core-components/src/layout/ProxiedSignInPage/types.test.ts @@ -15,10 +15,10 @@ */ import { TypeOf } from 'zod'; -import { DelegatedSession, delegatedSessionSchema } from './types'; +import { ProxiedSession, proxiedSessionSchema } from './types'; describe('types', () => { - const responseData: DelegatedSession = { + const responseData: ProxiedSession = { providerInfo: { stuff: 1, }, @@ -39,8 +39,8 @@ describe('types', () => { }; it('has a compatible schema type', () => { - function f(_b: TypeOf) {} + function f(_b: TypeOf) {} f(responseData); // no tsc errors - expect(delegatedSessionSchema.parse(responseData)).toEqual(responseData); + expect(proxiedSessionSchema.parse(responseData)).toEqual(responseData); }); }); diff --git a/packages/core-components/src/layout/DelegatedSignInPage/types.ts b/packages/core-components/src/layout/ProxiedSignInPage/types.ts similarity index 89% rename from packages/core-components/src/layout/DelegatedSignInPage/types.ts rename to packages/core-components/src/layout/ProxiedSignInPage/types.ts index 0ef65344a3..f5b54f306a 100644 --- a/packages/core-components/src/layout/DelegatedSignInPage/types.ts +++ b/packages/core-components/src/layout/ProxiedSignInPage/types.ts @@ -20,7 +20,7 @@ import { } from '@backstage/core-plugin-api'; import { z } from 'zod'; -export const delegatedSessionSchema = z.object({ +export const proxiedSessionSchema = z.object({ providerInfo: z.object({}).catchall(z.unknown()).optional(), profile: z.object({ email: z.string().optional(), @@ -39,12 +39,12 @@ export const delegatedSessionSchema = z.object({ }); /** - * Generic session information for delegated sign-in providers, e.g. common + * Generic session information for proxied sign-in providers, e.g. common * reverse authenticating proxy implementations. * * @public */ -export type DelegatedSession = { +export type ProxiedSession = { providerInfo?: { [key: string]: unknown }; profile: ProfileInfo; backstageIdentity: BackstageIdentityResponse; diff --git a/packages/core-components/src/layout/index.ts b/packages/core-components/src/layout/index.ts index f726481535..9bb7fbaa26 100644 --- a/packages/core-components/src/layout/index.ts +++ b/packages/core-components/src/layout/index.ts @@ -17,7 +17,6 @@ export * from './BottomLink'; export * from './Content'; export * from './ContentHeader'; -export * from './DelegatedSignInPage'; export * from './ErrorBoundary'; export * from './ErrorPage'; export * from './Header'; @@ -27,6 +26,7 @@ export * from './HomepageTimer'; export * from './InfoCard'; export * from './ItemCard'; export * from './Page'; +export * from './ProxiedSignInPage'; export * from './Sidebar'; export * from './SignInPage'; export * from './TabbedCard'; diff --git a/packages/core-components/src/setupTests.ts b/packages/core-components/src/setupTests.ts index 963c0f188b..c1d649f2ad 100644 --- a/packages/core-components/src/setupTests.ts +++ b/packages/core-components/src/setupTests.ts @@ -15,3 +15,4 @@ */ import '@testing-library/jest-dom'; +import 'cross-fetch/polyfill';