diff --git a/.superpowers/sdd/admin-user-management/task-2-report.md b/.superpowers/sdd/admin-user-management/task-2-report.md index 15921e6..59d7369 100644 --- a/.superpowers/sdd/admin-user-management/task-2-report.md +++ b/.superpowers/sdd/admin-user-management/task-2-report.md @@ -120,3 +120,70 @@ Every production behavior above was added only after the corresponding expected - 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. + +## Fix Round 1 + +### Review findings addressed + +- Removed `linkPlayerId` from the validated public registration DTO and from internal create DTO plumbing. `AuthController.register` now has a concrete `AuthRegisterLoginDto` body rather than `any`, `AuthService.register` copies only the four permitted registration fields, and the obsolete `UsersService.linkPlayerToUserId` path was removed. Only `AdminUsersService` now changes `Player.user`. +- Rebuilt existing-account social login around one database transaction. Candidate user rows are locked, any email change uses a narrow repository update, the user is reloaded with current role/status under an alias-scoped row lock, inactive state is rechecked, and only then is the JWT signed. The same method covers Facebook, Google, Twitter, and Apple. +- Restricted `GET users/:id/teams` to the authenticated user's own ID. The query now returns an explicit minimal projection containing only player ID/name and team ID/name, matching the fields consumed by the current modern team selector. +- Removed body coercion for role/status mutation IDs. Genuine integer numbers are required; booleans and numeric strings are rejected. +- Added a focused Nest HTTP boundary suite with actual URI versioning, global validation, controller decorators, JWT guard behavior, real `RolesGuard`, and HTTP serialization assertions. + +### RED evidence + +1. Registration isolation: + - Command: `npm test -- --runInBand auth.controller.spec.ts auth.service.spec.ts -t "narrow validated registration|public registration player"` + - Failure: controller parameter metadata was `Object` instead of `AuthRegisterLoginDto`; registration still attempted public player linkage. +2. Social-login race: + - Command: `npm test -- --runInBand auth.service.spec.ts -t "concurrently deactivated social|locks and reloads an existing"` + - Failure: existing flow bypassed the transaction repository, used entity-wide `UsersService.update`, and signed stale state. +3. Self-only safe team bootstrap: + - Command: `npm test -- --runInBand users.controller.security.spec.ts users.teams.spec.ts` + - Failure: `findMyTeams` did not exist and the controller still delegated arbitrary IDs to raw `findTeams`. +4. Strict numeric role/status bodies: + - Command: `npm test -- --runInBand admin-users.controller.spec.ts -t "non-number role"` + - Failure: both JSON `true` and `"1"` were coerced to valid enum ID `1`. +5. Nest HTTP boundary: + - Command: `npm test -- --runInBand admin-users.http.spec.ts` + - Initial infrastructure failure: the focused module did not wire the existing database-backed `IsNotExist` validator container. The test module was corrected to use the real validator with a mocked repository; no validation was weakened. + +### Files added + +- `src/users/admin-users.http.spec.ts` +- `src/users/users.teams.spec.ts` +- `src/users/dto/user-team-response.dto.ts` + +### Files modified + +- `src/auth/auth.controller.ts` +- `src/auth/auth.controller.spec.ts` +- `src/auth/auth.service.ts` +- `src/auth/auth.service.spec.ts` +- `src/auth/dto/auth-register-login.dto.ts` +- `src/users/admin-users.controller.spec.ts` +- `src/users/dto/admin-user.dto.ts` +- `src/users/dto/create-user.dto.ts` +- `src/users/users.controller.ts` +- `src/users/users.controller.security.spec.ts` +- `src/users/users.service.ts` + +### GREEN evidence + +- Focused Task 1 + Task 2 tests: + - Command: `npm test -- --runInBand users.service.spec.ts users.teams.spec.ts users.controller.security.spec.ts admin-users.controller.spec.ts admin-users.service.spec.ts admin-users.http.spec.ts auth.controller.spec.ts auth.service.spec.ts jwt.strategy.spec.ts logging.service.spec.ts AddPlayerLookupIndexes.spec.ts` + - Result: **11 suites passed, 56 tests passed, 0 failed**. +- Targeted ESLint across all Fix Round 1 source/spec files: **exit 0, no findings**. +- Backend build via `npm run build`: **exit 0**. +- `git diff --check`: **exit 0**. +- Both frontend directories: **no changes**. + +### Client contract impact + +- Both frontend codebases currently send `linkPlayerId` during invite registration. The backend now strips it and performs no assignment, as required; registration still succeeds, but player linkage must subsequently use the guarded admin assignment endpoint. +- Both frontends call `GET users/:currentUserId/teams`. That self-ID URL remains valid. The modern selector consumes only the retained player/team ID and name fields. The legacy frontend also displayed team balance and team role from this response; those sensitive/unneeded fields are no longer returned, and the legacy frontend was intentionally not edited. + +### Remaining limitation + +- No ready local PostgreSQL test database/harness was available without new infrastructure. No dependencies or testcontainers were added. Concurrency remains covered by transaction/alias-lock assertions and stale-state regressions; migration remains covered by exact up/down SQL and metadata tests. A live two-connection PostgreSQL race and migration run/revert remain recommended before rollout. diff --git a/myteamwallet_backend/src/auth/auth.controller.spec.ts b/myteamwallet_backend/src/auth/auth.controller.spec.ts index db87187..1466ed8 100644 --- a/myteamwallet_backend/src/auth/auth.controller.spec.ts +++ b/myteamwallet_backend/src/auth/auth.controller.spec.ts @@ -1,5 +1,6 @@ import { GUARDS_METADATA } from '@nestjs/common/constants'; import { AuthController } from './auth.controller'; +import { AuthRegisterLoginDto } from './dto/auth-register-login.dto'; describe('AuthController session enforcement', () => { it('protects GET auth/me with JWT validation', () => { @@ -15,4 +16,15 @@ describe('AuthController session enforcement', () => { it('does not expose self-deletion that can race with an admin promotion', () => { expect(AuthController.prototype).not.toHaveProperty('delete'); }); + + it('uses the narrow validated registration DTO instead of an untyped body', () => { + const parameterTypes = Reflect.getMetadata( + 'design:paramtypes', + AuthController.prototype, + 'register', + ); + + expect(parameterTypes[0]).toBe(AuthRegisterLoginDto); + expect(AuthRegisterLoginDto.prototype).not.toHaveProperty('linkPlayerId'); + }); }); diff --git a/myteamwallet_backend/src/auth/auth.controller.ts b/myteamwallet_backend/src/auth/auth.controller.ts index 59340c5..27300dc 100644 --- a/myteamwallet_backend/src/auth/auth.controller.ts +++ b/myteamwallet_backend/src/auth/auth.controller.ts @@ -26,6 +26,7 @@ import { ApiOkResponse, } from '@nestjs/swagger'; import { CreateInviteDTO } from './dto/create-invite.dto'; +import { AuthRegisterLoginDto } from './dto/auth-register-login.dto'; @ApiTags('Auth') @Controller({ @@ -50,7 +51,7 @@ export class AuthController { @Post('email/register') @HttpCode(HttpStatus.CREATED) - async register(@Body() createUserDto: any) { + async register(@Body() createUserDto: AuthRegisterLoginDto) { return this.service.register(createUserDto); } diff --git a/myteamwallet_backend/src/auth/auth.service.spec.ts b/myteamwallet_backend/src/auth/auth.service.spec.ts index e6845da..a780564 100644 --- a/myteamwallet_backend/src/auth/auth.service.spec.ts +++ b/myteamwallet_backend/src/auth/auth.service.spec.ts @@ -13,6 +13,7 @@ describe('AuthService inactive-user enforcement and safe logging', () => { let lockedUserQuery: any; let userRepository: any; let service: AuthService; + let mailService: any; beforeEach(() => { jwtService = { @@ -24,8 +25,10 @@ describe('AuthService inactive-user enforcement and safe logging', () => { findOne: jest.fn(), update: jest.fn(), create: jest.fn(), + linkPlayerToUserId: jest.fn(), }; logger = { info: jest.fn(), debug: jest.fn() }; + mailService = { userSignUp: jest.fn() }; confirmationUser = user(StatusEnum.inactive); confirmationUser.hash = 'confirmation-hash'; lockedUserQuery = { @@ -45,7 +48,7 @@ describe('AuthService inactive-user enforcement and safe logging', () => { jwtService, usersService, {} as any, - {} as any, + mailService, logger, dataSource, ); @@ -70,10 +73,12 @@ describe('AuthService inactive-user enforcement and safe logging', () => { }); it('rejects social login when the existing account is inactive', async () => { - const inactive = user(StatusEnum.inactive); - usersService.findOne - .mockResolvedValueOnce(inactive) - .mockResolvedValueOnce(undefined); + const inactive = socialUser( + AuthProvidersEnum.google, + RoleEnum.user, + StatusEnum.inactive, + ); + configureSocialQueries([inactive], inactive); await expect( service.validateSocialLogin(AuthProvidersEnum.google, { @@ -88,6 +93,61 @@ describe('AuthService inactive-user enforcement and safe logging', () => { expect(jwtService.sign).not.toHaveBeenCalled(); }); + it('reloads and rejects a concurrently deactivated social user instead of signing stale state', async () => { + const stale = socialUser( + AuthProvidersEnum.google, + RoleEnum.admin, + StatusEnum.active, + ); + const current = socialUser( + AuthProvidersEnum.google, + RoleEnum.user, + StatusEnum.inactive, + ); + configureSocialQueries([stale], current); + + await expect( + service.validateSocialLogin(AuthProvidersEnum.google, { + id: stale.socialId, + email: 'updated@example.com', + }), + ).rejects.toBeInstanceOf(ForbiddenException); + + expect(dataSource.transaction).toHaveBeenCalledTimes(1); + expect(userRepository.save).not.toHaveBeenCalledWith(stale); + expect(usersService.update).not.toHaveBeenCalled(); + expect(jwtService.sign).not.toHaveBeenCalled(); + }); + + it.each([ + AuthProvidersEnum.facebook, + AuthProvidersEnum.google, + AuthProvidersEnum.twitter, + AuthProvidersEnum.apple, + ])( + 'locks and reloads an existing %s user, narrowly updates email, and signs the current role', + async (provider) => { + const stale = socialUser(provider, RoleEnum.admin, StatusEnum.active); + const current = socialUser(provider, RoleEnum.user, StatusEnum.active); + configureSocialQueries([stale], current); + + const result = await service.validateSocialLogin(provider, { + id: stale.socialId, + email: 'updated@example.com', + }); + + expect(userRepository.update).toHaveBeenCalledWith(stale.id, { + email: 'updated@example.com', + }); + expect(userRepository.save).not.toHaveBeenCalledWith(stale); + expect(jwtService.sign).toHaveBeenCalledWith({ + id: current.id, + role: current.role, + }); + expect(result.user).toBe(current); + }, + ); + it('never includes an email in an unknown-user login audit event', async () => { usersService.findOne.mockResolvedValue(undefined); @@ -154,6 +214,26 @@ describe('AuthService inactive-user enforcement and safe logging', () => { expect(userRepository.save).toHaveBeenCalledWith(confirmationUser); }); + it('ignores a public registration player id and never mutates player ownership', async () => { + usersService.create.mockResolvedValue({ + id: 8, + email: 'new@example.com', + }); + + await service.register({ + email: 'new@example.com', + password: 'password', + firstName: 'New', + lastName: 'User', + linkPlayerId: 101, + } as any); + + expect(usersService.create.mock.calls[0][0]).not.toHaveProperty( + 'linkPlayerId', + ); + expect(usersService.linkPlayerToUserId).not.toHaveBeenCalled(); + }); + function user(statusId: StatusEnum) { return { id: 2, @@ -167,4 +247,45 @@ describe('AuthService inactive-user enforcement and safe logging', () => { }, }; } + + function socialUser( + provider: AuthProvidersEnum, + roleId: RoleEnum, + statusId: StatusEnum, + ) { + return { + ...user(statusId), + email: 'old@example.com', + socialId: `${provider}-id`, + provider, + role: { id: roleId, name: roleId === RoleEnum.admin ? 'Admin' : 'User' }, + hash: 'stale-hash', + }; + } + + function configureSocialQueries(candidates: any[], current: any) { + const candidateQuery = chain({ getMany: jest.fn(() => candidates) }); + const reloadQuery = chain({ getOne: jest.fn(() => current) }); + userRepository.createQueryBuilder = jest.fn((alias: string) => + alias === 'socialCandidate' ? candidateQuery : reloadQuery, + ); + userRepository.update = jest.fn(); + } + + function chain(overrides: Record) { + const query: Record = {}; + [ + 'leftJoinAndSelect', + 'where', + 'orWhere', + 'andWhere', + 'setParameter', + 'setParameters', + 'setLock', + 'orderBy', + ].forEach((method) => { + query[method] = jest.fn(() => query); + }); + return Object.assign(query, overrides); + } }); diff --git a/myteamwallet_backend/src/auth/auth.service.ts b/myteamwallet_backend/src/auth/auth.service.ts index 512e125..5739ceb 100644 --- a/myteamwallet_backend/src/auth/auth.service.ts +++ b/myteamwallet_backend/src/auth/auth.service.ts @@ -117,59 +117,68 @@ export class AuthService { authProvider: string, socialData: SocialInterface, ): Promise<{ token: string; user: User }> { - let user: User; const socialEmail = socialData.email?.toLowerCase(); - - const userByEmail = await this.usersService.findOne({ - email: socialEmail, - }); - - user = await this.usersService.findOne({ - socialId: socialData.id, - provider: authProvider, - }); - - if (user) { - await this.assertActiveUser(user); - if (socialEmail && !userByEmail) { - user.email = socialEmail; + return this.dataSource.transaction(async (manager) => { + const repository = manager.getRepository(User); + const candidateQuery = repository + .createQueryBuilder('socialCandidate') + .where( + 'socialCandidate.socialId = :socialId AND socialCandidate.provider = :authProvider', + { socialId: socialData.id, authProvider }, + ); + if (socialEmail) { + candidateQuery.orWhere('socialCandidate.email = :socialEmail', { + 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, + const candidates = await candidateQuery + .setLock('pessimistic_write', undefined, ['socialCandidate']) + .orderBy('socialCandidate.id', 'ASC') + .getMany(); + const socialUser = candidates.find( + (candidate) => + candidate.socialId === socialData.id && + candidate.provider === authProvider, + ); + const emailUser = socialEmail + ? candidates.find((candidate) => candidate.email === socialEmail) + : undefined; + let user = socialUser ?? emailUser; + if (!user) { + user = await repository.save( + repository.create({ + email: socialEmail, + firstName: socialData.firstName, + lastName: socialData.lastName, + socialId: socialData.id, + provider: authProvider, + role: { id: RoleEnum.user } as Role, + status: { id: StatusEnum.active } as Status, + }), + ); + } else if ( + socialUser && + socialEmail && + !emailUser && + socialUser.email !== socialEmail + ) { + await repository.update(socialUser.id, { email: socialEmail }); + } + const currentUser = await repository + .createQueryBuilder('currentSocialUser') + .leftJoinAndSelect('currentSocialUser.role', 'role') + .leftJoinAndSelect('currentSocialUser.status', 'status') + .where('currentSocialUser.id = :userId', { userId: user.id }) + .setLock('pessimistic_write', undefined, ['currentSocialUser']) + .getOne(); + if (!currentUser) throw new UnauthorizedException(); + await this.assertActiveUser(currentUser); + const token = await this.jwtService.sign({ + id: currentUser.id, + role: currentUser.role, }); - const status = plainToClass(Status, { - id: StatusEnum.active, - }); - - user = await this.usersService.create({ - email: socialEmail, - firstName: socialData.firstName, - lastName: socialData.lastName, - socialId: socialData.id, - provider: authProvider, - role, - status, - }); - - user = await this.usersService.findOne({ - id: user.id, - }); - } - - const jwtToken = await this.jwtService.sign({ - id: user.id, - role: user.role, + return { token, user: currentUser }; }); - - return { - token: jwtToken, - user, - }; } async register(dto: AuthRegisterLoginDto): Promise { @@ -179,8 +188,10 @@ export class AuthService { .digest('hex'); const user = await this.usersService.create({ - ...dto, email: dto.email, + password: dto.password, + firstName: dto.firstName, + lastName: dto.lastName, role: { id: RoleEnum.user, } as Role, @@ -190,10 +201,6 @@ export class AuthService { hash, }); - if (user && dto.linkPlayerId != null) { - await this.usersService.linkPlayerToUserId(user, dto.linkPlayerId); - } - await this.logger.info({ event: 'user_create', details: `userId=${user.id}`, diff --git a/myteamwallet_backend/src/auth/dto/auth-register-login.dto.ts b/myteamwallet_backend/src/auth/dto/auth-register-login.dto.ts index 6924cb3..dd10e97 100644 --- a/myteamwallet_backend/src/auth/dto/auth-register-login.dto.ts +++ b/myteamwallet_backend/src/auth/dto/auth-register-login.dto.ts @@ -23,7 +23,4 @@ export class AuthRegisterLoginDto { @ApiProperty({ example: 'Doe' }) @IsNotEmpty() lastName: string; - - @ApiProperty({ example: 27 }) - linkPlayerId: number | null; } diff --git a/myteamwallet_backend/src/users/admin-users.controller.spec.ts b/myteamwallet_backend/src/users/admin-users.controller.spec.ts index 203b37d..7cfd986 100644 --- a/myteamwallet_backend/src/users/admin-users.controller.spec.ts +++ b/myteamwallet_backend/src/users/admin-users.controller.spec.ts @@ -98,6 +98,17 @@ describe('admin user DTOs', () => { expect(await validate(status)).not.toEqual([]); }); + it.each([true, '1'])( + 'rejects non-number role and status bodies: %p', + async (value) => { + const role = plainToInstance(AdminUserRoleDto, { role: value }); + const status = plainToInstance(AdminUserStatusDto, { status: value }); + + 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', diff --git a/myteamwallet_backend/src/users/admin-users.http.spec.ts b/myteamwallet_backend/src/users/admin-users.http.spec.ts new file mode 100644 index 0000000..37930ac --- /dev/null +++ b/myteamwallet_backend/src/users/admin-users.http.spec.ts @@ -0,0 +1,173 @@ +import { + INestApplication, + UnauthorizedException, + ValidationPipe, + VersioningType, +} from '@nestjs/common'; +import { Reflector } from '@nestjs/core'; +import { AuthGuard } from '@nestjs/passport'; +import { Test } from '@nestjs/testing'; +import * as request from 'supertest'; +import { useContainer } from 'class-validator'; +import { DataSource } from 'typeorm'; +import { AuthController } from '../auth/auth.controller'; +import { AuthService } from '../auth/auth.service'; +import { RoleEnum } from '../roles/roles.enum'; +import { RolesGuard } from '../roles/roles.guard'; +import validationOptions from '../utils/validation-options'; +import { IsNotExist } from '../utils/validators/is-not-exists.validator'; +import { AdminUsersController } from './admin-users.controller'; +import { AdminUsersService } from './admin-users.service'; +import { UsersController } from './users.controller'; +import { UsersService } from './users.service'; + +describe('admin user HTTP security boundary', () => { + let app: INestApplication; + const safeUser = { + id: 2, + firstName: 'Target', + lastName: 'User', + email: 'target@example.com', + role: { id: RoleEnum.user, name: 'User' }, + status: { id: 1, name: 'Active' }, + assignments: [], + }; + const adminService = { + updateProfile: jest.fn(() => safeUser), + updateRole: jest.fn(() => safeUser), + updateStatus: jest.fn(() => safeUser), + findPlayers: jest.fn(), + assignPlayer: jest.fn(), + unlinkPlayer: jest.fn(), + }; + const usersService = { + findDirectory: jest.fn(), + findMyTeams: jest.fn(() => [ + { + id: 11, + firstName: 'Pat', + lastName: 'Player', + team: { id: 4, name: 'Alpha' }, + }, + ]), + }; + const authService = { register: jest.fn() }; + + beforeAll(async () => { + const module = await Test.createTestingModule({ + controllers: [AdminUsersController, UsersController, AuthController], + providers: [ + Reflector, + RolesGuard, + IsNotExist, + { + provide: DataSource, + useValue: { + getRepository: () => ({ findOne: jest.fn(() => undefined) }), + }, + }, + { provide: AdminUsersService, useValue: adminService }, + { provide: UsersService, useValue: usersService }, + { provide: AuthService, useValue: authService }, + ], + }) + .overrideGuard(AuthGuard('jwt')) + .useValue({ + canActivate(context) { + const httpRequest = context.switchToHttp().getRequest(); + const token = httpRequest.headers.authorization; + if (token === 'Bearer admin') { + httpRequest.user = { id: 1, role: { id: RoleEnum.admin } }; + return true; + } + if (token === 'Bearer user') { + httpRequest.user = { id: 7, role: { id: RoleEnum.user } }; + return true; + } + throw new UnauthorizedException(); + }, + }) + .compile(); + app = module.createNestApplication(); + useContainer(app, { fallbackOnErrors: true }); + app.setGlobalPrefix('api'); + app.enableVersioning({ type: VersioningType.URI }); + app.useGlobalPipes(new ValidationPipe(validationOptions)); + await app.init(); + }); + + afterAll(() => app.close()); + beforeEach(() => jest.clearAllMocks()); + + it('enforces JWT and current global admin role on an admin mutation', async () => { + await request(app.getHttpServer()) + .patch('/api/v1/admin/users/2/role') + .send({ role: RoleEnum.user }) + .expect(401); + await request(app.getHttpServer()) + .patch('/api/v1/admin/users/2/role') + .set('Authorization', 'Bearer user') + .send({ role: RoleEnum.user }) + .expect(403); + await request(app.getHttpServer()) + .patch('/api/v1/admin/users/2/role') + .set('Authorization', 'Bearer admin') + .send({ role: RoleEnum.user }) + .expect(200, safeUser); + }); + + it.each([true, '1'])( + 'rejects non-numeric role JSON at the HTTP validation boundary: %p', + async (role) => { + await request(app.getHttpServer()) + .patch('/api/v1/admin/users/2/role') + .set('Authorization', 'Bearer admin') + .send({ role }) + .expect(422); + expect(adminService.updateRole).not.toHaveBeenCalled(); + }, + ); + + it('strips public player assignment input from registration', async () => { + await request(app.getHttpServer()) + .post('/api/v1/auth/email/register') + .send({ + email: 'new@example.com', + password: 'password', + firstName: 'New', + lastName: 'User', + linkPlayerId: 101, + }) + .expect(201); + + expect(authService.register).toHaveBeenCalledWith({ + email: 'new@example.com', + password: 'password', + firstName: 'New', + lastName: 'User', + }); + }); + + it('restricts team bootstrap to self and serializes only the safe projection', async () => { + await request(app.getHttpServer()) + .get('/api/v1/users/8/teams') + .set('Authorization', 'Bearer user') + .expect(403); + const response = await request(app.getHttpServer()) + .get('/api/v1/users/7/teams') + .set('Authorization', 'Bearer user') + .expect(200); + + expect(response.body).toEqual([ + { + id: 11, + firstName: 'Pat', + lastName: 'Player', + team: { id: 4, name: 'Alpha' }, + }, + ]); + expect(JSON.stringify(response.body)).not.toMatch( + /provider|socialId|password|hash|balance|createdAt|updatedAt/i, + ); + }); +}); diff --git a/myteamwallet_backend/src/users/dto/admin-user.dto.ts b/myteamwallet_backend/src/users/dto/admin-user.dto.ts index 6efef9a..07b654d 100644 --- a/myteamwallet_backend/src/users/dto/admin-user.dto.ts +++ b/myteamwallet_backend/src/users/dto/admin-user.dto.ts @@ -24,13 +24,13 @@ export class AdminUserProfileDto { } export class AdminUserRoleDto { - @Type(() => Number) + @IsInt() @IsIn([RoleEnum.admin, RoleEnum.user]) role: RoleEnum; } export class AdminUserStatusDto { - @Type(() => Number) + @IsInt() @IsIn([StatusEnum.active, StatusEnum.inactive]) status: StatusEnum; } diff --git a/myteamwallet_backend/src/users/dto/create-user.dto.ts b/myteamwallet_backend/src/users/dto/create-user.dto.ts index 94b4819..feebb79 100644 --- a/myteamwallet_backend/src/users/dto/create-user.dto.ts +++ b/myteamwallet_backend/src/users/dto/create-user.dto.ts @@ -1,13 +1,7 @@ import { Transform } from 'class-transformer'; import { ApiProperty } from '@nestjs/swagger'; import { Role } from '../../roles/entities/role.entity'; -import { - IsEmail, - IsNotEmpty, - IsOptional, - MinLength, - Validate, -} from 'class-validator'; +import { IsEmail, IsNotEmpty, MinLength, Validate } from 'class-validator'; import { Status } from '../../statuses/entities/status.entity'; import { IsNotExist } from '../../utils/validators/is-not-exists.validator'; import { IsExist } from '../../utils/validators/is-exists.validator'; @@ -51,6 +45,4 @@ export class CreateUserDto { status?: Status; hash?: string | null; - - linkPlayerId?: number | null; } diff --git a/myteamwallet_backend/src/users/dto/user-team-response.dto.ts b/myteamwallet_backend/src/users/dto/user-team-response.dto.ts new file mode 100644 index 0000000..af78024 --- /dev/null +++ b/myteamwallet_backend/src/users/dto/user-team-response.dto.ts @@ -0,0 +1,11 @@ +export class UserTeamReferenceDto { + id: number; + name: string; +} + +export class UserTeamPlayerDto { + id: number; + firstName: string; + lastName: string; + team: UserTeamReferenceDto; +} diff --git a/myteamwallet_backend/src/users/users.controller.security.spec.ts b/myteamwallet_backend/src/users/users.controller.security.spec.ts index 45f456a..4150fc6 100644 --- a/myteamwallet_backend/src/users/users.controller.security.spec.ts +++ b/myteamwallet_backend/src/users/users.controller.security.spec.ts @@ -1,4 +1,5 @@ import { UsersController } from './users.controller'; +import { ForbiddenException } from '@nestjs/common'; describe('UsersController admin mutation isolation', () => { it('does not expose generic create, update, or delete handlers that bypass safeguards', () => { @@ -8,4 +9,17 @@ describe('UsersController admin mutation isolation', () => { expect(UsersController.prototype).not.toHaveProperty('findAll'); expect(UsersController.prototype).not.toHaveProperty('findOne'); }); + + it('restricts team bootstrap to the authenticated user id', async () => { + const service = { findMyTeams: jest.fn(() => []) }; + const controller = new UsersController(service as any); + const request = { user: { id: 7, role: { id: 2 } } }; + + expect(() => (controller as any).findTeamsOfPlayer(request, 8)).toThrow( + ForbiddenException, + ); + await (controller as any).findTeamsOfPlayer(request, 7); + + expect(service.findMyTeams).toHaveBeenCalledWith(7); + }); }); diff --git a/myteamwallet_backend/src/users/users.controller.ts b/myteamwallet_backend/src/users/users.controller.ts index 1ea9fda..c527f36 100644 --- a/myteamwallet_backend/src/users/users.controller.ts +++ b/myteamwallet_backend/src/users/users.controller.ts @@ -7,6 +7,8 @@ import { HttpStatus, HttpCode, Request, + ForbiddenException, + ParseIntPipe, } from '@nestjs/common'; import { UsersService } from './users.service'; import { ApiBearerAuth, ApiTags } from '@nestjs/swagger'; @@ -40,7 +42,13 @@ export class UsersController { @Roles([RoleEnum.user, RoleEnum.admin]) @Get(':id/teams') @HttpCode(HttpStatus.OK) - findTeamsOfPlayer(@Param('id') id: string) { - return this.usersService.findTeams({ id: +id }); + findTeamsOfPlayer( + @Request() request: { user: Pick }, + @Param('id', ParseIntPipe) id: number, + ) { + if (request.user.id !== id) { + throw new ForbiddenException('Users may only load their own teams'); + } + return this.usersService.findMyTeams(id); } } diff --git a/myteamwallet_backend/src/users/users.service.ts b/myteamwallet_backend/src/users/users.service.ts index 4197060..49b2abf 100644 --- a/myteamwallet_backend/src/users/users.service.ts +++ b/myteamwallet_backend/src/users/users.service.ts @@ -16,6 +16,7 @@ import { UserDirectorySummaryDto, } from './dto/user-directory-response.dto'; import { User } from './entities/user.entity'; +import { UserTeamPlayerDto } from './dto/user-team-response.dto'; @Injectable() export class UsersService { @@ -113,33 +114,32 @@ export class UsersService { await this.usersRepository.softDelete(id); } - async findTeams(fields: EntityCondition) { - const user = await this.findOne(fields); - if (!user) { - return []; - } - - const players = await this.playersRepository.find({ - where: { - user: { - id: user.id, - }, - }, - relations: ['team'], - }); - return players; - } - - async linkPlayerToUserId(user: User, playerId: number): Promise { - return new Promise(async (resolve) => { - const player = await this.playersRepository.findOneByOrFail({ - id: playerId, - }); - - player.user = user; - await this.playersRepository.save(player); - return resolve(true); - }); + async findMyTeams(userId: number): Promise { + const rows = await this.playersRepository + .createQueryBuilder('player') + .innerJoin('player.team', 'team') + .select([ + 'player.id AS player_id', + 'player.firstName AS first_name', + 'player.lastName AS last_name', + 'team.id AS team_id', + 'team.name AS team_name', + ]) + .where('player.userId = :userId', { userId }) + .orderBy('player.id', 'ASC') + .getRawMany<{ + player_id: number | string; + first_name: string; + last_name: string; + team_id: number | string; + team_name: string; + }>(); + return rows.map((row) => ({ + id: Number(row.player_id), + firstName: row.first_name, + lastName: row.last_name, + team: { id: Number(row.team_id), name: row.team_name }, + })); } private createSharedTeamsQuery(requesterId: number) { diff --git a/myteamwallet_backend/src/users/users.teams.spec.ts b/myteamwallet_backend/src/users/users.teams.spec.ts new file mode 100644 index 0000000..525ae6d --- /dev/null +++ b/myteamwallet_backend/src/users/users.teams.spec.ts @@ -0,0 +1,49 @@ +import { UsersService } from './users.service'; + +describe('UsersService safe current-user teams', () => { + it('returns only the player and team fields required by team bootstrap', async () => { + const rows = [ + { + player_id: 11, + first_name: 'Pat', + last_name: 'Player', + team_id: 4, + team_name: 'Alpha', + user_id: 7, + provider: 'google', + social_id: 'must-not-leak', + balance: '100.00', + }, + ]; + const query = chain({ getRawMany: jest.fn(() => rows) }); + const playersRepository = { + createQueryBuilder: jest.fn(() => query), + }; + const service = new UsersService({} as any, playersRepository as any); + + const result = await service.findMyTeams(7); + + expect(result).toEqual([ + { + id: 11, + firstName: 'Pat', + lastName: 'Player', + team: { id: 4, name: 'Alpha' }, + }, + ]); + expect(JSON.stringify(result)).not.toMatch( + /user|provider|social|role|status|password|hash|balance|createdAt|updatedAt/i, + ); + expect(query.where).toHaveBeenCalledWith('player.userId = :userId', { + userId: 7, + }); + }); + + function chain(overrides: Record) { + const query: Record = {}; + ['innerJoin', 'select', 'where', 'orderBy'].forEach((method) => { + query[method] = jest.fn(() => query); + }); + return Object.assign(query, overrides); + } +});