Resolve cookie configuration per request

Signed-off-by: Marcus Eide <eide@spotify.com>
2022-09-16 09:57:44 +02:00
parent 12943d1ade
commit 04c50e297d
4 changed files with 200 additions and 43 deletions
@@ -285,7 +285,6 @@ export type OAuthAdapterOptions = {
  persistScopes?: boolean;
  appOrigin: string;
  baseUrl: string;
-  cookieConfig: ReturnType<CookieConfigurer>;
  cookieConfigurer: CookieConfigurer;
  isOriginAllowed: (origin: string) => boolean;
  callbackUrl: string;
@@ -63,7 +63,7 @@ describe('OAuthAdapter', () => {
  }
  const providerInstance = new MyAuthProvider();
  const mockCookieConfig: ReturnType<CookieConfigurer> = {
-    domain: 'example.com',
+    domain: 'domain.org',
    path: '/auth/test-provider',
    secure: false,
  };
@@ -72,15 +72,14 @@ describe('OAuthAdapter', () => {
  const oAuthProviderOptions = {
    providerId: 'test-provider',
    appOrigin: 'http://localhost:3000',
-    cookieConfig: mockCookieConfig,
+    baseUrl: 'http://domain.org/auth',
    cookieConfigurer: mockCookieConfigurer,
-    baseUrl: 'http://example.com:7007',
    tokenIssuer: {
      issueToken: async () => 'my-id-token',
      listPublicKeys: async () => ({ keys: [] }),
    },
    isOriginAllowed: () => false,
-    callbackUrl: 'http://example.com:7007/auth/test-provider/frame/handler',
+    callbackUrl: 'http://domain.org/auth/test-provider/handler/frame',
  };

  it('sets the correct headers in start', async () => {
@@ -108,7 +107,14 @@ describe('OAuthAdapter', () => {
    expect(mockResponse.cookie).toHaveBeenCalledWith(
      `${oAuthProviderOptions.providerId}-nonce`,
      expect.any(String),
-      expect.objectContaining({ maxAge: TEN_MINUTES_MS }),
+      expect.objectContaining({
+        httpOnly: true,
+        path: '/auth/test-provider/handler',
+        maxAge: TEN_MINUTES_MS,
+        domain: 'domain.org',
+        sameSite: 'lax',
+        secure: false,
+      }),
    );
    // redirect checks
    expect(mockResponse.setHeader).toHaveBeenCalledTimes(2);
@@ -150,6 +156,7 @@ describe('OAuthAdapter', () => {
        httpOnly: true,
        path: '/auth/test-provider',
        maxAge: THOUSAND_DAYS_MS,
+        domain: 'domain.org',
        secure: false,
        sameSite: 'lax',
      }),
@@ -218,14 +225,17 @@ describe('OAuthAdapter', () => {
    } as unknown as express.Response;

    await oauthProvider.frameHandler(mockHandleReq, mockHandleRes);
-    expect(mockCookieConfigurer).toHaveBeenCalledTimes(1);
    expect(mockHandleRes.cookie).toHaveBeenCalledTimes(1);
    expect(mockHandleRes.cookie).toHaveBeenCalledWith(
      'test-provider-granted-scope',
      'user',
      expect.objectContaining({
+        httpOnly: true,
        path: '/auth/test-provider',
        maxAge: THOUSAND_DAYS_MS,
+        domain: 'domain.org',
+        secure: false,
+        sameSite: 'lax',
      }),
    );

@@ -260,6 +270,7 @@ describe('OAuthAdapter', () => {

    const mockRequest = {
      header: () => 'XMLHttpRequest',
+      get: jest.fn(),
    } as unknown as express.Request;

    const mockResponse = {
@@ -269,6 +280,7 @@ describe('OAuthAdapter', () => {
    } as unknown as express.Response;

    await oauthProvider.logout(mockRequest, mockResponse);
+    expect(mockRequest.get).toHaveBeenCalledTimes(1);
    expect(mockResponse.cookie).toHaveBeenCalledTimes(1);
    expect(mockResponse.cookie).toHaveBeenCalledWith(
      expect.stringContaining('test-provider-refresh-token'),
@@ -312,7 +324,46 @@ describe('OAuthAdapter', () => {
    });
  });

-  it('sets the correct cookie configuration using an unsecure callbackUrl', async () => {
+  it('sets new access-token when old cookie exists', async () => {
+    const oauthProvider = new OAuthAdapter(providerInstance, {
+      ...oAuthProviderOptions,
+      isOriginAllowed: () => false,
+    });
+
+    const mockRequest = {
+      header: () => 'XMLHttpRequest',
+      cookies: {
+        'test-provider-refresh-token': 'old-token',
+      },
+      query: {},
+      get: jest.fn(),
+    } as unknown as express.Request;
+
+    const mockResponse = {
+      json: jest.fn().mockReturnThis(),
+      status: jest.fn().mockReturnThis(),
+      cookie: jest.fn().mockReturnThis(),
+    } as unknown as express.Response;
+
+    await oauthProvider.refresh(mockRequest, mockResponse);
+    expect(mockRequest.get).toHaveBeenCalledTimes(1);
+    expect(mockCookieConfigurer).toHaveBeenCalledTimes(1);
+    expect(mockResponse.cookie).toHaveBeenCalledTimes(1);
+    expect(mockResponse.cookie).toHaveBeenCalledWith(
+      'test-provider-refresh-token',
+      'token',
+      expect.objectContaining({
+        httpOnly: true,
+        path: '/auth/test-provider',
+        maxAge: THOUSAND_DAYS_MS,
+        domain: 'domain.org',
+        secure: false,
+        sameSite: 'lax',
+      }),
+    );
+  });
+
+  it('sets the correct nonce cookie configuration', async () => {
    const config = {
      baseUrl: 'http://domain.org/auth',
      appUrl: 'http://domain.org',
@@ -321,7 +372,6 @@ describe('OAuthAdapter', () => {

    const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, {
      ...oAuthProviderOptions,
-      callbackUrl: 'http://authdomain.org/auth/test-provider/handler/frame',
    });

    const mockRequest = {
@@ -346,7 +396,8 @@ describe('OAuthAdapter', () => {
      expect.any(String),
      expect.objectContaining({
        httpOnly: true,
-        domain: 'authdomain.org',
+        domain: 'domain.org',
+        maxAge: TEN_MINUTES_MS,
        path: '/auth/test-provider/handler',
        secure: false,
        sameSite: 'lax',
@@ -361,6 +412,54 @@ describe('OAuthAdapter', () => {
      isOriginAllowed: () => false,
    };

+    const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, {
+      ...oAuthProviderOptions,
+      callbackUrl: 'https://domain.org/auth/test-provider/handler/frame',
+    });
+
+    const state = {
+      nonce: 'nonce',
+      env: 'development',
+    };
+
+    const mockRequest = {
+      cookies: {
+        'test-provider-nonce': 'nonce',
+      },
+      query: {
+        state: encodeState(state),
+      },
+    } as unknown as express.Request;
+
+    const mockResponse = {
+      cookie: jest.fn().mockReturnThis(),
+      setHeader: jest.fn().mockReturnThis(),
+      end: jest.fn().mockReturnThis(),
+    } as unknown as express.Response;
+
+    await oauthProvider.frameHandler(mockRequest, mockResponse);
+    expect(mockCookieConfigurer).not.toHaveBeenCalled();
+    expect(mockResponse.cookie).toHaveBeenCalledTimes(1);
+    expect(mockResponse.cookie).toHaveBeenCalledWith(
+      expect.stringContaining('test-provider-refresh-token'),
+      expect.stringContaining('token'),
+      expect.objectContaining({
+        httpOnly: true,
+        domain: 'domain.org',
+        path: '/auth/test-provider',
+        secure: true,
+        sameSite: 'lax',
+      }),
+    );
+  });
+
+  it('sets the correct cookie configuration when on different domains and secure', async () => {
+    const config = {
+      baseUrl: 'https://domain.org/auth',
+      appUrl: 'http://domain.org',
+      isOriginAllowed: () => false,
+    };
+
    const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, {
      ...oAuthProviderOptions,
      callbackUrl: 'https://authdomain.org/auth/test-provider/handler/frame',
@@ -402,7 +501,7 @@ describe('OAuthAdapter', () => {
    );
  });

-  it('sets the correct cookie configuration using state', async () => {
+  it('sets the correct cookie configuration using origin from state', async () => {
    const config = {
      baseUrl: 'https://domain.org/auth',
      appUrl: 'http://domain.org',
@@ -450,4 +549,49 @@ describe('OAuthAdapter', () => {
      }),
    );
  });
+
+  it('sets the correct cookie configuration using origin from header', async () => {
+    const config = {
+      baseUrl: 'https://domain.org/auth',
+      appUrl: 'http://domain.org',
+      isOriginAllowed: () => false,
+    };
+
+    const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, {
+      ...oAuthProviderOptions,
+      callbackUrl: 'https://domain.org/auth/test-provider/handler/frame',
+    });
+
+    const mockRequest = {
+      header: () => 'XMLHttpRequest',
+      cookies: {
+        'test-provider-refresh-token': 'old-token',
+      },
+      query: {},
+      get: jest.fn().mockReturnValue('http://other.domain'),
+    } as unknown as express.Request;
+
+    const mockResponse = {
+      json: jest.fn().mockReturnThis(),
+      status: jest.fn().mockReturnThis(),
+      cookie: jest.fn().mockReturnThis(),
+    } as unknown as express.Response;
+
+    await oauthProvider.refresh(mockRequest, mockResponse);
+    expect(mockRequest.get).toHaveBeenCalledTimes(1);
+    expect(mockCookieConfigurer).not.toHaveBeenCalled();
+    expect(mockResponse.cookie).toHaveBeenCalledTimes(1);
+    expect(mockResponse.cookie).toHaveBeenCalledWith(
+      'test-provider-refresh-token',
+      'token',
+      expect.objectContaining({
+        httpOnly: true,
+        path: '/auth/test-provider',
+        maxAge: THOUSAND_DAYS_MS,
+        domain: 'domain.org',
+        secure: true,
+        sameSite: 'none',
+      }),
+    );
+  });
 });
@@ -51,7 +51,6 @@ export type OAuthAdapterOptions = {
  persistScopes?: boolean;
  appOrigin: string;
  baseUrl: string;
-  cookieConfig: ReturnType<CookieConfigurer>;
  cookieConfigurer: CookieConfigurer;
  isOriginAllowed: (origin: string) => boolean;
  callbackUrl: string;
@@ -71,24 +70,17 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
    const { origin: appOrigin } = new URL(appUrl);

    const cookieConfigurer = config.cookieConfigurer ?? defaultCookieConfigurer;
-    const cookieConfig = cookieConfigurer({
-      providerId: options.providerId,
-      baseUrl: config.baseUrl,
-      callbackUrl: options.callbackUrl,
-      appOrigin,
-    });

    return new OAuthAdapter(handlers, {
      ...options,
      appOrigin,
      baseUrl,
-      cookieConfig,
      cookieConfigurer,
      isOriginAllowed,
    });
  }

-  private baseCookieOptions: CookieOptions;
+  private readonly baseCookieOptions: CookieOptions;

  constructor(
    private readonly handlers: OAuthHandlers,
@@ -97,7 +89,6 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
    this.baseCookieOptions = {
      httpOnly: true,
      sameSite: 'lax',
-      ...this.options.cookieConfig,
    };
  }

@@ -111,9 +102,11 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
      throw new InputError('No env provided in request query parameters');
    }

+    const cookieConfig = this.getCookieConfig();
+
    const nonce = crypto.randomBytes(16).toString('base64');
    // set a nonce cookie before redirecting to oauth provider
-    this.setNonceCookie(res, nonce);
+    this.setNonceCookie(res, nonce, cookieConfig);

    const state: OAuthState = { nonce, env, origin };

@@ -154,33 +147,23 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
        }
      }

-      // Update cookie options to reflect any changes for the appOrigin
-      const updatedCookieOptions = this.options.cookieConfigurer({
-        providerId: this.options.providerId,
-        baseUrl: this.options.baseUrl,
-        callbackUrl: this.options.callbackUrl,
-        appOrigin,
-      });
-      this.baseCookieOptions = {
-        ...this.baseCookieOptions,
-        ...updatedCookieOptions,
-      };
-
      // verify nonce cookie and state cookie on callback
      verifyNonce(req, this.options.providerId);

      const { response, refreshToken } = await this.handlers.handler(req);

+      const cookieConfig = this.getCookieConfig(appOrigin);
+
      // Store the scope that we have been granted for this session. This is useful if
      // the provider does not return granted scopes on refresh or if they are normalized.
      if (this.options.persistScopes && state.scope) {
-        this.setGrantedScopeCookie(res, state.scope);
+        this.setGrantedScopeCookie(res, state.scope, cookieConfig);
        response.providerInfo.scope = state.scope;
      }

      if (refreshToken) {
        // set new refresh token
-        this.setRefreshTokenCookie(res, refreshToken);
+        this.setRefreshTokenCookie(res, refreshToken, cookieConfig);
      }

      const identity = await this.populateIdentity(response.backstageIdentity);
@@ -208,7 +191,9 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
    }

    // remove refresh token cookie if it is set
-    this.removeRefreshTokenCookie(res);
+    const origin = req.get('origin');
+    const cookieConfig = this.getCookieConfig(origin);
+    this.removeRefreshTokenCookie(res, cookieConfig);

    res.status(200).end();
  }
@@ -248,7 +233,9 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
      );

      if (newRefreshToken && newRefreshToken !== refreshToken) {
-        this.setRefreshTokenCookie(res, newRefreshToken);
+        const origin = req.get('origin');
+        const cookieConfig = this.getCookieConfig(origin);
+        this.setRefreshTokenCookie(res, newRefreshToken, cookieConfig);
      }

      res.status(200).json({ ...response, backstageIdentity });
@@ -274,18 +261,28 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
    return prepareBackstageIdentityResponse(identity);
  }

-  private setNonceCookie = (res: express.Response, nonce: string) => {
+  private setNonceCookie = (
+    res: express.Response,
+    nonce: string,
+    cookieConfig: ReturnType<CookieConfigurer>,
+  ) => {
    res.cookie(`${this.options.providerId}-nonce`, nonce, {
      maxAge: TEN_MINUTES_MS,
      ...this.baseCookieOptions,
-      path: `${this.options.cookieConfig.path}/handler`,
+      ...cookieConfig,
+      path: `${cookieConfig.path}/handler`,
    });
  };

-  private setGrantedScopeCookie = (res: express.Response, scope: string) => {
+  private setGrantedScopeCookie = (
+    res: express.Response,
+    scope: string,
+    cookieConfig: ReturnType<CookieConfigurer>,
+  ) => {
    res.cookie(`${this.options.providerId}-granted-scope`, scope, {
      maxAge: THOUSAND_DAYS_MS,
      ...this.baseCookieOptions,
+      ...cookieConfig,
    });
  };

@@ -296,17 +293,32 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
  private setRefreshTokenCookie = (
    res: express.Response,
    refreshToken: string,
+    cookieConfig: ReturnType<CookieConfigurer>,
  ) => {
    res.cookie(`${this.options.providerId}-refresh-token`, refreshToken, {
      maxAge: THOUSAND_DAYS_MS,
      ...this.baseCookieOptions,
+      ...cookieConfig,
    });
  };

-  private removeRefreshTokenCookie = (res: express.Response) => {
+  private removeRefreshTokenCookie = (
+    res: express.Response,
+    cookieConfig: ReturnType<CookieConfigurer>,
+  ) => {
    res.cookie(`${this.options.providerId}-refresh-token`, '', {
      maxAge: 0,
      ...this.baseCookieOptions,
+      ...cookieConfig,
+    });
+  };
+
+  private getCookieConfig = (origin?: string) => {
+    return this.options.cookieConfigurer({
+      providerId: this.options.providerId,
+      baseUrl: this.options.baseUrl,
+      callbackUrl: this.options.callbackUrl,
+      appOrigin: origin ?? this.options.appOrigin,
    });
  };
 }