Prevent duplicate emails when user has existing access to a document. (#8263)

* check user has higher access

* membershipId column

* handle document shared email

* fix and cleanup

* tests

* jsdoc

* event changeset

* check collection permission

* change date in migration filename

* review

* rename migration filename to today

* required group, jsdoc

server/emails/templates/DocumentSharedEmail.tsx

@@ -1,6 +1,6 @@
 import * as React from "react";
 import { DocumentPermission } from "@shared/types";
-import { Document, UserMembership } from "@server/models";
+import { Document, GroupMembership, UserMembership } from "@server/models";
 import BaseEmail, { EmailMessageCategory, EmailProps } from "./BaseEmail";
 import Body from "./components/Body";
 import Button from "./components/Button";
@@ -11,13 +11,14 @@ import Heading from "./components/Heading";
 type InputProps = EmailProps & {
   userId: string;
   documentId: string;
+  membershipId?: string;
   actorName: string;
   teamUrl: string;
 };
 
 type BeforeSend = {
   document: Document;
-  membership: UserMembership;
+  membership: UserMembership | GroupMembership;
 };
 
 type Props = InputProps & BeforeSend;
@@ -33,18 +34,20 @@ export default class DocumentSharedEmail extends BaseEmail<
     return EmailMessageCategory.Notification;
   }
 
-  protected async beforeSend({ documentId, userId }: InputProps) {
+  protected async beforeSend({ documentId, membershipId }: InputProps) {
+    if (!membershipId) {
+      return false;
+    }
+
     const document = await Document.unscoped().findByPk(documentId);
     if (!document) {
       return false;
     }
 
-    const membership = await UserMembership.findOne({
-      where: {
-        documentId,
-        userId,
-      },
-    });
+    const membership =
+      (await UserMembership.findByPk(membershipId)) ??
+      (await GroupMembership.findByPk(membershipId));
+
     if (!membership) {
       return false;
     }

server/migrations/20250225153529-add-notification-membershipId.js

@@ -0,0 +1,14 @@
+"use strict";
+
+/** @type {import('sequelize-cli').Migration} */
+module.exports = {
+  async up(queryInterface, Sequelize) {
+    await queryInterface.addColumn("notifications", "membershipId", {
+      type: Sequelize.UUID,
+    });
+  },
+
+  async down(queryInterface, Sequelize) {
+    await queryInterface.removeColumn("notifications", "membershipId");
+  },
+};

server/models/Notification.ts

@@ -177,6 +177,10 @@ class Notification extends Model<
   @Column(DataType.UUID)
   teamId: string;
 
+  @AllowNull
+  @Column(DataType.UUID)
+  membershipId: string;
+
   @AfterCreate
   static async createEvent(
     model: Notification,
@@ -191,6 +195,7 @@ class Notification extends Model<
       documentId: model.documentId,
       collectionId: model.collectionId,
       actorId: model.actorId,
+      membershipId: model.membershipId,
     };
 
     if (options.transaction) {

server/queues/processors/EmailsProcessor.ts

@@ -56,6 +56,7 @@ export default class EmailsProcessor extends BaseProcessor {
             to: notification.user.email,
             userId: notification.userId,
             documentId: notification.documentId,
+            membershipId: notification.membershipId,
             teamUrl: notification.team.url,
             actorName: notification.actor.name,
           },

server/queues/tasks/CommentCreatedNotificationsTask.ts

@@ -10,7 +10,7 @@ import NotificationHelper from "@server/models/helpers/NotificationHelper";
 import { ProsemirrorHelper } from "@server/models/helpers/ProsemirrorHelper";
 import { sequelize } from "@server/storage/database";
 import { CommentEvent } from "@server/types";
-import { canUserAccessDocument } from "@server/utils/policies";
+import { canUserAccessDocument } from "@server/utils/permissions";
 import BaseTask, { TaskPriority } from "./BaseTask";
 
 export default class CommentCreatedNotificationsTask extends BaseTask<CommentEvent> {

server/queues/tasks/CommentUpdatedNotificationsTask.ts

@@ -4,7 +4,7 @@ import { MentionType, NotificationEventType } from "@shared/types";
 import { Comment, Document, Notification, User } from "@server/models";
 import { ProsemirrorHelper } from "@server/models/helpers/ProsemirrorHelper";
 import { CommentEvent, CommentUpdateEvent } from "@server/types";
-import { canUserAccessDocument } from "@server/utils/policies";
+import { canUserAccessDocument } from "@server/utils/permissions";
 import BaseTask, { TaskPriority } from "./BaseTask";
 
 export default class CommentUpdatedNotificationsTask extends BaseTask<CommentEvent> {

server/queues/tasks/DocumentAddGroupNotificationsTask.ts

@@ -1,6 +1,5 @@
 import { Op } from "sequelize";
-import Logger from "@server/logging/Logger";
-import { GroupUser, UserMembership } from "@server/models";
+import { GroupUser } from "@server/models";
 import { DocumentGroupEvent } from "@server/types";
 import BaseTask, { TaskPriority } from "./BaseTask";
 import DocumentAddUserNotificationsTask from "./DocumentAddUserNotificationsTask";
@@ -20,26 +19,9 @@ export default class DocumentAddGroupNotificationsTask extends BaseTask<Document
       async (groupUsers) => {
         await Promise.all(
           groupUsers.map(async (groupUser) => {
-            const userMembership = await UserMembership.findOne({
-              where: {
-                userId: groupUser.userId,
-                documentId: event.documentId,
-              },
-            });
-            if (userMembership) {
-              Logger.debug(
-                "task",
-                `Suppressing notification for user ${groupUser.userId} as they are already a member of the document`,
-                {
-                  documentId: event.documentId,
-                  userId: groupUser.userId,
-                }
-              );
-              return;
-            }
-
             await DocumentAddUserNotificationsTask.schedule({
               ...event,
+              modelId: event.data.membershipId,
               userId: groupUser.userId,
             });
           })

server/queues/tasks/DocumentAddUserNotificationsTask.ts

@@ -1,27 +1,65 @@
-import { NotificationEventType } from "@shared/types";
+import { DocumentPermission, NotificationEventType } from "@shared/types";
+import Logger from "@server/logging/Logger";
 import { Notification, User } from "@server/models";
 import { DocumentUserEvent } from "@server/types";
+import { isElevatedPermission } from "@server/utils/permissions";
 import BaseTask, { TaskPriority } from "./BaseTask";
 
 export default class DocumentAddUserNotificationsTask extends BaseTask<DocumentUserEvent> {
   public async perform(event: DocumentUserEvent) {
-    const recipient = await User.findByPk(event.userId);
-    if (!recipient) {
+    const permission = event.changes?.attributes.permission as
+      | DocumentPermission
+      | undefined;
+
+    if (!permission) {
+      Logger.info(
+        "task",
+        `permission not available in the DocumentAddUserNotificationsTask event`,
+        {
+          name: event.name,
+          modelId: event.modelId,
+        }
+      );
       return;
     }
 
+    const recipient = await User.findByPk(event.userId);
     if (
-      !recipient.isSuspended &&
-      recipient.subscribedToEventType(NotificationEventType.AddUserToDocument)
+      !recipient ||
+      recipient.isSuspended ||
+      !recipient.subscribedToEventType(NotificationEventType.AddUserToDocument)
     ) {
-      await Notification.create({
-        event: NotificationEventType.AddUserToDocument,
-        userId: event.userId,
-        actorId: event.actorId,
-        teamId: event.teamId,
-        documentId: event.documentId,
-      });
+      return;
     }
+
+    const isElevated = await isElevatedPermission({
+      userId: recipient.id,
+      documentId: event.documentId,
+      permission,
+      skipMembershipId: event.modelId,
+    });
+
+    if (!isElevated) {
+      Logger.debug(
+        "task",
+        `Suppressing notification for user ${event.userId} as the new permission does not elevate user's permission to the document`,
+        {
+          documentId: event.documentId,
+          userId: event.userId,
+          permission,
+        }
+      );
+      return;
+    }
+
+    await Notification.create({
+      event: NotificationEventType.AddUserToDocument,
+      userId: event.userId,
+      actorId: event.actorId,
+      teamId: event.teamId,
+      documentId: event.documentId,
+      membershipId: event.modelId,
+    });
   }
 
   public get options() {

server/queues/tasks/DocumentPublishedNotificationsTask.ts

@@ -4,7 +4,7 @@ import { Document, Notification, User } from "@server/models";
 import { DocumentHelper } from "@server/models/helpers/DocumentHelper";
 import NotificationHelper from "@server/models/helpers/NotificationHelper";
 import { DocumentEvent } from "@server/types";
-import { canUserAccessDocument } from "@server/utils/policies";
+import { canUserAccessDocument } from "@server/utils/permissions";
 import BaseTask, { TaskPriority } from "./BaseTask";
 
 export default class DocumentPublishedNotificationsTask extends BaseTask<DocumentEvent> {

server/queues/tasks/RevisionCreatedNotificationsTask.ts

@@ -9,7 +9,7 @@ import { Document, Revision, Notification, User, View } from "@server/models";
 import { DocumentHelper } from "@server/models/helpers/DocumentHelper";
 import NotificationHelper from "@server/models/helpers/NotificationHelper";
 import { RevisionEvent } from "@server/types";
-import { canUserAccessDocument } from "@server/utils/policies";
+import { canUserAccessDocument } from "@server/utils/permissions";
 import BaseTask, { TaskPriority } from "./BaseTask";
 
 export default class RevisionCreatedNotificationsTask extends BaseTask<RevisionEvent> {

server/test/factories.ts

@@ -294,11 +294,14 @@ export async function buildCollection(
     overrides.archivedById = overrides.userId;
   }
 
+  if (overrides.permission === undefined) {
+    overrides.permission = CollectionPermission.ReadWrite;
+  }
+
   return Collection.create({
     name: faker.lorem.words(2),
     description: faker.lorem.words(4),
     createdById: overrides.userId,
-    permission: CollectionPermission.ReadWrite,
     ...overrides,
   });
 }

server/types.ts

@@ -464,6 +464,7 @@ export type NotificationEvent = BaseEvent<Notification> & {
   commentId?: string;
   documentId?: string;
   collectionId?: string;
+  membershipId?: string;
 };
 
 export type Event =

server/utils/permissions.test.ts

@@ -0,0 +1,305 @@
+import { CollectionPermission, DocumentPermission } from "@shared/types";
+import { GroupMembership, UserMembership } from "@server/models";
+import {
+  buildCollection,
+  buildDocument,
+  buildGroup,
+  buildGroupUser,
+  buildUser,
+} from "@server/test/factories";
+import { getDocumentPermission, isElevatedPermission } from "./permissions";
+
+describe("permissions", () => {
+  describe("isElevatedPermission", () => {
+    it("should return false when user has higher permission through collection", async () => {
+      const user = await buildUser();
+      const collection = await buildCollection({
+        teamId: user.teamId,
+        permission: null,
+      });
+      const document = await buildDocument({
+        collectionId: collection.id,
+        teamId: user.teamId,
+      });
+      await UserMembership.create({
+        createdById: user.id,
+        collectionId: collection.id,
+        userId: user.id,
+        permission: CollectionPermission.ReadWrite,
+      });
+
+      const isElevated = await isElevatedPermission({
+        userId: user.id,
+        documentId: document.id,
+        permission: DocumentPermission.Read,
+      });
+
+      expect(isElevated).toBe(false);
+    });
+
+    it("should return false when user has higher permission through document", async () => {
+      const user = await buildUser();
+      const collection = await buildCollection({
+        teamId: user.teamId,
+        permission: null,
+      });
+      const document = await buildDocument({
+        collectionId: collection.id,
+        teamId: user.teamId,
+      });
+      const group = await buildGroup();
+      await Promise.all([
+        await buildGroupUser({
+          groupId: group.id,
+          userId: user.id,
+          teamId: user.teamId,
+        }),
+        await UserMembership.create({
+          createdById: user.id,
+          documentId: document.id,
+          userId: user.id,
+          permission: DocumentPermission.Read,
+        }),
+        await GroupMembership.create({
+          createdById: user.id,
+          documentId: document.id,
+          groupId: group.id,
+          permission: DocumentPermission.ReadWrite,
+        }),
+      ]);
+
+      const isElevated = await isElevatedPermission({
+        userId: user.id,
+        documentId: document.id,
+        permission: DocumentPermission.Read,
+      });
+
+      expect(isElevated).toBe(false);
+    });
+
+    it("should return false when user has the same permission", async () => {
+      const user = await buildUser();
+      const collection = await buildCollection({
+        teamId: user.teamId,
+        permission: null,
+      });
+      const document = await buildDocument({
+        collectionId: collection.id,
+        teamId: user.teamId,
+      });
+      const group = await buildGroup();
+      await Promise.all([
+        await buildGroupUser({
+          groupId: group.id,
+          userId: user.id,
+          teamId: user.teamId,
+        }),
+        await UserMembership.create({
+          createdById: user.id,
+          documentId: document.id,
+          userId: user.id,
+          permission: DocumentPermission.Read,
+        }),
+        await GroupMembership.create({
+          createdById: user.id,
+          documentId: document.id,
+          groupId: group.id,
+          permission: DocumentPermission.ReadWrite,
+        }),
+      ]);
+
+      const isElevated = await isElevatedPermission({
+        userId: user.id,
+        documentId: document.id,
+        permission: DocumentPermission.ReadWrite,
+      });
+
+      expect(isElevated).toBe(false);
+    });
+
+    it("should return true when user has lower permission", async () => {
+      const user = await buildUser();
+      const collection = await buildCollection({
+        teamId: user.teamId,
+        permission: null,
+      });
+      const document = await buildDocument({
+        collectionId: collection.id,
+        teamId: user.teamId,
+      });
+      const group = await buildGroup();
+      await Promise.all([
+        await buildGroupUser({
+          groupId: group.id,
+          userId: user.id,
+          teamId: user.teamId,
+        }),
+        await UserMembership.create({
+          createdById: user.id,
+          documentId: document.id,
+          userId: user.id,
+          permission: DocumentPermission.Read,
+        }),
+        await GroupMembership.create({
+          createdById: user.id,
+          documentId: document.id,
+          groupId: group.id,
+          permission: DocumentPermission.ReadWrite,
+        }),
+      ]);
+
+      const isElevated = await isElevatedPermission({
+        userId: user.id,
+        documentId: document.id,
+        permission: DocumentPermission.Admin,
+      });
+
+      expect(isElevated).toBe(true);
+    });
+
+    it("should return true when user does not have access", async () => {
+      const user = await buildUser();
+      const collection = await buildCollection({
+        teamId: user.teamId,
+        permission: null,
+      });
+      const document = await buildDocument({
+        collectionId: collection.id,
+        teamId: user.teamId,
+      });
+
+      const isElevated = await isElevatedPermission({
+        userId: user.id,
+        documentId: document.id,
+        permission: DocumentPermission.Admin,
+      });
+
+      expect(isElevated).toBe(true);
+    });
+  });
+
+  describe("getDocumentPermission", () => {
+    it("should return the highest provided permission through collection", async () => {
+      const user = await buildUser();
+      const collection = await buildCollection({
+        teamId: user.teamId,
+        permission: null,
+      });
+      const document = await buildDocument({
+        collectionId: collection.id,
+        teamId: user.teamId,
+      });
+      await UserMembership.create({
+        createdById: user.id,
+        collectionId: collection.id,
+        userId: user.id,
+        permission: CollectionPermission.ReadWrite,
+      });
+
+      const permission = await getDocumentPermission({
+        userId: user.id,
+        documentId: document.id,
+      });
+
+      expect(permission).toEqual(DocumentPermission.ReadWrite);
+    });
+
+    it("should return the highest provided permission through document", async () => {
+      const user = await buildUser();
+      const collection = await buildCollection({
+        teamId: user.teamId,
+        permission: null,
+      });
+      const document = await buildDocument({
+        collectionId: collection.id,
+        teamId: user.teamId,
+      });
+      const group = await buildGroup();
+      await Promise.all([
+        await buildGroupUser({
+          groupId: group.id,
+          userId: user.id,
+          teamId: user.teamId,
+        }),
+        await UserMembership.create({
+          createdById: user.id,
+          documentId: document.id,
+          userId: user.id,
+          permission: DocumentPermission.Read,
+        }),
+        await GroupMembership.create({
+          createdById: user.id,
+          documentId: document.id,
+          groupId: group.id,
+          permission: DocumentPermission.ReadWrite,
+        }),
+      ]);
+
+      const permission = await getDocumentPermission({
+        userId: user.id,
+        documentId: document.id,
+      });
+
+      expect(permission).toEqual(DocumentPermission.ReadWrite);
+    });
+
+    it("should return the highest provided permission with skipped membership", async () => {
+      const user = await buildUser();
+      const collection = await buildCollection({
+        teamId: user.teamId,
+        permission: null,
+      });
+      const document = await buildDocument({
+        collectionId: collection.id,
+        teamId: user.teamId,
+      });
+      const group = await buildGroup();
+      const [, , groupMembership] = await Promise.all([
+        await buildGroupUser({
+          groupId: group.id,
+          userId: user.id,
+          teamId: user.teamId,
+        }),
+        await UserMembership.create({
+          createdById: user.id,
+          documentId: document.id,
+          userId: user.id,
+          permission: DocumentPermission.Read,
+        }),
+        await GroupMembership.create({
+          createdById: user.id,
+          documentId: document.id,
+          groupId: group.id,
+          permission: DocumentPermission.ReadWrite,
+        }),
+      ]);
+
+      const permission = await getDocumentPermission({
+        userId: user.id,
+        documentId: document.id,
+        skipMembershipId: groupMembership.id,
+      });
+
+      expect(permission).toEqual(DocumentPermission.Read);
+    });
+
+    it("should return undefined when user does not have access", async () => {
+      const user = await buildUser();
+      const collection = await buildCollection({
+        teamId: user.teamId,
+        permission: null,
+      });
+      const document = await buildDocument({
+        collectionId: collection.id,
+        teamId: user.teamId,
+      });
+
+      const permission = await getDocumentPermission({
+        userId: user.id,
+        documentId: document.id,
+      });
+
+      expect(permission).toBeUndefined();
+    });
+  });
+});

server/utils/permissions.ts

@@ -0,0 +1,178 @@
+import compact from "lodash/compact";
+import orderBy from "lodash/orderBy";
+import { Op, WhereOptions } from "sequelize";
+import { CollectionPermission, DocumentPermission } from "@shared/types";
+import {
+  Document,
+  Group,
+  GroupMembership,
+  User,
+  UserMembership,
+} from "@server/models";
+import { authorize } from "@server/policies";
+
+// Higher value takes precedence
+export const CollectionPermissionPriority = {
+  [CollectionPermission.Admin]: 2,
+  [CollectionPermission.ReadWrite]: 1,
+  [CollectionPermission.Read]: 0,
+} satisfies Record<CollectionPermission, number>;
+
+// Higher value takes precedence
+export const DocumentPermissionPriority = {
+  [DocumentPermission.Admin]: 2,
+  [DocumentPermission.ReadWrite]: 1,
+  [DocumentPermission.Read]: 0,
+} satisfies Record<DocumentPermission, number>;
+
+/**
+ * Check if the given user can access a document
+ *
+ * @param user - The user to check
+ * @param documentId - The document to check
+ * @returns Boolean whether the user can access the document
+ */
+export const canUserAccessDocument = async (user: User, documentId: string) => {
+  try {
+    const document = await Document.findByPk(documentId, {
+      userId: user.id,
+    });
+    authorize(user, "read", document);
+    return true;
+  } catch (err) {
+    return false;
+  }
+};
+
+/**
+ * Determines whether the user's access to a document is being elevated with the new permission.
+ *
+ * @param {Object} params Input parameters.
+ * @param {string} params.userId The user to check.
+ * @param {string} params.documentId The document to check.
+ * @param {DocumentPermission} params.permission The new permission given to the user.
+ * @param {string} params.skipMembershipId The membership to skip when comparing the existing permissions.
+ * @returns {boolean} Whether the user has a higher access level
+ */
+export const isElevatedPermission = async ({
+  userId,
+  documentId,
+  permission,
+  skipMembershipId,
+}: {
+  userId: string;
+  documentId: string;
+  permission: DocumentPermission;
+  skipMembershipId?: string;
+}) => {
+  const existingPermission = await getDocumentPermission({
+    userId,
+    documentId,
+    skipMembershipId,
+  });
+
+  if (!existingPermission) {
+    return true;
+  }
+
+  return (
+    DocumentPermissionPriority[existingPermission] <
+    DocumentPermissionPriority[permission]
+  );
+};
+
+/**
+ * Returns the user's permission to a document.
+ *
+ * @param {Object} params Input parameters.
+ * @param {string} params.userId The user to check.
+ * @param {string} params.documentId The document to check.
+ * @param {string} params.skipMembershipId The membership to skip when comparing the existing permissions.
+ * @returns {DocumentPermission | undefined} Highest permission, if it exists.
+ */
+export const getDocumentPermission = async ({
+  userId,
+  documentId,
+  skipMembershipId,
+}: {
+  userId: string;
+  documentId: string;
+  skipMembershipId?: string;
+}): Promise<DocumentPermission | undefined> => {
+  const document = await Document.scope({
+    method: ["withCollectionPermissions", userId],
+  }).findOne({ where: { id: documentId } });
+
+  const permissions: DocumentPermission[] = [];
+
+  const collection = document?.collection;
+  if (collection) {
+    const collectionPermissions = orderBy(
+      compact([
+        collection.permission,
+        ...compact(
+          collection.memberships?.map(
+            (m) => m.permission as CollectionPermission
+          )
+        ),
+        ...compact(
+          collection.groupMemberships?.map(
+            (m) => m.permission as CollectionPermission
+          )
+        ),
+      ]),
+      (permission) => CollectionPermissionPriority[permission],
+      "desc"
+    );
+
+    if (collectionPermissions[0]) {
+      permissions.push(
+        collectionPermissions[0] === CollectionPermission.Read
+          ? DocumentPermission.Read
+          : DocumentPermission.ReadWrite
+      );
+    }
+  }
+
+  const userMembershipWhere: WhereOptions<UserMembership> = {
+    userId,
+    documentId,
+  };
+  const groupMembershipWhere: WhereOptions<GroupMembership> = {
+    documentId,
+  };
+
+  if (skipMembershipId) {
+    userMembershipWhere.id = { [Op.ne]: skipMembershipId };
+    groupMembershipWhere.id = { [Op.ne]: skipMembershipId };
+  }
+
+  const [userMemberships, groupMemberships] = await Promise.all([
+    UserMembership.findAll({
+      where: userMembershipWhere,
+    }),
+    GroupMembership.findAll({
+      where: groupMembershipWhere,
+      include: [
+        {
+          model: Group.filterByMember(userId),
+          as: "group",
+          required: true,
+        },
+      ],
+    }),
+  ]);
+
+  permissions.push(
+    ...userMemberships.map((m) => m.permission as DocumentPermission),
+    ...groupMemberships.map((m) => m.permission as DocumentPermission)
+  );
+
+  const orderedPermissions = orderBy(
+    permissions,
+    (permission) => DocumentPermissionPriority[permission],
+    "desc"
+  );
+
+  return orderedPermissions[0];
+};

server/utils/policies.ts

@@ -1,21 +0,0 @@
-import { Document, User } from "@server/models";
-import { authorize } from "@server/policies";
-
-/**
- * Check if the given user can access a document
- *
- * @param user - The user to check
- * @param documentId - The document to check
- * @returns Boolean whether the user can access the document
- */
-export const canUserAccessDocument = async (user: User, documentId: string) => {
-  try {
-    const document = await Document.findByPk(documentId, {
-      userId: user.id,
-    });
-    authorize(user, "read", document);
-    return true;
-  } catch (err) {
-    return false;
-  }
-};