address review comments

Signed-off-by: Fredrik Adelöw <freben@gmail.com>

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 => <DelegatedSignInPage {...props} provider="gcp-iap" />,
++    SignInPage: props => <ProxiedSignInPage {...props} provider="gcp-iap" />,
 ```
 
 After this, your app should be ready to leverage the Identity-Aware Proxy for

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 = {}> = T & {
   from: string;
@@ -765,6 +755,16 @@ export function Progress(
   props: PropsWithChildren<LinearProgressProps>,
 ): 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

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);

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<DelegatedSession>;
-      previous: DelegatedSession | undefined;
+      promise: Promise<ProxiedSession>;
+      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<DelegatedSession> {
+  async getSessionAsync(): Promise<ProxiedSession> {
     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<DelegatedSession> {
+  async fetchSession(): Promise<ProxiedSession> {
     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());
   }
 }

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();

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';

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<typeof delegatedSessionSchema>) {}
+    function f(_b: TypeOf<typeof proxiedSessionSchema>) {}
     f(responseData); // no tsc errors
-    expect(delegatedSessionSchema.parse(responseData)).toEqual(responseData);
+    expect(proxiedSessionSchema.parse(responseData)).toEqual(responseData);
   });
 });

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;

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';

packages/core-components/src/setupTests.ts

@@ -15,3 +15,4 @@
  */
 
 import '@testing-library/jest-dom';
+import 'cross-fetch/polyfill';