From 6bccb10ea10ccf04abde848b6d6e71b3d5fcb640 Mon Sep 17 00:00:00 2001 From: Nida Ali Date: Fri, 28 Aug 2026 01:48:24 -0400 Subject: [PATCH 1/3] test: cover the auth paths the design decisions depend on JwtStrategy had no unit coverage, and the behaviour the database re-read exists for -- a deleted or demoted user losing access on the next request rather than at token expiry -- was untested end to end. - unit: JwtStrategy.validate lookup, returned shape, database role winning over the token claim, and rejection when the user is gone - e2e: GET /auth/me, a still-unexpired token whose user was deleted, and a role change taking effect without re-issuing the token --- src/auth/strategies/jwt.strategy.spec.ts | 89 ++++++++++++++++++++++++ test/services.e2e-spec.ts | 60 ++++++++++++++++ 2 files changed, 149 insertions(+) create mode 100644 src/auth/strategies/jwt.strategy.spec.ts diff --git a/src/auth/strategies/jwt.strategy.spec.ts b/src/auth/strategies/jwt.strategy.spec.ts new file mode 100644 index 0000000..4547657 --- /dev/null +++ b/src/auth/strategies/jwt.strategy.spec.ts @@ -0,0 +1,89 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { ConfigService } from '@nestjs/config'; +import { getRepositoryToken } from '@nestjs/typeorm'; +import { UnauthorizedException } from '@nestjs/common'; +import { JwtStrategy } from './jwt.strategy'; +import { User, UserRole } from '../entities/user.entity'; +import { JwtPayload } from '../jwt-payload.interface'; + +describe('JwtStrategy', () => { + let strategy: JwtStrategy; + let userRepo: jest.Mocked; + + const payload: JwtPayload = { + sub: 'user-1', + email: 'viewer@example.com', + role: UserRole.VIEWER, + }; + + beforeEach(async () => { + userRepo = { findOne: jest.fn() }; + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + JwtStrategy, + { + provide: ConfigService, + useValue: { get: () => 'test-secret-at-least-16-characters' }, + }, + { provide: getRepositoryToken(User), useValue: userRepo }, + ], + }).compile(); + + strategy = module.get(JwtStrategy); + }); + + it('looks the user up by the token subject', async () => { + userRepo.findOne.mockResolvedValue({ + id: 'user-1', + email: 'viewer@example.com', + role: UserRole.VIEWER, + }); + + await strategy.validate(payload); + + expect(userRepo.findOne).toHaveBeenCalledWith({ + where: { id: payload.sub }, + }); + }); + + it('returns the shape that becomes request.user', async () => { + userRepo.findOne.mockResolvedValue({ + id: 'user-1', + email: 'viewer@example.com', + role: UserRole.VIEWER, + passwordHash: 'should-not-be-returned', + }); + + const result = await strategy.validate(payload); + + expect(result).toEqual({ + id: 'user-1', + email: 'viewer@example.com', + role: UserRole.VIEWER, + }); + expect(result).not.toHaveProperty('passwordHash'); + }); + + it('trusts the database role over the token claim', async () => { + // The token was issued while this user was a viewer; they are now an admin. + userRepo.findOne.mockResolvedValue({ + id: 'user-1', + email: 'viewer@example.com', + role: UserRole.ADMIN, + }); + + const result = await strategy.validate(payload); + + expect(payload.role).toBe(UserRole.VIEWER); + expect(result.role).toBe(UserRole.ADMIN); + }); + + it('rejects a token whose user no longer exists', async () => { + userRepo.findOne.mockResolvedValue(null); + + await expect(strategy.validate(payload)).rejects.toBeInstanceOf( + UnauthorizedException, + ); + }); +}); diff --git a/test/services.e2e-spec.ts b/test/services.e2e-spec.ts index 3fccafc..cfe7654 100644 --- a/test/services.e2e-spec.ts +++ b/test/services.e2e-spec.ts @@ -95,6 +95,22 @@ describe('Services API (e2e)', () => { .expect(400)); }); + describe('GET /api/auth/me', () => { + it('returns the caller identified by the token', async () => { + const res = await asViewer(api().get('/api/auth/me')).expect(200); + + expect(res.body).toEqual({ + id: expect.any(String), + email: TEST_VIEWER.email, + role: 'viewer', + }); + expect(res.body).not.toHaveProperty('passwordHash'); + }); + + it('rejects an unauthenticated caller', () => + api().get('/api/auth/me').expect(401)); + }); + describe('authorization', () => { it('rejects an unauthenticated read', () => api().get('/api/services').expect(401)); @@ -117,6 +133,50 @@ describe('Services API (e2e)', () => { asAdmin(api().post('/api/services')) .send({ name: 'Allowed', description: 'created by admin' }) .expect(201)); + + // The reason JwtStrategy.validate() re-reads the user rather than trusting + // the token claims: deleting a user must take effect on the next request, + // not whenever their token happens to expire. + it('rejects a still-unexpired token whose user has been deleted', async () => { + const token = await login(TEST_VIEWER); + + await api() + .get('/api/services') + .set('Authorization', `Bearer ${token}`) + .expect(200); + + await dataSource.query('DELETE FROM users WHERE email = $1', [ + TEST_VIEWER.email, + ]); + + // Same token, still cryptographically valid and well inside its expiry. + await api() + .get('/api/services') + .set('Authorization', `Bearer ${token}`) + .expect(401); + }); + + it('reflects a role change without re-issuing the token', async () => { + const token = await login(TEST_VIEWER); + + await api() + .post('/api/services') + .set('Authorization', `Bearer ${token}`) + .send({ name: 'Before promotion', description: 'x' }) + .expect(403); + + await dataSource.query('UPDATE users SET role = $1 WHERE email = $2', [ + 'admin', + TEST_VIEWER.email, + ]); + + // The token still says "viewer"; the guard uses the freshly-read role. + await api() + .post('/api/services') + .set('Authorization', `Bearer ${token}`) + .send({ name: 'After promotion', description: 'x' }) + .expect(201); + }); }); describe('GET /api/services', () => { From 3d03d75d1bf773948256f699e26b0a1322d283fb Mon Sep 17 00:00:00 2001 From: Nida Ali Date: Sun, 30 Aug 2026 22:11:22 -0400 Subject: [PATCH 2/3] test: assert the token claim is stale before the promoted request The role-change test relied on a comment to establish that the token still said "viewer". Decode and assert it, so a 201 can only mean the guard used the freshly-read database role. --- test/services.e2e-spec.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/services.e2e-spec.ts b/test/services.e2e-spec.ts index cfe7654..d8fca68 100644 --- a/test/services.e2e-spec.ts +++ b/test/services.e2e-spec.ts @@ -170,7 +170,14 @@ describe('Services API (e2e)', () => { TEST_VIEWER.email, ]); - // The token still says "viewer"; the guard uses the freshly-read role. + // Prove the premise rather than asserting it in a comment: the token is + // unchanged and still claims "viewer", so a 201 can only mean the guard + // used the freshly-read database role. + const claims = JSON.parse( + Buffer.from(token.split('.')[1], 'base64url').toString(), + ); + expect(claims.role).toBe('viewer'); + await api() .post('/api/services') .set('Authorization', `Bearer ${token}`) From bad153f8b9537234eb93061f958832a3a9e15257 Mon Sep 17 00:00:00 2001 From: Nida Ali Date: Mon, 31 Aug 2026 00:21:20 -0400 Subject: [PATCH 3/3] test: make the conflict tests provoke a reachable violation The create conflict tests passed a payload with no versions, so the 23505 they mocked could not have occurred for that input -- service names carry no unique constraint. The payload now repeats a version name, which is the only unique constraint a create can trip, and the message is asserted so it stays pointed at versions. --- src/services/services.service.spec.ts | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/services/services.service.spec.ts b/src/services/services.service.spec.ts index acc41cb..827eda1 100644 --- a/src/services/services.service.spec.ts +++ b/src/services/services.service.spec.ts @@ -243,17 +243,25 @@ describe('ServicesService', () => { describe('create', () => { it('translates a unique violation into 409 Conflict', async () => { - // Reachable only via duplicate version names in the payload: service - // names carry no unique constraint. + // The only unique constraint a create can trip is (service_id, name) on + // versions -- service names carry none -- so the payload that provokes + // it is one repeating a version name. const uniqueViolation = Object.assign( new QueryFailedError('INSERT', [], new Error('duplicate key')), { code: '23505' }, ); serviceRepo.save.mockRejectedValue(uniqueViolation); - await expect( - service.create({ name: 'Payment Gateway', description: 'x' }), - ).rejects.toBeInstanceOf(ConflictException); + const attempt = service.create({ + name: 'Any Service', + description: 'x', + versions: [{ name: 'v1.0.0' }, { name: 'v1.0.0' }], + }); + + await expect(attempt).rejects.toBeInstanceOf(ConflictException); + await expect(attempt).rejects.toThrow( + 'The submitted versions contain duplicate version names', + ); }); it('rethrows unrelated database errors untouched', async () => { @@ -264,7 +272,11 @@ describe('ServicesService', () => { serviceRepo.save.mockRejectedValue(other); await expect( - service.create({ name: 'Payment Gateway', description: 'x' }), + service.create({ + name: 'Any Service', + description: 'x', + versions: [{ name: 'v1.0.0' }], + }), ).rejects.toBe(other); });