From 78761b570b103a5f928a1aaeee55394fb1029543 Mon Sep 17 00:00:00 2001 From: Bastian Wagner Date: Sat, 1 Aug 2026 00:21:26 +0200 Subject: [PATCH] chore: untrack SDD reports --- .../admin-user-management/task-1-report.md | 161 --------------- .../admin-user-management/task-2-report.md | 189 ------------------ 2 files changed, 350 deletions(-) delete mode 100644 .superpowers/sdd/admin-user-management/task-1-report.md delete mode 100644 .superpowers/sdd/admin-user-management/task-2-report.md diff --git a/.superpowers/sdd/admin-user-management/task-1-report.md b/.superpowers/sdd/admin-user-management/task-1-report.md deleted file mode 100644 index 3220667..0000000 --- a/.superpowers/sdd/admin-user-management/task-1-report.md +++ /dev/null @@ -1,161 +0,0 @@ -# Task 1 implementation report: backend directory contract and query - -## Files changed - -- `myteamwallet_backend/src/users/dto/user-directory-query.dto.ts` — page, limit, and optional search input validation. -- `myteamwallet_backend/src/users/dto/user-directory-response.dto.ts` — explicit safe directory, admin, assignment, team, and reference response DTOs. -- `myteamwallet_backend/src/users/users.service.ts` — scoped directory query, search, pagination, deduplication, and explicit entity-to-DTO mapping. -- `myteamwallet_backend/src/users/users.controller.ts` — authenticated `GET /api/v1/users/directory` endpoint, declared before `:id`. -- `myteamwallet_backend/src/users/users.service.spec.ts` — focused contract coverage. - -## RED test evidence - -Command: - -```powershell -npm test -- users/users.service.spec.ts --runInBand -``` - -Result: failed as expected, 7/7 tests failed with `TypeError: service.findDirectory is not a function`. This proved the missing directory-query behavior before implementation. - -## GREEN verification - -Commands and results: - -```powershell -npm test -- users/users.service.spec.ts --runInBand -``` - -Passed: 1 suite, 7 tests. Covers cross-team isolation, non-admin email/secret redaction, inactive visibility, admin visibility, deduplication before pagination, search, and pagination metadata. - -```powershell -.\node_modules\.bin\eslint.cmd src\users\users.service.ts src\users\users.controller.ts src\users\users.service.spec.ts src\users\dto\user-directory-query.dto.ts src\users\dto\user-directory-response.dto.ts --max-warnings=0 -``` - -Passed with no warnings or errors. - -```powershell -npm run build -``` - -Passed: Nest build completed successfully. - -```powershell -git diff --check -``` - -Passed with no whitespace errors. - -## Design notes - -- `findDirectory(requester, query)` returns `{ data, page, limit, total, hasNextPage }`. -- A non-admin's shared-team set is derived from their active player assignments. Only users with an assignment in that set are included, and each returned assignment is filtered to that same set. -- Inactive target users and inactive assignments remain visible when their team is shared. -- Admins receive all non-deleted users and every linked player assignment. Their records extend the safe base summary with `email` and the existing `{ id, name }` role shape. -- The query maps selected DTO fields explicitly. It never serializes a `User` or `Player` entity, so passwords, hashes, social IDs, providers, and other authentication fields cannot leak through this endpoint. -- User IDs are ordered before search/pagination for deterministic pages. Users are the primary result set, which guarantees deduplication before pagination even when they have multiple player assignments. - -## Self-review - -- Confirmed `GET directory` is registered before `GET :id`. -- Confirmed non-admin searches only operate after visibility filtering and do not include email. -- Confirmed admin search may include email and admin mapping includes role/status using the backend's existing `{ id, name }` shapes. -- Confirmed an admin with no player assignment is included and an unassigned non-admin is not exposed to other non-admins. -- Confirmed assignment mapping includes team/team-role summary fields only, never its linked user entity. - -## Concerns - -- The repository-wide Jest suite has documented pre-existing placeholder dependency failures in the SDD ledger; this task verified its focused suite, lint, build, and whitespace check. - -## Fix Round 1 - -### Files changed - -- `myteamwallet_backend/src/users/users.service.ts` — replaces whole-entity loading with database-side raw projections for visibility, search, distinct count, deterministic ordering, pagination, and assignment filtering. -- `myteamwallet_backend/src/users/users.service.spec.ts` — adds the inactive-requester regression and runs the directory contract against query-builder doubles that reject entity hydration and unsafe projected authentication fields. - -### RED evidence - -Test file: `myteamwallet_backend/src/users/users.service.spec.ts` - -Command: - -```powershell -npm test -- users/users.service.spec.ts --runInBand -``` - -Result: failed as expected with 2 failures. `treats an inactive requester assignment as a shared team membership` received `[]` instead of `[1, 2]`; `does not hydrate whole user entities for the directory` rejected with `directory queries must use a safe database projection` because the old code called `usersRepository.find`. - -### GREEN verification - -```powershell -npm test -- users/users.service.spec.ts --runInBand -``` - -Passed: 1 suite, 9 tests. - -```powershell -.\node_modules\.bin\eslint.cmd src\users\users.service.ts src\users\users.service.spec.ts --max-warnings=0 -``` - -Passed with no warnings or errors. - -```powershell -npm run build -``` - -Passed: Nest build completed successfully. - -```powershell -git diff --check -``` - -Passed with no whitespace errors. - -### Implementation notes - -- Shared-team membership now uses every requester `Player` row, including inactive ones, exactly as required by the directory plan. -- The user query joins only `status` and `role`, projects safe raw columns, applies shared-team visibility/search in SQL, counts `DISTINCT user.id`, orders by `user.id`, and applies offset/limit before mapping. -- Assignment rows are fetched only for the selected page of user IDs and are scoped with the same shared-team subquery for non-admins. No directory query selects or hydrates `User` authentication columns. - -## Fix Round 2 - -### Files changed - -- `myteamwallet_backend/src/users/users.service.spec.ts` — strengthens the inactive-requester regression with a QueryBuilder boundary that rejects `requesterPlayer.active` in the shared-team predicate. - -### RED evidence - -Test file: `myteamwallet_backend/src/users/users.service.spec.ts` - -After installing the boundary guard, the shared-team query was deliberately mutated to add `requesterPlayer.active = :active`. - -```powershell -npm test -- users/users.service.spec.ts --runInBand -``` - -Result: failed as expected, 1/9 tests failed. `treats an inactive requester assignment as a shared team membership` failed with `shared-team membership must not filter inactive requester assignments`. The mutation was then removed; the production query remains user-ID-only. - -### GREEN verification - -```powershell -npm test -- users/users.service.spec.ts --runInBand -``` - -Passed: 1 suite, 9 tests. - -```powershell -.\node_modules\.bin\eslint.cmd src\users\users.service.spec.ts --max-warnings=0 -``` - -Passed with no warnings or errors. - -```powershell -git diff --check -``` - -Passed with no whitespace errors. - -### Implementation notes - -- The test double checks the actual shared-team predicate supplied by the service, rather than returning fixed rows alone. It rejects only for the inactive-requester regression if a predicate references `requesterPlayer.active`, so the test now fails for the realistic authorization regression while preserving the existing output-contract assertions. diff --git a/.superpowers/sdd/admin-user-management/task-2-report.md b/.superpowers/sdd/admin-user-management/task-2-report.md deleted file mode 100644 index 59d7369..0000000 --- a/.superpowers/sdd/admin-user-management/task-2-report.md +++ /dev/null @@ -1,189 +0,0 @@ -# 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.