fix(notifications): scope user notification reads

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

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"),

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",

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<Role> = {}): Promise<Role> => {
+    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<User> => {
+    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<unknown> => {
+    const fn = UserNotifications.access?.read;
+    expect(typeof fn).toBe("function");
+    return await (fn as (args: { req: unknown }) => Promise<unknown>)({ 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);
+});