From e6acfdcac736a0f2e9ac65b6b6604b6b57028dfc Mon Sep 17 00:00:00 2001 From: Bastian Wagner Date: Fri, 31 Jul 2026 23:37:54 +0200 Subject: [PATCH] feat: secure admin user management --- .../admin-user-management/task-2-report.md | 122 ++++++ .../src/auth/auth.controller.spec.ts | 18 + .../src/auth/auth.controller.ts | 11 +- .../src/auth/auth.service.spec.ts | 170 ++++++++ myteamwallet_backend/src/auth/auth.service.ts | 120 +++--- .../src/auth/strategies/jwt.strategy.spec.ts | 67 +++ .../src/auth/strategies/jwt.strategy.ts | 15 +- .../database/logging/logging.service.spec.ts | 29 +- .../src/database/logging/logging.service.ts | 25 +- .../logging/model/logging-event.type.ts | 5 + .../1785517200000-AddPlayerLookupIndexes.ts | 19 + .../migrations/AddPlayerLookupIndexes.spec.ts | 33 ++ .../src/players/entities/player.entity.ts | 7 + .../src/users/admin-users.controller.spec.ts | 118 ++++++ .../src/users/admin-users.controller.ts | 102 +++++ .../src/users/admin-users.service.spec.ts | 330 +++++++++++++++ .../src/users/admin-users.service.ts | 381 ++++++++++++++++++ .../users/dto/admin-player-response.dto.ts | 31 ++ .../src/users/dto/admin-user.dto.ts | 67 +++ .../users/users.controller.security.spec.ts | 11 + .../src/users/users.controller.ts | 56 --- .../src/users/users.module.ts | 9 +- 22 files changed, 1598 insertions(+), 148 deletions(-) create mode 100644 .superpowers/sdd/admin-user-management/task-2-report.md create mode 100644 myteamwallet_backend/src/auth/auth.controller.spec.ts create mode 100644 myteamwallet_backend/src/auth/auth.service.spec.ts create mode 100644 myteamwallet_backend/src/auth/strategies/jwt.strategy.spec.ts create mode 100644 myteamwallet_backend/src/database/migrations/1785517200000-AddPlayerLookupIndexes.ts create mode 100644 myteamwallet_backend/src/database/migrations/AddPlayerLookupIndexes.spec.ts create mode 100644 myteamwallet_backend/src/users/admin-users.controller.spec.ts create mode 100644 myteamwallet_backend/src/users/admin-users.controller.ts create mode 100644 myteamwallet_backend/src/users/admin-users.service.spec.ts create mode 100644 myteamwallet_backend/src/users/admin-users.service.ts create mode 100644 myteamwallet_backend/src/users/dto/admin-player-response.dto.ts create mode 100644 myteamwallet_backend/src/users/dto/admin-user.dto.ts create mode 100644 myteamwallet_backend/src/users/users.controller.security.spec.ts diff --git a/.superpowers/sdd/admin-user-management/task-2-report.md b/.superpowers/sdd/admin-user-management/task-2-report.md new file mode 100644 index 0000000..15921e6 --- /dev/null +++ b/.superpowers/sdd/admin-user-management/task-2-report.md @@ -0,0 +1,122 @@ +# Task 2 Report: Backend admin mutations and authentication enforcement + +## Status + +Implemented and verified on top of Task 1 commit `4105460`. + +## Delivered API + +- Added versioned global-admin controller at `admin/users` (effective path follows the existing global `/api` prefix and URI versioning). +- Added narrow mutations: + - `PATCH admin/users/:id/profile` (`firstName`, `lastName` only) + - `PATCH admin/users/:id/role` (strict numeric `RoleEnum.admin|user` ID) + - `PATCH admin/users/:id/status` (strict numeric `StatusEnum.active|inactive` ID) + - `PUT admin/users/:userId/players/:playerId` + - `DELETE admin/users/:userId/players/:playerId` +- Added `GET admin/users/players` with search, optional `teamId`, `all|assigned|unassigned`, page, and limit. +- Kept `GET users/directory` as the user list source. Removed superseded generic user create/read/update/delete handlers that exposed unsafe/raw shapes or bypassed the narrow mutation safeguards. +- Removed `DELETE auth/me`, which could race with a promotion and bypass last-admin protection. + +## TDD RED evidence + +The following failures were observed before their production implementations: + +1. `npm test -- --runInBand admin-users.controller.spec.ts` + - Failed to compile because `AdminUsersController` and the narrow DTOs did not exist. +2. `npm test -- --runInBand admin-users.service.spec.ts` + - Failed to compile because `AdminUsersService` did not exist. +3. `npm test -- --runInBand auth.service.spec.ts jwt.strategy.spec.ts` + - Inactive password login produced the ordinary password failure, social login issued a token, logs contained email/token values, and `JwtStrategy` accepted only the JWT snapshot. +4. `npm test -- --runInBand AddPlayerLookupIndexes.spec.ts` + - Failed to compile because the reversible lookup-index migration did not exist. +5. `npm test -- --runInBand auth.service.spec.ts auth.controller.spec.ts` + - `GET auth/me` had no JWT guard and the service accepted/refreshed an inactive user. +6. `npm test -- --runInBand users.controller.security.spec.ts` + - Generic `UsersController` mutations were still present and bypassed the new invariants. +7. `npm test -- --runInBand admin-users.service.spec.ts -t "updates only names"` + - The locked user lookup did not use alias-scoped `FOR UPDATE`, exposing a PostgreSQL outer-join runtime failure. +8. `npm test -- --runInBand admin-users.controller.spec.ts -t "reverse-map"` + - Numeric enum reverse-map names such as `"admin"` passed validation. +9. `npm test -- --runInBand logging.service.spec.ts admin-users.service.spec.ts -t "caller transaction manager|updates only names"` + - Audit logging had no transaction-manager support and ran after commit. +10. `npm test -- --runInBand auth.service.spec.ts admin-users.service.spec.ts -t "serializes email confirmation|explicit paginated player"` + - Email confirmation had no row-lock transaction, and a numeric driver boolean was returned as `1` instead of `true`. + +Every production behavior above was added only after the corresponding expected RED was captured. + +## Final GREEN evidence + +- Focused backend tests: + - Command: `npm test -- --runInBand users.service.spec.ts users.controller.security.spec.ts admin-users.controller.spec.ts admin-users.service.spec.ts auth.controller.spec.ts auth.service.spec.ts jwt.strategy.spec.ts logging.service.spec.ts AddPlayerLookupIndexes.spec.ts` + - Result: **9 suites passed, 40 tests passed, 0 failed**. +- Targeted lint across every touched backend TypeScript file: + - Command: direct project ESLint invocation over 21 touched source/spec files. + - Result: **exit 0, no findings**. +- Backend build: + - Command: `npm run build` + - Result: **exit 0**. +- Migration up/down smoke coverage: + - Exact `CREATE INDEX` and reverse-order `DROP INDEX` SQL asserted in `AddPlayerLookupIndexes.spec.ts`. + - TypeORM entity index metadata asserted to match both migration names. +- Diff checks: + - `git diff --check`: **exit 0**. + - Both frontend directories: **no changes**. + +## Security and concurrency design + +- The controller is class-level protected by JWT auth, `RolesGuard`, and `Roles([RoleEnum.admin])`. +- Mutation DTOs are narrow and whitelisted. Role/status accept only strict numeric IDs, avoiding class-validator numeric-enum reverse-map strings. +- Role and status changes execute in transactions and lock the active-admin set in stable user-ID order. This serializes concurrent demotions/deactivations so the last active admin cannot be lost. +- Self-demotion and self-deactivation are rejected inside the locked transaction. +- User row locks use explicit query builders with `FOR UPDATE OF` the user alias. Role/status are loaded with left joins, preserving support for nullable relations without asking PostgreSQL to lock nullable joined rows. +- Deactivation changes only `User.status` and revokes any outstanding confirmation hash; it does not alter `Player.user`. +- Email confirmation locks the same user row and re-checks the hash inside its transaction. This serializes confirmation against administrative deactivation and prevents an old/racing confirmation link from reactivating a deactivated account. +- Assignment and reassignment lock the player row before changing `Player.user`; unlink verifies the locked row is still linked to the requested user. +- All mutation responses are explicit Task 1-compatible admin summaries. Player search uses its own explicit player/team/current-user projection. Password, hash, social ID, and tokens are never mapped. +- Admin audit events contain actor ID in `userId` and target/action IDs in details. Audit insertion uses the same transaction manager as the mutation, so an audit failure rolls back the security-sensitive change. +- Password and social login reject inactive accounts. `JwtStrategy` reloads the non-deleted database user on every request, rejects inactive/missing users, and returns the current database role/status rather than trusting token role claims. +- `GET auth/me` is JWT guarded and independently checks current active status before any refresh behavior. + +## Files + +### Added + +- `src/users/admin-users.controller.ts` +- `src/users/admin-users.controller.spec.ts` +- `src/users/admin-users.service.ts` +- `src/users/admin-users.service.spec.ts` +- `src/users/users.controller.security.spec.ts` +- `src/users/dto/admin-user.dto.ts` +- `src/users/dto/admin-player-response.dto.ts` +- `src/auth/auth.controller.spec.ts` +- `src/auth/auth.service.spec.ts` +- `src/auth/strategies/jwt.strategy.spec.ts` +- `src/database/migrations/1785517200000-AddPlayerLookupIndexes.ts` +- `src/database/migrations/AddPlayerLookupIndexes.spec.ts` + +### Modified + +- `src/users/users.controller.ts` +- `src/users/users.module.ts` +- `src/auth/auth.controller.ts` +- `src/auth/auth.service.ts` +- `src/auth/strategies/jwt.strategy.ts` +- `src/database/logging/logging.service.ts` +- `src/database/logging/logging.service.spec.ts` +- `src/database/logging/model/logging-event.type.ts` +- `src/players/entities/player.entity.ts` + +## Self-review + +- Checked every endpoint for server-side global-admin authorization and removed legacy mutation bypasses. +- Checked response construction for password/hash/social-ID/token leakage. +- Checked role/status races, lock acquisition order, nullable-relation SQL shape, player reassignment ownership, and confirmation/deactivation ordering. +- Checked all touched logging details for email, password, token, hash, or social-ID values. +- Checked migration names against entity metadata and down ordering. +- Confirmed no frontend changes. + +## Concerns / follow-up + +- The lock/concurrency and migration tests are focused unit/SQL-shape tests; no live PostgreSQL instance was available for a two-connection race test or an actual migration run/revert. A database-backed integration test remains advisable before production rollout. +- Removing superseded generic user CRUD/read routes and `DELETE auth/me` is intentionally security-hardening and may affect undocumented external clients. Repository frontend searches showed no use of those removed routes. +- Full unrelated backend test-suite repair was intentionally out of scope; the focused Task 1 + Task 2 suite and backend build are green. diff --git a/myteamwallet_backend/src/auth/auth.controller.spec.ts b/myteamwallet_backend/src/auth/auth.controller.spec.ts new file mode 100644 index 0000000..db87187 --- /dev/null +++ b/myteamwallet_backend/src/auth/auth.controller.spec.ts @@ -0,0 +1,18 @@ +import { GUARDS_METADATA } from '@nestjs/common/constants'; +import { AuthController } from './auth.controller'; + +describe('AuthController session enforcement', () => { + it('protects GET auth/me with JWT validation', () => { + const guards = Reflect.getMetadata( + GUARDS_METADATA, + AuthController.prototype.me, + ); + + expect(guards).toBeDefined(); + expect(guards).toHaveLength(1); + }); + + it('does not expose self-deletion that can race with an admin promotion', () => { + expect(AuthController.prototype).not.toHaveProperty('delete'); + }); +}); diff --git a/myteamwallet_backend/src/auth/auth.controller.ts b/myteamwallet_backend/src/auth/auth.controller.ts index 0da3c2a..59340c5 100644 --- a/myteamwallet_backend/src/auth/auth.controller.ts +++ b/myteamwallet_backend/src/auth/auth.controller.ts @@ -8,7 +8,6 @@ import { Post, UseGuards, Patch, - Delete, UseInterceptors, ClassSerializerInterceptor, SerializeOptions, @@ -81,7 +80,7 @@ export class AuthController { groups: ['exposeProvider'], }) @Get('me') - // @UseGuards(AuthGuard('jwt')) + @UseGuards(AuthGuard('jwt')) @HttpCode(HttpStatus.OK) public me(@Request() request: Request) { return this.service.me(request.headers['authorization']); @@ -95,14 +94,6 @@ export class AuthController { return this.service.update(request.user, userDto); } - @ApiBearerAuth() - @Delete('me') - @UseGuards(AuthGuard('jwt')) - @HttpCode(HttpStatus.OK) - public async delete(@Request() request) { - return this.service.softDelete(request.user); - } - @ApiOperation({ summary: 'Erstellt Registrierungstoken', description: diff --git a/myteamwallet_backend/src/auth/auth.service.spec.ts b/myteamwallet_backend/src/auth/auth.service.spec.ts new file mode 100644 index 0000000..e6845da --- /dev/null +++ b/myteamwallet_backend/src/auth/auth.service.spec.ts @@ -0,0 +1,170 @@ +import { ForbiddenException } from '@nestjs/common'; +import { AuthProvidersEnum } from './auth-providers.enum'; +import { AuthService } from './auth.service'; +import { RoleEnum } from '../roles/roles.enum'; +import { StatusEnum } from '../statuses/statuses.enum'; + +describe('AuthService inactive-user enforcement and safe logging', () => { + let jwtService: any; + let usersService: any; + let logger: any; + let dataSource: any; + let confirmationUser: any; + let lockedUserQuery: any; + let userRepository: any; + let service: AuthService; + + beforeEach(() => { + jwtService = { + sign: jest.fn(() => 'signed-token'), + verify: jest.fn(), + decode: jest.fn(), + }; + usersService = { + findOne: jest.fn(), + update: jest.fn(), + create: jest.fn(), + }; + logger = { info: jest.fn(), debug: jest.fn() }; + confirmationUser = user(StatusEnum.inactive); + confirmationUser.hash = 'confirmation-hash'; + lockedUserQuery = { + where: jest.fn().mockReturnThis(), + setLock: jest.fn().mockReturnThis(), + getOne: jest.fn(() => confirmationUser), + }; + userRepository = { + createQueryBuilder: jest.fn(() => lockedUserQuery), + save: jest.fn((value) => Promise.resolve(value)), + }; + const manager = { getRepository: jest.fn(() => userRepository) }; + dataSource = { + transaction: jest.fn((work) => work(manager)), + }; + service = new AuthService( + jwtService, + usersService, + {} as any, + {} as any, + logger, + dataSource, + ); + }); + + it('rejects password login for an inactive user before issuing a token', async () => { + usersService.findOne.mockResolvedValue(user(StatusEnum.inactive)); + + await expect( + service.validateLogin({ + email: 'inactive@example.com', + password: 'password', + }), + ).rejects.toBeInstanceOf(ForbiddenException); + + expect(jwtService.sign).not.toHaveBeenCalled(); + expect(logger.info).toHaveBeenCalledWith({ + event: 'user_login_fail', + details: 'userId=2 reason=inactive', + userId: 2, + }); + }); + + it('rejects social login when the existing account is inactive', async () => { + const inactive = user(StatusEnum.inactive); + usersService.findOne + .mockResolvedValueOnce(inactive) + .mockResolvedValueOnce(undefined); + + await expect( + service.validateSocialLogin(AuthProvidersEnum.google, { + id: 'social-id', + email: inactive.email, + firstName: 'Inactive', + lastName: 'User', + }), + ).rejects.toBeInstanceOf(ForbiddenException); + + expect(usersService.update).not.toHaveBeenCalled(); + expect(jwtService.sign).not.toHaveBeenCalled(); + }); + + it('never includes an email in an unknown-user login audit event', async () => { + usersService.findOne.mockResolvedValue(undefined); + + await expect( + service.validateLogin({ + email: 'secret@example.com', + password: 'secret-password', + }), + ).rejects.toBeDefined(); + + expect(logger.info).toHaveBeenCalledWith({ + event: 'user_login_fail', + details: 'reason=user_not_found', + userId: -1, + }); + expect(JSON.stringify(logger.info.mock.calls)).not.toContain( + 'secret@example.com', + ); + expect(JSON.stringify(logger.info.mock.calls)).not.toContain( + 'secret-password', + ); + }); + + it('never includes a rejected invite token in logging details', async () => { + jwtService.verify.mockImplementation(() => { + throw new Error('invalid'); + }); + + await expect( + service.getTeamFromInvite('secret-token'), + ).rejects.toBeDefined(); + + expect(logger.info).toHaveBeenCalledWith({ + event: 'user_invite_link_validate_fail', + details: 'invitation validation failed', + userId: 0, + }); + expect(JSON.stringify(logger.info.mock.calls)).not.toContain( + 'secret-token', + ); + }); + + it('does not refresh an inactive user through the me endpoint flow', async () => { + jwtService.verify.mockReturnValue({ id: 2 }); + usersService.findOne.mockResolvedValue(user(StatusEnum.inactive)); + + await expect(service.me('Bearer existing-token')).rejects.toBeInstanceOf( + ForbiddenException, + ); + expect(jwtService.sign).not.toHaveBeenCalled(); + }); + + it('serializes email confirmation on the user row and consumes the hash', async () => { + await service.confirmEmail('confirmation-hash'); + + expect(dataSource.transaction).toHaveBeenCalledTimes(1); + expect(lockedUserQuery.setLock).toHaveBeenCalledWith( + 'pessimistic_write', + undefined, + ['user'], + ); + expect(confirmationUser.status).toEqual({ id: StatusEnum.active }); + expect(confirmationUser.hash).toBeNull(); + expect(userRepository.save).toHaveBeenCalledWith(confirmationUser); + }); + + function user(statusId: StatusEnum) { + return { + id: 2, + email: 'inactive@example.com', + password: 'password-hash', + provider: AuthProvidersEnum.email, + role: { id: RoleEnum.user, name: 'User' }, + status: { + id: statusId, + name: statusId === StatusEnum.active ? 'Active' : 'Inactive', + }, + }; + } +}); diff --git a/myteamwallet_backend/src/auth/auth.service.ts b/myteamwallet_backend/src/auth/auth.service.ts index 4a7ccec..512e125 100644 --- a/myteamwallet_backend/src/auth/auth.service.ts +++ b/myteamwallet_backend/src/auth/auth.service.ts @@ -1,4 +1,10 @@ -import { HttpException, HttpStatus, Injectable } from '@nestjs/common'; +import { + ForbiddenException, + HttpException, + HttpStatus, + Injectable, + UnauthorizedException, +} from '@nestjs/common'; import { JwtService } from '@nestjs/jwt'; import { User } from '../users/entities/user.entity'; import * as bcrypt from 'bcryptjs'; @@ -19,6 +25,7 @@ import { ForgotService } from 'src/forgot/forgot.service'; import { MailService } from 'src/mail/mail.service'; import { CreateInviteDTO } from './dto/create-invite.dto'; import { LoggingService } from 'src/database/logging/logging.service'; +import { DataSource } from 'typeorm'; @Injectable() export class AuthService { @@ -28,6 +35,7 @@ export class AuthService { private forgotService: ForgotService, private mailService: MailService, private logger: LoggingService, + private dataSource: DataSource, ) {} async validateLogin( @@ -40,7 +48,7 @@ export class AuthService { if (!user) { await this.logger.info({ event: 'user_login_fail', - details: `mail not found: ${loginDto.email}`, + details: 'reason=user_not_found', userId: -1, }); throw new HttpException( @@ -54,6 +62,8 @@ export class AuthService { ); } + await this.assertActiveUser(user); + if (user.provider !== AuthProvidersEnum.email) { throw new HttpException( { @@ -79,7 +89,7 @@ export class AuthService { await this.logger.info({ event: 'user_login_success', - details: `logged in: ${loginDto.email}`, + details: `userId=${user.id}`, userId: user.id, }); @@ -87,7 +97,7 @@ export class AuthService { } else { await this.logger.info({ event: 'user_login_fail', - details: `incorrect password for user: ${loginDto.email}`, + details: `userId=${user.id} reason=incorrect_password`, userId: user.id, }); @@ -120,12 +130,14 @@ export class AuthService { }); if (user) { + await this.assertActiveUser(user); if (socialEmail && !userByEmail) { user.email = socialEmail; } await this.usersService.update(user.id, user); } else if (userByEmail) { user = userByEmail; + await this.assertActiveUser(user); } else { const role = plainToClass(Role, { id: RoleEnum.user, @@ -184,7 +196,7 @@ export class AuthService { await this.logger.info({ event: 'user_create', - details: `user created with mail: ${dto.email}`, + details: `userId=${user.id}`, userId: user.id, }); @@ -197,25 +209,26 @@ export class AuthService { } async confirmEmail(hash: string): Promise { - const user = await this.usersService.findOne({ - hash, + await this.dataSource.transaction(async (manager) => { + const repository = manager.getRepository(User); + const user = await repository + .createQueryBuilder('user') + .where('user.hash = :hash', { hash }) + .setLock('pessimistic_write', undefined, ['user']) + .getOne(); + if (!user) { + throw new HttpException( + { + status: HttpStatus.NOT_FOUND, + error: `notFound`, + }, + HttpStatus.NOT_FOUND, + ); + } + user.hash = null; + user.status = plainToClass(Status, { id: StatusEnum.active }); + await repository.save(user); }); - - if (!user) { - throw new HttpException( - { - status: HttpStatus.NOT_FOUND, - error: `notFound`, - }, - HttpStatus.NOT_FOUND, - ); - } - - user.hash = null; - user.status = plainToClass(Status, { - id: StatusEnum.active, - }); - await user.save(); } async forgotPassword(email: string): Promise { @@ -279,40 +292,31 @@ export class AuthService { async me(token: string): Promise { token = token.replace('Bearer ', ''); - let role: any; + let payload: any; + let refreshToken = false; try { - role = this.jwtService.verify(token); - - const u = await this.usersService.findOne({ - id: role.id, - }); - - await this.logger.debug({ - event: 'user_token_verification_success', - details: `Email: ${u.email}`, - userId: u.id, - }); - - return u; - } catch (error) { - const role = this.jwtService.decode(token); - - const user = await this.usersService.findOne({ - id: (role as any).id, - }); + payload = this.jwtService.verify(token); + } catch { + payload = this.jwtService.decode(token); + refreshToken = true; + } + if (!payload?.id) throw new UnauthorizedException(); + const user = await this.usersService.findOne({ id: payload.id }); + if (!user) throw new UnauthorizedException(); + await this.assertActiveUser(user); + if (refreshToken) { const t = await this.jwtService.sign({ id: user.id, role: user.role, }); - user['token'] = t; - await this.logger.debug({ - event: 'user_token_verification_success', - details: `Email: ${user.email}`, - userId: user.id, - }); - return user; } + await this.logger.debug({ + event: 'user_token_verification_success', + details: `userId=${user.id}`, + userId: user.id, + }); + return user; } async update(user: User, userDto: AuthUpdateDto): Promise { @@ -358,10 +362,6 @@ export class AuthService { }); } - async softDelete(user: User): Promise { - await this.usersService.softDelete(user.id); - } - async createTeamInvite(object: CreateInviteDTO) { const token = await this.jwtService.sign(object, { expiresIn: '30d', @@ -392,7 +392,7 @@ export class AuthService { } catch { await this.logger.info({ event: 'user_invite_link_validate_fail', - details: `validation failed for token ${token}`, + details: 'invitation validation failed', userId: 0, }); @@ -402,4 +402,14 @@ export class AuthService { ); } } + + private async assertActiveUser(user: User): Promise { + if (user.status?.id === StatusEnum.active) return; + await this.logger.info({ + event: 'user_login_fail', + details: `userId=${user.id} reason=inactive`, + userId: user.id, + }); + throw new ForbiddenException('User account is inactive'); + } } diff --git a/myteamwallet_backend/src/auth/strategies/jwt.strategy.spec.ts b/myteamwallet_backend/src/auth/strategies/jwt.strategy.spec.ts new file mode 100644 index 0000000..980050e --- /dev/null +++ b/myteamwallet_backend/src/auth/strategies/jwt.strategy.spec.ts @@ -0,0 +1,67 @@ +import { UnauthorizedException } from '@nestjs/common'; +import { RoleEnum } from '../../roles/roles.enum'; +import { StatusEnum } from '../../statuses/statuses.enum'; +import { JwtStrategy } from './jwt.strategy'; + +describe('JwtStrategy', () => { + const jwtService = {} as any; + const configService = { get: jest.fn(() => 'secret') } as any; + let usersService: any; + let strategy: JwtStrategy; + + beforeEach(() => { + usersService = { findOne: jest.fn() }; + strategy = new JwtStrategy(jwtService, configService, usersService); + }); + + it('reloads the current database user and replaces a stale token role', async () => { + usersService.findOne.mockResolvedValue({ + id: 2, + role: { id: RoleEnum.user, name: 'User' }, + status: { id: StatusEnum.active, name: 'Active' }, + password: 'must-not-be-exposed', + }); + + const result = await strategy.validate({ + id: 2, + role: { id: RoleEnum.admin }, + iat: 1, + exp: 2, + } as any); + + expect(usersService.findOne).toHaveBeenCalledWith({ id: 2 }); + expect(result).toEqual({ + id: 2, + role: { id: RoleEnum.user, name: 'User' }, + status: { id: StatusEnum.active, name: 'Active' }, + }); + expect(result).not.toHaveProperty('password'); + }); + + it('rejects a missing or soft-deleted database user', async () => { + usersService.findOne.mockResolvedValue(undefined); + + await expect( + strategy.validate({ id: 2, iat: 1, exp: 2 } as any), + ).rejects.toBeInstanceOf(UnauthorizedException); + }); + + it('rejects an inactive current database user', async () => { + usersService.findOne.mockResolvedValue({ + id: 2, + role: { id: RoleEnum.user }, + status: { id: StatusEnum.inactive }, + }); + + await expect( + strategy.validate({ id: 2, iat: 1, exp: 2 } as any), + ).rejects.toBeInstanceOf(UnauthorizedException); + }); + + it('rejects a payload without an id without querying the database', async () => { + await expect( + strategy.validate({ iat: 1, exp: 2 } as any), + ).rejects.toBeInstanceOf(UnauthorizedException); + expect(usersService.findOne).not.toHaveBeenCalled(); + }); +}); diff --git a/myteamwallet_backend/src/auth/strategies/jwt.strategy.ts b/myteamwallet_backend/src/auth/strategies/jwt.strategy.ts index 26a5c96..84a1a8a 100644 --- a/myteamwallet_backend/src/auth/strategies/jwt.strategy.ts +++ b/myteamwallet_backend/src/auth/strategies/jwt.strategy.ts @@ -4,6 +4,8 @@ import { JwtService } from '@nestjs/jwt'; import { PassportStrategy } from '@nestjs/passport'; import { User } from '../../users/entities/user.entity'; import { ConfigService } from '@nestjs/config'; +import { UsersService } from '../../users/users.service'; +import { StatusEnum } from '../../statuses/statuses.enum'; type JwtPayload = Pick & { iat: number; exp: number }; @@ -12,6 +14,7 @@ export class JwtStrategy extends PassportStrategy(Strategy) { constructor( private jwtService: JwtService, private configService: ConfigService, + private usersService: UsersService, ) { super({ jwtFromRequest: ExtractJwt.fromAuthHeaderAsBearerToken(), @@ -19,10 +22,18 @@ export class JwtStrategy extends PassportStrategy(Strategy) { }); } - public validate(payload: JwtPayload) { + public async validate(payload: JwtPayload) { if (!payload.id) { throw new UnauthorizedException(); } - return payload; + const user = await this.usersService.findOne({ id: payload.id }); + if (!user || user.status?.id !== StatusEnum.active) { + throw new UnauthorizedException(); + } + return { + id: user.id, + role: user.role ?? null, + status: user.status ?? null, + }; } } diff --git a/myteamwallet_backend/src/database/logging/logging.service.spec.ts b/myteamwallet_backend/src/database/logging/logging.service.spec.ts index 35b8391..54ef725 100644 --- a/myteamwallet_backend/src/database/logging/logging.service.spec.ts +++ b/myteamwallet_backend/src/database/logging/logging.service.spec.ts @@ -1,18 +1,25 @@ -import { Test, TestingModule } from '@nestjs/testing'; import { LoggingService } from './logging.service'; describe('LoggingService', () => { - let service: LoggingService; + it('can persist an event through the caller transaction manager', async () => { + const defaultRepository = { save: jest.fn() }; + const transactionRepository = { save: jest.fn() }; + const manager = { + getRepository: jest.fn(() => transactionRepository), + } as any; + const service = new LoggingService(defaultRepository as any); + const event = { + event: 'admin_user_profile_update' as const, + details: 'targetUserId=2', + userId: 1, + }; - beforeEach(async () => { - const module: TestingModule = await Test.createTestingModule({ - providers: [LoggingService], - }).compile(); + await service.info(event, manager); - service = module.get(LoggingService); - }); - - it('should be defined', () => { - expect(service).toBeDefined(); + expect(transactionRepository.save).toHaveBeenCalledWith({ + ...event, + level: 'INFO', + }); + expect(defaultRepository.save).not.toHaveBeenCalled(); }); }); diff --git a/myteamwallet_backend/src/database/logging/logging.service.ts b/myteamwallet_backend/src/database/logging/logging.service.ts index 6febd80..99ae584 100644 --- a/myteamwallet_backend/src/database/logging/logging.service.ts +++ b/myteamwallet_backend/src/database/logging/logging.service.ts @@ -1,6 +1,6 @@ import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; -import { Repository } from 'typeorm'; +import { EntityManager, Repository } from 'typeorm'; import { CreateLogDTO } from './dto/create-log.dto'; import { LogEntry } from './entities/log-entry.entity'; import { LOGEVENT } from './model/logging-event.type'; @@ -23,22 +23,25 @@ export class LoggingService { }); } - async info({ - event, - details, - userId, - }: { - event: LOGEVENT; - details: string; - userId: number; - }) { + async info( + { + event, + details, + userId, + }: { + event: LOGEVENT; + details: string; + userId: number; + }, + manager?: EntityManager, + ) { const e: CreateLogDTO = { event, details, userId, level: 'INFO', }; - await this.repository.save(e); + await (manager?.getRepository(LogEntry) ?? this.repository).save(e); } async error({ diff --git a/myteamwallet_backend/src/database/logging/model/logging-event.type.ts b/myteamwallet_backend/src/database/logging/model/logging-event.type.ts index 08ccfe3..38182d2 100644 --- a/myteamwallet_backend/src/database/logging/model/logging-event.type.ts +++ b/myteamwallet_backend/src/database/logging/model/logging-event.type.ts @@ -15,6 +15,11 @@ export type LOGEVENT = | 'transaction_create_fail' | 'transaction_reverse' | 'player_creation' + | 'admin_user_profile_update' + | 'admin_user_role_update' + | 'admin_user_status_update' + | 'admin_player_assign' + | 'admin_player_unlink' | 'team_create'; export type LOGLEVEL = 'FATAL' | 'ERROR' | 'WARN' | 'INFO' | 'DEBUG' | 'TRACE'; diff --git a/myteamwallet_backend/src/database/migrations/1785517200000-AddPlayerLookupIndexes.ts b/myteamwallet_backend/src/database/migrations/1785517200000-AddPlayerLookupIndexes.ts new file mode 100644 index 0000000..655ca5c --- /dev/null +++ b/myteamwallet_backend/src/database/migrations/1785517200000-AddPlayerLookupIndexes.ts @@ -0,0 +1,19 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +export class AddPlayerLookupIndexes1785517200000 implements MigrationInterface { + name = 'AddPlayerLookupIndexes1785517200000'; + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query( + 'CREATE INDEX "IDX_player_team_id" ON "player" ("teamId")', + ); + await queryRunner.query( + 'CREATE INDEX "IDX_player_user_id" ON "player" ("userId")', + ); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query('DROP INDEX "IDX_player_user_id"'); + await queryRunner.query('DROP INDEX "IDX_player_team_id"'); + } +} diff --git a/myteamwallet_backend/src/database/migrations/AddPlayerLookupIndexes.spec.ts b/myteamwallet_backend/src/database/migrations/AddPlayerLookupIndexes.spec.ts new file mode 100644 index 0000000..44ce866 --- /dev/null +++ b/myteamwallet_backend/src/database/migrations/AddPlayerLookupIndexes.spec.ts @@ -0,0 +1,33 @@ +import { getMetadataArgsStorage } from 'typeorm'; +import { Player } from '../../players/entities/player.entity'; +import { AddPlayerLookupIndexes1785517200000 } from './1785517200000-AddPlayerLookupIndexes'; + +describe('AddPlayerLookupIndexes1785517200000', () => { + it('adds reversible indexes for team and user foreign-key lookups', async () => { + const queryRunner = { query: jest.fn() } as any; + const migration = new AddPlayerLookupIndexes1785517200000(); + + await migration.up(queryRunner); + expect(queryRunner.query.mock.calls.map(([sql]) => sql)).toEqual([ + 'CREATE INDEX "IDX_player_team_id" ON "player" ("teamId")', + 'CREATE INDEX "IDX_player_user_id" ON "player" ("userId")', + ]); + + queryRunner.query.mockClear(); + await migration.down(queryRunner); + expect(queryRunner.query.mock.calls.map(([sql]) => sql)).toEqual([ + 'DROP INDEX "IDX_player_user_id"', + 'DROP INDEX "IDX_player_team_id"', + ]); + }); + + it('keeps entity index metadata aligned with the migration', () => { + const playerIndexes = getMetadataArgsStorage() + .indices.filter((index) => index.target === Player) + .map((index) => index.name); + + expect(playerIndexes).toEqual( + expect.arrayContaining(['IDX_player_team_id', 'IDX_player_user_id']), + ); + }); +}); diff --git a/myteamwallet_backend/src/players/entities/player.entity.ts b/myteamwallet_backend/src/players/entities/player.entity.ts index bf0a89b..654072d 100644 --- a/myteamwallet_backend/src/players/entities/player.entity.ts +++ b/myteamwallet_backend/src/players/entities/player.entity.ts @@ -3,9 +3,11 @@ import { BeforeInsert, Column, Entity, + Index, ManyToOne, OneToMany, PrimaryGeneratedColumn, + RelationId, } from 'typeorm'; import { EntityHelper } from 'src/utils/entity-helper'; import { TeamRole } from 'src/team-roles/entities/team-roles.entity'; @@ -29,6 +31,7 @@ export class Player extends EntityHelper { }) teamRole?: TeamRole | null; + @Index('IDX_player_team_id') @ManyToOne(() => Team, { eager: true, }) @@ -37,11 +40,15 @@ export class Player extends EntityHelper { @Column({ type: 'decimal', precision: 10, scale: 2, default: 0 }) balance: number; + @Index('IDX_player_user_id') @ManyToOne(() => User, (user) => user.players, { eager: true, }) user?: User | null; + @RelationId((player: Player) => player.user) + userId?: number | null; + @OneToMany(() => Transaction, (transaction) => transaction.player) transactions: Transaction[]; diff --git a/myteamwallet_backend/src/users/admin-users.controller.spec.ts b/myteamwallet_backend/src/users/admin-users.controller.spec.ts new file mode 100644 index 0000000..203b37d --- /dev/null +++ b/myteamwallet_backend/src/users/admin-users.controller.spec.ts @@ -0,0 +1,118 @@ +import { GUARDS_METADATA, PATH_METADATA } from '@nestjs/common/constants'; +import { validate } from 'class-validator'; +import { plainToInstance } from 'class-transformer'; +import { RoleEnum } from '../roles/roles.enum'; +import { RolesGuard } from '../roles/roles.guard'; +import { AdminUsersController } from './admin-users.controller'; +import { + AdminPlayerQueryDto, + AdminUserProfileDto, + AdminUserRoleDto, + AdminUserStatusDto, +} from './dto/admin-user.dto'; + +describe('AdminUsersController', () => { + const service = { + updateProfile: jest.fn(), + updateRole: jest.fn(), + updateStatus: jest.fn(), + findPlayers: jest.fn(), + assignPlayer: jest.fn(), + unlinkPlayer: jest.fn(), + }; + const controller = new AdminUsersController(service as any); + const actor = { user: { id: 7, role: { id: RoleEnum.admin } } }; + + beforeEach(() => jest.clearAllMocks()); + + it('uses a separate versioned admin/users controller guarded by the global admin role', () => { + expect(Reflect.getMetadata(PATH_METADATA, AdminUsersController)).toBe( + 'admin/users', + ); + expect(Reflect.getMetadata('roles', AdminUsersController)).toEqual([ + RoleEnum.admin, + ]); + expect( + Reflect.getMetadata(GUARDS_METADATA, AdminUsersController), + ).toContain(RolesGuard); + }); + + it('passes the authenticated actor and target to profile, role, and status mutations', async () => { + await controller.updateProfile(actor as any, 2, { + firstName: 'New', + lastName: 'Name', + }); + await controller.updateRole(actor as any, 2, { role: RoleEnum.user }); + await controller.updateStatus(actor as any, 2, { + status: 2, + }); + + expect(service.updateProfile).toHaveBeenCalledWith(7, 2, { + firstName: 'New', + lastName: 'Name', + }); + expect(service.updateRole).toHaveBeenCalledWith(7, 2, RoleEnum.user); + expect(service.updateStatus).toHaveBeenCalledWith(7, 2, 2); + }); + + it('passes player search and atomic assignment operations to the service', async () => { + const query = { assignment: 'unassigned' as const, page: 2, limit: 10 }; + + await controller.findPlayers(query); + await controller.assignPlayer(actor as any, 3, 11); + await controller.unlinkPlayer(actor as any, 3, 11); + + expect(service.findPlayers).toHaveBeenCalledWith(query); + expect(service.assignPlayer).toHaveBeenCalledWith(7, 3, 11); + expect(service.unlinkPlayer).toHaveBeenCalledWith(7, 3, 11); + }); +}); + +describe('admin user DTOs', () => { + it('accepts only profile names in the profile DTO', async () => { + const dto = plainToInstance(AdminUserProfileDto, { + firstName: 'Ada', + lastName: 'Admin', + email: 'must-not-pass@example.com', + role: RoleEnum.user, + status: 2, + }); + + expect(await validate(dto, { whitelist: true })).toEqual([]); + expect(dto).toEqual({ firstName: 'Ada', lastName: 'Admin' }); + }); + + it('rejects role and status ids outside their enums', async () => { + const role = plainToInstance(AdminUserRoleDto, { role: 999 }); + const status = plainToInstance(AdminUserStatusDto, { status: 999 }); + + expect(await validate(role)).not.toEqual([]); + expect(await validate(status)).not.toEqual([]); + }); + + it('rejects numeric-enum reverse-map names instead of sending strings to integer foreign keys', async () => { + const role = plainToInstance(AdminUserRoleDto, { role: 'admin' }); + const status = plainToInstance(AdminUserStatusDto, { status: 'inactive' }); + + expect(await validate(role)).not.toEqual([]); + expect(await validate(status)).not.toEqual([]); + }); + + it('validates player assignment filters and pagination bounds', async () => { + const valid = plainToInstance(AdminPlayerQueryDto, { + assignment: 'assigned', + teamId: '4', + page: '2', + limit: '50', + }); + const invalid = plainToInstance(AdminPlayerQueryDto, { + assignment: 'someone', + page: 0, + limit: 51, + }); + + expect(await validate(valid)).toEqual([]); + expect(valid).toMatchObject({ teamId: 4, page: 2, limit: 50 }); + expect(await validate(invalid)).not.toEqual([]); + }); +}); diff --git a/myteamwallet_backend/src/users/admin-users.controller.ts b/myteamwallet_backend/src/users/admin-users.controller.ts new file mode 100644 index 0000000..2cf59cd --- /dev/null +++ b/myteamwallet_backend/src/users/admin-users.controller.ts @@ -0,0 +1,102 @@ +import { + Body, + Controller, + Delete, + Get, + HttpCode, + HttpStatus, + Param, + ParseIntPipe, + Patch, + Put, + Query, + Request, + UseGuards, +} from '@nestjs/common'; +import { ApiBearerAuth, ApiTags } from '@nestjs/swagger'; +import { AuthGuard } from '@nestjs/passport'; +import { Roles } from '../roles/roles.decorator'; +import { RoleEnum } from '../roles/roles.enum'; +import { RolesGuard } from '../roles/roles.guard'; +import { AdminUsersService } from './admin-users.service'; +import { + AdminPlayerQueryDto, + AdminUserProfileDto, + AdminUserRoleDto, + AdminUserStatusDto, +} from './dto/admin-user.dto'; + +type AdminRequest = { user: { id: number; role: { id: RoleEnum } } }; + +@ApiBearerAuth() +@ApiTags('Admin users') +@UseGuards(AuthGuard('jwt'), RolesGuard) +@Roles([RoleEnum.admin]) +@Controller({ path: 'admin/users', version: '1' }) +export class AdminUsersController { + constructor(private readonly adminUsersService: AdminUsersService) {} + + @Patch(':id/profile') + @HttpCode(HttpStatus.OK) + updateProfile( + @Request() request: AdminRequest, + @Param('id', ParseIntPipe) id: number, + @Body() dto: AdminUserProfileDto, + ) { + return this.adminUsersService.updateProfile(request.user.id, id, dto); + } + + @Patch(':id/role') + @HttpCode(HttpStatus.OK) + updateRole( + @Request() request: AdminRequest, + @Param('id', ParseIntPipe) id: number, + @Body() dto: AdminUserRoleDto, + ) { + return this.adminUsersService.updateRole(request.user.id, id, dto.role); + } + + @Patch(':id/status') + @HttpCode(HttpStatus.OK) + updateStatus( + @Request() request: AdminRequest, + @Param('id', ParseIntPipe) id: number, + @Body() dto: AdminUserStatusDto, + ) { + return this.adminUsersService.updateStatus(request.user.id, id, dto.status); + } + + @Get('players') + @HttpCode(HttpStatus.OK) + findPlayers(@Query() query: AdminPlayerQueryDto) { + return this.adminUsersService.findPlayers(query); + } + + @Put(':userId/players/:playerId') + @HttpCode(HttpStatus.OK) + assignPlayer( + @Request() request: AdminRequest, + @Param('userId', ParseIntPipe) userId: number, + @Param('playerId', ParseIntPipe) playerId: number, + ) { + return this.adminUsersService.assignPlayer( + request.user.id, + userId, + playerId, + ); + } + + @Delete(':userId/players/:playerId') + @HttpCode(HttpStatus.OK) + unlinkPlayer( + @Request() request: AdminRequest, + @Param('userId', ParseIntPipe) userId: number, + @Param('playerId', ParseIntPipe) playerId: number, + ) { + return this.adminUsersService.unlinkPlayer( + request.user.id, + userId, + playerId, + ); + } +} diff --git a/myteamwallet_backend/src/users/admin-users.service.spec.ts b/myteamwallet_backend/src/users/admin-users.service.spec.ts new file mode 100644 index 0000000..ec4460f --- /dev/null +++ b/myteamwallet_backend/src/users/admin-users.service.spec.ts @@ -0,0 +1,330 @@ +import { ConflictException, ForbiddenException } from '@nestjs/common'; +import { RoleEnum } from '../roles/roles.enum'; +import { StatusEnum } from '../statuses/statuses.enum'; +import { AdminUsersService } from './admin-users.service'; + +describe('AdminUsersService', () => { + const team = { id: 10, name: 'Alpha', alias: 'alpha' }; + const teamRole = { id: 1, name: 'Player' }; + const actorId = 1; + let target: any; + let activeAdmins: any[]; + let player: any; + let playerRows: any[]; + let playerTotal: number; + let lockQuery: any; + let lockedUserQuery: any; + let playerQuery: any; + let lockedPlayerQuery: any; + let userRepository: any; + let playerRepository: any; + let manager: any; + let dataSource: any; + let logger: any; + let service: AdminUsersService; + + beforeEach(() => { + target = user(2, RoleEnum.user, StatusEnum.active); + activeAdmins = [user(actorId, RoleEnum.admin, StatusEnum.active)]; + player = assignment(101, null); + playerRows = []; + playerTotal = 0; + lockQuery = chain({ getMany: jest.fn(() => activeAdmins) }); + lockedUserQuery = chain({ getOne: jest.fn(() => target) }); + playerQuery = chain({ + getRawMany: jest.fn(() => playerRows), + getCount: jest.fn(() => playerTotal), + }); + lockedPlayerQuery = chain({ getOne: jest.fn(() => player) }); + userRepository = { + createQueryBuilder: jest.fn((alias: string) => + alias === 'lockedUser' ? lockedUserQuery : lockQuery, + ), + findOne: jest.fn(() => target), + save: jest.fn((value) => Promise.resolve(value)), + }; + playerRepository = { + createQueryBuilder: jest.fn((alias: string) => + alias === 'lockedPlayer' ? lockedPlayerQuery : playerQuery, + ), + findOne: jest.fn(() => player), + save: jest.fn((value) => Promise.resolve(value)), + }; + manager = { + getRepository: jest.fn((entity) => + entity.name === 'User' ? userRepository : playerRepository, + ), + }; + dataSource = { + manager, + transaction: jest.fn((work) => work(manager)), + }; + logger = { info: jest.fn() }; + service = new AdminUsersService(dataSource, logger); + }); + + it('updates only names and returns an explicit Task 1-compatible safe admin summary', async () => { + target.players = [assignment(101, target)]; + + const result = await service.updateProfile(actorId, target.id, { + firstName: 'New', + lastName: 'Name', + }); + + expect(userRepository.save).toHaveBeenCalledWith( + expect.objectContaining({ firstName: 'New', lastName: 'Name' }), + ); + expect(result).toEqual({ + id: 2, + firstName: 'New', + lastName: 'Name', + email: 'user-2@example.com', + role: { id: RoleEnum.user, name: 'User' }, + status: { id: StatusEnum.active, name: 'Active' }, + assignments: [playerSummary(101)], + }); + expect(result).not.toHaveProperty('password'); + expect(result).not.toHaveProperty('hash'); + expect(result).not.toHaveProperty('socialId'); + expect(logger.info).toHaveBeenCalledWith( + { + event: 'admin_user_profile_update', + details: 'targetUserId=2', + userId: actorId, + }, + manager, + ); + expect(lockedUserQuery.setLock).toHaveBeenCalledWith( + 'pessimistic_write', + undefined, + ['lockedUser'], + ); + expect(lockedUserQuery.leftJoinAndSelect).toHaveBeenCalledTimes(2); + }); + + it('rejects self-demotion and self-deactivation before saving', async () => { + target = user(actorId, RoleEnum.admin, StatusEnum.active); + + await expect( + service.updateRole(actorId, actorId, RoleEnum.user), + ).rejects.toBeInstanceOf(ForbiddenException); + await expect( + service.updateStatus(actorId, actorId, StatusEnum.inactive), + ).rejects.toBeInstanceOf(ForbiddenException); + expect(userRepository.save).not.toHaveBeenCalled(); + }); + + it('locks the active-admin set and rejects loss of the last active admin', async () => { + target = user(2, RoleEnum.admin, StatusEnum.active); + activeAdmins = [target]; + + await expect( + service.updateRole(actorId, target.id, RoleEnum.user), + ).rejects.toBeInstanceOf(ConflictException); + + expect(lockQuery.setLock).toHaveBeenCalledWith( + 'pessimistic_write', + undefined, + ['user'], + ); + expect(dataSource.transaction).toHaveBeenCalledTimes(1); + expect(userRepository.save).not.toHaveBeenCalled(); + }); + + it('allows a locked role change when another active admin remains', async () => { + target = user(2, RoleEnum.admin, StatusEnum.active); + activeAdmins = [target, user(3, RoleEnum.admin, StatusEnum.active)]; + + const result = await service.updateRole(actorId, target.id, RoleEnum.user); + + expect(target.role).toEqual({ id: RoleEnum.user }); + expect(result.role).toEqual({ id: RoleEnum.user, name: undefined }); + expect(logger.info).toHaveBeenCalledWith( + { + event: 'admin_user_role_update', + details: 'targetUserId=2 roleId=2', + userId: actorId, + }, + manager, + ); + }); + + it('deactivates under the same lock without changing player assignments', async () => { + target = user(2, RoleEnum.user, StatusEnum.active); + target.players = [assignment(101, target)]; + + const result = await service.updateStatus( + actorId, + target.id, + StatusEnum.inactive, + ); + + expect(target.status).toEqual({ id: StatusEnum.inactive }); + expect(target.hash).toBeNull(); + expect(result.assignments).toEqual([playerSummary(101)]); + expect(playerRepository.save).not.toHaveBeenCalled(); + }); + + it('returns an explicit paginated player projection with safe current-user summaries', async () => { + playerRows = [ + { + player_id: 101, + first_name: 'Pat', + last_name: 'Player', + active: 1, + team_id: 10, + team_name: 'Alpha', + team_alias: 'alpha', + user_id: 2, + user_first_name: 'Target', + user_last_name: 'User', + status_id: StatusEnum.active, + status_name: 'Active', + }, + ]; + playerTotal = 1; + + const result = await service.findPlayers({ + search: 'pat', + teamId: 10, + assignment: 'assigned', + page: 1, + limit: 20, + }); + + expect(result).toEqual({ + data: [ + { + id: 101, + firstName: 'Pat', + lastName: 'Player', + active: true, + team, + currentUser: { + id: 2, + firstName: 'Target', + lastName: 'User', + status: { id: StatusEnum.active, name: 'Active' }, + }, + }, + ], + page: 1, + limit: 20, + total: 1, + hasNextPage: false, + }); + expect(playerQuery.andWhere).toHaveBeenCalledWith( + 'player.teamId = :teamId', + { + teamId: 10, + }, + ); + expect(playerQuery.andWhere).toHaveBeenCalledWith( + 'player.userId IS NOT NULL', + ); + }); + + it('atomically reassigns a locked player and returns the refreshed target summary', async () => { + const previous = user(9, RoleEnum.user, StatusEnum.active); + player = assignment(101, previous); + target.players = [player]; + + const result = await service.assignPlayer(actorId, target.id, player.id); + + expect(dataSource.transaction).toHaveBeenCalledTimes(1); + expect(lockedPlayerQuery.setLock).toHaveBeenCalledWith('pessimistic_write'); + expect(player.user).toBe(target); + expect(playerRepository.save).toHaveBeenCalledWith(player); + expect(result.id).toBe(target.id); + expect(logger.info).toHaveBeenCalledWith( + { + event: 'admin_player_assign', + details: 'targetUserId=2 playerId=101 previousUserId=9', + userId: actorId, + }, + manager, + ); + }); + + it('unlinks only when the locked player is currently linked to the target user', async () => { + player = assignment(101, user(9, RoleEnum.user, StatusEnum.active)); + + await expect( + service.unlinkPlayer(actorId, target.id, player.id), + ).rejects.toBeInstanceOf(ConflictException); + expect(playerRepository.save).not.toHaveBeenCalled(); + + player.user = target; + player.userId = target.id; + await service.unlinkPlayer(actorId, target.id, player.id); + expect(player.user).toBeNull(); + expect(playerRepository.save).toHaveBeenCalledWith(player); + }); + + function user(id: number, roleId: RoleEnum, statusId: StatusEnum) { + return { + id, + firstName: id === 2 ? 'Target' : 'Admin', + lastName: id === 2 ? 'User' : String(id), + email: `user-${id}@example.com`, + password: `password-${id}`, + hash: `hash-${id}`, + socialId: `social-${id}`, + role: { + id: roleId, + name: roleId === RoleEnum.admin ? 'Admin' : 'User', + }, + status: { + id: statusId, + name: statusId === StatusEnum.active ? 'Active' : 'Inactive', + }, + players: [], + }; + } + + function assignment(id: number, assignedUser: any) { + return { + id, + firstName: 'Pat', + lastName: 'Player', + active: true, + team, + teamRole, + user: assignedUser, + userId: assignedUser?.id ?? null, + }; + } + + function playerSummary(id: number) { + return { + id, + firstName: 'Pat', + lastName: 'Player', + active: true, + team, + teamRole, + }; + } + + function chain(overrides: Record) { + const query: Record = {}; + [ + 'innerJoin', + 'innerJoinAndSelect', + 'leftJoinAndSelect', + 'leftJoin', + 'select', + 'where', + 'andWhere', + 'setParameter', + 'setParameters', + 'setLock', + 'orderBy', + 'offset', + 'limit', + ].forEach((method) => { + query[method] = jest.fn(() => query); + }); + return Object.assign(query, overrides); + } +}); diff --git a/myteamwallet_backend/src/users/admin-users.service.ts b/myteamwallet_backend/src/users/admin-users.service.ts new file mode 100644 index 0000000..fe5916a --- /dev/null +++ b/myteamwallet_backend/src/users/admin-users.service.ts @@ -0,0 +1,381 @@ +import { + ConflictException, + ForbiddenException, + Injectable, + NotFoundException, +} from '@nestjs/common'; +import { Brackets, DataSource, EntityManager, Repository } from 'typeorm'; +import { LoggingService } from '../database/logging/logging.service'; +import { Player } from '../players/entities/player.entity'; +import { Role } from '../roles/entities/role.entity'; +import { RoleEnum } from '../roles/roles.enum'; +import { Status } from '../statuses/entities/status.entity'; +import { StatusEnum } from '../statuses/statuses.enum'; +import { + AdminPlayerPageDto, + AdminPlayerSummaryDto, +} from './dto/admin-player-response.dto'; +import { AdminPlayerQueryDto, AdminUserProfileDto } from './dto/admin-user.dto'; +import { AdminUserDirectorySummaryDto } from './dto/user-directory-response.dto'; +import { User } from './entities/user.entity'; + +type PlayerSearchRow = { + player_id: number | string; + first_name: string; + last_name: string; + active: boolean; + team_id: number | string; + team_name: string; + team_alias: string; + user_id: number | string | null; + user_first_name: string | null; + user_last_name: string | null; + status_id: number | string | null; + status_name: string | null; +}; + +@Injectable() +export class AdminUsersService { + constructor( + private readonly dataSource: DataSource, + private readonly logger: LoggingService, + ) {} + + async updateProfile( + actorId: number, + targetUserId: number, + dto: AdminUserProfileDto, + ): Promise { + const summary = await this.dataSource.transaction(async (manager) => { + const repository = manager.getRepository(User); + const target = await this.findLockedUser(repository, targetUserId); + if (dto.firstName !== undefined) target.firstName = dto.firstName; + if (dto.lastName !== undefined) target.lastName = dto.lastName; + await repository.save(target); + const summary = await this.findAdminSummary(manager, targetUserId); + await this.log( + manager, + 'admin_user_profile_update', + actorId, + `targetUserId=${targetUserId}`, + ); + return summary; + }); + return summary; + } + + async updateRole( + actorId: number, + targetUserId: number, + roleId: RoleEnum, + ): Promise { + const summary = await this.dataSource.transaction(async (manager) => { + const activeAdmins = await this.lockActiveAdmins(manager); + const repository = manager.getRepository(User); + const target = await this.findLockedUser(repository, targetUserId); + const isDemotion = + target.role?.id === RoleEnum.admin && roleId !== RoleEnum.admin; + if (actorId === targetUserId && isDemotion) { + throw new ForbiddenException('Administrators cannot demote themselves'); + } + if ( + isDemotion && + target.status?.id === StatusEnum.active && + activeAdmins.length <= 1 + ) { + throw new ConflictException('At least one active admin must remain'); + } + target.role = { id: roleId } as Role; + await repository.save(target); + const summary = await this.findAdminSummary(manager, targetUserId); + await this.log( + manager, + 'admin_user_role_update', + actorId, + `targetUserId=${targetUserId} roleId=${roleId}`, + ); + return summary; + }); + return summary; + } + + async updateStatus( + actorId: number, + targetUserId: number, + statusId: StatusEnum, + ): Promise { + const summary = await this.dataSource.transaction(async (manager) => { + const activeAdmins = await this.lockActiveAdmins(manager); + const repository = manager.getRepository(User); + const target = await this.findLockedUser(repository, targetUserId); + const isDeactivation = + target.status?.id === StatusEnum.active && + statusId === StatusEnum.inactive; + if (actorId === targetUserId && isDeactivation) { + throw new ForbiddenException( + 'Administrators cannot deactivate themselves', + ); + } + if ( + isDeactivation && + target.role?.id === RoleEnum.admin && + activeAdmins.length <= 1 + ) { + throw new ConflictException('At least one active admin must remain'); + } + target.status = { id: statusId } as Status; + if (statusId === StatusEnum.inactive) target.hash = null; + await repository.save(target); + const summary = await this.findAdminSummary(manager, targetUserId); + await this.log( + manager, + 'admin_user_status_update', + actorId, + `targetUserId=${targetUserId} statusId=${statusId}`, + ); + return summary; + }); + return summary; + } + + async findPlayers(query: AdminPlayerQueryDto): Promise { + const page = query.page ?? 1; + const limit = query.limit ?? 20; + const repository = this.dataSource.manager.getRepository(Player); + const builder = repository + .createQueryBuilder('player') + .innerJoin('player.team', 'team') + .leftJoin('player.user', 'currentUser') + .leftJoin('currentUser.status', 'currentStatus'); + const term = query.search?.trim().toLocaleLowerCase(); + if (term) { + builder + .andWhere( + new Brackets((where) => + where + .where('LOWER(player.firstName) LIKE :playerSearch') + .orWhere('LOWER(player.lastName) LIKE :playerSearch') + .orWhere('LOWER(team.name) LIKE :playerSearch'), + ), + ) + .setParameter('playerSearch', `%${term}%`); + } + if (query.teamId !== undefined) { + builder.andWhere('player.teamId = :teamId', { teamId: query.teamId }); + } + if (query.assignment === 'assigned') { + builder.andWhere('player.userId IS NOT NULL'); + } else if (query.assignment === 'unassigned') { + builder.andWhere('player.userId IS NULL'); + } + const total = await builder.getCount(); + const rows = await builder + .select([ + 'player.id AS player_id', + 'player.firstName AS first_name', + 'player.lastName AS last_name', + 'player.active AS active', + 'team.id AS team_id', + 'team.name AS team_name', + 'team.alias AS team_alias', + 'currentUser.id AS user_id', + 'currentUser.firstName AS user_first_name', + 'currentUser.lastName AS user_last_name', + 'currentStatus.id AS status_id', + 'currentStatus.name AS status_name', + ]) + .orderBy('player.id', 'ASC') + .offset((page - 1) * limit) + .limit(limit) + .getRawMany(); + return { + data: rows.map((row) => this.mapPlayerSearchRow(row)), + page, + limit, + total, + hasNextPage: page * limit < total, + }; + } + + async assignPlayer( + actorId: number, + targetUserId: number, + playerId: number, + ): Promise { + let previousUserId: number | null = null; + const summary = await this.dataSource.transaction(async (manager) => { + const userRepository = manager.getRepository(User); + const playerRepository = manager.getRepository(Player); + const target = await userRepository.findOne({ + where: { id: targetUserId }, + }); + if (!target) throw new NotFoundException('User not found'); + const lockedPlayer = await this.findLockedPlayer( + playerRepository, + playerId, + ); + previousUserId = lockedPlayer.userId ?? null; + lockedPlayer.user = target; + await playerRepository.save(lockedPlayer); + const summary = await this.findAdminSummary(manager, targetUserId); + await this.log( + manager, + 'admin_player_assign', + actorId, + `targetUserId=${targetUserId} playerId=${playerId} previousUserId=${ + previousUserId ?? 'none' + }`, + ); + return summary; + }); + return summary; + } + + async unlinkPlayer( + actorId: number, + targetUserId: number, + playerId: number, + ): Promise { + const summary = await this.dataSource.transaction(async (manager) => { + const playerRepository = manager.getRepository(Player); + const lockedPlayer = await this.findLockedPlayer( + playerRepository, + playerId, + ); + if (lockedPlayer.userId !== targetUserId) { + throw new ConflictException('Player is not assigned to this user'); + } + lockedPlayer.user = null; + await playerRepository.save(lockedPlayer); + const summary = await this.findAdminSummary(manager, targetUserId); + await this.log( + manager, + 'admin_player_unlink', + actorId, + `targetUserId=${targetUserId} playerId=${playerId}`, + ); + return summary; + }); + return summary; + } + + private lockActiveAdmins(manager: EntityManager): Promise { + return manager + .getRepository(User) + .createQueryBuilder('user') + .innerJoinAndSelect('user.role', 'role') + .innerJoinAndSelect('user.status', 'status') + .where('role.id = :adminRole', { adminRole: RoleEnum.admin }) + .andWhere('status.id = :activeStatus', { + activeStatus: StatusEnum.active, + }) + .setLock('pessimistic_write', undefined, ['user']) + .orderBy('user.id', 'ASC') + .getMany(); + } + + private async findLockedUser( + repository: Repository, + userId: number, + ): Promise { + const user = await repository + .createQueryBuilder('lockedUser') + .leftJoinAndSelect('lockedUser.role', 'role') + .leftJoinAndSelect('lockedUser.status', 'status') + .where('lockedUser.id = :userId', { userId }) + .setLock('pessimistic_write', undefined, ['lockedUser']) + .getOne(); + if (!user) throw new NotFoundException('User not found'); + return user; + } + + private async findLockedPlayer( + repository: Repository, + playerId: number, + ): Promise { + const player = await repository + .createQueryBuilder('lockedPlayer') + .where('lockedPlayer.id = :playerId', { playerId }) + .setLock('pessimistic_write') + .getOne(); + if (!player) throw new NotFoundException('Player not found'); + return player; + } + + private async findAdminSummary( + manager: EntityManager, + userId: number, + ): Promise { + const user = await manager.getRepository(User).findOne({ + where: { id: userId }, + relations: { + role: true, + status: true, + players: { team: true, teamRole: true }, + }, + }); + if (!user) throw new NotFoundException('User not found'); + return { + id: user.id, + firstName: user.firstName, + lastName: user.lastName, + email: user.email, + role: user.role ? { id: user.role.id, name: user.role.name } : null, + status: user.status + ? { id: user.status.id, name: user.status.name } + : null, + assignments: (user.players ?? []).map((player) => ({ + id: player.id, + firstName: player.firstName, + lastName: player.lastName, + active: player.active, + team: { + id: player.team.id, + name: player.team.name, + alias: player.team.alias, + }, + teamRole: player.teamRole + ? { id: player.teamRole.id, name: player.teamRole.name } + : null, + })), + }; + } + + private mapPlayerSearchRow(row: PlayerSearchRow): AdminPlayerSummaryDto { + return { + id: Number(row.player_id), + firstName: row.first_name, + lastName: row.last_name, + active: Boolean(row.active), + team: { + id: Number(row.team_id), + name: row.team_name, + alias: row.team_alias, + }, + currentUser: + row.user_id === null + ? null + : { + id: Number(row.user_id), + firstName: row.user_first_name, + lastName: row.user_last_name, + status: + row.status_id === null + ? null + : { + id: Number(row.status_id), + name: row.status_name ?? undefined, + }, + }, + }; + } + + private log( + manager: EntityManager, + event: Parameters[0]['event'], + userId: number, + details: string, + ) { + return this.logger.info({ event, userId, details }, manager); + } +} diff --git a/myteamwallet_backend/src/users/dto/admin-player-response.dto.ts b/myteamwallet_backend/src/users/dto/admin-player-response.dto.ts new file mode 100644 index 0000000..58753ca --- /dev/null +++ b/myteamwallet_backend/src/users/dto/admin-player-response.dto.ts @@ -0,0 +1,31 @@ +import { UserDirectoryReferenceDto } from './user-directory-response.dto'; + +export class AdminPlayerTeamDto { + id: number; + name: string; + alias: string; +} + +export class AdminPlayerCurrentUserDto { + id: number; + firstName: string | null; + lastName: string | null; + status: UserDirectoryReferenceDto | null; +} + +export class AdminPlayerSummaryDto { + id: number; + firstName: string; + lastName: string; + active: boolean; + team: AdminPlayerTeamDto; + currentUser: AdminPlayerCurrentUserDto | null; +} + +export class AdminPlayerPageDto { + data: AdminPlayerSummaryDto[]; + page: number; + limit: number; + total: number; + hasNextPage: boolean; +} diff --git a/myteamwallet_backend/src/users/dto/admin-user.dto.ts b/myteamwallet_backend/src/users/dto/admin-user.dto.ts new file mode 100644 index 0000000..6efef9a --- /dev/null +++ b/myteamwallet_backend/src/users/dto/admin-user.dto.ts @@ -0,0 +1,67 @@ +import { Type } from 'class-transformer'; +import { + IsIn, + IsInt, + IsOptional, + IsString, + Max, + MaxLength, + Min, +} from 'class-validator'; +import { RoleEnum } from '../../roles/roles.enum'; +import { StatusEnum } from '../../statuses/statuses.enum'; + +export class AdminUserProfileDto { + @IsOptional() + @IsString() + @MaxLength(255) + firstName?: string | null; + + @IsOptional() + @IsString() + @MaxLength(255) + lastName?: string | null; +} + +export class AdminUserRoleDto { + @Type(() => Number) + @IsIn([RoleEnum.admin, RoleEnum.user]) + role: RoleEnum; +} + +export class AdminUserStatusDto { + @Type(() => Number) + @IsIn([StatusEnum.active, StatusEnum.inactive]) + status: StatusEnum; +} + +export type AdminPlayerAssignmentFilter = 'all' | 'assigned' | 'unassigned'; + +export class AdminPlayerQueryDto { + @IsOptional() + @IsString() + search?: string; + + @IsOptional() + @Type(() => Number) + @IsInt() + @Min(1) + teamId?: number; + + @IsOptional() + @IsIn(['all', 'assigned', 'unassigned']) + assignment: AdminPlayerAssignmentFilter = 'all'; + + @IsOptional() + @Type(() => Number) + @IsInt() + @Min(1) + page = 1; + + @IsOptional() + @Type(() => Number) + @IsInt() + @Min(1) + @Max(50) + limit = 20; +} diff --git a/myteamwallet_backend/src/users/users.controller.security.spec.ts b/myteamwallet_backend/src/users/users.controller.security.spec.ts new file mode 100644 index 0000000..45f456a --- /dev/null +++ b/myteamwallet_backend/src/users/users.controller.security.spec.ts @@ -0,0 +1,11 @@ +import { UsersController } from './users.controller'; + +describe('UsersController admin mutation isolation', () => { + it('does not expose generic create, update, or delete handlers that bypass safeguards', () => { + expect(UsersController.prototype).not.toHaveProperty('create'); + expect(UsersController.prototype).not.toHaveProperty('update'); + expect(UsersController.prototype).not.toHaveProperty('remove'); + expect(UsersController.prototype).not.toHaveProperty('findAll'); + expect(UsersController.prototype).not.toHaveProperty('findOne'); + }); +}); diff --git a/myteamwallet_backend/src/users/users.controller.ts b/myteamwallet_backend/src/users/users.controller.ts index 2a40f30..1ea9fda 100644 --- a/myteamwallet_backend/src/users/users.controller.ts +++ b/myteamwallet_backend/src/users/users.controller.ts @@ -1,28 +1,19 @@ import { Controller, Get, - Post, - Body, - Patch, Param, - Delete, UseGuards, Query, - DefaultValuePipe, - ParseIntPipe, HttpStatus, HttpCode, Request, } from '@nestjs/common'; import { UsersService } from './users.service'; -import { CreateUserDto } from './dto/create-user.dto'; -import { UpdateUserDto } from './dto/update-user.dto'; import { ApiBearerAuth, ApiTags } from '@nestjs/swagger'; import { Roles } from 'src/roles/roles.decorator'; import { RoleEnum } from 'src/roles/roles.enum'; import { AuthGuard } from '@nestjs/passport'; import { RolesGuard } from 'src/roles/roles.guard'; -import { infinityPagination } from 'src/utils/infinity-pagination'; import { UserDirectoryQueryDto } from './dto/user-directory-query.dto'; import { User } from './entities/user.entity'; @@ -36,33 +27,6 @@ import { User } from './entities/user.entity'; export class UsersController { constructor(private readonly usersService: UsersService) {} - @Roles([RoleEnum.admin]) - @Post() - @HttpCode(HttpStatus.CREATED) - create(@Body() createProfileDto: CreateUserDto) { - return this.usersService.create(createProfileDto); - } - - @Roles([RoleEnum.admin]) - @Get() - @HttpCode(HttpStatus.OK) - async findAll( - @Query('page', new DefaultValuePipe(1), ParseIntPipe) page: number, - @Query('limit', new DefaultValuePipe(10), ParseIntPipe) limit: number, - ) { - if (limit > 50) { - limit = 50; - } - - return infinityPagination( - await this.usersService.findManyWithPagination({ - page, - limit, - }), - { page, limit }, - ); - } - @Roles([RoleEnum.user, RoleEnum.admin]) @Get('directory') @HttpCode(HttpStatus.OK) @@ -73,30 +37,10 @@ export class UsersController { return this.usersService.findDirectory(request.user, query); } - @Roles([RoleEnum.admin]) - @Get(':id') - @HttpCode(HttpStatus.OK) - findOne(@Param('id') id: string) { - return this.usersService.findOne({ id: +id }); - } - @Roles([RoleEnum.user, RoleEnum.admin]) @Get(':id/teams') @HttpCode(HttpStatus.OK) findTeamsOfPlayer(@Param('id') id: string) { return this.usersService.findTeams({ id: +id }); } - - @Roles([RoleEnum.admin]) - @Patch(':id') - @HttpCode(HttpStatus.OK) - update(@Param('id') id: number, @Body() updateProfileDto: UpdateUserDto) { - return this.usersService.update(id, updateProfileDto); - } - - @Roles([RoleEnum.admin]) - @Delete(':id') - remove(@Param('id') id: number) { - return this.usersService.softDelete(id); - } } diff --git a/myteamwallet_backend/src/users/users.module.ts b/myteamwallet_backend/src/users/users.module.ts index 39bb9a6..9104762 100644 --- a/myteamwallet_backend/src/users/users.module.ts +++ b/myteamwallet_backend/src/users/users.module.ts @@ -7,11 +7,14 @@ import { IsExist } from 'src/utils/validators/is-exists.validator'; import { IsNotExist } from 'src/utils/validators/is-not-exists.validator'; import { Team } from 'src/teams/entities/team.entity'; import { Player } from 'src/players/entities/player.entity'; +import { LoggingModule } from 'src/database/logging/logging.module'; +import { AdminUsersController } from './admin-users.controller'; +import { AdminUsersService } from './admin-users.service'; @Module({ - imports: [TypeOrmModule.forFeature([User, Team, Player])], - controllers: [UsersController], - providers: [IsExist, IsNotExist, UsersService], + imports: [TypeOrmModule.forFeature([User, Team, Player]), LoggingModule], + controllers: [UsersController, AdminUsersController], + providers: [IsExist, IsNotExist, UsersService, AdminUsersService], exports: [UsersService], }) export class UsersModule {}