fix: Remove associated notifications when access is lost

app/components/WebsocketProvider.tsx

@@ -715,6 +715,10 @@ function useNotificationHandlers() {
       }
     );
 
+    socket.on("notifications.delete", (event: WebsocketEntityDeletedEvent) => {
+      notifications.remove(event.modelId);
+    });
+
     socket.on(
       "subscriptions.create",
       (event: PartialExcept<Subscription, "id">) => {

plugins/webhooks/server/tasks/DeliverWebhookTask.ts

@@ -169,6 +169,7 @@ export default class DeliverWebhookTask extends BaseTask<Props> {
       case "authenticationProviders.update":
       case "notifications.create":
       case "notifications.update":
+      case "notifications.delete":
       case "access_requests.create":
         // Ignored
         return;

server/queues/processors/NotificationsRevokeProcessor.test.ts

@@ -0,0 +1,175 @@
+import { CollectionPermission } from "@shared/types";
+import { AuthenticationType } from "@server/types";
+import {
+  buildCollection,
+  buildDocument,
+  buildGroup,
+  buildGroupUser,
+  buildNotification,
+  buildTeam,
+  buildUser,
+} from "@server/test/factories";
+import RevokeUserNotificationsTask from "../tasks/RevokeUserNotificationsTask";
+import NotificationsRevokeProcessor from "./NotificationsRevokeProcessor";
+
+describe("NotificationsRevokeProcessor", () => {
+  const schedule = vi
+    .spyOn(RevokeUserNotificationsTask.prototype, "schedule")
+    .mockResolvedValue(undefined as never);
+
+  beforeEach(() => {
+    schedule.mockClear();
+  });
+
+  it("should schedule a check for the removed user", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({ teamId: team.id });
+
+    await new NotificationsRevokeProcessor().perform({
+      name: "collections.remove_user",
+      userId: user.id,
+      modelId: collection.id,
+      collectionId: collection.id,
+      teamId: team.id,
+      actorId: user.id,
+      ip: "127.0.0.1",
+      authType: AuthenticationType.APP,
+      data: {},
+    });
+
+    expect(schedule).toHaveBeenCalledWith(
+      expect.objectContaining({
+        userId: user.id,
+        collectionId: collection.id,
+      })
+    );
+  });
+
+  it("should schedule a check for every member of a removed group", async () => {
+    const team = await buildTeam();
+    const group = await buildGroup({ teamId: team.id });
+    const document = await buildDocument({ teamId: team.id });
+    const [first, second] = await Promise.all([
+      buildUser({ teamId: team.id }),
+      buildUser({ teamId: team.id }),
+    ]);
+    await buildGroupUser({
+      teamId: team.id,
+      groupId: group.id,
+      userId: first.id,
+    });
+    await buildGroupUser({
+      teamId: team.id,
+      groupId: group.id,
+      userId: second.id,
+    });
+
+    await new NotificationsRevokeProcessor().perform({
+      name: "documents.remove_group",
+      modelId: group.id,
+      documentId: document.id,
+      teamId: team.id,
+      actorId: first.id,
+      ip: "127.0.0.1",
+      data: { membershipId: "membership" },
+    });
+
+    expect(schedule).toHaveBeenCalledTimes(2);
+    expect(schedule).toHaveBeenCalledWith(
+      expect.objectContaining({ userId: first.id, documentId: document.id })
+    );
+    expect(schedule).toHaveBeenCalledWith(
+      expect.objectContaining({ userId: second.id, documentId: document.id })
+    );
+  });
+
+  it("should schedule a scope-less check for a user removed from a group", async () => {
+    const team = await buildTeam();
+    const group = await buildGroup({ teamId: team.id });
+    const user = await buildUser({ teamId: team.id });
+
+    await new NotificationsRevokeProcessor().perform({
+      name: "groups.remove_user",
+      userId: user.id,
+      modelId: group.id,
+      teamId: team.id,
+      actorId: user.id,
+      ip: "127.0.0.1",
+    });
+
+    expect(schedule).toHaveBeenCalledWith({ userId: user.id });
+  });
+
+  it("should schedule a scope-less check for every member of a deleted group", async () => {
+    const team = await buildTeam();
+    const group = await buildGroup({ teamId: team.id });
+    const [first, second] = await Promise.all([
+      buildUser({ teamId: team.id }),
+      buildUser({ teamId: team.id }),
+    ]);
+    await buildGroupUser({
+      teamId: team.id,
+      groupId: group.id,
+      userId: first.id,
+    });
+    await buildGroupUser({
+      teamId: team.id,
+      groupId: group.id,
+      userId: second.id,
+    });
+    await group.destroy();
+
+    await new NotificationsRevokeProcessor().perform({
+      name: "groups.delete",
+      modelId: group.id,
+      teamId: team.id,
+      actorId: first.id,
+      ip: "127.0.0.1",
+    });
+
+    expect(schedule).toHaveBeenCalledTimes(2);
+    expect(schedule).toHaveBeenCalledWith({ userId: first.id });
+    expect(schedule).toHaveBeenCalledWith({ userId: second.id });
+  });
+
+  it("should schedule a check for every notified user when a collection becomes private", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const other = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: CollectionPermission.Read,
+    });
+    const document = await buildDocument({
+      teamId: team.id,
+      collectionId: collection.id,
+    });
+    await buildNotification({
+      teamId: team.id,
+      userId: user.id,
+      documentId: document.id,
+    });
+    await buildNotification({
+      teamId: team.id,
+      userId: other.id,
+      collectionId: collection.id,
+    });
+
+    await new NotificationsRevokeProcessor().perform({
+      name: "collections.permission_changed",
+      collectionId: collection.id,
+      teamId: team.id,
+      actorId: user.id,
+      ip: "127.0.0.1",
+    });
+
+    expect(schedule).toHaveBeenCalledTimes(2);
+    expect(schedule).toHaveBeenCalledWith(
+      expect.objectContaining({ userId: user.id, collectionId: collection.id })
+    );
+    expect(schedule).toHaveBeenCalledWith(
+      expect.objectContaining({ userId: other.id, collectionId: collection.id })
+    );
+  });
+});

server/queues/processors/NotificationsRevokeProcessor.ts

@@ -0,0 +1,130 @@
+import { Op } from "sequelize";
+import { Document, GroupUser, Notification } from "@server/models";
+import type {
+  CollectionEvent,
+  CollectionGroupEvent,
+  CollectionUserEvent,
+  DocumentGroupEvent,
+  DocumentUserEvent,
+  Event,
+  GroupEvent,
+} from "@server/types";
+import type { RevokeScope } from "../tasks/RevokeUserNotificationsTask";
+import RevokeUserNotificationsTask from "../tasks/RevokeUserNotificationsTask";
+import BaseProcessor from "./BaseProcessor";
+
+type ReceivedEvent =
+  | CollectionEvent
+  | CollectionUserEvent
+  | CollectionGroupEvent
+  | DocumentUserEvent
+  | DocumentGroupEvent
+  | GroupEvent;
+
+/**
+ * Revokes notifications that reference a document or collection once the recipient loses access to it.
+ */
+export default class NotificationsRevokeProcessor extends BaseProcessor {
+  static applicableEvents: Event["name"][] = [
+    "collections.remove_user",
+    "collections.remove_group",
+    "collections.permission_changed",
+    "documents.remove_user",
+    "documents.remove_group",
+    "groups.remove_user",
+    "groups.delete",
+  ];
+
+  async perform(event: ReceivedEvent) {
+    switch (event.name) {
+      case "collections.remove_user":
+        await new RevokeUserNotificationsTask().schedule({
+          userId: event.userId,
+          collectionId: event.collectionId,
+        });
+        return;
+
+      case "documents.remove_user":
+        await new RevokeUserNotificationsTask().schedule({
+          userId: event.userId,
+          documentId: event.documentId,
+        });
+        return;
+
+      case "collections.remove_group":
+        return this.handleRemoveGroup(event.modelId, {
+          collectionId: event.collectionId,
+        });
+
+      case "documents.remove_group":
+        return this.handleRemoveGroup(event.modelId, {
+          documentId: event.documentId,
+        });
+
+      // Access lost through a group cannot be scoped to a single collection or document, so all
+      // of the user's notifications are re-checked.
+      case "groups.remove_user":
+        await new RevokeUserNotificationsTask().schedule({
+          userId: event.userId,
+        });
+        return;
+
+      case "groups.delete":
+        return this.handleRemoveGroup(event.modelId, {});
+
+      case "collections.permission_changed":
+        return this.handlePermissionChanged(event);
+    }
+  }
+
+  private async handleRemoveGroup(groupId: string, scope: RevokeScope) {
+    await GroupUser.findAllInBatches<GroupUser>(
+      {
+        attributes: ["userId"],
+        where: { groupId },
+        batchLimit: 1000,
+      },
+      async (groupUsers) => {
+        await Promise.all(
+          groupUsers.map((groupUser) =>
+            new RevokeUserNotificationsTask().schedule({
+              ...scope,
+              userId: groupUser.userId,
+            })
+          )
+        );
+      }
+    );
+  }
+
+  private async handlePermissionChanged(event: CollectionEvent) {
+    const notifications = await Notification.unscoped().findAll({
+      attributes: ["userId"],
+      where: {
+        [Op.or]: [
+          { collectionId: event.collectionId },
+          { "$document.collectionId$": event.collectionId },
+        ],
+      },
+      include: [
+        {
+          model: Document.unscoped(),
+          as: "document",
+          attributes: [],
+          required: false,
+          paranoid: false,
+        },
+      ],
+      group: ["notification.userId"],
+    });
+
+    await Promise.all(
+      notifications.map((notification) =>
+        new RevokeUserNotificationsTask().schedule({
+          userId: notification.userId,
+          collectionId: event.collectionId,
+        })
+      )
+    );
+  }
+}

server/queues/processors/WebsocketsProcessor.ts

@@ -682,6 +682,12 @@ export default class WebsocketsProcessor {
         return socketio.to(`user-${event.userId}`).emit(event.name, data);
       }
 
+      case "notifications.delete": {
+        return socketio.to(`user-${event.userId}`).emit(event.name, {
+          modelId: event.modelId,
+        });
+      }
+
       case "stars.create":
       case "stars.update": {
         const star = await Star.findByPk(event.modelId);

server/queues/tasks/RevokeUserNotificationsTask.test.ts

@@ -0,0 +1,247 @@
+import {
+  CollectionPermission,
+  DocumentPermission,
+  NotificationEventType,
+} from "@shared/types";
+import { Notification, UserMembership } from "@server/models";
+import {
+  buildCollection,
+  buildDocument,
+  buildDraftDocument,
+  buildNotification,
+  buildTeam,
+  buildUser,
+} from "@server/test/factories";
+import RevokeUserNotificationsTask from "./RevokeUserNotificationsTask";
+
+describe("RevokeUserNotificationsTask", () => {
+  it("should revoke notifications for a collection the user can no longer read", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    const document = await buildDocument({
+      teamId: team.id,
+      collectionId: collection.id,
+    });
+    const notification = await buildNotification({
+      teamId: team.id,
+      userId: user.id,
+      documentId: document.id,
+    });
+
+    await new RevokeUserNotificationsTask().perform({
+      userId: user.id,
+      collectionId: collection.id,
+    });
+
+    expect(await Notification.findByPk(notification.id)).toBeNull();
+  });
+
+  it("should retain notifications when the user can still read the document", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: CollectionPermission.Read,
+    });
+    const document = await buildDocument({
+      teamId: team.id,
+      collectionId: collection.id,
+    });
+    const notification = await buildNotification({
+      teamId: team.id,
+      userId: user.id,
+      documentId: document.id,
+    });
+
+    await new RevokeUserNotificationsTask().perform({
+      userId: user.id,
+      collectionId: collection.id,
+    });
+
+    expect(await Notification.findByPk(notification.id)).not.toBeNull();
+  });
+
+  it("should retain notifications when the user still has a document membership", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    const document = await buildDocument({
+      teamId: team.id,
+      collectionId: collection.id,
+    });
+    await UserMembership.create({
+      userId: user.id,
+      documentId: document.id,
+      createdById: user.id,
+      permission: DocumentPermission.Read,
+    });
+    const notification = await buildNotification({
+      teamId: team.id,
+      userId: user.id,
+      documentId: document.id,
+    });
+
+    await new RevokeUserNotificationsTask().perform({
+      userId: user.id,
+      collectionId: collection.id,
+    });
+
+    expect(await Notification.findByPk(notification.id)).not.toBeNull();
+  });
+
+  it("should revoke notifications for nested documents when scoped to a document", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    const parent = await buildDocument({
+      teamId: team.id,
+      collectionId: collection.id,
+    });
+    const child = await buildDocument({
+      teamId: team.id,
+      collectionId: collection.id,
+      parentDocumentId: parent.id,
+    });
+    const notification = await buildNotification({
+      teamId: team.id,
+      userId: user.id,
+      documentId: child.id,
+    });
+
+    await new RevokeUserNotificationsTask().perform({
+      userId: user.id,
+      documentId: parent.id,
+    });
+
+    expect(await Notification.findByPk(notification.id)).toBeNull();
+  });
+
+  it("should revoke notifications for a draft the user can no longer read", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    const document = await buildDraftDocument({
+      teamId: team.id,
+      collectionId: collection.id,
+    });
+    const notification = await buildNotification({
+      teamId: team.id,
+      userId: user.id,
+      documentId: document.id,
+    });
+
+    await new RevokeUserNotificationsTask().perform({
+      userId: user.id,
+      collectionId: collection.id,
+    });
+
+    expect(await Notification.findByPk(notification.id)).toBeNull();
+  });
+
+  it("should revoke notifications about the collection itself", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    const notification = await buildNotification({
+      teamId: team.id,
+      userId: user.id,
+      collectionId: collection.id,
+      event: NotificationEventType.AddUserToCollection,
+    });
+
+    await new RevokeUserNotificationsTask().perform({
+      userId: user.id,
+      collectionId: collection.id,
+    });
+
+    expect(await Notification.findByPk(notification.id)).toBeNull();
+  });
+
+  it("should re-check all notifications when no scope is given", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const privateCollection = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    const openCollection = await buildCollection({
+      teamId: team.id,
+      permission: CollectionPermission.Read,
+    });
+    const unreadable = await buildDocument({
+      teamId: team.id,
+      collectionId: privateCollection.id,
+    });
+    const readable = await buildDocument({
+      teamId: team.id,
+      collectionId: openCollection.id,
+    });
+    const revoked = await buildNotification({
+      teamId: team.id,
+      userId: user.id,
+      documentId: unreadable.id,
+    });
+    const retained = await buildNotification({
+      teamId: team.id,
+      userId: user.id,
+      documentId: readable.id,
+    });
+    const collectionNotification = await buildNotification({
+      teamId: team.id,
+      userId: user.id,
+      collectionId: privateCollection.id,
+      event: NotificationEventType.AddUserToCollection,
+    });
+
+    await new RevokeUserNotificationsTask().perform({ userId: user.id });
+
+    expect(await Notification.findByPk(revoked.id)).toBeNull();
+    expect(await Notification.findByPk(retained.id)).not.toBeNull();
+    expect(await Notification.findByPk(collectionNotification.id)).toBeNull();
+  });
+
+  it("should not revoke notifications outside of the given collection", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    const other = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    const document = await buildDocument({
+      teamId: team.id,
+      collectionId: other.id,
+    });
+    const notification = await buildNotification({
+      teamId: team.id,
+      userId: user.id,
+      documentId: document.id,
+    });
+
+    await new RevokeUserNotificationsTask().perform({
+      userId: user.id,
+      collectionId: collection.id,
+    });
+
+    expect(await Notification.findByPk(notification.id)).not.toBeNull();
+  });
+});

server/queues/tasks/RevokeUserNotificationsTask.ts

@@ -0,0 +1,194 @@
+import { Op } from "sequelize";
+import { compact, uniq } from "es-toolkit/compat";
+import Logger from "@server/logging/Logger";
+import {
+  Collection,
+  Document,
+  Event,
+  Notification,
+  User,
+} from "@server/models";
+import { can } from "@server/policies";
+import { BaseTask, TaskPriority } from "./base/BaseTask";
+
+export type RevokeScope = {
+  /** When provided, only notifications for this document, and its nested documents, are considered. */
+  documentId?: string;
+  /** When provided, only notifications for this collection, and the documents within it, are considered. */
+  collectionId?: string;
+};
+
+type Props = RevokeScope & {
+  /** The user whose notifications should be re-checked. */
+  userId: string;
+};
+
+/**
+ * Destroys the notifications of a user that reference a document or collection they can no longer
+ * read, so that the record – which embeds the document title and text – does not outlive access.
+ */
+export default class RevokeUserNotificationsTask extends BaseTask<Props> {
+  public async perform({ userId, documentId, collectionId }: Props) {
+    const user = await User.findByPk(userId);
+
+    if (!user) {
+      return;
+    }
+
+    const notifications = await this.findInScope(userId, {
+      documentId,
+      collectionId,
+    });
+
+    const candidateIds = uniq(
+      compact(notifications.map((notification) => notification.documentId))
+    );
+
+    // Documents that are no longer visible to the user are not returned by this query at all, in
+    // which case the notification is left alone – only a document that exists and is unreadable is
+    // grounds for revoking.
+    const documents = candidateIds.length
+      ? await Document.scope({
+          method: ["withMembership", user.id],
+        }).findAll({
+          // The minimum needed to evaluate the read policy, alongside the memberships and
+          // collection loaded by the scope.
+          attributes: [
+            "id",
+            "teamId",
+            "collectionId",
+            "createdById",
+            "publishedAt",
+          ],
+          where: { id: candidateIds },
+        })
+      : [];
+
+    const unreadableDocumentIds = new Set(
+      documents
+        .filter((document) => !can(user, "read", document))
+        .map((document) => document.id)
+    );
+
+    const revokeIds = notifications
+      .filter(
+        (notification) =>
+          notification.documentId &&
+          unreadableDocumentIds.has(notification.documentId)
+      )
+      .map((notification) => notification.id);
+
+    // Notifications about a collection itself carry no document, so are checked separately.
+    const collectionIds = uniq(
+      compact(
+        notifications
+          .filter((notification) => !notification.documentId)
+          .map((notification) => notification.collectionId)
+      )
+    );
+
+    for (const id of collectionIds) {
+      const collection = await Collection.findByPk(id, { userId: user.id });
+      if (collection && !can(user, "read", collection)) {
+        revokeIds.push(
+          ...notifications
+            .filter(
+              (notification) =>
+                !notification.documentId && notification.collectionId === id
+            )
+            .map((notification) => notification.id)
+        );
+      }
+    }
+
+    if (!revokeIds.length) {
+      return;
+    }
+
+    Logger.debug(
+      "task",
+      `Revoking ${revokeIds.length} notifications for user ${user.id}`
+    );
+
+    await Notification.destroy({ where: { id: revokeIds } });
+
+    // Notify the client so the notification does not linger in the UI until the next reload.
+    await Promise.all(
+      revokeIds.map((id) =>
+        Event.schedule({
+          name: "notifications.delete",
+          modelId: id,
+          userId: user.id,
+          teamId: user.teamId,
+        })
+      )
+    );
+  }
+
+  public get options() {
+    return {
+      ...super.options,
+      priority: TaskPriority.Background,
+    };
+  }
+
+  /**
+   * Loads the user's notifications that fall within the given scope, joining through the document
+   * association so that the collection filter is applied in the database rather than in memory.
+   */
+  private async findInScope(userId: string, scope: RevokeScope) {
+    const attributes = ["id", "documentId", "collectionId"];
+
+    if (scope.documentId) {
+      return Notification.unscoped().findAll({
+        attributes,
+        where: {
+          userId,
+          documentId: await this.documentAndChildIds(scope.documentId),
+        },
+      });
+    }
+
+    if (scope.collectionId) {
+      return Notification.unscoped().findAll({
+        attributes,
+        where: {
+          userId,
+          [Op.or]: [
+            { collectionId: scope.collectionId },
+            { "$document.collectionId$": scope.collectionId },
+          ],
+        },
+        include: [
+          {
+            model: Document.unscoped(),
+            as: "document",
+            attributes: [],
+            required: false,
+            paranoid: false,
+          },
+        ],
+      });
+    }
+
+    return Notification.unscoped().findAll({
+      attributes,
+      where: { userId },
+    });
+  }
+
+  /**
+   * Returns the given document id along with the ids of all documents nested beneath it, as
+   * memberships – and therefore access – cascade to nested documents.
+   */
+  private async documentAndChildIds(documentId: string) {
+    const document = Document.build({ id: documentId });
+
+    return [
+      documentId,
+      ...(await document.findAllChildDocumentIds(undefined, {
+        paranoid: false,
+      })),
+    ];
+  }
+}

server/types.ts

@@ -456,7 +456,10 @@ export type WebhookSubscriptionEvent = BaseEvent<WebhookSubscription> & {
 };
 
 export type NotificationEvent = BaseEvent<Notification> & {
-  name: "notifications.create" | "notifications.update";
+  name:
+    | "notifications.create"
+    | "notifications.update"
+    | "notifications.delete";
   modelId: string;
   teamId: string;
   userId: string;