From 548a56e058a204741632c237a6b644a637edb404 Mon Sep 17 00:00:00 2001 From: Apoorv Mishra Date: Wed, 27 Dec 2023 20:42:39 +0530 Subject: [PATCH] Accomodate membership `id` (#6221) * fix: accomodate membership id * fix: remove only * fix: event handling * fix: tests * fix: use transaction * Remove useless test --------- Co-authored-by: Tom Moor --- app/components/WebsocketProvider.tsx | 10 +++- app/stores/CollectionGroupMembershipsStore.ts | 9 ++- app/stores/MembershipsStore.ts | 21 ++++++- .../server/tasks/DeliverWebhookTask.ts | 4 +- .../presenters/collectionGroupMembership.ts | 2 +- server/presenters/membership.ts | 2 +- .../api/collections/collections.test.ts | 26 +++++++++ server/routes/api/collections/collections.ts | 55 ++++++++++++++----- server/types.ts | 5 +- 9 files changed, 106 insertions(+), 28 deletions(-) diff --git a/app/components/WebsocketProvider.tsx b/app/components/WebsocketProvider.tsx index 08c2b1ee9..b86eaafe9 100644 --- a/app/components/WebsocketProvider.tsx +++ b/app/components/WebsocketProvider.tsx @@ -400,14 +400,20 @@ class WebsocketProvider extends React.Component { err instanceof NotFoundError ) { collections.remove(event.collectionId); - memberships.remove(`${event.userId}-${event.collectionId}`); + memberships.revoke({ + userId: event.userId, + collectionId: event.collectionId, + }); return; } } documents.removeCollectionDocuments(event.collectionId); } else { - memberships.remove(`${event.userId}-${event.collectionId}`); + memberships.revoke({ + userId: event.userId, + collectionId: event.collectionId, + }); } } ); diff --git a/app/stores/CollectionGroupMembershipsStore.ts b/app/stores/CollectionGroupMembershipsStore.ts index bf1fcc6ad..0b7580871 100644 --- a/app/stores/CollectionGroupMembershipsStore.ts +++ b/app/stores/CollectionGroupMembershipsStore.ts @@ -71,13 +71,18 @@ export default class CollectionGroupMembershipsStore extends Store m.groupId === groupId && m.collectionId === collectionId + ); + if (membership) { + this.remove(membership.id); + } } @action removeCollectionMemberships = (collectionId: string) => { this.data.forEach((membership, key) => { - if (key.includes(collectionId)) { + if (membership.collectionId === collectionId) { this.remove(key); } }); diff --git a/app/stores/MembershipsStore.ts b/app/stores/MembershipsStore.ts index 6e361fb68..24adf8f15 100644 --- a/app/stores/MembershipsStore.ts +++ b/app/stores/MembershipsStore.ts @@ -71,16 +71,31 @@ export default class MembershipsStore extends Store { id: collectionId, userId, }); - this.remove(`${userId}-${collectionId}`); - this.rootStore.users.remove(userId); + this.revoke({ userId, collectionId }); } @action removeCollectionMemberships = (collectionId: string) => { this.data.forEach((membership, key) => { - if (key.includes(collectionId)) { + if (membership.collectionId === collectionId) { this.remove(key); } }); }; + + @action + revoke = ({ + userId, + collectionId, + }: { + collectionId: string; + userId: string; + }) => { + const membership = Array.from(this.data.values()).find( + (m) => m.userId === userId && m.collectionId === collectionId + ); + if (membership) { + this.remove(membership.id); + } + }; } diff --git a/plugins/webhooks/server/tasks/DeliverWebhookTask.ts b/plugins/webhooks/server/tasks/DeliverWebhookTask.ts index 230e871cc..827b150df 100644 --- a/plugins/webhooks/server/tasks/DeliverWebhookTask.ts +++ b/plugins/webhooks/server/tasks/DeliverWebhookTask.ts @@ -441,7 +441,7 @@ export default class DeliverWebhookTask extends BaseTask { event, subscription, payload: { - id: `${event.userId}-${event.collectionId}`, + id: event.data.membershipId, model: model && presentMembership(model), collection: model && presentCollection(model.collection!), user: model && presentUser(model.user), @@ -468,7 +468,7 @@ export default class DeliverWebhookTask extends BaseTask { event, subscription, payload: { - id: `${event.modelId}-${event.collectionId}`, + id: event.data.membershipId, model: model && presentCollectionGroupMembership(model), collection: model && presentCollection(model.collection!), group: model && presentGroup(model.group), diff --git a/server/presenters/collectionGroupMembership.ts b/server/presenters/collectionGroupMembership.ts index 64dac840b..065d2d41e 100644 --- a/server/presenters/collectionGroupMembership.ts +++ b/server/presenters/collectionGroupMembership.ts @@ -12,7 +12,7 @@ export default function presentCollectionGroupMembership( membership: GroupPermission ): Membership { return { - id: `${membership.groupId}-${membership.collectionId}`, + id: membership.id, groupId: membership.groupId, collectionId: membership.collectionId, permission: membership.permission, diff --git a/server/presenters/membership.ts b/server/presenters/membership.ts index 81dd80b5a..fcc1c0399 100644 --- a/server/presenters/membership.ts +++ b/server/presenters/membership.ts @@ -12,7 +12,7 @@ export default function presentMembership( membership: UserPermission ): Membership { return { - id: `${membership.userId}-${membership.collectionId}`, + id: membership.id, userId: membership.userId, collectionId: membership.collectionId, permission: membership.permission, diff --git a/server/routes/api/collections/collections.test.ts b/server/routes/api/collections/collections.test.ts index 2bf832d7d..8e6304552 100644 --- a/server/routes/api/collections/collections.test.ts +++ b/server/routes/api/collections/collections.test.ts @@ -699,6 +699,28 @@ describe("#collections.remove_user", () => { expect(users.length).toEqual(1); }); + it("should fail with status 400 bad request if user is not a member", async () => { + const admin = await buildAdmin(); + const collection = await buildCollection({ + teamId: admin.teamId, + userId: admin.id, + permission: null, + }); + const nonMember = await buildUser({ + teamId: admin.teamId, + }); + const res = await server.post("/api/collections.remove_user", { + body: { + token: admin.getJwtToken(), + id: collection.id, + userId: nonMember.id, + }, + }); + const body = await res.json(); + expect(res.status).toEqual(400); + expect(body.message).toEqual("User is not a collection member"); + }); + it("should require user in team", async () => { const user = await buildUser(); const collection = await buildCollection({ @@ -768,11 +790,13 @@ describe("#collections.group_memberships", () => { id: collection.id, }, }); + const [membership] = await collection.$get("collectionGroupMemberships"); const body = await res.json(); expect(res.status).toEqual(200); expect(body.data.groups.length).toEqual(1); expect(body.data.groups[0].id).toEqual(group.id); expect(body.data.collectionGroupMemberships.length).toEqual(1); + expect(body.data.collectionGroupMemberships[0].id).toEqual(membership.id); expect(body.data.collectionGroupMemberships[0].permission).toEqual( CollectionPermission.ReadWrite ); @@ -906,11 +930,13 @@ describe("#collections.memberships", () => { id: collection.id, }, }); + const [membership] = await collection.$get("memberships"); const body = await res.json(); expect(res.status).toEqual(200); expect(body.data.users.length).toEqual(1); expect(body.data.users[0].id).toEqual(user.id); expect(body.data.memberships.length).toEqual(1); + expect(body.data.memberships[0].id).toEqual(membership.id); expect(body.data.memberships[0].permission).toEqual( CollectionPermission.Admin ); diff --git a/server/routes/api/collections/collections.ts b/server/routes/api/collections/collections.ts index 4374ea227..7781a9bbf 100644 --- a/server/routes/api/collections/collections.ts +++ b/server/routes/api/collections/collections.ts @@ -249,6 +249,7 @@ router.post( modelId: groupId, data: { name: group.name, + membershipId: membership.id, }, ip: ctx.request.ip, }); @@ -267,31 +268,46 @@ router.post( "collections.remove_group", auth(), validate(T.CollectionsRemoveGroupSchema), + transaction(), async (ctx: APIContext) => { const { id, groupId } = ctx.input.body; const { user } = ctx.state.auth; + const { transaction } = ctx.state; const collection = await Collection.scope({ method: ["withMembership", user.id], - }).findByPk(id); + }).findByPk(id, { transaction }); authorize(user, "update", collection); - const group = await Group.findByPk(groupId); + const group = await Group.findByPk(groupId, { transaction }); authorize(user, "read", group); - await collection.$remove("group", group); - await Event.create({ - name: "collections.remove_group", - collectionId: collection.id, - teamId: collection.teamId, - actorId: user.id, - modelId: groupId, - data: { - name: group.name, - }, - ip: ctx.request.ip, + const [membership] = await collection.$get("collectionGroupMemberships", { + where: { groupId }, + transaction, }); + if (!membership) { + ctx.throw(400, "This Group is not a part of the collection"); + } + + await collection.$remove("group", group); + await Event.create( + { + name: "collections.remove_group", + collectionId: collection.id, + teamId: collection.teamId, + actorId: user.id, + modelId: groupId, + data: { + name: group.name, + membershipId: membership.id, + }, + ip: ctx.request.ip, + }, + { transaction } + ); + ctx.body = { success: true, }; @@ -416,6 +432,7 @@ router.post( actorId: actor.id, data: { name: user.name, + membershipId: membership.id, }, ip: ctx.request.ip, }, @@ -436,8 +453,8 @@ router.post( router.post( "collections.remove_user", auth(), - transaction(), validate(T.CollectionsRemoveUserSchema), + transaction(), async (ctx: APIContext) => { const { auth, transaction } = ctx.state; const actor = auth.user; @@ -451,7 +468,16 @@ router.post( const user = await User.findByPk(userId, { transaction }); authorize(actor, "read", user); + const [membership] = await collection.$get("memberships", { + where: { userId }, + transaction, + }); + if (!membership) { + ctx.throw(400, "User is not a collection member"); + } + await collection.$remove("user", user, { transaction }); + await Event.create( { name: "collections.remove_user", @@ -461,6 +487,7 @@ router.post( actorId: actor.id, data: { name: user.name, + membershipId: membership.id, }, ip: ctx.request.ip, }, diff --git a/server/types.ts b/server/types.ts index b73170c2f..b07bbd16c 100644 --- a/server/types.ts +++ b/server/types.ts @@ -199,15 +199,14 @@ export type CollectionUserEvent = BaseEvent & { name: "collections.add_user" | "collections.remove_user"; userId: string; collectionId: string; + data: { name: string; membershipId: string }; }; export type CollectionGroupEvent = BaseEvent & { name: "collections.add_group" | "collections.remove_group"; collectionId: string; modelId: string; - data: { - name: string; - }; + data: { name: string; membershipId: string }; }; export type CollectionEvent = BaseEvent &