Login cauthentication flow change (#300)

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

Author: gkrajniak

integration-tests/auth.integration-spec.ts

@@ -24,10 +24,10 @@ describe('AuthController', () => {
     await app.init();
   });
 
-  describe('POST /rest/auth', () => {
+  describe('GET /callback', () => {
     it('should fail due to the lack of auth code param in the query params', async () => {
       await request(app.getHttpServer())
-        .post(`/rest/auth?coder=uio`)
+        .get(`/callback?coder=uio`)
         .accept(acceptLanguage)
         .expect(400)
         .expect((response) => {
@@ -39,9 +39,11 @@ describe('AuthController', () => {
 
     it('should pass with auth code param', async () => {
       await request(app.getHttpServer())
-        .post(`/rest/auth?code=auth_code`)
+        .get(
+          `/callback?code=auth_code&state=aHR0cDovL3N1Yi5sb2NhbGhvc3Q6NDMwMC9fbHVpZ2lOb25jZT1QUUNWODdlck5uVkZzNE1rMzUxNw==`,
+        )
         .accept(acceptLanguage)
-        .expect(201);
+        .expect(302);
     });
   });
 

src/auth/auth.controller.spec.ts

@@ -20,6 +20,7 @@ describe('AuthController', () => {
 
   beforeEach(async () => {
     jest.resetAllMocks();
+    (responseMock.redirect as any) = jest.fn((href: string) => href);
 
     const module: TestingModule = await Test.createTestingModule({
       imports: [PortalModule.create({})],
@@ -32,69 +33,91 @@ describe('AuthController', () => {
       .useValue(cookiesServiceMock)
       .compile();
     controller = module.get<AuthController>(AuthController);
+    process.env.BASE_DOMAINS_DEFAULT = 'localhost';
   });
 
   it('should be defined', () => {
     expect(controller).toBeDefined();
   });
 
   describe('auth', () => {
-    it('should get the config for tenant', async () => {
-      // arrange
-      const callback = jest.spyOn(authCallbackMock, 'handleSuccess');
-      const getTokenForCode = jest.spyOn(
-        authTokenServiceMock,
-        'exchangeTokenForCode',
-      );
-      requestMock.query = { code: 'foo' };
-      const idToken = 'id_token';
+    it('redirects to decoded state URL after successful token exchange', async () => {
+      const decodedUrl = 'http://sub.localhost:4300/';
+      requestMock.query = {
+        code: 'foo',
+        state: encodeURIComponent(btoa(`${decodedUrl}_luigiNonce=SOME_NONCE`)),
+      } as any;
+
       const authTokenResponse = {
-        id_token: idToken,
+        id_token: 'id',
         refresh_token: 'ref',
-        expires_in: '12312',
-        access_token: 'access',
+        access_token: 'acc',
+        expires_in: '111',
       } as AuthTokenData;
-      getTokenForCode.mockResolvedValue(authTokenResponse);
+      authTokenServiceMock.exchangeTokenForCode.mockResolvedValue(
+        authTokenResponse,
+      );
 
-      // act
-      const tokenResponse = await controller.auth(requestMock, responseMock);
+      const result = await controller.auth(requestMock, responseMock);
 
-      // assert
-      expect(callback).toHaveBeenCalledWith(
+      expect(authTokenServiceMock.exchangeTokenForCode).toHaveBeenCalledWith(
         requestMock,
         responseMock,
-        authTokenResponse,
+        'foo',
       );
-      expect(getTokenForCode).toHaveBeenCalledWith(
+      expect(authCallbackMock.handleSuccess).toHaveBeenCalledWith(
         requestMock,
         responseMock,
-        'foo',
+        authTokenResponse,
       );
-      expect((tokenResponse as AuthTokenData).refresh_token).toBeUndefined();
+      expect(responseMock.redirect).toHaveBeenCalledWith(decodedUrl);
+      expect(result).toBe(decodedUrl);
     });
 
-    it('should log the error if there is a problem retrieving the token', async () => {
-      // arrange
-      const getTokenForCode = jest.spyOn(
-        authTokenServiceMock,
-        'exchangeTokenForCode',
+    it('redirects to /logout with error when token exchange fails', async () => {
+      const origin = 'http://sub.localhost:4300';
+      const stateUrl = `${origin}/path?x=1#frag`;
+      requestMock.query = {
+        code: 'foo',
+        state: encodeURIComponent(btoa(`${stateUrl}_luigiNonce=N`)),
+      } as any;
+
+      authTokenServiceMock.exchangeTokenForCode.mockRejectedValue(
+        new Error('boom'),
       );
-      requestMock.query = { code: 'foo' };
-      getTokenForCode.mockRejectedValue(new Error('error'));
 
-      // act
-      const response = await controller.auth(requestMock, responseMock);
+      const result = await controller.auth(requestMock, responseMock);
 
-      // assert
-      expect(response).toBeUndefined();
-      expect(getTokenForCode).toHaveBeenCalledWith(
+      expect(authCallbackMock.handleFailure).toHaveBeenCalledWith(
         requestMock,
         responseMock,
-        'foo',
       );
+      expect(responseMock.redirect).toHaveBeenCalledWith(
+        `${origin}/logout?error=loginError`,
+      );
+      expect(result).toBe(`${origin}/logout?error=loginError`);
     });
 
-    it('should ', () => {});
+    it('redirects to /logout with error when state URL domain is not allowed', async () => {
+      const badOrigin = 'http://malicious.example.com';
+      const stateUrl = `${badOrigin}/`;
+      requestMock.query = {
+        code: 'foo',
+        state: encodeURIComponent(btoa(`${stateUrl}_luigiNonce=N`)),
+      } as any;
+
+      const result = await controller.auth(requestMock, responseMock);
+
+      expect(authTokenServiceMock.exchangeTokenForCode).not.toHaveBeenCalled();
+      expect(authCallbackMock.handleFailure).toHaveBeenCalledWith(
+        requestMock,
+        responseMock,
+      );
+      expect(responseMock.redirect).toHaveBeenCalledWith(
+        `${badOrigin}/logout?error=loginError`,
+      );
+      expect(result).toBe(`${badOrigin}/logout?error=loginError`);
+    });
   });
 
   describe('refresh', () => {
@@ -149,7 +172,6 @@ describe('AuthController', () => {
 
     it('should remove the auth cookies on auth server error', async () => {
       // arrange
-      const logoutRedirectUrl = 'logoutRedirectUrl';
       cookiesServiceMock.getAuthCookie.mockReturnValue('authCookie');
       authTokenServiceMock.exchangeTokenForRefreshToken.mockRejectedValue(
         new Error('error'),

src/auth/auth.controller.ts

@@ -13,8 +13,9 @@ import {
   UseGuards,
 } from '@nestjs/common';
 import type { Request, Response } from 'express';
+import url from 'url';
 
-@Controller('/rest/auth')
+@Controller('/')
 export class AuthController {
   private logger: Logger = new Logger(AuthController.name);
 
@@ -26,34 +27,57 @@ export class AuthController {
   ) {}
 
   @UseGuards(RequestCodeParamGuard)
-  @Post('')
-  async auth(
-    @Req() request: Request,
-    @Res({ passthrough: true }) response: Response,
-  ): Promise<AuthTokenData | void> {
-    let authTokenData: AuthTokenData = null;
+  @Get('callback')
+  async auth(@Req() request: Request, @Res() response: Response): Response {
+    const { code, state } = request.query;
+    let postLoginRedirectUrl = this.createAppStateUrl(state);
+
     try {
-      authTokenData = await this.authTokenService.exchangeTokenForCode(
+      if (!this.isDomainOrSubdomain(postLoginRedirectUrl)) {
+        throw new Error('Bad redirection url: ' + postLoginRedirectUrl);
+      }
+
+      const authTokenData = await this.authTokenService.exchangeTokenForCode(
         request,
         response,
-        request.query.code.toString(),
+        code,
       );
+
+      await this.handleTokenRetrieval(request, response, authTokenData);
     } catch (e: any) {
       this.logger.error(
         `Error while retrieving token from code, logging out: ${e}`,
       );
-      return await this.handleAuthError(request, response);
+      await this.handleAuthError(request, response);
+      postLoginRedirectUrl = new URL(`${postLoginRedirectUrl.origin}/logout`);
+      postLoginRedirectUrl.searchParams.set('error', 'loginError');
     }
-    return await this.handleTokenRetrieval(request, response, authTokenData);
+
+    return response.redirect(postLoginRedirectUrl.href);
+  }
+
+  private isDomainOrSubdomain(appStateUrl: url.URL) {
+    const baseDomain = process.env['BASE_DOMAINS_DEFAULT'];
+    if (!baseDomain) return false;
+
+    const hostname = appStateUrl.hostname;
+    return hostname === baseDomain || hostname.endsWith(`.${baseDomain}`);
+  }
+
+  private createAppStateUrl(state: string): url.URL {
+    const decodedState = atob(decodeURIComponent(state)).split('_luigiNonce=');
+    const appState = decodeURI(decodedState[0] || '');
+    return new URL(appState);
   }
 
-  @Get('refresh')
+  @Post('rest/auth/refresh')
   async refresh(
     @Req() request: Request,
     @Res({ passthrough: true }) response: Response,
   ): Promise<AuthTokenData | void> {
     const refreshToken = this.cookiesService.getAuthCookie(request);
     if (!refreshToken) {
+      this.logger.warn('No refresh token present');
       return;
     }
 

src/portal.module.spec.ts

@@ -45,7 +45,7 @@ describe('PortalModule', () => {
 
     const expectedModule = ServeStaticModule.forRoot({
       rootPath: expectedPath,
-      exclude: ['/rest'],
+      exclude: ['/rest', '/callback'],
     });
 
     const portalModule = PortalModule.create({

src/portal.module.ts

@@ -240,7 +240,7 @@ export class PortalModule implements NestModule {
       moduleImports.push(
         ServeStaticModule.forRoot({
           rootPath: options.frontendDistSources,
-          exclude: ['/rest'],
+          exclude: ['/rest', '/callback'],
         }),
       );
     }

src/services/guards/request-code-param.guard.spec.ts

@@ -1,51 +1,61 @@
-import { RequestCodeParamGuard } from './request-code-param.guard';
+import { RequestCodeParamGuard } from './request-code-param.guard.js';
 import { ExecutionContext, HttpException, HttpStatus } from '@nestjs/common';
-import { Test, TestingModule } from '@nestjs/testing';
-
-// Adjust the import path as needed
 
 describe('RequestCodeParamGuard', () => {
   let guard: RequestCodeParamGuard;
 
-  beforeEach(async () => {
-    const module: TestingModule = await Test.createTestingModule({
-      providers: [RequestCodeParamGuard],
-    }).compile();
+  const createContext = (query: any): ExecutionContext =>
+    ({
+      switchToHttp: () => ({
+        getRequest: () => ({ query }),
+      }),
+    }) as unknown as ExecutionContext;
 
-    guard = module.get<RequestCodeParamGuard>(RequestCodeParamGuard);
+  beforeEach(() => {
+    guard = new RequestCodeParamGuard();
   });
 
-  it('should be defined', () => {
-    expect(guard).toBeDefined();
+  it('returns true when code and state exist', () => {
+    const ctx = createContext({ code: 'abc', state: 'xyz' });
+    expect(guard.canActivate(ctx)).toBe(true);
   });
 
-  it('should allow access when code is provided', () => {
-    const context = {
-      switchToHttp: () => ({
-        getRequest: () => ({
-          query: { code: 'validCode' },
-        }),
-      }),
-    } as ExecutionContext;
-
-    expect(guard.canActivate(context)).toBe(true);
+  it('throws 400 when code is missing', () => {
+    const ctx = createContext({ state: 'xyz' });
+    try {
+      guard.canActivate(ctx);
+      fail('should throw');
+    } catch (e) {
+      expect(e).toBeInstanceOf(HttpException);
+      expect((e as HttpException).getStatus()).toBe(HttpStatus.BAD_REQUEST);
+      expect((e as HttpException).message).toBe(
+        "No 'code' was provided in the query.",
+      );
+    }
   });
 
-  it('should throw HttpException when code is not provided', () => {
-    const context = {
-      switchToHttp: () => ({
-        getRequest: () => ({
-          query: {},
-        }),
-      }),
-    } as ExecutionContext;
+  it('throws 400 when state is missing', () => {
+    const ctx = createContext({ code: 'abc' });
+    try {
+      guard.canActivate(ctx);
+      fail('should throw');
+    } catch (e) {
+      expect(e).toBeInstanceOf(HttpException);
+      expect((e as HttpException).getStatus()).toBe(HttpStatus.BAD_REQUEST);
+      expect((e as HttpException).message).toBe(
+        "No 'state' was provided in the query.",
+      );
+    }
+  });
 
+  it('throws 400 when both are missing', () => {
+    const ctx = createContext({});
     try {
-      guard.canActivate(context);
-    } catch (error) {
-      expect(error).toBeInstanceOf(HttpException);
-      expect(error.getStatus()).toBe(HttpStatus.BAD_REQUEST);
-      expect(error.message).toBe("No 'code' was provided in the query.");
+      guard.canActivate(ctx);
+      fail('should throw');
+    } catch (e) {
+      expect(e).toBeInstanceOf(HttpException);
+      expect((e as HttpException).getStatus()).toBe(HttpStatus.BAD_REQUEST);
     }
   });
 });

src/services/guards/request-code-param.guard.ts

@@ -10,7 +10,7 @@ import {
 export class RequestCodeParamGuard implements CanActivate {
   canActivate(context: ExecutionContext): boolean {
     const request = context.switchToHttp().getRequest();
-    const code = request.query.code;
+    const { code, state } = request.query;
 
     if (!code) {
       throw new HttpException(
@@ -19,6 +19,13 @@ export class RequestCodeParamGuard implements CanActivate {
       );
     }
 
+    if (!state) {
+      throw new HttpException(
+        "No 'state' was provided in the query.",
+        HttpStatus.BAD_REQUEST,
+      );
+    }
+
     return true;
   }
 }