From f389ac6414e449aff102611be14522b189fa2449 Mon Sep 17 00:00:00 2001 From: Saumya Pandey Date: Sun, 29 Aug 2021 10:44:09 +0530 Subject: [PATCH] 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 | 1 + .../Document/components/SharePopover.js | 13 ++++++-- server/api/shares.js | 27 ++++++++++++--- server/api/shares.test.js | 33 +++++++++++++++---- server/models/Share.js | 21 ++++++++++++ server/policies/share.js | 5 ++- 6 files changed, 85 insertions(+), 15 deletions(-) diff --git a/app/scenes/Document/components/ShareButton.js b/app/scenes/Document/components/ShareButton.js index 2862d385f..af7d1606e 100644 --- a/app/scenes/Document/components/ShareButton.js +++ b/app/scenes/Document/components/ShareButton.js @@ -61,6 +61,7 @@ function ShareButton({ document }: Props) { share={share} sharedParent={sharedParent} onSubmit={popover.hide} + visible={popover.visible} /> diff --git a/app/scenes/Document/components/SharePopover.js b/app/scenes/Document/components/SharePopover.js index d4aa4712f..10c9d544a 100644 --- a/app/scenes/Document/components/SharePopover.js +++ b/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) => { diff --git a/server/api/shares.js b/server/api/shares.js index 374d9355d..fd65f7eb6 100644 --- a/server/api/shares.js +++ b/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: { diff --git a/server/api/shares.test.js b/server/api/shares.test.js index 61b65bcad..a55e607f8 100644 --- a/server/api/shares.test.js +++ b/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 () => { diff --git a/server/models/Share.js b/server/models/Share.js index 3b4f82474..bc0a30b19 100644 --- a/server/models/Share.js +++ b/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) { diff --git a/server/policies/share.js b/server/policies/share.js index 75192b2d6..b5db8b459 100644 --- a/server/policies/share.js +++ b/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; });