Updates to redirect implementation after initial PR review.

Signed-off-by: headphonejames <generalfuzz@gmail.com>
2023-02-03 12:03:01 -08:00
parent 9a9170047b
commit ae4d826fb2
29 changed files with 107 additions and 178 deletions
@@ -19,7 +19,6 @@ import {
  safelyEncodeURIComponent,
  ensuresXRequestedWith,
  postMessageResponse,
-  redirectMessageResponse,
 } from './authFlowHelpers';
 import { WebMessageResponse } from './types';

@@ -179,22 +178,6 @@ describe('oauth helpers', () => {
    });
  });

-  describe('redirectMessageResponse', () => {
-    const redirectUrl = 'http://localhost:3000/catalog';
-    it('should perform redirect', () => {
-      const mockResponse = {
-        end: jest.fn().mockReturnThis(),
-        setHeader: jest.fn().mockReturnThis(),
-        redirect: jest.fn().mockReturnThis(),
-      } as unknown as express.Response;
-
-      redirectMessageResponse(mockResponse, redirectUrl);
-      expect(mockResponse.redirect).toHaveBeenCalledTimes(1);
-      expect(mockResponse.end).not.toHaveBeenCalled();
-      expect(mockResponse.setHeader).not.toHaveBeenCalled();
-    });
-  });
-
  describe('ensuresXRequestedWith', () => {
    it('should return false if no header present', () => {
      const mockRequest = {
@@ -69,14 +69,6 @@ export const postMessageResponse = (
  res.end(`<html><body><script>${script}</script></body></html>`);
 };

-/** @public */
-export const redirectMessageResponse = (
-  res: express.Response,
-  redirectUrl: string,
-) => {
-  res.redirect(redirectUrl);
-};
-
 /** @public */
 export const ensuresXRequestedWith = (req: express.Request) => {
  const requiredHeader = req.header('X-Requested-With');
@@ -74,7 +74,6 @@ describe('OAuthAdapter', () => {
    providerId: 'test-provider',
    appOrigin: 'http://localhost:3000',
    baseUrl: 'http://domain.org/auth',
-    isPopupAuthenticationRequest: true,
    cookieConfigurer: mockCookieConfigurer,
    tokenIssuer: {
      issueToken: async () => 'my-id-token',
@@ -174,10 +173,14 @@ describe('OAuthAdapter', () => {
    const oauthProvider = new OAuthAdapter(providerInstance, {
      ...oAuthProviderOptions,
      isOriginAllowed: () => false,
-      isPopupAuthenticationRequest: false,
    });

-    const mockRequest = createEncodedQueryMockRequest(defaultState);
+    const state = {
+      ...defaultState,
+      redirectUrl: 'http://localhost:3000',
+      authFlow: 'redirect',
+    };
+    const mockRequest = createEncodedQueryMockRequest(state);

    await oauthProvider.frameHandler(mockRequest, mockResponse);
    expect(mockResponse.redirect).toHaveBeenCalledTimes(1);
@@ -343,7 +346,6 @@ describe('OAuthAdapter', () => {
      baseUrl: 'http://domain.org/auth',
      appUrl: 'http://domain.org',
      isOriginAllowed: () => false,
-      isPopupAuthenticationRequest: true,
    };

    const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, {
@@ -365,7 +367,6 @@ describe('OAuthAdapter', () => {
    baseUrl: 'http://domain.org/auth',
    appUrl: 'http://domain.org',
    isOriginAllowed: () => false,
-    isPopupAuthenticationRequest: true,
  };

  const mockStartRequestWithOrigin = {
@@ -505,7 +506,7 @@ describe('OAuthAdapter', () => {
    expect(mockResponse.redirect).not.toHaveBeenCalled();
  });

-  it('executed a response redirect when isPopupAuthenticationRequest is false', async () => {
+  it('executed a response redirect when authFlow query string is set to "redirect"', async () => {
    const handlers = {
      start: jest.fn(async (_req: { state: OAuthState }) => ({
        url: '/url',
@@ -516,7 +517,6 @@ describe('OAuthAdapter', () => {
    };
    const configWithNoPopupEnabled = {
      ...configOriginAllowed,
-      isPopupAuthenticationRequest: false,
    };
    const oauthProvider = OAuthAdapter.fromConfig(
      configWithNoPopupEnabled,
@@ -531,6 +531,7 @@ describe('OAuthAdapter', () => {
      ...defaultState,
      origin: 'http://other.domain',
      redirectUrl: 'http://domain.org',
+      authFlow: 'redirect',
    };

    const mockRequest = {
@@ -540,7 +541,6 @@ describe('OAuthAdapter', () => {

    await oauthProvider.frameHandler(mockRequest, mockResponse);
    expect(mockRequest.get).not.toHaveBeenCalled();
-    expect(mockResponse.end).not.toHaveBeenCalled();
    expect(mockCookieConfigurer).not.toHaveBeenCalled();
    expect(mockResponse.cookie).not.toHaveBeenCalled();
    expect(mockResponse.redirect).toHaveBeenCalledTimes(1);
@@ -35,7 +35,6 @@ import {
 import { defaultCookieConfigurer, readState, verifyNonce } from './helpers';
 import {
  postMessageResponse,
-  redirectMessageResponse,
  ensuresXRequestedWith,
  WebMessageResponse,
 } from '../flow';
@@ -61,7 +60,6 @@ export type OAuthAdapterOptions = {
  cookieConfigurer: CookieConfigurer;
  isOriginAllowed: (origin: string) => boolean;
  callbackUrl: string;
-  isPopupAuthenticationRequest: boolean;
 };

 /** @public */
@@ -74,8 +72,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
      'providerId' | 'persistScopes' | 'callbackUrl' | 'redirectUrl'
    >,
  ): OAuthAdapter {
-    const { appUrl, baseUrl, isOriginAllowed, isPopupAuthenticationRequest } =
-      config;
+    const { appUrl, baseUrl, isOriginAllowed } = config;
    const { origin: appOrigin } = new URL(appUrl);

    const cookieConfigurer = config.cookieConfigurer ?? defaultCookieConfigurer;
@@ -86,7 +83,6 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
      baseUrl,
      cookieConfigurer,
      isOriginAllowed,
-      isPopupAuthenticationRequest: isPopupAuthenticationRequest,
    });
  }

@@ -108,6 +104,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
    const env = req.query.env?.toString();
    const origin = req.query.origin?.toString();
    const redirectUrl = req.query.redirectUrl?.toString();
+    const authFlow = req.query.authFlow?.toString();
    if (!env) {
      throw new InputError('No env provided in request query parameters');
    }
@@ -118,7 +115,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
    // set a nonce cookie before redirecting to oauth provider
    this.setNonceCookie(res, nonce, cookieConfig);

-    const state: OAuthState = { nonce, env, origin, redirectUrl };
+    const state: OAuthState = { nonce, env, origin, redirectUrl, authFlow };

    // If scopes are persisted then we pass them through the state so that we
    // can set the cookie on successful auth
@@ -184,8 +181,8 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
        response: { ...response, backstageIdentity: identity },
      };

-      if (!this.options.isPopupAuthenticationRequest) {
-        return redirectMessageResponse(res, redirectUrl);
+      if (state.authFlow === 'redirect') {
+        res.redirect(redirectUrl);
      }
      // post message back to popup if successful
      return postMessageResponse(res, appOrigin, responseObj);
@@ -91,6 +91,7 @@ export type OAuthState = {
  origin?: string;
  scope?: string;
  redirectUrl?: string;
+  authFlow?: string;
 };

 /** @public */
@@ -131,8 +131,6 @@ export type AuthProviderConfig = {
   * The function used to resolve cookie configuration based on the auth provider options.
   */
  cookieConfigurer?: CookieConfigurer;
-
-  isPopupAuthenticationRequest: boolean;
 };

 /** @public */
@@ -107,8 +107,6 @@ export async function createRouter(
    ...providerFactories,
  };
  const providersConfig = config.getConfig('auth.providers');
-  const isPopupAuthenticationRequest =
-    config.getOptionalBoolean('auth.usePopup') ?? true;

  const configuredProviders = providersConfig.keys();

@@ -126,7 +124,6 @@ export async function createRouter(
            baseUrl: authUrl,
            appUrl,
            isOriginAllowed,
-            isPopupAuthenticationRequest: isPopupAuthenticationRequest,
          },
          config: providersConfig.getConfig(providerId),
          logger,