From 26e6db1afd62594f7ee38f70c0e72db214098f32 Mon Sep 17 00:00:00 2001
From: Tom Moor <tom.moor@gmail.com>
Date: Tue, 10 Nov 2020 20:35:41 -0800
Subject: [PATCH] fix: Update document policy to disable sharing for read-only
 users (#1638)

* fix: Update document policy to disable sharing for read-only users

* test: Update test for new permission logic
---
 server/api/shares.test.js   |  9 +++------
 server/policies/document.js | 23 +++++------------------
 2 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/server/api/shares.test.js b/server/api/shares.test.js
index b9b768d88..a7078f24d 100644
--- a/server/api/shares.test.js
+++ b/server/api/shares.test.js
@@ -149,9 +149,10 @@ describe("#shares.create", () => {
     expect(body.data.documentTitle).toBe(document.title);
   });
 
-  it("should allow creating a share record for document in read-only collection", async () => {
+  it("should not allow creating a share record with read-only permissions", async () => {
     const { user, document, collection } = await seed();
     collection.private = true;
+
     await collection.save();
 
     await CollectionUser.create({
@@ -164,11 +165,7 @@ describe("#shares.create", () => {
     const res = await server.post("/api/shares.create", {
       body: { token: user.getJwtToken(), documentId: document.id },
     });
-    const body = await res.json();
-
-    expect(res.status).toEqual(200);
-    expect(body.data.published).toBe(false);
-    expect(body.data.documentTitle).toBe(document.title);
+    expect(res.status).toEqual(403);
   });
 
   it("should allow creating a share record if link previously revoked", async () => {
diff --git a/server/policies/document.js b/server/policies/document.js
index 32bf90241..310734a8f 100644
--- a/server/policies/document.js
+++ b/server/policies/document.js
@@ -16,18 +16,6 @@ allow(User, ["read", "download"], Document, (user, document) => {
   return user.teamId === document.teamId;
 });
 
-allow(User, ["share"], Document, (user, document) => {
-  if (document.archivedAt) return false;
-  if (document.deletedAt) return false;
-
-  // existence of collection option is not required here to account for share tokens
-  if (document.collection && cannot(user, "read", document.collection)) {
-    return false;
-  }
-
-  return user.teamId === document.teamId;
-});
-
 allow(User, ["star", "unstar"], Document, (user, document) => {
   if (document.archivedAt) return false;
   if (document.deletedAt) return false;
@@ -43,15 +31,14 @@ allow(User, ["star", "unstar"], Document, (user, document) => {
   return user.teamId === document.teamId;
 });
 
-allow(User, "update", Document, (user, document) => {
+allow(User, ["update", "share"], Document, (user, document) => {
   if (document.archivedAt) return false;
   if (document.deletedAt) return false;
 
-  invariant(
-    document.collection,
-    "collection is missing, did you forget to include in the query scope?"
-  );
-  if (cannot(user, "update", document.collection)) return false;
+  // existence of collection option is not required here to account for share tokens
+  if (document.collection && cannot(user, "update", document.collection)) {
+    return false;
+  }
 
   return user.teamId === document.teamId;
 });