Stop task logs and contributor pages leaking to people who shouldn't see them

src/contributors/hooks/useContributorData.ts

@@ -4,7 +4,7 @@ import getProjectPrivilege from "src/projectprivileges/queries/getProjectPrivile
 import getTeamNames from "src/teams/queries/getTeamNames"
 
 export const useContributorData = (contributorId: number, projectId: number) => {
-  const [contributor] = useQuery(getContributor, { contributorId: contributorId })
+  const [contributor] = useQuery(getContributor, { contributorId, projectId })
   const contributorUser = contributor?.users[0]
 
   const [contributorPrivilege] = useQuery(getProjectPrivilege, {

src/contributors/queries/getContributor.test.ts

@@ -0,0 +1,78 @@
+import { beforeEach, describe, expect, test, vi } from "vitest"
+
+const privileges = vi.fn()
+const memberFindFirst = vi.fn()
+vi.mock("db", () => ({
+  default: {
+    projectPrivilege: { findMany: (...args: unknown[]) => privileges(...args) },
+    projectMember: { findFirst: (...args: unknown[]) => memberFindFirst(...args) },
+  },
+  MemberPrivileges: { PROJECT_MANAGER: "PROJECT_MANAGER", CONTRIBUTOR: "CONTRIBUTOR" },
+}))
+
+import getContributor from "./getContributor"
+
+const ME = 7
+const ctx: any = { session: { userId: ME, $isAuthorized: () => true, $authorize: () => undefined } }
+const person = (userId: number) => ({ id: 41, projectId: 3, users: [{ id: userId }] })
+
+describe("getContributor", () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+  })
+
+  test("someone outside the project can't look anyone up in it", async () => {
+    privileges.mockResolvedValue([{ projectId: 99, privilege: "PROJECT_MANAGER" }])
+    await expect(getContributor({ contributorId: 41, projectId: 3 }, ctx)).rejects.toThrow(
+      "Contributor not found"
+    )
+    expect(memberFindFirst).not.toHaveBeenCalled()
+  })
+
+  test("the lookup is limited to the project in the request", async () => {
+    privileges.mockResolvedValue([{ projectId: 3, privilege: "PROJECT_MANAGER" }])
+    memberFindFirst.mockResolvedValue(person(55))
+    await getContributor({ contributorId: 41, projectId: 3 }, ctx)
+    expect(memberFindFirst.mock.calls[0]![0].where).toEqual({ id: 41, projectId: 3, name: null })
+  })
+
+  test("a contributor who isn't in that project is not found", async () => {
+    privileges.mockResolvedValue([{ projectId: 3, privilege: "PROJECT_MANAGER" }])
+    memberFindFirst.mockResolvedValue(null)
+    await expect(getContributor({ contributorId: 41, projectId: 3 }, ctx)).rejects.toThrow(
+      "Contributor not found"
+    )
+  })
+
+  test("project managers can open anyone in their project", async () => {
+    privileges.mockResolvedValue([{ projectId: 3, privilege: "PROJECT_MANAGER" }])
+    memberFindFirst.mockResolvedValue(person(55))
+    await expect(getContributor({ contributorId: 41, projectId: 3 }, ctx)).resolves.toMatchObject({
+      id: 41,
+    })
+  })
+
+  test("a contributor can open their own page", async () => {
+    privileges.mockResolvedValue([{ projectId: 3, privilege: "CONTRIBUTOR" }])
+    memberFindFirst.mockResolvedValue(person(ME))
+    await expect(getContributor({ contributorId: 41, projectId: 3 }, ctx)).resolves.toMatchObject({
+      id: 41,
+    })
+  })
+
+  test("a contributor can open a teammate's page", async () => {
+    privileges.mockResolvedValue([{ projectId: 3, privilege: "CONTRIBUTOR" }])
+    memberFindFirst.mockResolvedValueOnce(person(55)).mockResolvedValueOnce({ id: 8 }) // a shared team
+    await expect(getContributor({ contributorId: 41, projectId: 3 }, ctx)).resolves.toMatchObject({
+      id: 41,
+    })
+  })
+
+  test("a contributor can't open someone they share no team with", async () => {
+    privileges.mockResolvedValue([{ projectId: 3, privilege: "CONTRIBUTOR" }])
+    memberFindFirst.mockResolvedValueOnce(person(55)).mockResolvedValueOnce(null)
+    await expect(getContributor({ contributorId: 41, projectId: 3 }, ctx)).rejects.toThrow(
+      "Contributor not found"
+    )
+  })
+})

src/contributors/queries/getContributor.ts

@@ -1,32 +1,64 @@
 import { resolver } from "@blitzjs/rpc"
+import { NotFoundError } from "blitz"
 import db from "db"
 import { ProjectMemberWithUsers } from "src/core/types"
+import { getProjectAccess } from "src/projectprivileges/utils/getProjectAccess"
 
 interface GetContributorInput {
   contributorId: number
+  projectId: number
 }
 
 export default resolver.pipe(
   resolver.authorize(),
-  async ({ contributorId }: GetContributorInput): Promise<ProjectMemberWithUsers | null> => {
-    // Directly query the database for the specific contributor
+  async (
+    { contributorId, projectId }: GetContributorInput,
+    ctx
+  ): Promise<ProjectMemberWithUsers | null> => {
+    const userId = ctx.session.userId as number
+    const access = await getProjectAccess(userId)
+
+    // Not your project: reported as not found, so ids can't be probed
+    if (!access.memberProjectIds.includes(projectId)) {
+      throw new NotFoundError("Contributor not found")
+    }
+
+    // Directly query the database for the specific contributor, within this project
     const contributor = await db.projectMember.findFirst({
-      where: { id: contributorId, name: null }, // Ensures we're getting a contributor (where name is null)
+      where: { id: contributorId, projectId, name: null }, // name null means a person, not a team
       include: {
         users: true, // Include the related users
       },
     })
 
     // If no contributor is found, return null
     if (!contributor) {
-      throw new Error("Contributor not found")
+      throw new NotFoundError("Contributor not found")
     }
 
     // Check if the contributor has more than one user and throw an error
     if (contributor.users.length !== 1) {
       throw new Error(`Contributor has ${contributor.users.length} users! Expected exactly 1.`)
     }
 
+    // Project managers may open anyone in their project. Everyone else may open themselves and
+    // the people on a team with them, which are the contributor pages the app links them to.
+    if (!access.managerProjectIds.includes(projectId)) {
+      const person = contributor.users[0]!
+      const isSelf = person.id === userId
+      const sharesATeam =
+        !isSelf &&
+        (await db.projectMember.findFirst({
+          where: {
+            projectId,
+            name: { not: null },
+            AND: [{ users: { some: { id: userId } } }, { users: { some: { id: person.id } } }],
+          },
+          select: { id: true },
+        })) !== null
+      if (!isSelf && !sharesATeam) throw new NotFoundError("Contributor not found")
+    }
+
     return contributor // This is automatically typed as ProjectMemberWithUsers
   }
 )

src/projectprivileges/utils/getProjectAccess.ts

@@ -0,0 +1,24 @@
+import db, { MemberPrivileges } from "db"
+
+export type ProjectAccess = {
+  /** Every project the user belongs to, in any role. */
+  memberProjectIds: number[]
+  /** The projects where the user is a project manager. */
+  managerProjectIds: number[]
+}
+
+// What the signed-in user may reach on the server. Queries that take a filter from the
+// browser use this to stay inside the user's own projects, since the browser can't be trusted
+// to ask only for what it is allowed to see.
+export async function getProjectAccess(userId: number): Promise<ProjectAccess> {
+  const privileges = await db.projectPrivilege.findMany({
+    where: { userId },
+    select: { projectId: true, privilege: true },
+  })
+  return {
+    memberProjectIds: privileges.map((row) => row.projectId),
+    managerProjectIds: privileges
+      .filter((row) => row.privilege === MemberPrivileges.PROJECT_MANAGER)
+      .map((row) => row.projectId),
+  }
+}

src/tasklogs/queries/getTaskLogs.test.ts

@@ -0,0 +1,109 @@
+import { beforeEach, describe, expect, test, vi } from "vitest"
+
+// vi.mock is hoisted above these, so the mock reaches them lazily at call time
+const privileges = vi.fn()
+const taskLogFindMany = vi.fn()
+const taskLogCount = vi.fn()
+vi.mock("db", () => ({
+  default: {
+    projectPrivilege: { findMany: (...args: unknown[]) => privileges(...args) },
+    taskLog: {
+      findMany: (...args: unknown[]) => taskLogFindMany(...args),
+      count: (...args: unknown[]) => taskLogCount(...args),
+    },
+  },
+  MemberPrivileges: { PROJECT_MANAGER: "PROJECT_MANAGER", CONTRIBUTOR: "CONTRIBUTOR" },
+}))
+
+import getTaskLogs from "./getTaskLogs"
+
+const ME = 7
+const ctx: any = { session: { userId: ME, $isAuthorized: () => true, $authorize: () => undefined } }
+
+// Two projects: I manage project 1 and am a contributor in project 2 (project 3 is not mine)
+const MY_PRIVILEGES = [
+  { projectId: 1, privilege: "PROJECT_MANAGER" },
+  { projectId: 2, privilege: "CONTRIBUTOR" },
+]
+
+// What the owner lookup returns for each log id
+const OWNERS: Record<number, { projectId: number; assignedUserIds: number[] }> = {
+  10: { projectId: 1, assignedUserIds: [99] }, // someone else's, in a project I manage
+  20: { projectId: 2, assignedUserIds: [ME] }, // mine, in a project where I'm a contributor
+  21: { projectId: 2, assignedUserIds: [99] }, // someone else's, in that project
+  22: { projectId: 2, assignedUserIds: [ME, 99] }, // a team of mine
+}
+
+const fetchedLogs = (...ids: number[]) =>
+  ids.map((id) => ({ id, status: "COMPLETED", metadata: { answer: `secret ${id}` } }))
+
+function setUp(ids: number[]) {
+  privileges.mockResolvedValue(MY_PRIVILEGES)
+  taskLogCount.mockResolvedValue(ids.length)
+  taskLogFindMany.mockImplementation(async (args: any) =>
+    args.select
+      ? args.where.id.in.map((id: number) => ({
+          id,
+          task: { projectId: OWNERS[id]!.projectId },
+          assignedTo: { users: OWNERS[id]!.assignedUserIds.map((userId) => ({ id: userId })) },
+        }))
+      : fetchedLogs(...ids)
+  )
+}
+
+describe("getTaskLogs", () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+  })
+
+  test("the filter from the browser is narrowed to the user's own projects", async () => {
+    setUp([20])
+    const requested = { assignedToId: 5 }
+    await getTaskLogs({ where: requested }, ctx)
+    const expected = { AND: [requested, { task: { projectId: { in: [1, 2] } } }] }
+    expect(taskLogFindMany.mock.calls[0]![0].where).toEqual(expected)
+    expect(taskLogCount.mock.calls[0]![0].where).toEqual(expected)
+  })
+
+  test("with no filter at all it still only covers the user's projects", async () => {
+    setUp([])
+    await getTaskLogs({}, ctx)
+    expect(taskLogFindMany.mock.calls[0]![0].where).toEqual({
+      task: { projectId: { in: [1, 2] } },
+    })
+  })
+
+  test("project managers keep every response in their projects", async () => {
+    setUp([10])
+    const { taskLogs } = await getTaskLogs({}, ctx)
+    expect(taskLogs[0]!.metadata).toEqual({ answer: "secret 10" })
+  })
+
+  test("others' responses are blanked, but my own and my teams' are kept, in a project where I'm a contributor", async () => {
+    setUp([20, 21, 22])
+    const { taskLogs } = await getTaskLogs({}, ctx)
+    const byId = Object.fromEntries(taskLogs.map((log: any) => [log.id, log]))
+    expect(byId[20].metadata).toEqual({ answer: "secret 20" })
+    expect(byId[21].metadata).toBeNull()
+    expect(byId[22].metadata).toEqual({ answer: "secret 22" })
+    // everything else about the log is still there, so summaries and counts keep working
+    expect(byId[21].status).toBe("COMPLETED")
+  })
+
+  test("paged results are covered by the same rule", async () => {
+    setUp([20, 21])
+    const { taskLogs } = await getTaskLogs({ take: 2 }, ctx)
+    expect(taskLogs.find((log: any) => log.id === 21)!.metadata).toBeNull()
+    expect(taskLogs.find((log: any) => log.id === 20)!.metadata).toEqual({ answer: "secret 20" })
+  })
+
+  test("a user with no projects gets a filter that matches nothing", async () => {
+    privileges.mockResolvedValue([])
+    taskLogFindMany.mockResolvedValue([])
+    taskLogCount.mockResolvedValue(0)
+    await getTaskLogs({ where: { assignedToId: 5 } }, ctx)
+    expect(taskLogFindMany.mock.calls[0]![0].where).toEqual({
+      AND: [{ assignedToId: 5 }, { task: { projectId: { in: [] } } }],
+    })
+  })
+})

src/tasklogs/queries/getTaskLogs.ts

@@ -1,16 +1,59 @@
 import { resolver } from "@blitzjs/rpc"
 import db, { Prisma } from "db"
 import { paginate } from "blitz"
+import { getProjectAccess, ProjectAccess } from "src/projectprivileges/utils/getProjectAccess"
 
 // Define input types for the query
 interface GetTaskLogsInput
   extends Pick<Prisma.TaskLogFindManyArgs, "where" | "orderBy" | "include" | "skip" | "take"> {}
 
+// Form responses are only for the people they concern: project managers see them for their
+// projects, and everyone else only for logs assigned to them or to a team they are on. Other
+// logs are still returned (status, approval and dates feed summaries) with the response blanked.
+async function hideOthersResponses<T extends { id: number }>(
+  taskLogs: T[],
+  userId: number,
+  access: ProjectAccess
+): Promise<T[]> {
+  if (taskLogs.length === 0) return taskLogs
+  const owners = await db.taskLog.findMany({
+    where: { id: { in: taskLogs.map((log) => log.id) } },
+    select: {
+      id: true,
+      task: { select: { projectId: true } },
+      assignedTo: { select: { users: { select: { id: true } } } },
+    },
+  })
+  const mayRead = new Set(
+    owners
+      .filter(
+        (log) =>
+          access.managerProjectIds.includes(log.task.projectId) ||
+          log.assignedTo.users.some((user) => user.id === userId)
+      )
+      .map((log) => log.id)
+  )
+  return taskLogs.map((log) =>
+    mayRead.has(log.id) || !("metadata" in log) ? log : { ...log, metadata: null }
+  )
+}
+
 export default resolver.pipe(
   resolver.authorize(), // Automatically handles authorization
-  async ({ where, orderBy, include, skip = 0, take }: GetTaskLogsInput) => {
+  async ({ where: requestedWhere, orderBy, include, skip = 0, take }: GetTaskLogsInput, ctx) => {
+    const userId = ctx.session.userId as number
+    const access = await getProjectAccess(userId)
+
+    // The filter comes from the browser, so it is always narrowed to the user's own projects
+    const inMyProjects: Prisma.TaskLogWhereInput = {
+      task: { projectId: { in: access.memberProjectIds } },
+    }
+    const where: Prisma.TaskLogWhereInput = requestedWhere
+      ? { AND: [requestedWhere, inMyProjects] }
+      : inMyProjects
+
     if (typeof take !== "number") {
-      const [taskLogs, count] = await Promise.all([
+      const [foundLogs, count] = await Promise.all([
         db.taskLog.findMany({
           where,
           orderBy,
@@ -20,6 +63,7 @@ export default resolver.pipe(
         db.taskLog.count({ where }),
       ])
 
+      const taskLogs = await hideOthersResponses(foundLogs, userId, access)
       return {
         taskLogs,
         nextPage: null,
@@ -29,7 +73,7 @@ export default resolver.pipe(
     }
 
     const {
-      items: taskLogs,
+      items: foundLogs,
       hasMore,
       nextPage,
       count,
@@ -46,6 +90,7 @@ export default resolver.pipe(
         }),
     })
 
+    const taskLogs = await hideOthersResponses(foundLogs, userId, access)
     return {
       taskLogs,
       nextPage,