190 lines
13 KiB
Markdown
190 lines
13 KiB
Markdown
# 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.
|
|
|
|
## 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.
|