feat: clean up auth flow (#290)

Make the cookie with refresh token strict and request bound - exact host
Refactor oidc discovery service to recive an url as a parameter

---------

Co-authored-by: Grzegorz Krajniak <[email protected]>

Author: gkrajniak

src/auth/auth-config.service.ts

@@ -110,7 +110,8 @@ export class EnvAuthConfigService implements AuthConfigService {
 
     const idpEnvName = this.formatIdpNameForEnvVar(idpName);
 
-    const oidc = await this.discoveryService.getOIDC(idpEnvName);
+    const oidcUrl = process.env[`DISCOVERY_ENDPOINT_${idpEnvName}`];
+    const oidc = await this.discoveryService.getOIDC(oidcUrl);
     const oauthServerUrl =
       oidc?.authorization_endpoint ??
       process.env[`AUTH_SERVER_URL_${idpEnvName}`];

src/auth/auth-token.service.spec.ts

@@ -77,11 +77,10 @@ describe('AuthTokenService', () => {
         'openmfp_auth_cookie',
         refreshTokenValue,
         {
-          domain: 'example.com',
           path: '/',
           httpOnly: true,
           secure: true,
-          sameSite: 'lax',
+          sameSite: 'strict',
         },
       );
     }

src/auth/auth-token.service.ts

@@ -1,4 +1,4 @@
-import { EnvService } from '../env/env.service.js';
+import { EnvService } from '../env/index.js';
 import { AUTH_CONFIG_INJECTION_TOKEN } from '../injection-tokens.js';
 import { CookiesService } from '../services/index.js';
 import {
@@ -43,7 +43,7 @@ export class AuthTokenService {
     code: string,
   ): Promise<AuthTokenData> {
     const authConfig = await this.authConfigService.getAuthConfig(request);
-    const redirectUri = this.getRedirectUri(request, authConfig);
+    const redirectUri = this.getRedirectUri(request);
 
     const body = new URLSearchParams({
       client_id: authConfig.clientId,
@@ -127,10 +127,7 @@ export class AuthTokenService {
    *  When running locally after executing token retrieval the call will be redirected to localhost and port set in FRONTEND_PORT system variable,
    *  otherwise to the host of the initiating request
    */
-  private getRedirectUri(
-    request: Request,
-    currentAuthEnv: ServerAuthVariables,
-  ) {
+  private getRedirectUri(request: Request) {
     let redirectionUrl: string;
     const env = this.envService.getEnv();
     const isStandardOrEmptyPort =
@@ -141,7 +138,7 @@ export class AuthTokenService {
     if (env.isLocal) {
       redirectionUrl = `http://localhost${port}`;
     } else {
-      redirectionUrl = `https://${currentAuthEnv.baseDomain}${port}`;
+      redirectionUrl = `https://${request.hostname}${port}`;
     }
     return `${redirectionUrl}/callback?storageType=none`;
   }

src/env/discovery.service.spec.ts

@@ -1,4 +1,4 @@
-import { DiscoveryService } from './discovery.service';
+import { DiscoveryService } from './discovery.service.js';
 import { HttpService } from '@nestjs/axios';
 import { Test, TestingModule } from '@nestjs/testing';
 import { AxiosError } from 'axios';
@@ -52,11 +52,11 @@ describe('DiscoveryService', () => {
         'example.com/authorization_endpoint',
       );
       expect(oidc.token_endpoint).toEqual('example.com/token_endpoint');
-      expect(service['oidcResultPerIdp']['APP']).toEqual({
+      expect(service['oidcResultPerUrl']['APP']).toEqual({
         authorization_endpoint: 'example.com/authorization_endpoint',
         token_endpoint: 'example.com/token_endpoint',
       });
-      expect(service['oidcResultPerIdp']['APP2']).toBeUndefined();
+      expect(service['oidcResultPerUrl']['APP2']).toBeUndefined();
     });
 
     it('should not get oauthServerUrl and oauthTokenUrl and throw error when httpService returns error', async () => {
@@ -89,13 +89,13 @@ describe('DiscoveryService', () => {
 
       process.env['DISCOVERY_ENDPOINT_APP'] = 'example.com';
 
-      await expect(service.getOIDC('APP')).rejects.toThrow(
+      await expect(service.getOIDC('example.com')).rejects.toThrow(
         'Invalid response from discovery service: Response status: 101, OIDC endpoint: example.com',
       );
     });
 
     it('should get null when DISCOVERY_ENDPOINT does not exist', async () => {
-      const oidc = await service.getOIDC('APP');
+      const oidc = await service.getOIDC('');
 
       expect(oidc).toBeNull();
     });
@@ -116,7 +116,7 @@ describe('DiscoveryService', () => {
 
       process.env['DISCOVERY_ENDPOINT_APP'] = 'example.com';
 
-      await expect(service.getOIDC('APP')).rejects.toThrow(
+      await expect(service.getOIDC('example.com')).rejects.toThrow(
         'Invalid response from discovery service: Response status: 200, OIDC endpoint: example.com',
       );
     });
@@ -138,10 +138,10 @@ describe('DiscoveryService', () => {
 
       process.env['DISCOVERY_ENDPOINT_APP'] = 'example.com';
 
-      await expect(service.getOIDC('APP')).rejects.toThrow(
+      await expect(service.getOIDC('example.com')).rejects.toThrow(
         'Invalid response from discovery service: Response status: 200, OIDC endpoint: example.com',
       );
-      expect(service['oidcResultPerIdp']['APP']).toBeUndefined();
+      expect(service['oidcResultPerUrl']['example.com']).toBeUndefined();
     });
   });
 });

src/env/discovery.service.ts

@@ -10,16 +10,15 @@ interface OIDC {
 
 @Injectable()
 export class DiscoveryService {
-  private oidcResultPerIdp: Record<string, OIDC> = {};
+  private oidcResultPerUrl: Record<string, OIDC> = {};
 
   constructor(private httpService: HttpService) {}
 
-  public async getOIDC(idpEnvName: string): Promise<OIDC> {
-    const oidcUrl = process.env[`DISCOVERY_ENDPOINT_${idpEnvName}`];
+  public async getOIDC(oidcUrl: string): Promise<OIDC> {
     if (!oidcUrl) return null;
 
-    if (this.oidcResultPerIdp[idpEnvName]) {
-      return this.oidcResultPerIdp[idpEnvName];
+    if (this.oidcResultPerUrl[oidcUrl]) {
+      return this.oidcResultPerUrl[oidcUrl];
     }
 
     const oidcResult = await firstValueFrom(
@@ -36,7 +35,7 @@ export class DiscoveryService {
       const oidc = oidcResult.data;
 
       if (oidc.authorization_endpoint && oidc.token_endpoint) {
-        this.oidcResultPerIdp[idpEnvName] = oidc;
+        this.oidcResultPerUrl[oidcUrl] = oidc;
         return oidc;
       }
     }

src/services/cookies.service.spec.ts

@@ -1,4 +1,4 @@
-import { AuthTokenData } from '../auth/auth-token.service.js';
+import { AuthTokenData } from '../auth/index.js';
 import { CookiesService } from './cookies.service.js';
 import { Test, TestingModule } from '@nestjs/testing';
 import type { Request, Response } from 'express';
@@ -56,11 +56,10 @@ describe('CookiesService', () => {
         'openmfp_auth_cookie',
         'test-refresh-token',
         {
-          domain: 'test-hostname',
           path: '/',
           httpOnly: true,
           secure: true,
-          sameSite: 'lax',
+          sameSite: 'strict',
         },
       );
     });
@@ -81,10 +80,9 @@ describe('CookiesService', () => {
       expect(mockResponse.clearCookie).toHaveBeenCalledWith(
         'openmfp_auth_cookie',
         {
-          domain: 'test-hostname',
           httpOnly: true,
           path: '/',
-          sameSite: 'lax',
+          sameSite: 'strict',
           secure: true,
         },
       );

src/services/cookies.service.ts

@@ -1,4 +1,4 @@
-import { AuthTokenData } from '../auth/auth-token.service.js';
+import { AuthTokenData } from '../auth/index.js';
 import { Injectable } from '@nestjs/common';
 import type { Request, Response } from 'express';
 
@@ -27,13 +27,12 @@ export class CookiesService {
     response.clearCookie(authCookie, this.cookieParams(request));
   }
 
-  private cookieParams(request: Request) {
+  private cookieParams(_request: Request) {
     return {
-      domain: request.hostname,
       httpOnly: true,
       secure: true,
       path: '/',
-      sameSite: 'lax',
+      sameSite: 'strict',
     };
   }
 }