Merge pull request #691 from Marvell-Consulting/worktree-SW-1246-auth-tests

Expand test coverage for authentication routes (#565)

Author: wheelsandcogs

jest.config.ts

@@ -8,13 +8,7 @@ const sharedConfig = {
   // openid-client and its deps are now published as ESM and need transpiling to CJS
   transformIgnorePatterns: ['/node_modules/(?!(openid-client|oauth4webapi|jose|nanoid)/)'],
   testEnvironment: 'node' as const,
-  coveragePathIgnorePatterns: [
-    '/node_modules',
-    '/test/',
-    '/src/migrations',
-    '/src/controllers/auth.ts',
-    'src/middleware/passport-auth.ts'
-  ]
+  coveragePathIgnorePatterns: ['/node_modules', '/test/', '/src/migrations']
 };
 
 const config: Config = {

src/controllers/auth.ts

@@ -14,7 +14,7 @@ import { UserDTO } from '../dtos/user/user-dto';
 const domain = new URL(config.auth.jwt.cookieDomain).hostname;
 logger.debug(`JWT cookie domain is '${domain}'`);
 
-const checkTokenFitsInCookie = (token: string): void => {
+export const checkTokenFitsInCookie = (token: string): void => {
   const maxCookieSize = 4096; // Maximum size of a cookie in bytes
   const tokenSize = Buffer.byteLength(token, 'utf8');
 

src/middleware/passport-auth.ts

@@ -109,20 +109,28 @@ const initEntraId = async (entraIdConfig: EntraIdConfig): Promise<void> => {
     callbackURL: `${config.backend.url}/auth/entraid/callback`
   };
 
-  const verify: VerifyFunction = async (tokens: Tokens, done: AuthenticateCallback) => {
+  passport.use(AuthProvider.EntraId, new OpenIdStrategy(strategyOptions, entraIdVerify(openidConfig)));
+};
+
+// Extracted as a named factory so the verify branches can be unit-tested without standing up a live
+// OIDC discovery. `openidConfig` is captured so userinfo can be fetched against the discovered provider.
+export const entraIdVerify =
+  (openidConfig: OpenIdConfig): VerifyFunction =>
+  async (tokens: Tokens, done: AuthenticateCallback) => {
     logger.debug('auth callback from entraid received');
-    const { sub } = tokens.claims()!;
 
-    logger.debug('fetching user info from entraid...');
-    const userInfo = await openIdClient.fetchUserInfo(openidConfig, tokens.access_token, sub);
+    try {
+      const { sub } = tokens.claims()!;
 
-    if (!userInfo?.sub || !userInfo?.email) {
-      logger.warn('entraid auth failed: account is missing user id or email address and we need both');
-      done(null, undefined, { message: 'entraid account does not have a user id or email, cannot login' });
-      return;
-    }
+      logger.debug('fetching user info from entraid...');
+      const userInfo = await openIdClient.fetchUserInfo(openidConfig, tokens.access_token, sub);
+
+      if (!userInfo?.sub || !userInfo?.email) {
+        logger.warn('entraid auth failed: account is missing user id or email address and we need both');
+        done(null, undefined, { message: 'entraid account does not have a user id or email, cannot login' });
+        return;
+      }
 
-    try {
       logger.debug('checking if user has previously logged in...');
 
       const existingUserById = await UserRepository.findOne({
@@ -178,6 +186,3 @@ const initEntraId = async (entraIdConfig: EntraIdConfig): Promise<void> => {
       done(null, undefined, { message: 'Unknown error' });
     }
   };
-
-  passport.use(AuthProvider.EntraId, new OpenIdStrategy(strategyOptions, verify));
-};

test/integration/routes/auth.test.ts

@@ -1,9 +1,18 @@
 import request from 'supertest';
+import jwt from 'jsonwebtoken';
 
 import app from '../../../src/app';
 import { initPassport } from '../../../src/middleware/passport-auth';
 import { ensureWorkerDataSources, resetDatabase } from '../../helpers/reset-database';
 import { config } from '../../../src/config';
+import { dbManager } from '../../../src/db/database-manager';
+import { UserDTO } from '../../../src/dtos/user/user-dto';
+import { UserGroup } from '../../../src/entities/user/user-group';
+import { UserGroupRole } from '../../../src/entities/user/user-group-role';
+import { GroupRole } from '../../../src/enums/group-role';
+import { Locale } from '../../../src/enums/locale';
+import { getTestUser, getTestUserGroup } from '../../helpers/get-test-user';
+import { getAuthHeader } from '../../helpers/auth-header';
 
 // Need to mock blob storage as it is included in services middleware for every route
 // avoids the "Jest did not exit one second after the test run has completed"
@@ -18,6 +27,8 @@ jest.mock('../../../src/services/blob-storage', () => {
 });
 
 describe('Auth routes', () => {
+  const callbackURL = `${config.frontend.url}/auth/callback`;
+
   beforeAll(async () => {
     await ensureWorkerDataSources();
     await resetDatabase();
@@ -30,4 +41,102 @@ describe('Auth routes', () => {
     expect(res.status).toBe(200);
     expect(res.body).toEqual({ enabled: expectedProviders });
   });
+
+  describe('GET /auth/local (loginLocal)', () => {
+    test('redirects to the frontend callback and sets a jwt cookie for a known user', async () => {
+      const user = getTestUser('Local Success');
+      await user.save();
+
+      const res = await request(app).get('/auth/local').query({ username: user.providerUserId });
+
+      expect(res.status).toBe(302);
+      expect(res.headers.location).toBe(callbackURL);
+
+      const cookies = res.headers['set-cookie'] as unknown as string[];
+      const jwtCookie = cookies.find((cookie) => cookie.startsWith('jwt='));
+      expect(jwtCookie).toBeDefined();
+      expect(jwtCookie).toContain('HttpOnly');
+    });
+
+    test('redirects with error=login when no username is provided', async () => {
+      const res = await request(app).get('/auth/local');
+      expect(res.status).toBe(302);
+      expect(res.headers.location).toBe(`${callbackURL}?error=login`);
+      expect(res.headers['set-cookie']).toBeUndefined();
+    });
+
+    test('redirects with error=login when the user does not exist', async () => {
+      const res = await request(app).get('/auth/local').query({ username: 'no-such-user' });
+      expect(res.status).toBe(302);
+      expect(res.headers.location).toBe(`${callbackURL}?error=login`);
+      expect(res.headers['set-cookie']).toBeUndefined();
+    });
+  });
+
+  // Exercises the JWT strategy branches in passport-auth.ts via the protected /healthcheck/jwt probe.
+  // healthcheck.test.ts keeps a single 200 case as a smoke test of the route itself.
+  describe('JWT auth middleware (passport-auth)', () => {
+    test('/healthcheck/jwt returns 401 without a bearer token', async () => {
+      const res = await request(app).get('/healthcheck/jwt');
+      expect(res.status).toBe(401);
+    });
+
+    test('/healthcheck/jwt returns 401 with an invalid bearer token', async () => {
+      const res = await request(app).get('/healthcheck/jwt').set({ Authorization: 'Bearer this-is-not-a-token' });
+      expect(res.status).toBe(401);
+    });
+
+    test('/healthcheck/jwt returns 401 with a valid token for a user that does not exist', async () => {
+      const unknownUser = getTestUser('Unknown JWT User');
+      const res = await request(app).get('/healthcheck/jwt').set(getAuthHeader(unknownUser));
+      expect(res.status).toBe(401);
+    });
+
+    test('/healthcheck/jwt returns 401 for an expired token', async () => {
+      const user = getTestUser('Expired Token User');
+      await user.save();
+
+      const token = jwt.sign({ user: UserDTO.fromUser(user, Locale.English) }, config.auth.jwt.secret, {
+        expiresIn: '-1s'
+      });
+
+      const res = await request(app)
+        .get('/healthcheck/jwt')
+        .set({ Authorization: `Bearer ${token}` });
+      expect(res.status).toBe(401);
+    });
+
+    test('/healthcheck/jwt returns 401 when the user permissions have changed since the token was issued', async () => {
+      const group = await dbManager
+        .getPublisherDataSource()
+        .getRepository(UserGroup)
+        .save(getTestUserGroup('Perm Change Group'));
+
+      const user = getTestUser('Perm Change User');
+      user.groupRoles = [UserGroupRole.create({ group, roles: [GroupRole.Editor] })];
+      await user.save();
+
+      // token captures the user while they hold the Editor role
+      const authHeader = getAuthHeader(user);
+
+      // revoke the role in the database so the live permissions no longer match the token
+      await dbManager.getPublisherDataSource().getRepository(UserGroupRole).delete({ userId: user.id });
+
+      const res = await request(app).get('/healthcheck/jwt').set(authHeader);
+      expect(res.status).toBe(401);
+    });
+
+    test('/healthcheck/jwt returns 200 with a valid bearer token', async () => {
+      const user = getTestUser('Valid JWT User');
+      await user.save();
+
+      const res = await request(app).get('/healthcheck/jwt').set(getAuthHeader(user));
+
+      expect(res.status).toBe(200);
+      expect(res.body).toEqual({
+        message: 'success',
+        user: UserDTO.fromUser(user, Locale.English)
+      });
+    });
+  });
 });

test/integration/routes/healthcheck.test.ts

@@ -116,23 +116,10 @@ describe('Healthcheck', () => {
     });
   });
 
+  // Smoke test that /healthcheck/jwt is wired to JWT auth and returns 200 for a valid user. The full
+  // set of JWT strategy branches (missing / invalid / expired token, unknown user, changed permissions)
+  // lives in test/integration/routes/auth.test.ts alongside the other passport-auth tests.
   describe('Authentication', () => {
-    test('/heathcheck/jwt returns 401 without a bearer token', async () => {
-      const res = await request(app).get('/healthcheck/jwt');
-      expect(res.status).toBe(401);
-    });
-
-    test('/heathcheck/jwt returns 401 with an invalid bearer token', async () => {
-      const res = await request(app).get('/healthcheck/jwt').set({ Authorization: 'Bearer this-is-not-a-token' });
-      expect(res.status).toBe(401);
-    });
-
-    test('/heathcheck/jwt returns 401 with a valid bearer token but inactive user', async () => {
-      const inactiveUser = getTestUser('Inactive User');
-      const res = await request(app).get('/healthcheck/jwt').set(getAuthHeader(inactiveUser));
-      expect(res.status).toBe(401);
-    });
-
     test('/heathcheck/jwt returns 200 with a valid bearer token', async () => {
       const testUser = getTestUser();
       await testUser.save();

test/unit/controllers/auth.test.ts

@@ -0,0 +1,80 @@
+import { Request, Response } from 'express';
+import passport from 'passport';
+import jwt from 'jsonwebtoken';
+
+import { checkTokenFitsInCookie, loginEntraID } from '../../../src/controllers/auth';
+import { config } from '../../../src/config';
+
+describe('auth controller', () => {
+  describe('checkTokenFitsInCookie', () => {
+    it('does not throw for a token within the 4096 byte limit', () => {
+      expect(() => checkTokenFitsInCookie('a'.repeat(4096))).not.toThrow();
+    });
+
+    it('throws for a token larger than 4096 bytes', () => {
+      expect(() => checkTokenFitsInCookie('a'.repeat(4097))).toThrow(/exceeds the maximum cookie size/);
+    });
+  });
+
+  describe('loginEntraID', () => {
+    const returnURL = `${config.frontend.url}/auth/callback`;
+
+    let req: { login: jest.Mock };
+    let res: Partial<Response> & { redirect: jest.Mock; cookie: jest.Mock };
+    let next: jest.Mock;
+
+    // Replace passport.authenticate with a stub that immediately invokes the controller's
+    // callback with the supplied (err, user, info), mimicking a finished EntraID round-trip.
+    const stubAuthenticate = (err: Error | null, user: unknown, info?: Record<string, string>) => {
+      jest
+        .spyOn(passport, 'authenticate')
+        .mockImplementation(
+          ((_strategy: unknown, _opts: unknown, cb: (e: Error | null, u: unknown, i?: unknown) => void) => () =>
+            cb(err, user, info)) as unknown as typeof passport.authenticate
+        );
+    };
+
+    beforeEach(() => {
+      req = { login: jest.fn((_user, _opts, cb: (e: Error | null) => void) => cb(null)) };
+      res = { redirect: jest.fn(), cookie: jest.fn() };
+      next = jest.fn();
+    });
+
+    afterEach(() => jest.restoreAllMocks());
+
+    it('redirects with error=provider when passport returns an error', () => {
+      stubAuthenticate(new Error('boom'), undefined);
+      loginEntraID(req as unknown as Request, res as Response, next);
+      expect(res.redirect).toHaveBeenCalledWith(`${returnURL}?error=provider`);
+      expect(res.cookie).not.toHaveBeenCalled();
+    });
+
+    it('redirects with error=provider when no user is returned', () => {
+      stubAuthenticate(null, undefined, { message: 'no user' });
+      loginEntraID(req as unknown as Request, res as Response, next);
+      expect(res.redirect).toHaveBeenCalledWith(`${returnURL}?error=provider`);
+    });
+
+    it('redirects with error=login when req.login fails', () => {
+      stubAuthenticate(null, { id: 'user-1', email: 'a@b.com' });
+      req.login = jest.fn((_user, _opts, cb: (e: Error | null) => void) => cb(new Error('login failed')));
+      loginEntraID(req as unknown as Request, res as Response, next);
+      expect(res.redirect).toHaveBeenCalledWith(`${returnURL}?error=login`);
+      expect(res.cookie).not.toHaveBeenCalled();
+    });
+
+    it('sets a jwt cookie and redirects to the callback on success', () => {
+      const user = { id: 'user-1', email: 'a@b.com', name: 'A B', status: 'active', globalRoles: [], groupRoles: [] };
+      stubAuthenticate(null, user);
+
+      loginEntraID(req as unknown as Request, res as Response, next);
+
+      expect(res.cookie).toHaveBeenCalledWith('jwt', expect.any(String), expect.objectContaining({ httpOnly: true }));
+      expect(res.redirect).toHaveBeenCalledWith(returnURL);
+
+      const token = res.cookie.mock.calls[0][1] as string;
+      const decoded = jwt.verify(token, config.auth.jwt.secret) as { user: { id: string } };
+      expect(decoded.user.id).toBe('user-1');
+    });
+  });
+});