diff --git a/src/collections/users/UserNotifications.ts b/src/collections/users/UserNotifications.ts index f43cde8..c95828d 100644 --- a/src/collections/users/UserNotifications.ts +++ b/src/collections/users/UserNotifications.ts @@ -1,6 +1,6 @@ import { CollectionConfig } from "payload"; import type { User } from "@/payload-types"; -import { requirePermission, hasPermission } from "@/utils/access-control/hasPermission"; +import { requirePermission, hasPermission, isSuperuser } from "@/utils/access-control/hasPermission"; export const UserNotifications: CollectionConfig = { slug: "user-notifications", @@ -15,7 +15,10 @@ export const UserNotifications: CollectionConfig = { read: async ({ req }) => { const user = req.user as User | null; if (!user) return false; - if (await hasPermission(req.payload, user, "user-notifications:read")) return true; + // Read-all is gated on elevation (not the collection's read permission) so a + // mis-granted `user-notifications:read` can never leak other users' docs. + if (await isSuperuser(req.payload, user)) return true; + if (await hasPermission(req.payload, user, "system:admin-access")) return true; return { user: { equals: user.id } }; }, create: requirePermission("user-notifications:create"), diff --git a/src/tools/seed/seedRoles.ts b/src/tools/seed/seedRoles.ts index 979000e..07efc1b 100644 --- a/src/tools/seed/seedRoles.ts +++ b/src/tools/seed/seedRoles.ts @@ -10,7 +10,6 @@ const USER_PERMISSIONS: Permission[] = [ "qualifications:read", "assignments:read", "experience:read", - "user-notifications:read", "missions:read", "mission-attendances:read", "campaigns:read", diff --git a/tests/int/notification-access.int.spec.ts b/tests/int/notification-access.int.spec.ts new file mode 100644 index 0000000..5ee19bb --- /dev/null +++ b/tests/int/notification-access.int.spec.ts @@ -0,0 +1,125 @@ +import { getPayload, Payload } from "payload"; +import config from "@/payload.config"; + +import { afterAll, beforeAll, describe, expect, it } from "vitest"; + +import type { Role, User } from "@/payload-types"; +import { UserNotifications } from "@/collections/users/UserNotifications"; +import { invalidatePermissionCache } from "@/utils/access-control/loadUserPermissions"; + +let payload: Payload; + +const RUN = `ntf-${Date.now().toString(36)}`; +const TIMEOUT = 30_000; + +describe("User notification read access control", () => { + const roleIds: number[] = []; + const userIds: number[] = []; + + let plainUser: User; + let adminUser: User; + let devUser: User; + let leakyUser: User; + + const makeRole = async (label: string, extra: Partial = {}): Promise => { + const role = (await payload.create({ + collection: "roles", + data: { name: `${RUN}-${label}`, slug: `${RUN}-${label}`, ...extra }, + overrideAccess: true, + depth: 0, + })) as unknown as Role; + roleIds.push(role.id); + return role; + }; + + const makeUser = async (label: string, roleId: number): Promise => { + const user = (await payload.create({ + collection: "users", + data: { + username: `${RUN}-${label}`, + discordUsername: `${RUN}-${label}`, + displayName: label.toUpperCase(), + steamId: `7656119${Math.floor(Math.random() * 1e9)}`, + password: "Test123", + // Permission resolution reads roleDocs (the dynamic RBAC relationship), not + // the legacy `roles` enum — so assign a real role doc to grant permissions. + roleDocs: [roleId], + }, + overrideAccess: true, + depth: 0, + })) as unknown as User; + userIds.push(user.id); + return user; + }; + + // Invoke the collection's read access control exactly as Payload would for an + // authenticated request. The vitest environment has no Next.js HTTP server, so the + // REST endpoint (/api/user-notifications) is not reachable — calling the access + // function directly is the standard way to unit-test Payload access control. + const readDecision = async (user: User | null): Promise => { + const fn = UserNotifications.access?.read; + expect(typeof fn).toBe("function"); + return await (fn as (args: { req: unknown }) => Promise)({ req: { user, payload } }); + }; + + beforeAll(async () => { + const payloadConfig = await config; + payload = await getPayload({ config: payloadConfig }); + invalidatePermissionCache(); + + const plainRole = await makeRole("plain", { permissions: [] }); + const adminRole = await makeRole("admin", { permissions: ["system:admin-access"] }); + const devRole = await makeRole("dev", { isSuperuser: true }); + // A role carrying the collection's own read permission. Before the fix, holding + // `user-notifications:read` alone granted read-all; after the fix it must stay scoped. + const leakyRole = await makeRole("leaky", { permissions: ["user-notifications:read"] }); + + plainUser = await makeUser("plain", plainRole.id); + adminUser = await makeUser("admin", adminRole.id); + devUser = await makeUser("dev", devRole.id); + leakyUser = await makeUser("leaky", leakyRole.id); + }, TIMEOUT); + + afterAll(async () => { + if (!payload) return; + for (const id of userIds) { + const profiles = await payload + .find({ collection: "profiles", where: { user: { equals: id } }, limit: 5, depth: 0, overrideAccess: true }) + .catch(() => null); + for (const p of profiles?.docs ?? []) { + await payload.delete({ collection: "profiles", id: p.id, overrideAccess: true }).catch(() => {}); + } + await payload.delete({ collection: "users", id, overrideAccess: true }).catch(() => {}); + } + for (const id of roleIds) { + await payload.delete({ collection: "roles", id, overrideAccess: true }).catch(() => {}); + } + }); + + it("scopes a regular user's reads to their own notifications", async () => { + const decision = await readDecision(plainUser); + // The bug: this returned `true` (read-all), leaking every user's notifications. + expect(decision).not.toBe(true); + expect(decision).toMatchObject({ user: { equals: plainUser.id } }); + }, TIMEOUT); + + it("grants read-all to admin elevation (system:admin-access)", async () => { + expect(await readDecision(adminUser)).toBe(true); + }, TIMEOUT); + + it("grants read-all to superuser roles", async () => { + expect(await readDecision(devUser)).toBe(true); + }, TIMEOUT); + + it("denies anonymous (no user) reads", async () => { + expect(await readDecision(null)).toBe(false); + }, TIMEOUT); + + it("does not treat the collection's own read permission as read-all", async () => { + // Defense in depth: even a role holding `user-notifications:read` must stay scoped, + // because read-all is now gated on elevation only. + const decision = await readDecision(leakyUser); + expect(decision).not.toBe(true); + expect(decision).toMatchObject({ user: { equals: leakyUser.id } }); + }, TIMEOUT); +});