fix: Improvements in share feat (#2502)

* Make request only when popover is visible

* Update policies required for shares.create shares.update

* Create withCollection scope

* Remove team share check from shares.create

* Update tests

app/scenes/Document/components/ShareButton.js

@@ -61,6 +61,7 @@ function ShareButton({ document }: Props) {
           share={share}
           sharedParent={sharedParent}
           onSubmit={popover.hide}
+          visible={popover.visible}
         />
       </Popover>
     </>

app/scenes/Document/components/SharePopover.js

@@ -23,9 +23,16 @@ type Props = {|
   share: Share,
   sharedParent: ?Share,
   onSubmit: () => void,
+  visible: boolean,
 |};
 
-function SharePopover({ document, share, sharedParent, onSubmit }: Props) {
+function SharePopover({
+  document,
+  share,
+  sharedParent,
+  onSubmit,
+  visible,
+}: Props) {
   const { t } = useTranslation();
   const { policies, shares, auth } = useStores();
   const { showToast } = useToasts();
@@ -41,9 +48,9 @@ function SharePopover({ document, share, sharedParent, onSubmit }: Props) {
   const isPubliclyShared = (share && share.published) || sharedParent;
 
   React.useEffect(() => {
-    document.share();
+    if (visible) document.share();
     return () => clearTimeout(timeout.current);
-  }, [document]);
+  }, [document, visible]);
 
   const handlePublishedChange = React.useCallback(
     async (event) => {

server/api/shares.js

@@ -3,7 +3,7 @@ import Router from "koa-router";
 import Sequelize from "sequelize";
 import { NotFoundError } from "../errors";
 import auth from "../middlewares/authentication";
-import { Document, User, Event, Share, Team } from "../models";
+import { Document, User, Event, Share, Team, Collection } from "../models";
 import policy from "../policies";
 import { presentShare, presentPolicies } from "../presenters";
 import pagination from "./middlewares/pagination";
@@ -18,7 +18,9 @@ router.post("shares.info", auth(), async (ctx) => {
 
   const user = ctx.state.user;
   let shares = [];
-  let share = await Share.findOne({
+  let share = await Share.scope({
+    method: ["withCollection", user.id],
+  }).findOne({
     where: id
       ? {
           id,
@@ -119,6 +121,14 @@ router.post("shares.list", auth(), pagination(), async (ctx) => {
         where: {
           collectionId: collectionIds,
         },
+        include: [
+          {
+            model: Collection.scope({
+              method: ["withMembership", user.id],
+            }),
+            as: "collection",
+          },
+        ],
       },
       {
         model: User,
@@ -147,7 +157,14 @@ router.post("shares.update", auth(), async (ctx) => {
   ctx.assertUuid(id, "id is required");
 
   const { user } = ctx.state;
-  const share = await Share.findByPk(id);
+  const team = await Team.findByPk(user.teamId);
+  authorize(user, "share", team);
+
+  // fetch the share with document and collection.
+  const share = await Share.scope({
+    method: ["withCollection", user.id],
+  }).findByPk(id);
+
   authorize(user, "update", share);
 
   if (published !== undefined) {
@@ -190,8 +207,8 @@ router.post("shares.create", auth(), async (ctx) => {
   const user = ctx.state.user;
   const document = await Document.findByPk(documentId, { userId: user.id });
   const team = await Team.findByPk(user.teamId);
-  authorize(user, "share", document);
-  authorize(user, "share", team);
+  // user could be creating the share link to share with team members
+  authorize(user, "read", document);
 
   const [share, isCreated] = await Share.findOrCreate({
     where: {

server/api/shares.test.js

@@ -150,7 +150,7 @@ describe("#shares.create", () => {
     expect(body.data.documentTitle).toBe(document.title);
   });
 
-  it("should not allow creating a share record with read-only permissions", async () => {
+  it("should allow creating a share record with read-only permissions but no publishing", async () => {
     const { user, document, collection } = await seed();
     collection.permission = null;
 
@@ -166,7 +166,14 @@ describe("#shares.create", () => {
     const res = await server.post("/api/shares.create", {
       body: { token: user.getJwtToken(), documentId: document.id },
     });
-    expect(res.status).toEqual(403);
+    const body = await res.json();
+    expect(res.status).toEqual(200);
+
+    const response = await server.post("/api/shares.update", {
+      body: { token: user.getJwtToken(), id: body.data.id, published: true },
+    });
+
+    expect(response.status).toEqual(403);
   });
 
   it("should allow creating a share record if link previously revoked", async () => {
@@ -203,22 +210,36 @@ describe("#shares.create", () => {
     expect(body.data.id).toBe(share.id);
   });
 
-  it("should not allow creating a share record if team sharing disabled", async () => {
+  it("should allow creating a share record if team sharing disabled but not publishing", async () => {
     const { user, document, team } = await seed();
     await team.update({ sharing: false });
     const res = await server.post("/api/shares.create", {
       body: { token: user.getJwtToken(), documentId: document.id },
     });
-    expect(res.status).toEqual(403);
+    const body = await res.json();
+    expect(res.status).toEqual(200);
+
+    const response = await server.post("/api/shares.update", {
+      body: { token: user.getJwtToken(), id: body.data.id, published: true },
+    });
+
+    expect(response.status).toEqual(403);
   });
 
-  it("should not allow creating a share record if collection sharing disabled", async () => {
+  it("should allow creating a share record if collection sharing disabled but not publishing", async () => {
     const { user, collection, document } = await seed();
     await collection.update({ sharing: false });
     const res = await server.post("/api/shares.create", {
       body: { token: user.getJwtToken(), documentId: document.id },
     });
-    expect(res.status).toEqual(403);
+    const body = await res.json();
+    expect(res.status).toEqual(200);
+
+    const response = await server.post("/api/shares.update", {
+      body: { token: user.getJwtToken(), id: body.data.id, published: true },
+    });
+
+    expect(response.status).toEqual(403);
   });
 
   it("should require authentication", async () => {

server/models/Share.js

@@ -44,6 +44,27 @@ Share.associate = (models) => {
       { association: "team" },
     ],
   });
+  Share.addScope("withCollection", (userId) => {
+    return {
+      include: [
+        {
+          model: models.Document,
+          paranoid: true,
+          as: "document",
+          include: [
+            {
+              model: models.Collection.scope({
+                method: ["withMembership", userId],
+              }),
+              as: "collection",
+            },
+          ],
+        },
+        { association: "user", paranoid: false },
+        { association: "team" },
+      ],
+    };
+  });
 };
 
 Share.prototype.revoke = function (userId) {

server/policies/share.js

@@ -3,7 +3,7 @@ import { AdminRequiredError } from "../errors";
 import { Share, User } from "../models";
 import policy from "./policy";
 
-const { allow } = policy;
+const { allow, cannot } = policy;
 
 allow(User, "read", Share, (user, share) => {
   return user.teamId === share.teamId;
@@ -11,6 +11,9 @@ allow(User, "read", Share, (user, share) => {
 
 allow(User, "update", Share, (user, share) => {
   if (user.isViewer) return false;
+
+  // only the user who can share the document publicaly can update the share.
+  if (cannot(user, "share", share.document)) return false;
   return user.teamId === share.teamId;
 });