From a19fb25bea9b932dde3757253a617e8afd39efd5 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Thu, 20 Jun 2024 09:18:18 -0400 Subject: [PATCH] fix: Open permissions for guests that have collection manage permission (#7075) * fix: Opens up permissions for guests that have collection manage permission * tsc * tests --- server/policies/document.test.ts | 207 ++++++++++++++++--------------- server/policies/document.ts | 27 ++-- 2 files changed, 123 insertions(+), 111 deletions(-) diff --git a/server/policies/document.test.ts b/server/policies/document.test.ts index 98adb7b16..ef8922e84 100644 --- a/server/policies/document.test.ts +++ b/server/policies/document.test.ts @@ -316,112 +316,121 @@ describe("archived document", () => { }); describe("read document", () => { - it("should allow read permissions for team member", async () => { - const team = await buildTeam(); - const user = await buildUser({ teamId: team.id }); - const collection = await buildCollection({ - teamId: team.id, - permission: null, - }); - const doc = await buildDocument({ - teamId: team.id, - collectionId: collection.id, - }); - await UserMembership.create({ - userId: user.id, - documentId: doc.id, - permission: DocumentPermission.Read, - createdById: user.id, - }); + for (const role of Object.values(UserRole)) { + it(`should allow read permissions for ${role}`, async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const collection = await buildCollection({ + teamId: team.id, + permission: null, + }); + const doc = await buildDocument({ + teamId: team.id, + collectionId: collection.id, + }); + await UserMembership.create({ + userId: user.id, + documentId: doc.id, + permission: DocumentPermission.Read, + createdById: user.id, + }); - // reload to get membership - const document = await Document.findByPk(doc.id, { userId: user.id }); - const abilities = serialize(user, document); - expect(abilities.read).toEqual(true); - expect(abilities.download).toEqual(true); - expect(abilities.subscribe).toEqual(true); - expect(abilities.unsubscribe).toEqual(true); - expect(abilities.comment).toEqual(true); - expect(abilities.update).toEqual(false); - expect(abilities.createChildDocument).toEqual(false); - expect(abilities.manageUsers).toEqual(false); - expect(abilities.archive).toEqual(false); - expect(abilities.delete).toEqual(false); - expect(abilities.share).toEqual(false); - expect(abilities.move).toEqual(false); - }); + // reload to get membership + const document = await Document.findByPk(doc.id, { userId: user.id }); + const abilities = serialize(user, document); + expect(abilities.read).toEqual(true); + expect(abilities.download).toEqual(true); + expect(abilities.subscribe).toEqual(true); + expect(abilities.unsubscribe).toEqual(true); + expect(abilities.comment).toEqual(true); + expect(abilities.update).toEqual(false); + expect(abilities.createChildDocument).toEqual(false); + expect(abilities.manageUsers).toEqual(false); + expect(abilities.archive).toEqual(false); + expect(abilities.delete).toEqual(false); + expect(abilities.share).toEqual(false); + expect(abilities.move).toEqual(false); + }); + } }); describe("read_write document", () => { - it("should allow write permissions for team member", async () => { - const team = await buildTeam(); - const user = await buildUser({ teamId: team.id }); - const collection = await buildCollection({ - teamId: team.id, - permission: null, - }); - const doc = await buildDocument({ - teamId: team.id, - collectionId: collection.id, - }); - await UserMembership.create({ - userId: user.id, - documentId: doc.id, - permission: DocumentPermission.ReadWrite, - createdById: user.id, - }); + for (const role of Object.values(UserRole)) { + it(`should allow write permissions for ${role}`, async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id, role }); + const collection = await buildCollection({ + teamId: team.id, + permission: null, + }); + const doc = await buildDocument({ + teamId: team.id, + collectionId: collection.id, + }); + await UserMembership.create({ + userId: user.id, + documentId: doc.id, + permission: DocumentPermission.ReadWrite, + createdById: user.id, + }); - // reload to get membership - const document = await Document.findByPk(doc.id, { userId: user.id }); - const abilities = serialize(user, document); - expect(abilities.read).toEqual(true); - expect(abilities.download).toEqual(true); - expect(abilities.update).toEqual(true); - expect(abilities.delete).toEqual(true); - expect(abilities.subscribe).toEqual(true); - expect(abilities.unsubscribe).toEqual(true); - expect(abilities.comment).toEqual(true); - expect(abilities.createChildDocument).toEqual(false); - expect(abilities.manageUsers).toEqual(false); - expect(abilities.archive).toEqual(false); - expect(abilities.share).toEqual(false); - expect(abilities.move).toEqual(false); - }); + // reload to get membership + const document = await Document.findByPk(doc.id, { userId: user.id }); + const abilities = serialize(user, document); + expect(abilities.read).toEqual(true); + expect(abilities.download).toEqual(true); + expect(abilities.update).toEqual(true); + expect(abilities.delete).toEqual(true); + expect(abilities.subscribe).toEqual(true); + expect(abilities.unsubscribe).toEqual(true); + expect(abilities.comment).toEqual(true); + expect(abilities.createChildDocument).toEqual(false); + expect(abilities.manageUsers).toEqual(false); + expect(abilities.archive).toEqual(false); + expect(abilities.share).toEqual(false); + expect(abilities.move).toEqual(false); + }); + } }); describe("manage document", () => { - it("should allow write permissions, user management, and sub-document creation", async () => { - const team = await buildTeam(); - const user = await buildUser({ teamId: team.id }); - const collection = await buildCollection({ - teamId: team.id, - permission: null, - }); - const doc = await buildDocument({ - teamId: team.id, - collectionId: collection.id, - }); - await UserMembership.create({ - userId: user.id, - documentId: doc.id, - permission: DocumentPermission.Admin, - createdById: user.id, - }); + for (const role of Object.values(UserRole)) { + it(`should allow write permissions, user management, and sub-document creation for ${role}`, async () => { + const team = await buildTeam(); + const user = await buildUser({ + teamId: team.id, + role, + }); + const collection = await buildCollection({ + teamId: team.id, + permission: null, + }); + const doc = await buildDocument({ + teamId: team.id, + collectionId: collection.id, + }); + await UserMembership.create({ + userId: user.id, + documentId: doc.id, + permission: DocumentPermission.Admin, + createdById: user.id, + }); - // reload to get membership - const document = await Document.findByPk(doc.id, { userId: user.id }); - const abilities = serialize(user, document); - expect(abilities.read).toEqual(true); - expect(abilities.download).toEqual(true); - expect(abilities.update).toEqual(true); - expect(abilities.delete).toEqual(true); - expect(abilities.subscribe).toEqual(true); - expect(abilities.unsubscribe).toEqual(true); - expect(abilities.comment).toEqual(true); - expect(abilities.createChildDocument).toEqual(true); - expect(abilities.manageUsers).toEqual(true); - expect(abilities.archive).toEqual(true); - expect(abilities.move).toEqual(true); - expect(abilities.share).toEqual(false); - }); + // reload to get membership + const document = await Document.findByPk(doc.id, { userId: user.id }); + const abilities = serialize(user, document); + expect(abilities.read).toEqual(true); + expect(abilities.download).toEqual(true); + expect(abilities.update).toEqual(true); + expect(abilities.delete).toEqual(true); + expect(abilities.subscribe).toEqual(true); + expect(abilities.unsubscribe).toEqual(true); + expect(abilities.comment).toEqual(true); + expect(abilities.createChildDocument).toEqual(true); + expect(abilities.manageUsers).toEqual(true); + expect(abilities.archive).toEqual(true); + expect(abilities.move).toEqual(false); + expect(abilities.share).toEqual(false); + }); + } }); diff --git a/server/policies/document.ts b/server/policies/document.ts index 19163ddd9..d53f5fae5 100644 --- a/server/policies/document.ts +++ b/server/policies/document.ts @@ -35,10 +35,9 @@ allow(User, "read", Document, (actor, document) => ); allow(User, ["listRevisions", "listViews"], Document, (actor, document) => - and( - // - can(actor, "read", document), - !actor.isGuest + or( + and(can(actor, "read", document), !actor.isGuest), + and(can(actor, "update", document), actor.isGuest) ) ); @@ -83,7 +82,6 @@ allow(User, "share", Document, (actor, document) => isTeamMutable(actor), !!document?.isActive, !document?.template, - !actor.isGuest, or(!document?.collection, can(actor, "share", document?.collection)) ) ); @@ -114,12 +112,21 @@ allow(User, "publish", Document, (actor, document) => ) ); -allow(User, ["move", "duplicate", "manageUsers"], Document, (actor, document) => +allow(User, ["manageUsers", "duplicate"], Document, (actor, document) => and( - !actor.isGuest, can(actor, "update", document), or( includesMembership(document, [DocumentPermission.Admin]), + can(actor, "updateDocument", document?.collection), + !!document?.isDraft && actor.id === document?.createdById + ) + ) +); + +allow(User, "move", Document, (actor, document) => + and( + can(actor, "update", document), + or( can(actor, "updateDocument", document?.collection), and(!!document?.isDraft && actor.id === document?.createdById) ) @@ -134,8 +141,7 @@ allow(User, "createChildDocument", Document, (actor, document) => can(actor, "read", document?.collection) ), !document?.isDraft, - !document?.template, - !actor.isGuest + !document?.template ) ); @@ -164,7 +170,6 @@ allow(User, "delete", Document, (actor, document) => and( isTeamModel(actor, document), isTeamMutable(actor), - !actor.isGuest, !document?.isDeleted, or( can(actor, "unarchive", document), @@ -193,7 +198,6 @@ allow(User, ["restore", "permanentDelete"], Document, (actor, document) => allow(User, "archive", Document, (actor, document) => and( - !actor.isGuest, !document?.template, !document?.isDraft, !!document?.isActive, @@ -207,7 +211,6 @@ allow(User, "archive", Document, (actor, document) => allow(User, "unarchive", Document, (actor, document) => and( - !actor.isGuest, !document?.template, !document?.isDraft, !document?.isDeleted,