fix: query user directory safely

2026-07-31 22:29:15 +02:00
parent c382234746
commit d738b49cbf
3 changed files with 428 additions and 99 deletions
--- a/.superpowers/sdd/admin-user-management/task-1-report.md
+++ b/.superpowers/sdd/admin-user-management/task-1-report.md
@@ -65,5 +65,55 @@ Passed with no whitespace errors.

 ## Concerns

- The service intentionally fetches the user and player directory sets and applies the authorization filter in memory. Task 2's planned foreign-key index work can support a future query-builder optimization without changing this safe response contract.
 - 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.
--- a/myteamwallet_backend/src/users/users.service.spec.ts
+++ b/myteamwallet_backend/src/users/users.service.spec.ts
@@ -32,14 +32,33 @@ describe('UsersService directory', () => {
    assignment(502, users[4], teamB),
  ];

-  const usersRepository = { find: jest.fn() };
-  const playersRepository = { find: jest.fn() };
+  let userRows: ReturnType<typeof directoryUserRow>[];
+  let assignmentRows: ReturnType<typeof directoryAssignmentRow>[];
+  let total: number;
+  const usersRepository = {
+    find: jest.fn(() => {
+      throw new Error('directory queries must use a safe database projection');
+    }),
+    createQueryBuilder: jest.fn(() => createDirectoryUsersQuery()),
+  };
+  const playersRepository = {
+    find: jest.fn(() => {
+      throw new Error('directory queries must use a safe database projection');
+    }),
+    createQueryBuilder: jest.fn((alias: string) =>
+      alias === 'requesterPlayer'
+        ? createSharedTeamsQuery()
+        : createDirectoryAssignmentsQuery(),
+    ),
+  };
  let service: UsersService;

  beforeEach(() => {
-    jest.resetAllMocks();
-    usersRepository.find.mockResolvedValue(users);
-    playersRepository.find.mockResolvedValue(players);
+    jest.clearAllMocks();
+    setDirectoryResult(
+      [users[0], users[1], users[2], users[4]],
+      [players[0], players[1], players[2], players[4]],
+    );
    service = new UsersService(
      usersRepository as any,
      playersRepository as any,
@@ -85,7 +104,34 @@ describe('UsersService directory', () => {
    });
  });

+  it('treats an inactive requester assignment as a shared team membership', async () => {
+    setDirectoryResult(
+      [users[0], users[1]],
+      [
+        assignment(101, users[0], teamA, false),
+        assignment(201, users[1], teamA),
+      ],
+    );
+
+    const result = await directoryFor(users[0]);
+
+    expect(result.data.map((entry) => entry.id)).toEqual([1, 2]);
+  });
+
+  it('does not hydrate whole user entities for the directory', async () => {
+    const result = await directoryFor(users[0]);
+
+    expect(
+      result.data.every(
+        (entry) =>
+          !('password' in entry || 'hash' in entry || 'socialId' in entry),
+      ),
+    ).toBe(true);
+  });
+
  it('returns every user and assignment with email and role for an admin requester', async () => {
+    setDirectoryResult(users, players, true);
+
    const result = await directoryFor(users[5]);
    const multiple = result.data.find((entry) => entry.id === 5);
    const outsider = result.data.find((entry) => entry.id === 4);
@@ -105,6 +151,13 @@ describe('UsersService directory', () => {
  });

  it('deduplicates a user with assignments in more than one shared team before pagination', async () => {
+    setDirectoryResult(
+      [users[2], users[4]],
+      [players[2], players[4]],
+      false,
+      4,
+    );
+
    const result = await directoryFor(users[0], { page: 2, limit: 2 });

    expect(result.data.map((entry) => entry.id)).toEqual([3, 5]);
@@ -113,7 +166,9 @@ describe('UsersService directory', () => {
  });

  it('searches visible names case-insensitively without exposing outside-team users', async () => {
+    setDirectoryResult([users[4]], [players[4]]);
    const matched = await directoryFor(users[0], { search: 'mOrGaN' });
+    setDirectoryResult([], []);
    const hidden = await directoryFor(users[0], { search: 'outside' });

    expect(matched.data.map((entry) => entry.id)).toEqual([5]);
@@ -121,6 +176,13 @@ describe('UsersService directory', () => {
  });

  it('paginates the deduplicated, filtered directory and reports the next page', async () => {
+    setDirectoryResult(
+      [users[0], users[1]],
+      [players[0], players[1]],
+      false,
+      4,
+    );
+
    const result = await directoryFor(users[0], { page: 1, limit: 2 });

    expect(result).toMatchObject({
@@ -205,4 +267,111 @@ describe('UsersService directory', () => {
      teamRole: playerRole,
    };
  }
+
+  function setDirectoryResult(
+    directoryUsers: typeof users,
+    directoryPlayers: typeof players,
+    isAdmin = false,
+    directoryTotal = directoryUsers.length,
+  ) {
+    userRows = directoryUsers.map((directoryUser) =>
+      directoryUserRow(directoryUser, isAdmin),
+    );
+    assignmentRows = directoryPlayers.map(directoryAssignmentRow);
+    total = directoryTotal;
+  }
+
+  function directoryUserRow(
+    directoryUser: typeof users[number],
+    isAdmin = false,
+  ) {
+    return {
+      user_id: directoryUser.id,
+      first_name: directoryUser.firstName,
+      last_name: directoryUser.lastName,
+      status_id: directoryUser.status.id,
+      status_name: directoryUser.status.name,
+      ...(isAdmin
+        ? {
+            email: directoryUser.email,
+            role_id: directoryUser.role.id,
+            role_name: directoryUser.role.name,
+          }
+        : {}),
+    };
+  }
+
+  function directoryAssignmentRow(directoryPlayer: typeof players[number]) {
+    return {
+      player_id: directoryPlayer.id,
+      user_id: directoryPlayer.user.id,
+      first_name: directoryPlayer.firstName,
+      last_name: directoryPlayer.lastName,
+      active: directoryPlayer.active,
+      team_id: directoryPlayer.team.id,
+      team_name: directoryPlayer.team.name,
+      team_alias: directoryPlayer.team.alias,
+      team_role_id: directoryPlayer.teamRole.id,
+      team_role_name: directoryPlayer.teamRole.name,
+    };
+  }
+
+  function createDirectoryUsersQuery() {
+    const query: any = {
+      leftJoin: () => query,
+      innerJoin: () => query,
+      setParameters: () => query,
+      setParameter: () => query,
+      andWhere: () => query,
+      distinct: () => query,
+      orderBy: () => query,
+      offset: () => query,
+      limit: () => query,
+      select: (selection: string | string[]) => {
+        const selected = Array.isArray(selection) ? selection : [selection];
+        if (selected.join(' ').match(/password|hash|socialId|provider/i)) {
+          throw new Error(
+            'directory projection contains an authentication field',
+          );
+        }
+        return query;
+      },
+      clone: () => createDirectoryCountQuery(),
+      getRawMany: () => Promise.resolve(userRows),
+    };
+    return query;
+  }
+
+  function createDirectoryCountQuery() {
+    const query: any = {
+      select: () => query,
+      getRawOne: () => Promise.resolve({ total: String(total) }),
+    };
+    return query;
+  }
+
+  function createSharedTeamsQuery() {
+    const query: any = {
+      select: () => query,
+      where: () => query,
+      getQuery: () =>
+        'SELECT requesterPlayer.teamId FROM player requesterPlayer',
+      getParameters: () => ({ requesterId: users[0].id }),
+    };
+    return query;
+  }
+
+  function createDirectoryAssignmentsQuery() {
+    const query: any = {
+      innerJoin: () => query,
+      leftJoin: () => query,
+      select: () => query,
+      where: () => query,
+      andWhere: () => query,
+      setParameters: () => query,
+      orderBy: () => query,
+      getRawMany: () => Promise.resolve(assignmentRows),
+    };
+    return query;
+  }
 });
--- a/myteamwallet_backend/src/users/users.service.ts
+++ b/myteamwallet_backend/src/users/users.service.ts
@@ -3,7 +3,7 @@ import { InjectRepository } from '@nestjs/typeorm';
 import { Player } from 'src/players/entities/player.entity';
 import { EntityCondition } from 'src/utils/types/entity-condition.type';
 import { IPaginationOptions } from 'src/utils/types/pagination-options';
-import { Repository } from 'typeorm';
+import { Brackets, Repository, SelectQueryBuilder } from 'typeorm';
 import { RoleEnum } from '../roles/roles.enum';
 import { CreateUserDto } from './dto/create-user.dto';
 import { UpdateUserDto } from './dto/update-user.dto';
@@ -14,7 +14,6 @@ import {
  UserDirectoryPageDto,
  UserDirectoryReferenceDto,
  UserDirectorySummaryDto,
-  UserDirectoryTeamDto,
 } from './dto/user-directory-response.dto';
 import { User } from './entities/user.entity';

@@ -44,49 +43,44 @@ export class UsersService {
    requester: Pick<User, 'id' | 'role'>,
    query: UserDirectoryQueryDto,
  ): Promise<UserDirectoryPageDto> {
-    const [users, players] = await Promise.all([
-      this.usersRepository.find({ order: { id: 'ASC' } }),
-      this.playersRepository.find({
-        relations: ['user', 'team', 'teamRole'],
-        order: { id: 'ASC' },
-      }),
-    ]);
    const isAdmin = requester.role?.id === RoleEnum.admin;
-    const sharedTeamIds = new Set(
-      players
-        .filter((player) => player.user?.id === requester.id && player.active)
-        .map((player) => player.team.id),
-    );
-    const assignmentsByUserId = new Map<number, Player[]>();
-
-    for (const player of players) {
-      if (!player.user || (!isAdmin && !sharedTeamIds.has(player.team.id))) {
-        continue;
-      }
-
-      const assignments = assignmentsByUserId.get(player.user.id) ?? [];
-      assignments.push(player);
-      assignmentsByUserId.set(player.user.id, assignments);
-    }
-
-    const visibleUsers = users.filter(
-      (user) => isAdmin || assignmentsByUserId.has(user.id),
-    );
-    const searchedUsers = this.filterDirectorySearch(
-      visibleUsers,
-      assignmentsByUserId,
-      query.search,
-      isAdmin,
-    );
-    const total = searchedUsers.length;
    const page = query.page ?? 1;
    const limit = query.limit ?? 20;
-    const data = searchedUsers
-      .slice((page - 1) * limit, page * limit)
-      .map((user) =>
+    const sharedTeamsQuery = isAdmin
+      ? undefined
+      : this.createSharedTeamsQuery(requester.id);
+    const directoryQuery = this.createDirectoryUsersQuery(
+      isAdmin,
+      sharedTeamsQuery,
+      query.search,
+    );
+    const totalResult = await directoryQuery
+      .clone()
+      .select('COUNT(DISTINCT user.id)', 'total')
+      .getRawOne<{ total: string }>();
+    const userRows = await directoryQuery
+      .select(this.directoryUserSelects(isAdmin))
+      .distinct(true)
+      .orderBy('user.id', 'ASC')
+      .offset((page - 1) * limit)
+      .limit(limit)
+      .getRawMany<DirectoryUserRow>();
+    const assignments = await this.findDirectoryAssignments(
+      userRows.map((user) => Number(user.user_id)),
+      isAdmin,
+      sharedTeamsQuery,
+    );
+    const assignmentsByUserId = assignments.reduce((byUserId, assignment) => {
+      const userAssignments = byUserId.get(Number(assignment.user_id)) ?? [];
+      userAssignments.push(assignment);
+      byUserId.set(Number(assignment.user_id), userAssignments);
+      return byUserId;
+    }, new Map<number, DirectoryAssignmentRow[]>());
+    const total = Number(totalResult?.total ?? 0);
+    const data = userRows.map((user) =>
      this.mapDirectoryUser(
        user,
-          assignmentsByUserId.get(user.id) ?? [],
+        assignmentsByUserId.get(Number(user.user_id)) ?? [],
        isAdmin,
      ),
    );
@@ -148,46 +142,132 @@ export class UsersService {
    });
  }

-  private filterDirectorySearch(
-    users: User[],
-    assignmentsByUserId: Map<number, Player[]>,
-    search: string | undefined,
-    includeEmail: boolean,
-  ): User[] {
-    const term = search?.trim().toLocaleLowerCase();
-    if (!term) {
-      return users;
+  private createSharedTeamsQuery(requesterId: number) {
+    return this.playersRepository
+      .createQueryBuilder('requesterPlayer')
+      .select('requesterPlayer.teamId')
+      .where('requesterPlayer.userId = :requesterId', { requesterId });
  }

-    return users.filter((user) => {
-      const assignments = assignmentsByUserId.get(user.id) ?? [];
-      const values = [
-        user.firstName,
-        user.lastName,
-        ...(includeEmail ? [user.email] : []),
-        ...assignments.flatMap((assignment) => [
-          assignment.firstName,
-          assignment.lastName,
-        ]),
-      ];
+  private createDirectoryUsersQuery(
+    isAdmin: boolean,
+    sharedTeamsQuery: SelectQueryBuilder<Player> | undefined,
+    search: string | undefined,
+  ) {
+    const query = this.usersRepository
+      .createQueryBuilder('user')
+      .leftJoin('user.status', 'status')
+      .leftJoin('user.role', 'role');

-      return values.some((value) => value?.toLocaleLowerCase().includes(term));
+    if (!isAdmin && sharedTeamsQuery) {
+      query
+        .innerJoin(
+          'user.players',
+          'visibleAssignment',
+          `visibleAssignment.teamId IN (${sharedTeamsQuery.getQuery()})`,
+        )
+        .setParameters(sharedTeamsQuery.getParameters());
+    }
+
+    const term = search?.trim().toLocaleLowerCase();
+    if (!term) {
+      return query;
+    }
+
+    const assignmentAlias = isAdmin ? 'searchAssignment' : 'visibleAssignment';
+    if (isAdmin) {
+      query.leftJoin('user.players', assignmentAlias);
+    }
+
+    const searchColumns = [
+      'user.firstName',
+      'user.lastName',
+      `${assignmentAlias}.firstName`,
+      `${assignmentAlias}.lastName`,
+      ...(isAdmin ? ['user.email'] : []),
+    ];
+    query.andWhere(
+      new Brackets((where) => {
+        searchColumns.forEach((column, index) => {
+          const expression = `LOWER(${column}) LIKE :directorySearch`;
+          if (index === 0) {
+            where.where(expression);
+          } else {
+            where.orWhere(expression);
+          }
        });
+      }),
+    );
+    return query.setParameter('directorySearch', `%${term}%`);
+  }
+
+  private directoryUserSelects(isAdmin: boolean): string[] {
+    return [
+      'user.id AS user_id',
+      'user.firstName AS first_name',
+      'user.lastName AS last_name',
+      'status.id AS status_id',
+      'status.name AS status_name',
+      ...(isAdmin
+        ? [
+            'user.email AS email',
+            'role.id AS role_id',
+            'role.name AS role_name',
+          ]
+        : []),
+    ];
+  }
+
+  private async findDirectoryAssignments(
+    userIds: number[],
+    isAdmin: boolean,
+    sharedTeamsQuery: SelectQueryBuilder<Player> | undefined,
+  ): Promise<DirectoryAssignmentRow[]> {
+    if (userIds.length === 0) {
+      return [];
+    }
+
+    const query = this.playersRepository
+      .createQueryBuilder('player')
+      .innerJoin('player.team', 'team')
+      .leftJoin('player.teamRole', 'teamRole')
+      .select([
+        'player.id AS player_id',
+        'player.userId AS user_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',
+        'teamRole.id AS team_role_id',
+        'teamRole.name AS team_role_name',
+      ])
+      .where('player.userId IN (:...userIds)', { userIds })
+      .orderBy('player.id', 'ASC');
+
+    if (!isAdmin && sharedTeamsQuery) {
+      query
+        .andWhere(`player.teamId IN (${sharedTeamsQuery.getQuery()})`)
+        .setParameters(sharedTeamsQuery.getParameters());
+    }
+
+    return query.getRawMany<DirectoryAssignmentRow>();
  }

  private mapDirectoryUser(
-    user: User,
-    assignments: Player[],
+    user: DirectoryUserRow,
+    assignments: DirectoryAssignmentRow[],
    includeAdminFields: boolean,
  ): UserDirectorySummaryDto | AdminUserDirectorySummaryDto {
    const summary: UserDirectorySummaryDto = {
-      id: user.id,
-      firstName: user.firstName,
-      lastName: user.lastName,
-      status: this.mapDirectoryReference(user.status),
-      assignments: assignments
-        .sort((left, right) => left.id - right.id)
-        .map((assignment) => this.mapDirectoryAssignment(assignment)),
+      id: Number(user.user_id),
+      firstName: user.first_name,
+      lastName: user.last_name,
+      status: this.mapDirectoryReference(user.status_id, user.status_name),
+      assignments: assignments.map((assignment) =>
+        this.mapDirectoryAssignment(assignment),
+      ),
    };

    if (!includeAdminFields) {
@@ -197,39 +277,69 @@ export class UsersService {
    return {
      ...summary,
      email: user.email,
-      role: this.mapDirectoryReference(user.role),
+      role: this.mapDirectoryReference(user.role_id, user.role_name),
    };
  }

-  private mapDirectoryAssignment(player: Player): UserDirectoryAssignmentDto {
+  private mapDirectoryAssignment(
+    player: DirectoryAssignmentRow,
+  ): UserDirectoryAssignmentDto {
    return {
-      id: player.id,
-      firstName: player.firstName,
-      lastName: player.lastName,
-      active: player.active,
-      team: this.mapDirectoryTeam(player.team),
-      teamRole: this.mapDirectoryReference(player.teamRole),
+      id: Number(player.player_id),
+      firstName: player.first_name,
+      lastName: player.last_name,
+      active: this.asBoolean(player.active),
+      team: {
+        id: Number(player.team_id),
+        name: player.team_name,
+        alias: player.team_alias,
+      },
+      teamRole: this.mapDirectoryReference(
+        player.team_role_id,
+        player.team_role_name,
+      ),
    };
  }

-  private mapDirectoryTeam(team: Player['team']): UserDirectoryTeamDto {
-    return {
-      id: team.id,
-      name: team.name,
-      alias: team.alias,
-    };
+  private asBoolean(value: boolean | number | string): boolean {
+    return value === true || value === 1 || value === '1';
  }

  private mapDirectoryReference(
-    reference: { id: number; name?: string } | null | undefined,
+    id: number | string | null,
+    name: string | null,
  ): UserDirectoryReferenceDto | null {
-    if (!reference) {
+    if (id == null) {
      return null;
    }

    return {
-      id: reference.id,
-      name: reference.name,
+      id: Number(id),
+      name: name ?? undefined,
    };
  }
 }
+
+interface DirectoryUserRow {
+  user_id: number | string;
+  first_name: string | null;
+  last_name: string | null;
+  status_id: number | string | null;
+  status_name: string | null;
+  email?: string | null;
+  role_id?: number | string | null;
+  role_name?: string | null;
+}
+
+interface DirectoryAssignmentRow {
+  player_id: number | string;
+  user_id: number | string;
+  first_name: string;
+  last_name: string;
+  active: boolean | number | string;
+  team_id: number | string;
+  team_name: string;
+  team_alias: string;
+  team_role_id: number | string | null;
+  team_role_name: string | null;
+}