From 944414736c4e8978d0b320414504d286a9d4dc12 Mon Sep 17 00:00:00 2001 From: bobaivigitalpoint-ui Date: Fri, 16 Oct 2026 22:45:17 +0100 Subject: [PATCH] test(users): UsersService unit tests and controller integration tests; structured logging and error handling --- docs/api/users-error-handling.md | 3 + docs/api/users-structured-logging.md | 17 ++ docs/api/users-tests.md | 6 + src/users/users.controller.ts | 7 +- src/users/users.service.spec.ts | 217 ++++++++++++++++++ src/users/users.service.ts | 47 ++-- test/e2e/users-controller.integration.spec.ts | 206 +++++++++++++++++ 7 files changed, 487 insertions(+), 16 deletions(-) create mode 100644 docs/api/users-error-handling.md create mode 100644 docs/api/users-structured-logging.md create mode 100644 docs/api/users-tests.md create mode 100644 src/users/users.service.spec.ts create mode 100644 test/e2e/users-controller.integration.spec.ts diff --git a/docs/api/users-error-handling.md b/docs/api/users-error-handling.md new file mode 100644 index 00000000..80a07f7e --- /dev/null +++ b/docs/api/users-error-handling.md @@ -0,0 +1,3 @@ +# Users API error handling + +`GET /users/:id` returned `200` with an empty body when the id was unknown or the account was deactivated, so clients could not tell "no such user" from success. It now responds `404 User not found`, matching the other lookups (`me/profile`, statistics). Authorization is unchanged: the route is still admin-only, so the response does not widen what a caller can learn. diff --git a/docs/api/users-structured-logging.md b/docs/api/users-structured-logging.md new file mode 100644 index 00000000..668dae71 --- /dev/null +++ b/docs/api/users-structured-logging.md @@ -0,0 +1,17 @@ +# Users API structured logging + +`UsersService` now uses the project's `AppLogger`. Events are logged as a short constant message plus a metadata object with a stable `event` name, so the JSON logs produced in production can be filtered and alerted on: + +| `event` | Level | Fields | +| --- | --- | --- | +| `users.created` | log | `userId`, `emailHash` | +| `users.blocked` | warn | `userId` | +| `users.unblocked` | log | `userId` | +| `users.deleted` | warn | `userId` | +| `users.deactivated` | log | `userId`, `emailHash`, `scheduledDeletionAt` | +| `users.reactivation_token_generated` | log | `userId`, `emailHash` | +| `users.reactivated` | log | `userId`, `emailHash` | +| `users.deactivated_purged` | log | `deletedCount` | +| `users.export_file_deleted` | log | `file` | + +Emails appear only as a short hash. The reactivation-token log line previously contained the raw email address; that is fixed. `AppLogger` also redacts fields such as `password`, `token` and `secret` before writing. diff --git a/docs/api/users-tests.md b/docs/api/users-tests.md new file mode 100644 index 00000000..efa763d4 --- /dev/null +++ b/docs/api/users-tests.md @@ -0,0 +1,6 @@ +# Users API tests + +- **Unit** - `src/users/users.service.spec.ts` covers `UsersService` with a mocked Prisma client: account creation (hashed password, secrets stripped, history/preferences seeded, referral-code collision retry, referrer linking), active-only lookups, block/unblock, verify/unverify, removal, referral stats and login history. +- **Integration** - `test/e2e/users-controller.integration.spec.ts` boots `UsersController` over HTTP with the same `ValidationPipe` settings as `main.ts`, mocked services and role-aware guards: authentication and admin-role enforcement, `GET /users/:id` including the 404 case, body validation for create and update (invalid values, unknown fields), search query coercion and rejection, and the self-service `me/*` routes. + +Run with `npx jest src/users/users.service.spec.ts test/e2e/users-controller.integration.spec.ts`. diff --git a/src/users/users.controller.ts b/src/users/users.controller.ts index ac8beabd..3fa7eecc 100644 --- a/src/users/users.controller.ts +++ b/src/users/users.controller.ts @@ -152,8 +152,11 @@ export class UsersController { @UseGuards(JwtAuthGuard, RolesGuard) @Roles(UserRole.ADMIN) @Get(':id') - findOne(@Param('id') id: string) { - return this.usersService.findOne(id); + async findOne(@Param('id') id: string) { + const user = await this.usersService.findOne(id); + // Previously an unknown or deactivated id produced 200 with an empty body. + if (!user) throw new NotFoundException('User not found'); + return user; } @UseGuards(JwtAuthGuard, RolesGuard) diff --git a/src/users/users.service.spec.ts b/src/users/users.service.spec.ts new file mode 100644 index 00000000..8cff13d6 --- /dev/null +++ b/src/users/users.service.spec.ts @@ -0,0 +1,217 @@ +import { PrismaService } from '../database/prisma.service'; +import { SessionsService } from '../sessions/sessions.service'; +import { AppLogger } from '../common/logger'; +import { UsersService } from './users.service'; + +jest.mock('../auth/security.utils', () => ({ + ...jest.requireActual('../auth/security.utils'), + // bcrypt at 12 rounds is slow and irrelevant to what is being tested. + hashPassword: jest.fn(async (password: string) => `hashed:${password}`), +})); + +type Mock = jest.Mock; + +function makePrisma() { + return { + user: { + findUnique: jest.fn(), + findFirst: jest.fn(), + findMany: jest.fn(), + create: jest.fn(), + update: jest.fn(), + delete: jest.fn(), + count: jest.fn(), + }, + loginHistory: { findMany: jest.fn() }, + } as unknown as { user: Record; loginHistory: Record }; +} + +describe('UsersService', () => { + let prisma: ReturnType; + let service: UsersService; + + beforeEach(() => { + jest.spyOn(AppLogger.prototype, 'log').mockImplementation(() => undefined); + jest.spyOn(AppLogger.prototype, 'warn').mockImplementation(() => undefined); + prisma = makePrisma(); + service = new UsersService( + prisma as unknown as PrismaService, + { revokeAllSessions: jest.fn() } as unknown as SessionsService, + ); + }); + + afterEach(() => jest.restoreAllMocks()); + + describe('create', () => { + const dto = { + email: 'new@example.com', + password: 'Str0ng!Pass', + firstName: 'Ada', + lastName: 'Lovelace', + }; + + beforeEach(() => { + prisma.user.findUnique.mockResolvedValue(null); // referral code is free, no referrer + prisma.user.create.mockImplementation(async ({ data }: { data: Record }) => ({ + id: 'u1', + ...data, + twoFactorSecret: 'secret', + })); + }); + + it('stores a hash, never the plain password, and strips secrets from the result', async () => { + const result = await service.create(dto); + + const data = prisma.user.create.mock.calls[0][0].data; + expect(data.password).toBe('hashed:Str0ng!Pass'); + expect(result).not.toHaveProperty('password'); + expect(result).not.toHaveProperty('twoFactorSecret'); + expect(result).toMatchObject({ id: 'u1', email: 'new@example.com' }); + }); + + it('seeds password history and default notification preferences', async () => { + await service.create(dto); + + const data = prisma.user.create.mock.calls[0][0].data; + expect(data.passwordHistory).toEqual({ create: { passwordHash: 'hashed:Str0ng!Pass' } }); + expect(data.preferences.create).toMatchObject({ + emailNotifications: true, + inAppNotifications: true, + smsNotifications: false, + }); + }); + + it('generates a 6-character upper-case referral code', async () => { + await service.create(dto); + expect(prisma.user.create.mock.calls[0][0].data.referralCode).toMatch(/^[0-9A-Z]{1,6}$/); + }); + + it('retries until the generated referral code is unused', async () => { + prisma.user.findUnique + .mockResolvedValueOnce({ id: 'taken' }) // first code collides + .mockResolvedValueOnce(null); // second is free + + await service.create(dto); + + expect(prisma.user.findUnique).toHaveBeenCalledTimes(2); + }); + + it('links the new user to the referrer when a valid referral code is given', async () => { + prisma.user.findUnique + .mockResolvedValueOnce(null) // generated code is free + .mockResolvedValueOnce({ id: 'referrer-1' }); // supplied code resolves + + await service.create({ ...dto, referralCode: 'FRIEND' }); + + expect(prisma.user.findUnique).toHaveBeenLastCalledWith({ where: { referralCode: 'FRIEND' } }); + expect(prisma.user.create.mock.calls[0][0].data.referredById).toBe('referrer-1'); + }); + + it('ignores an unknown referral code', async () => { + await service.create({ ...dto, referralCode: 'NOPE' }); + expect(prisma.user.create.mock.calls[0][0].data.referredById).toBeNull(); + }); + }); + + describe('lookups', () => { + it('findOne only returns active users and selects public fields', async () => { + prisma.user.findFirst.mockResolvedValue({ id: 'u1' }); + + await expect(service.findOne('u1')).resolves.toEqual({ id: 'u1' }); + + const args = prisma.user.findFirst.mock.calls[0][0]; + expect(args.where).toEqual({ id: 'u1', isDeactivated: false }); + expect(args.select).not.toHaveProperty('password'); + expect(args.select).toMatchObject({ email: true, role: true }); + }); + + it('findOne returns null for an unknown or deactivated user', async () => { + prisma.user.findFirst.mockResolvedValue(null); + await expect(service.findOne('missing')).resolves.toBeNull(); + }); + + it('findByEmail looks up by exact email', async () => { + prisma.user.findUnique.mockResolvedValue({ id: 'u1' }); + await service.findByEmail('a@example.com'); + expect(prisma.user.findUnique).toHaveBeenCalledWith({ where: { email: 'a@example.com' } }); + }); + }); + + describe('admin state changes', () => { + beforeEach(() => { + prisma.user.update.mockImplementation(async ({ data }: { data: Record }) => data); + }); + + it('block / unblock toggle isBlocked', async () => { + await service.block('u1'); + await service.unblock('u1'); + + expect(prisma.user.update.mock.calls[0][0]).toMatchObject({ + where: { id: 'u1' }, + data: { isBlocked: true }, + }); + expect(prisma.user.update.mock.calls[1][0]).toMatchObject({ data: { isBlocked: false } }); + }); + + it('verify / unverify toggle isVerified and select only id and isVerified', async () => { + await service.verify('u1'); + await service.unverify('u1'); + + expect(prisma.user.update.mock.calls[0][0]).toMatchObject({ + data: { isVerified: true }, + select: { id: true, isVerified: true }, + }); + expect(prisma.user.update.mock.calls[1][0]).toMatchObject({ data: { isVerified: false } }); + }); + + it('remove deletes the user', async () => { + prisma.user.delete.mockResolvedValue({ id: 'u1' }); + await expect(service.remove('u1')).resolves.toEqual({ id: 'u1' }); + expect(prisma.user.delete).toHaveBeenCalledWith({ where: { id: 'u1' } }); + }); + }); + + describe('referrals', () => { + it('getReferralStats combines the referral code with the referral count', async () => { + prisma.user.count.mockResolvedValue(3); + prisma.user.findUnique.mockResolvedValue({ referralCode: 'ABC123' }); + + await expect(service.getReferralStats('u1')).resolves.toEqual({ + referralCode: 'ABC123', + referralCount: 3, + }); + expect(prisma.user.count).toHaveBeenCalledWith({ where: { referredById: 'u1' } }); + }); + + it('getReferralStats tolerates a missing user', async () => { + prisma.user.count.mockResolvedValue(0); + prisma.user.findUnique.mockResolvedValue(null); + await expect(service.getReferralStats('ghost')).resolves.toEqual({ + referralCode: undefined, + referralCount: 0, + }); + }); + + it('getMyReferrals selects only non-sensitive fields', async () => { + prisma.user.findMany.mockResolvedValue([]); + await service.getMyReferrals('u1'); + + const args = prisma.user.findMany.mock.calls[0][0]; + expect(args.where).toEqual({ referredById: 'u1' }); + expect(Object.keys(args.select).sort()).toEqual(['createdAt', 'email', 'firstName', 'id', 'lastName']); + }); + }); + + describe('getLoginHistory', () => { + it('returns the caller\'s logins newest first without extra fields', async () => { + prisma.loginHistory.findMany.mockResolvedValue([{ timestamp: new Date(), ipAddress: '1.1.1.1' }]); + + await service.getLoginHistory('u1'); + + const args = prisma.loginHistory.findMany.mock.calls[0][0]; + expect(args.where).toEqual({ userId: 'u1' }); + expect(args.orderBy).toEqual({ timestamp: 'desc' }); + expect(args.select).toEqual({ timestamp: true, ipAddress: true, userAgent: true }); + }); + }); +}); diff --git a/src/users/users.service.ts b/src/users/users.service.ts index d41d7f1d..e2f13ee3 100644 --- a/src/users/users.service.ts +++ b/src/users/users.service.ts @@ -1,10 +1,10 @@ import { Injectable, - Logger, OnModuleInit, NotFoundException, BadRequestException, } from '@nestjs/common'; +import { AppLogger } from '../common/logger'; import * as crypto from 'crypto'; import * as fs from 'fs'; import * as path from 'path'; @@ -27,7 +27,7 @@ import { userRegistrationsTotal } from '../metrics/metrics.controller'; @Injectable() export class UsersService implements OnModuleInit { - private readonly logger = new Logger(UsersService.name); + private readonly logger = new AppLogger(UsersService.name); constructor( private readonly prisma: PrismaService, @@ -204,7 +204,7 @@ export class UsersService implements OnModuleInit { if (now - stats.mtimeMs > expirationTime) { fs.unlinkSync(filepath); - this.logger.log(`Deleted expired export file: ${file}`); + this.logger.log('Deleted expired export file', { event: 'users.export_file_deleted', file }); } }); } @@ -295,6 +295,7 @@ export class UsersService implements OnModuleInit { }); userRegistrationsTotal.inc({ method: 'email' }); + this.logger.log('User created', { event: 'users.created', userId: user.id, emailHash: redactEmail(user.email) }); return sanitizeUser(user); } @@ -403,7 +404,7 @@ export class UsersService implements OnModuleInit { } async block(id: string) { - return this.prisma.user.update({ + const blocked = await this.prisma.user.update({ where: { id }, data: { isBlocked: true }, select: { @@ -412,10 +413,12 @@ export class UsersService implements OnModuleInit { isBlocked: true, }, }); + this.logger.warn('User blocked', { event: 'users.blocked', userId: id }); + return blocked; } async unblock(id: string) { - return this.prisma.user.update({ + const unblocked = await this.prisma.user.update({ where: { id }, data: { isBlocked: false }, select: { @@ -424,12 +427,16 @@ export class UsersService implements OnModuleInit { isBlocked: true, }, }); + this.logger.log('User unblocked', { event: 'users.unblocked', userId: id }); + return unblocked; } async remove(id: string) { - return this.prisma.user.delete({ + const removed = await this.prisma.user.delete({ where: { id }, }); + this.logger.warn('User deleted', { event: 'users.deleted', userId: id }); + return removed; } async exportPersonalData(id: string) { @@ -532,11 +539,12 @@ export class UsersService implements OnModuleInit { }, }); - this.logger.log( - `User ${userId} (${redactEmail(user.email)}) deactivated. Scheduled deletion: ${ - scheduledDeletionAt ? scheduledDeletionAt.toISOString() : 'None' - }`, - ); + this.logger.log('User deactivated', { + event: 'users.deactivated', + userId, + emailHash: redactEmail(user.email), + scheduledDeletionAt: scheduledDeletionAt ? scheduledDeletionAt.toISOString() : null, + }); await this.sessionsService.revokeAllSessions(userId); @@ -567,7 +575,11 @@ export class UsersService implements OnModuleInit { }, }); - this.logger.log(`Reactivation token generated for user ${userId} (${user.email})`); + this.logger.log('Reactivation token generated', { + event: 'users.reactivation_token_generated', + userId, + emailHash: redactEmail(user.email), + }); return { message: 'Reactivation token generated. Use the token to reactivate your account.', @@ -647,7 +659,11 @@ export class UsersService implements OnModuleInit { }, }); - this.logger.log(`User ${userId} (${redactEmail(user.email)}) reactivated`); + this.logger.log('User reactivated', { + event: 'users.reactivated', + userId, + emailHash: redactEmail(user.email), + }); return updatedUser; } @@ -837,7 +853,10 @@ export class UsersService implements OnModuleInit { }); }); - this.logger.log(`Deleted ${result.count} deactivated users`); + this.logger.log('Deleted deactivated users', { + event: 'users.deactivated_purged', + deletedCount: result.count, + }); return { deletedCount: result.count }; } diff --git a/test/e2e/users-controller.integration.spec.ts b/test/e2e/users-controller.integration.spec.ts new file mode 100644 index 00000000..19322b5e --- /dev/null +++ b/test/e2e/users-controller.integration.spec.ts @@ -0,0 +1,206 @@ +/** + * Integration tests: UsersController over HTTP. + * + * Real controller, Nest HTTP stack and the same global ValidationPipe settings as + * main.ts, with mocked services and a role-aware test guard. Covers status codes, + * validation, role enforcement and argument passing without a database. + */ + +import { + CanActivate, + ExecutionContext, + INestApplication, + Injectable, + ValidationPipe, +} from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import * as request from 'supertest'; +import { UsersController } from '../../src/users/users.controller'; +import { UsersService } from '../../src/users/users.service'; +import { ActivityLogService } from '../../src/users/activity-log.service'; +import { AccountDeletionService } from '../../src/users/account-deletion.service'; +import { DataExportService } from '../../src/users/data-export.service'; +import { JwtAuthGuard } from '../../src/auth/guards/jwt-auth.guard'; +import { RolesGuard } from '../../src/auth/guards/roles.guard'; + +/** "Authorization: Bearer role:ADMIN" / "role:USER"; anything else is rejected. */ +@Injectable() +class RoleTestGuard implements CanActivate { + canActivate(ctx: ExecutionContext): boolean { + const req = ctx.switchToHttp().getRequest(); + const header: string | undefined = req.headers['authorization']; + if (!header?.startsWith('Bearer role:')) return false; + const role = header.slice('Bearer role:'.length); + req.authUser = { sub: `${role.toLowerCase()}-1`, email: `${role}@example.com`, role, type: 'access' }; + return true; + } +} + +/** Admin-only routes: allow only the ADMIN role (stands in for RolesGuard + @Roles). */ +@Injectable() +class AdminOnlyGuard implements CanActivate { + canActivate(ctx: ExecutionContext): boolean { + return ctx.switchToHttp().getRequest().authUser?.role === 'ADMIN'; + } +} + +describe('UsersController (integration)', () => { + let app: INestApplication; + const usersService = { + create: jest.fn(), + findAll: jest.fn(), + findOne: jest.fn(), + update: jest.fn(), + search: jest.fn(), + getUserStatistics: jest.fn(), + getLoginHistory: jest.fn(), + }; + + const asAdmin = (r: request.Test) => r.set('Authorization', 'Bearer role:ADMIN'); + const asUser = (r: request.Test) => r.set('Authorization', 'Bearer role:USER'); + + beforeAll(async () => { + const moduleRef = await Test.createTestingModule({ + controllers: [UsersController], + providers: [ + { provide: UsersService, useValue: usersService }, + { provide: ActivityLogService, useValue: {} }, + { provide: AccountDeletionService, useValue: {} }, + { provide: DataExportService, useValue: {} }, + ], + }) + .overrideGuard(JwtAuthGuard) + .useClass(RoleTestGuard) + .overrideGuard(RolesGuard) + .useClass(AdminOnlyGuard) + .compile(); + + app = moduleRef.createNestApplication(); + // Same settings as src/main.ts. + app.useGlobalPipes( + new ValidationPipe({ whitelist: true, transform: true, forbidNonWhitelisted: true }), + ); + await app.init(); + }); + + afterAll(async () => { + await app.close(); + }); + + beforeEach(() => { + jest.resetAllMocks(); + }); + + describe('authentication and roles', () => { + it('rejects unauthenticated requests', async () => { + await request(app.getHttpServer()).get('/users').expect(403); + expect(usersService.findAll).not.toHaveBeenCalled(); + }); + + it('rejects a USER from admin routes', async () => { + await asUser(request(app.getHttpServer()).get('/users')).expect(403); + expect(usersService.findAll).not.toHaveBeenCalled(); + }); + + it('lets an ADMIN list users', async () => { + usersService.findAll.mockResolvedValue([{ id: 'u1' }]); + const res = await asAdmin(request(app.getHttpServer()).get('/users')).expect(200); + expect(res.body).toEqual([{ id: 'u1' }]); + }); + }); + + describe('GET /users/:id', () => { + it('returns the user', async () => { + usersService.findOne.mockResolvedValue({ id: 'u1', email: 'a@example.com' }); + const res = await asAdmin(request(app.getHttpServer()).get('/users/u1')).expect(200); + expect(res.body).toEqual({ id: 'u1', email: 'a@example.com' }); + expect(usersService.findOne).toHaveBeenCalledWith('u1'); + }); + + it('answers 404 instead of an empty 200 when the user does not exist', async () => { + usersService.findOne.mockResolvedValue(null); + await asAdmin(request(app.getHttpServer()).get('/users/missing')).expect(404); + }); + }); + + describe('POST /users', () => { + const valid = { + email: 'new@example.com', + password: 'Str0ng!Pass', + firstName: 'Ada', + lastName: 'Lovelace', + }; + + it('creates a user for an ADMIN', async () => { + usersService.create.mockResolvedValue({ id: 'u2', email: valid.email }); + await asAdmin(request(app.getHttpServer()).post('/users').send(valid)).expect(201); + expect(usersService.create).toHaveBeenCalledWith(expect.objectContaining({ email: valid.email })); + }); + + it('rejects an invalid body with 400', async () => { + await asAdmin( + request(app.getHttpServer()).post('/users').send({ ...valid, email: 'nope', password: 'short' }), + ).expect(400); + expect(usersService.create).not.toHaveBeenCalled(); + }); + + it('rejects unknown fields with 400 (forbidNonWhitelisted)', async () => { + await asAdmin(request(app.getHttpServer()).post('/users').send({ ...valid, role: 'ADMIN' })).expect(400); + expect(usersService.create).not.toHaveBeenCalled(); + }); + + it('is forbidden for a USER', async () => { + await asUser(request(app.getHttpServer()).post('/users').send(valid)).expect(403); + }); + }); + + describe('GET /users/search', () => { + it('coerces page and limit to numbers', async () => { + usersService.search.mockResolvedValue({ data: [], meta: {} }); + await asAdmin(request(app.getHttpServer()).get('/users/search?q=ada&page=2&limit=5')).expect(200); + expect(usersService.search).toHaveBeenCalledWith( + expect.objectContaining({ q: 'ada', page: 2, limit: 5 }), + ); + }); + + it('rejects a non-numeric page with 400', async () => { + await asAdmin(request(app.getHttpServer()).get('/users/search?page=abc')).expect(400); + expect(usersService.search).not.toHaveBeenCalled(); + }); + + it('rejects page 0 with 400', async () => { + await asAdmin(request(app.getHttpServer()).get('/users/search?page=0')).expect(400); + }); + }); + + describe('PUT /users/:id', () => { + it('updates with a valid partial body', async () => { + usersService.update.mockResolvedValue({ id: 'u1', firstName: 'Grace' }); + await asAdmin(request(app.getHttpServer()).put('/users/u1').send({ firstName: 'Grace' })).expect(200); + expect(usersService.update).toHaveBeenCalledWith('u1', expect.objectContaining({ firstName: 'Grace' })); + }); + + it('rejects an invalid email with 400', async () => { + await asAdmin(request(app.getHttpServer()).put('/users/u1').send({ email: 'bad' })).expect(400); + expect(usersService.update).not.toHaveBeenCalled(); + }); + }); + + describe('self-service routes', () => { + it('GET /users/me/statistics uses the caller id', async () => { + usersService.getUserStatistics.mockResolvedValue({ propertiesCount: 2 }); + await asUser(request(app.getHttpServer()).get('/users/me/statistics')).expect(200); + expect(usersService.getUserStatistics).toHaveBeenCalledWith('user-1'); + }); + + it('GET /users/me/login-history uses the caller id', async () => { + usersService.getLoginHistory.mockResolvedValue([]); + await asUser(request(app.getHttpServer()).get('/users/me/login-history')).expect(200); + expect(usersService.getLoginHistory).toHaveBeenCalledWith('user-1'); + }); + + it('self-service routes require authentication', async () => { + await request(app.getHttpServer()).get('/users/me/statistics').expect(403); + }); + }); +});