From da4a10e8771bd6c9383b93eb66a95492bc4dd0d9 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sat, 9 Jul 2022 13:28:56 +0200 Subject: [PATCH] chore: Remove `shares.info` apiVersion 1 (#3758) * chore: Remove shares.info apiVersion 1 * fix: Sporadic test failure --- app/stores/SharesStore.ts | 1 - .../api/__snapshots__/shares.test.ts.snap | 8 +- server/routes/api/shares.test.ts | 338 +++++++----------- server/routes/api/shares.ts | 44 +-- server/routes/api/team.test.ts | 4 +- 5 files changed, 148 insertions(+), 247 deletions(-) diff --git a/app/stores/SharesStore.ts b/app/stores/SharesStore.ts index 9ed6211e3e41..ccc1cf72e7a0 100644 --- a/app/stores/SharesStore.ts +++ b/app/stores/SharesStore.ts @@ -59,7 +59,6 @@ export default class SharesStore extends BaseStore { try { const res = await client.post(`/${this.modelName}s.info`, { documentId, - apiVersion: 2, }); if (isUndefined(res)) { diff --git a/server/routes/api/__snapshots__/shares.test.ts.snap b/server/routes/api/__snapshots__/shares.test.ts.snap index d0f68fd9f359..a7cd3a6fdb36 100644 --- a/server/routes/api/__snapshots__/shares.test.ts.snap +++ b/server/routes/api/__snapshots__/shares.test.ts.snap @@ -9,7 +9,7 @@ Object { } `; -exports[`#shares.info should require authentication 1`] = ` +exports[`#shares.list should require authentication 1`] = ` Object { "error": "authentication_required", "message": "Authentication required", @@ -18,7 +18,7 @@ Object { } `; -exports[`#shares.list should require authentication 1`] = ` +exports[`#shares.revoke should require authentication 1`] = ` Object { "error": "authentication_required", "message": "Authentication required", @@ -27,7 +27,7 @@ Object { } `; -exports[`#shares.revoke should require authentication 1`] = ` +exports[`#shares.update should require authentication 1`] = ` Object { "error": "authentication_required", "message": "Authentication required", @@ -36,7 +36,7 @@ Object { } `; -exports[`#shares.update should require authentication 1`] = ` +exports[`should require authentication 1`] = ` Object { "error": "authentication_required", "message": "Authentication required", diff --git a/server/routes/api/shares.test.ts b/server/routes/api/shares.test.ts index 60342bad146c..f1010539a569 100644 --- a/server/routes/api/shares.test.ts +++ b/server/routes/api/shares.test.ts @@ -1,7 +1,13 @@ import TestServer from "fetch-test-server"; import { CollectionUser } from "@server/models"; import webService from "@server/services/web"; -import { buildUser, buildDocument, buildShare } from "@server/test/factories"; +import { + buildUser, + buildDocument, + buildShare, + buildAdmin, + buildCollection, +} from "@server/test/factories"; import { flushdb, seed } from "@server/test/support"; const app = webService(); @@ -287,48 +293,6 @@ describe("#shares.create", () => { }); describe("#shares.info", () => { - it("should allow reading share by id", async () => { - const { user, document } = await seed(); - const share = await buildShare({ - documentId: document.id, - teamId: user.teamId, - userId: user.id, - }); - const res = await server.post("/api/shares.info", { - body: { - token: user.getJwtToken(), - id: share.id, - }, - }); - const body = await res.json(); - expect(res.status).toEqual(200); - expect(body.data.id).toBe(share.id); - expect(body.data.createdBy.id).toBe(user.id); - }); - - it("should allow reading share created by deleted user", async () => { - const { user, document } = await seed(); - const author = await buildUser({ - teamId: user.teamId, - }); - const share = await buildShare({ - documentId: document.id, - teamId: author.teamId, - userId: author.id, - }); - await author.destroy(); - const res = await server.post("/api/shares.info", { - body: { - token: user.getJwtToken(), - id: share.id, - }, - }); - const body = await res.json(); - expect(res.status).toEqual(200); - expect(body.data.id).toBe(share.id); - expect(body.data.createdBy.id).toBe(author.id); - }); - it("should allow reading share by documentId", async () => { const { user, document } = await seed(); const share = await buildShare({ @@ -344,214 +308,162 @@ describe("#shares.info", () => { }); const body = await res.json(); expect(res.status).toEqual(200); - expect(body.data.id).toBe(share.id); - expect(body.data.published).toBe(true); + expect(body.data.shares.length).toBe(1); + expect(body.data.shares[0].id).toBe(share.id); + expect(body.data.shares[0].published).toBe(true); }); - - it("should find share created by another user", async () => { - const { admin, document } = await seed(); - const user = await buildUser({ - teamId: admin.teamId, + it("should return share for parent document with includeChildDocuments=true", async () => { + const { user, document, collection } = await seed(); + const childDocument = await buildDocument({ + teamId: document.teamId, + parentDocumentId: document.id, + collectionId: collection.id, }); const share = await buildShare({ documentId: document.id, - teamId: admin.teamId, - userId: admin.id, + teamId: document.teamId, + userId: user.id, + includeChildDocuments: true, }); + await collection.addDocumentToStructure(childDocument, 0); const res = await server.post("/api/shares.info", { body: { token: user.getJwtToken(), - documentId: document.id, + documentId: childDocument.id, }, }); const body = await res.json(); expect(res.status).toEqual(200); - expect(body.data.id).toBe(share.id); - expect(body.data.published).toBe(true); - }); - - it("should not find revoked share", async () => { - const { user, document } = await seed(); - const share = await buildShare({ - documentId: document.id, - teamId: user.teamId, - userId: user.id, - }); - await share.revoke(user.id); - const res = await server.post("/api/shares.info", { - body: { - token: user.getJwtToken(), - documentId: document.id, - }, + expect(body.data.shares.length).toBe(1); + expect(body.data.shares[0].id).toBe(share.id); + expect(body.data.shares[0].documentId).toBe(document.id); + expect(body.data.shares[0].published).toBe(true); + expect(body.data.shares[0].includeChildDocuments).toBe(true); + expect(body.policies.length).toBe(1); + expect(body.policies[0].abilities.update).toBe(true); + }); + it("should not return share for parent document with includeChildDocuments=false", async () => { + const { user, document, collection } = await seed(); + const childDocument = await buildDocument({ + teamId: document.teamId, + parentDocumentId: document.id, + collectionId: collection.id, }); - expect(res.status).toEqual(204); - }); - - it("should not find share for deleted document", async () => { - const { user, document } = await seed(); await buildShare({ documentId: document.id, - teamId: user.teamId, + teamId: document.teamId, userId: user.id, + includeChildDocuments: false, }); - await document.delete(user.id); + await collection.addDocumentToStructure(childDocument, 0); const res = await server.post("/api/shares.info", { body: { token: user.getJwtToken(), - documentId: document.id, + documentId: childDocument.id, }, }); expect(res.status).toEqual(204); }); - describe("apiVersion=2", () => { - it("should allow reading share by documentId", async () => { - const { user, document } = await seed(); - const share = await buildShare({ - documentId: document.id, - teamId: user.teamId, - userId: user.id, - }); - const res = await server.post("/api/shares.info", { - body: { - token: user.getJwtToken(), - documentId: document.id, - apiVersion: 2, - }, - }); - const body = await res.json(); - expect(res.status).toEqual(200); - expect(body.data.shares.length).toBe(1); - expect(body.data.shares[0].id).toBe(share.id); - expect(body.data.shares[0].published).toBe(true); - }); - it("should return share for parent document with includeChildDocuments=true", async () => { - const { user, document, collection } = await seed(); - const childDocument = await buildDocument({ - teamId: document.teamId, - parentDocumentId: document.id, - collectionId: collection.id, - }); - const share = await buildShare({ - documentId: document.id, - teamId: document.teamId, - userId: user.id, - includeChildDocuments: true, - }); - await collection.addDocumentToStructure(childDocument, 0); - const res = await server.post("/api/shares.info", { - body: { - token: user.getJwtToken(), - documentId: childDocument.id, - apiVersion: 2, - }, - }); - const body = await res.json(); - expect(res.status).toEqual(200); - expect(body.data.shares.length).toBe(1); - expect(body.data.shares[0].id).toBe(share.id); - expect(body.data.shares[0].documentId).toBe(document.id); - expect(body.data.shares[0].published).toBe(true); - expect(body.data.shares[0].includeChildDocuments).toBe(true); - expect(body.policies.length).toBe(1); - expect(body.policies[0].abilities.update).toBe(true); - }); - it("should not return share for parent document with includeChildDocuments=false", async () => { - const { user, document, collection } = await seed(); - const childDocument = await buildDocument({ - teamId: document.teamId, - parentDocumentId: document.id, - collectionId: collection.id, - }); - await buildShare({ - documentId: document.id, - teamId: document.teamId, - userId: user.id, - includeChildDocuments: false, - }); - await collection.addDocumentToStructure(childDocument, 0); - const res = await server.post("/api/shares.info", { - body: { - token: user.getJwtToken(), - documentId: childDocument.id, - apiVersion: 2, - }, - }); - expect(res.status).toEqual(204); - }); - it("should return shares for parent document and current document", async () => { - const { user, document, collection } = await seed(); - const childDocument = await buildDocument({ - teamId: document.teamId, - parentDocumentId: document.id, - collectionId: collection.id, - }); - const share = await buildShare({ - documentId: childDocument.id, - teamId: user.teamId, - userId: user.id, - includeChildDocuments: false, - }); - const share2 = await buildShare({ - documentId: document.id, - teamId: document.teamId, - userId: user.id, - includeChildDocuments: true, - }); - await collection.addDocumentToStructure(childDocument, 0); - const res = await server.post("/api/shares.info", { - body: { - token: user.getJwtToken(), - documentId: childDocument.id, - apiVersion: 2, - }, - }); - const body = await res.json(); - expect(res.status).toEqual(200); - expect(body.data.shares.length).toBe(2); - expect(body.data.shares[0].id).toBe(share.id); - expect(body.data.shares[0].includeChildDocuments).toBe(false); - expect(body.data.shares[0].documentId).toBe(childDocument.id); - expect(body.data.shares[0].published).toBe(true); - expect(body.data.shares[1].id).toBe(share2.id); - expect(body.data.shares[1].documentId).toBe(document.id); - expect(body.data.shares[1].published).toBe(true); - expect(body.data.shares[1].includeChildDocuments).toBe(true); + it("should return shares for parent document and current document", async () => { + const { user, document, collection } = await seed(); + const childDocument = await buildDocument({ + teamId: document.teamId, + parentDocumentId: document.id, + collectionId: collection.id, }); - }); - - it("should require authentication", async () => { - const { user, document } = await seed(); const share = await buildShare({ - documentId: document.id, + documentId: childDocument.id, teamId: user.teamId, userId: user.id, + includeChildDocuments: false, + }); + const share2 = await buildShare({ + documentId: document.id, + teamId: document.teamId, + userId: user.id, + includeChildDocuments: true, }); + await collection.addDocumentToStructure(childDocument, 0); const res = await server.post("/api/shares.info", { body: { - id: share.id, + token: user.getJwtToken(), + documentId: childDocument.id, }, }); const body = await res.json(); - expect(res.status).toEqual(401); - expect(body).toMatchSnapshot(); + expect(res.status).toEqual(200); + expect(body.data.shares.length).toBe(2); + expect(body.data.shares[0].id).toBe(share.id); + expect(body.data.shares[0].includeChildDocuments).toBe(false); + expect(body.data.shares[0].documentId).toBe(childDocument.id); + expect(body.data.shares[0].published).toBe(true); + expect(body.data.shares[1].id).toBe(share2.id); + expect(body.data.shares[1].documentId).toBe(document.id); + expect(body.data.shares[1].published).toBe(true); + expect(body.data.shares[1].includeChildDocuments).toBe(true); }); +}); - it("should require authorization", async () => { - const { admin, document } = await seed(); - const user = await buildUser(); - const share = await buildShare({ +it("should not find share by documentId in private collection", async () => { + const admin = await buildAdmin(); + const collection = await buildCollection({ + permission: null, + teamId: admin.teamId, + }); + const document = await buildDocument({ + collectionId: collection.id, + userId: admin.id, + teamId: admin.teamId, + }); + const user = await buildUser({ + teamId: admin.teamId, + }); + await buildShare({ + documentId: document.id, + teamId: admin.teamId, + userId: admin.id, + }); + const res = await server.post("/api/shares.info", { + body: { + token: user.getJwtToken(), documentId: document.id, - teamId: admin.teamId, - userId: admin.id, - }); - const res = await server.post("/api/shares.info", { - body: { - token: user.getJwtToken(), - id: share.id, - }, - }); - expect(res.status).toEqual(403); + }, }); + expect(res.status).toEqual(403); +}); + +it("should require authentication", async () => { + const { user, document } = await seed(); + const share = await buildShare({ + documentId: document.id, + teamId: user.teamId, + userId: user.id, + }); + const res = await server.post("/api/shares.info", { + body: { + id: share.id, + }, + }); + const body = await res.json(); + expect(res.status).toEqual(401); + expect(body).toMatchSnapshot(); +}); + +it("should require authorization", async () => { + const { admin, document } = await seed(); + const user = await buildUser(); + const share = await buildShare({ + documentId: document.id, + teamId: admin.teamId, + userId: admin.id, + }); + const res = await server.post("/api/shares.info", { + body: { + token: user.getJwtToken(), + id: share.id, + }, + }); + expect(res.status).toEqual(403); }); describe("#shares.update", () => { diff --git a/server/routes/api/shares.ts b/server/routes/api/shares.ts index ea6ffa97d97d..c77f19be4e2a 100644 --- a/server/routes/api/shares.ts +++ b/server/routes/api/shares.ts @@ -11,8 +11,14 @@ import pagination from "./middlewares/pagination"; const router = new Router(); router.post("shares.info", auth(), async (ctx) => { - const { id, documentId, apiVersion } = ctx.body; - assertUuid(id || documentId, "id or documentId is required"); + const { id, documentId } = ctx.body; + assertPresent(id || documentId, "id or documentId is required"); + if (id) { + assertUuid(id, "id is must be a uuid"); + } + if (documentId) { + assertUuid(documentId, "documentId is must be a uuid"); + } const { user } = ctx.state; const shares = []; @@ -35,37 +41,21 @@ router.post("shares.info", auth(), async (ctx) => { }, }); - // Deprecated API response returns just the share for the current documentId - if (apiVersion !== 2) { - if (!share || !share.document) { - ctx.response.status = 204; - return; - } - - authorize(user, "read", share); - ctx.body = { - data: presentShare(share, user.isAdmin), - policies: presentPolicies(user, [share]), - }; - return; - } - - // API version 2 returns the response for the current documentId and any - // parent documents that are publicly shared and accessible to the user + // We return the response for the current documentId and any parent documents + // that are publicly shared and accessible to the user if (share && share.document) { authorize(user, "read", share); shares.push(share); } if (documentId) { - const document = await Document.unscoped() - .scope("withCollection") - .findOne({ - where: { - id: documentId, - }, - }); - const parentIds = document?.collection?.getDocumentParents(documentId); + const document = await Document.findByPk(documentId, { + userId: user.id, + }); + authorize(user, "read", document); + + const collection = await document.$get("collection"); + const parentIds = collection?.getDocumentParents(documentId); const parentShare = parentIds ? await Share.scope({ method: ["withCollectionPermissions", user.id], diff --git a/server/routes/api/team.test.ts b/server/routes/api/team.test.ts index b613cf6893fe..0bd54d62196d 100644 --- a/server/routes/api/team.test.ts +++ b/server/routes/api/team.test.ts @@ -95,9 +95,9 @@ describe("#team.update", () => { }); const body = await res.json(); expect(res.status).toEqual(200); - expect(body.data.allowedDomains).toEqual([ - "example-company.org", + expect(body.data.allowedDomains.sort()).toEqual([ "example-company.net", + "example-company.org", ]); const teamDomains: TeamDomain[] = await TeamDomain.findAll({