From 6520e4469c679688b3b0fcd2b1dd195b171d67a4 Mon Sep 17 00:00:00 2001 From: David Nguyen Date: Thu, 25 Jun 2026 15:54:46 +1000 Subject: [PATCH] fix: resolve permission issues --- .../organisation-permission-hierarchy.spec.ts | 342 ++++++++++++++++++ .../delete-organisation-group.ts | 19 +- .../delete-organisation-members.ts | 67 +++- .../organisation-router/leave-organisation.ts | 8 + .../resend-organisation-member-invite.ts | 18 +- 5 files changed, 447 insertions(+), 7 deletions(-) create mode 100644 packages/app-tests/e2e/organisations/organisation-permission-hierarchy.spec.ts diff --git a/packages/app-tests/e2e/organisations/organisation-permission-hierarchy.spec.ts b/packages/app-tests/e2e/organisations/organisation-permission-hierarchy.spec.ts new file mode 100644 index 000000000..b688c7cd9 --- /dev/null +++ b/packages/app-tests/e2e/organisations/organisation-permission-hierarchy.spec.ts @@ -0,0 +1,342 @@ +import { NEXT_PUBLIC_WEBAPP_URL } from '@documenso/lib/constants/app'; +import { generateDatabaseId, nanoid } from '@documenso/lib/universal/id'; +import { prisma } from '@documenso/prisma'; +import { seedOrganisationMembers } from '@documenso/prisma/seed/organisations'; +import { seedUser } from '@documenso/prisma/seed/users'; +import { expect, type Page, test } from '@playwright/test'; +import { OrganisationGroupType, type OrganisationMemberRole } from '@prisma/client'; + +import { apiSignin } from '../fixtures/authentication'; + +const WEBAPP_BASE_URL = NEXT_PUBLIC_WEBAPP_URL(); + +/** + * Calls a tRPC mutation directly using the cookies of whoever is currently + * signed in on the page context. This deliberately bypasses the UI: the + * authorisation checks under test live on the server, and the UI may simply + * hide a button rather than reject the request, which would mask a backend gap. + */ +const trpcMutation = async (page: Page, procedure: string, input: Record) => { + return await page.request.post(`${WEBAPP_BASE_URL}/api/trpc/${procedure}`, { + headers: { 'content-type': 'application/json' }, + data: JSON.stringify({ json: input }), + }); +}; + +const getOrganisationMember = async (userId: number, organisationId: string) => { + return await prisma.organisationMember.findFirstOrThrow({ + where: { + userId, + organisationId, + }, + }); +}; + +const createCustomGroup = async (organisationId: string, organisationRole: OrganisationMemberRole) => { + return await prisma.organisationGroup.create({ + data: { + id: generateDatabaseId('org_group'), + organisationId, + name: `custom-${organisationRole}-${nanoid()}`, + type: OrganisationGroupType.CUSTOM, + organisationRole, + }, + }); +}; + +const createPendingInvite = async (organisationId: string, organisationRole: OrganisationMemberRole) => { + return await prisma.organisationMemberInvite.create({ + data: { + id: generateDatabaseId('member_invite'), + email: `invite-${nanoid()}@test.documenso.com`, + token: nanoid(32), + organisationId, + organisationRole, + }, + }); +}; + +test.describe('[ORGANISATION_PERMISSION_HIERARCHY]: member deletion', () => { + test('a manager cannot delete an admin via member.delete', async ({ page }) => { + const { organisation } = await seedUser({ isPersonalOrganisation: false }); + + const [managerUser, adminUser] = await seedOrganisationMembers({ + members: [ + { name: 'Manager', organisationRole: 'MANAGER' }, + { name: 'Admin', organisationRole: 'ADMIN' }, + ], + organisationId: organisation.id, + }); + + const adminMember = await getOrganisationMember(adminUser.id, organisation.id); + + await apiSignin({ page, email: managerUser.email }); + + const res = await trpcMutation(page, 'organisation.member.delete', { + organisationId: organisation.id, + organisationMemberId: adminMember.id, + }); + + expect(res.ok()).toBeFalsy(); + + // The admin must still be a member of the organisation. + const stillExists = await prisma.organisationMember.findFirst({ + where: { id: adminMember.id }, + }); + + expect(stillExists).not.toBeNull(); + }); + + test('a manager cannot delete an admin via member.deleteMany', async ({ page }) => { + const { organisation } = await seedUser({ isPersonalOrganisation: false }); + + const [managerUser, adminUser] = await seedOrganisationMembers({ + members: [ + { name: 'Manager', organisationRole: 'MANAGER' }, + { name: 'Admin', organisationRole: 'ADMIN' }, + ], + organisationId: organisation.id, + }); + + const adminMember = await getOrganisationMember(adminUser.id, organisation.id); + + await apiSignin({ page, email: managerUser.email }); + + const res = await trpcMutation(page, 'organisation.member.deleteMany', { + organisationId: organisation.id, + organisationMemberIds: [adminMember.id], + }); + + expect(res.ok()).toBeFalsy(); + + const stillExists = await prisma.organisationMember.findFirst({ + where: { id: adminMember.id }, + }); + + expect(stillExists).not.toBeNull(); + }); + + test('a manager cannot delete the organisation owner', async ({ page }) => { + const { user: ownerUser, organisation } = await seedUser({ isPersonalOrganisation: false }); + + const [managerUser] = await seedOrganisationMembers({ + members: [{ name: 'Manager', organisationRole: 'MANAGER' }], + organisationId: organisation.id, + }); + + const ownerMember = await getOrganisationMember(ownerUser.id, organisation.id); + + await apiSignin({ page, email: managerUser.email }); + + const res = await trpcMutation(page, 'organisation.member.deleteMany', { + organisationId: organisation.id, + organisationMemberIds: [ownerMember.id], + }); + + expect(res.ok()).toBeFalsy(); + + const stillExists = await prisma.organisationMember.findFirst({ + where: { id: ownerMember.id }, + }); + + expect(stillExists).not.toBeNull(); + }); + + test('an admin cannot delete the organisation owner', async ({ page }) => { + const { user: ownerUser, organisation } = await seedUser({ isPersonalOrganisation: false }); + + const [adminUser] = await seedOrganisationMembers({ + members: [{ name: 'Admin', organisationRole: 'ADMIN' }], + organisationId: organisation.id, + }); + + const ownerMember = await getOrganisationMember(ownerUser.id, organisation.id); + + await apiSignin({ page, email: adminUser.email }); + + const res = await trpcMutation(page, 'organisation.member.deleteMany', { + organisationId: organisation.id, + organisationMemberIds: [ownerMember.id], + }); + + expect(res.ok()).toBeFalsy(); + + const stillExists = await prisma.organisationMember.findFirst({ + where: { id: ownerMember.id }, + }); + + expect(stillExists).not.toBeNull(); + }); + + test('a manager can still delete a regular member (positive control)', async ({ page }) => { + const { organisation } = await seedUser({ isPersonalOrganisation: false }); + + const [managerUser, memberUser] = await seedOrganisationMembers({ + members: [ + { name: 'Manager', organisationRole: 'MANAGER' }, + { name: 'Member', organisationRole: 'MEMBER' }, + ], + organisationId: organisation.id, + }); + + const member = await getOrganisationMember(memberUser.id, organisation.id); + + await apiSignin({ page, email: managerUser.email }); + + const res = await trpcMutation(page, 'organisation.member.deleteMany', { + organisationId: organisation.id, + organisationMemberIds: [member.id], + }); + + expect(res.ok()).toBeTruthy(); + + const deleted = await prisma.organisationMember.findFirst({ + where: { id: member.id }, + }); + + expect(deleted).toBeNull(); + }); +}); + +test.describe('[ORGANISATION_PERMISSION_HIERARCHY]: group deletion', () => { + test('a manager cannot delete an admin-role group', async ({ page }) => { + const { organisation } = await seedUser({ isPersonalOrganisation: false }); + + const [managerUser] = await seedOrganisationMembers({ + members: [{ name: 'Manager', organisationRole: 'MANAGER' }], + organisationId: organisation.id, + }); + + const adminGroup = await createCustomGroup(organisation.id, 'ADMIN'); + + await apiSignin({ page, email: managerUser.email }); + + const res = await trpcMutation(page, 'organisation.group.delete', { + organisationId: organisation.id, + groupId: adminGroup.id, + }); + + expect(res.ok()).toBeFalsy(); + + const stillExists = await prisma.organisationGroup.findFirst({ + where: { id: adminGroup.id }, + }); + + expect(stillExists).not.toBeNull(); + }); + + test('a manager can delete a member-role group (positive control)', async ({ page }) => { + const { organisation } = await seedUser({ isPersonalOrganisation: false }); + + const [managerUser] = await seedOrganisationMembers({ + members: [{ name: 'Manager', organisationRole: 'MANAGER' }], + organisationId: organisation.id, + }); + + const memberGroup = await createCustomGroup(organisation.id, 'MEMBER'); + + await apiSignin({ page, email: managerUser.email }); + + const res = await trpcMutation(page, 'organisation.group.delete', { + organisationId: organisation.id, + groupId: memberGroup.id, + }); + + expect(res.ok()).toBeTruthy(); + + const deleted = await prisma.organisationGroup.findFirst({ + where: { id: memberGroup.id }, + }); + + expect(deleted).toBeNull(); + }); +}); + +test.describe('[ORGANISATION_PERMISSION_HIERARCHY]: invite resend', () => { + test('a manager cannot resend an admin-role invite', async ({ page }) => { + const { organisation } = await seedUser({ isPersonalOrganisation: false }); + + const [managerUser] = await seedOrganisationMembers({ + members: [{ name: 'Manager', organisationRole: 'MANAGER' }], + organisationId: organisation.id, + }); + + const adminInvite = await createPendingInvite(organisation.id, 'ADMIN'); + + await apiSignin({ page, email: managerUser.email }); + + const res = await trpcMutation(page, 'organisation.member.invite.resend', { + organisationId: organisation.id, + invitationId: adminInvite.id, + }); + + expect(res.ok()).toBeFalsy(); + }); + + test('a manager can resend a member-role invite (positive control)', async ({ page }) => { + const { organisation } = await seedUser({ isPersonalOrganisation: false }); + + const [managerUser] = await seedOrganisationMembers({ + members: [{ name: 'Manager', organisationRole: 'MANAGER' }], + organisationId: organisation.id, + }); + + const memberInvite = await createPendingInvite(organisation.id, 'MEMBER'); + + await apiSignin({ page, email: managerUser.email }); + + const res = await trpcMutation(page, 'organisation.member.invite.resend', { + organisationId: organisation.id, + invitationId: memberInvite.id, + }); + + expect(res.ok()).toBeTruthy(); + }); +}); + +test.describe('[ORGANISATION_PERMISSION_HIERARCHY]: leaving an organisation', () => { + test('the owner cannot leave without transferring ownership first', async ({ page }) => { + const { user: ownerUser, organisation } = await seedUser({ isPersonalOrganisation: false }); + + const ownerMember = await getOrganisationMember(ownerUser.id, organisation.id); + + await apiSignin({ page, email: ownerUser.email }); + + const res = await trpcMutation(page, 'organisation.leave', { + organisationId: organisation.id, + }); + + expect(res.ok()).toBeFalsy(); + + const stillExists = await prisma.organisationMember.findFirst({ + where: { id: ownerMember.id }, + }); + + expect(stillExists).not.toBeNull(); + }); + + test('a non-owner member can still leave (positive control)', async ({ page }) => { + const { organisation } = await seedUser({ isPersonalOrganisation: false }); + + const [memberUser] = await seedOrganisationMembers({ + members: [{ name: 'Member', organisationRole: 'MEMBER' }], + organisationId: organisation.id, + }); + + const member = await getOrganisationMember(memberUser.id, organisation.id); + + await apiSignin({ page, email: memberUser.email }); + + const res = await trpcMutation(page, 'organisation.leave', { + organisationId: organisation.id, + }); + + expect(res.ok()).toBeTruthy(); + + const deleted = await prisma.organisationMember.findFirst({ + where: { id: member.id }, + }); + + expect(deleted).toBeNull(); + }); +}); diff --git a/packages/trpc/server/organisation-router/delete-organisation-group.ts b/packages/trpc/server/organisation-router/delete-organisation-group.ts index b1230ab34..33bac9dc6 100644 --- a/packages/trpc/server/organisation-router/delete-organisation-group.ts +++ b/packages/trpc/server/organisation-router/delete-organisation-group.ts @@ -1,6 +1,7 @@ import { ORGANISATION_MEMBER_ROLE_PERMISSIONS_MAP } from '@documenso/lib/constants/organisations'; import { AppError, AppErrorCode } from '@documenso/lib/errors/app-error'; -import { buildOrganisationWhereQuery } from '@documenso/lib/utils/organisations'; +import { getMemberOrganisationRole } from '@documenso/lib/server-only/team/get-member-roles'; +import { buildOrganisationWhereQuery, isOrganisationRoleWithinUserHierarchy } from '@documenso/lib/utils/organisations'; import { prisma } from '@documenso/prisma'; import { OrganisationGroupType } from '@prisma/client'; @@ -59,6 +60,22 @@ export const deleteOrganisationGroupRoute = authenticatedProcedure }); } + const currentUserOrganisationRole = await getMemberOrganisationRole({ + organisationId, + reference: { + type: 'User', + id: user.id, + }, + }); + + // A user cannot delete a group whose role is higher than their own + // (e.g. a manager deleting an admin-role group). + if (!isOrganisationRoleWithinUserHierarchy(currentUserOrganisationRole, group.organisationRole)) { + throw new AppError(AppErrorCode.UNAUTHORIZED, { + message: 'You are not allowed to delete this organisation group', + }); + } + await prisma.organisationGroup.delete({ where: { id: groupId, diff --git a/packages/trpc/server/organisation-router/delete-organisation-members.ts b/packages/trpc/server/organisation-router/delete-organisation-members.ts index dd5822265..d19e3b71b 100644 --- a/packages/trpc/server/organisation-router/delete-organisation-members.ts +++ b/packages/trpc/server/organisation-router/delete-organisation-members.ts @@ -1,8 +1,15 @@ import { syncMemberCountWithStripeSeatPlan } from '@documenso/ee/server-only/stripe/update-subscription-item-quantity'; -import { ORGANISATION_MEMBER_ROLE_PERMISSIONS_MAP } from '@documenso/lib/constants/organisations'; +import { + ORGANISATION_MEMBER_ROLE_HIERARCHY, + ORGANISATION_MEMBER_ROLE_PERMISSIONS_MAP, +} from '@documenso/lib/constants/organisations'; import { AppError, AppErrorCode } from '@documenso/lib/errors/app-error'; import { jobs } from '@documenso/lib/jobs/client'; -import { buildOrganisationWhereQuery } from '@documenso/lib/utils/organisations'; +import { + buildOrganisationWhereQuery, + getHighestOrganisationRoleInGroup, + isOrganisationRoleWithinUserHierarchy, +} from '@documenso/lib/utils/organisations'; import { prisma } from '@documenso/prisma'; import { OrganisationMemberInviteStatus } from '@documenso/prisma/client'; @@ -60,9 +67,12 @@ export const deleteOrganisationMembers = async ({ }, }, members: { - select: { - id: true, - userId: true, + include: { + organisationGroupMembers: { + include: { + group: true, + }, + }, }, }, invites: { @@ -84,6 +94,41 @@ export const deleteOrganisationMembers = async ({ const membersToDelete = organisation.members.filter((member) => organisationMemberIds.includes(member.id)); + const currentUserMember = organisation.members.find((member) => member.userId === userId); + + if (!currentUserMember) { + throw new AppError(AppErrorCode.UNAUTHORIZED); + } + + const currentUserOrganisationRole = getHighestOrganisationRoleInGroup( + currentUserMember.organisationGroupMembers.map(({ group }) => group), + ); + + // The roles the current user is allowed to act on (their own role and below). + const manageableOrganisationRoles = ORGANISATION_MEMBER_ROLE_HIERARCHY[currentUserOrganisationRole]; + + for (const member of membersToDelete) { + // The organisation owner can never be removed via this route. Ownership must + // be transferred first (mirrors the admin and update-member routes). + if (member.userId === organisation.ownerUserId) { + throw new AppError(AppErrorCode.UNAUTHORIZED, { + message: 'Cannot remove the organisation owner', + }); + } + + const memberOrganisationRole = getHighestOrganisationRoleInGroup( + member.organisationGroupMembers.map(({ group }) => group), + ); + + // A user cannot remove a member whose role is higher than their own + // (e.g. a manager removing an admin). + if (!isOrganisationRoleWithinUserHierarchy(currentUserOrganisationRole, memberOrganisationRole)) { + throw new AppError(AppErrorCode.UNAUTHORIZED, { + message: 'Cannot remove a member with a higher role', + }); + } + } + const inviteCount = organisation.invites.length; const newMemberCount = organisation.members.length + inviteCount - membersToDelete.length; @@ -123,6 +168,18 @@ export const deleteOrganisationMembers = async ({ in: organisationMemberIds, }, organisationId, + userId: { + not: organisation.ownerUserId, + }, + organisationGroupMembers: { + none: { + group: { + organisationRole: { + notIn: manageableOrganisationRoles, + }, + }, + }, + }, }, }); }); diff --git a/packages/trpc/server/organisation-router/leave-organisation.ts b/packages/trpc/server/organisation-router/leave-organisation.ts index e04f5d26f..8e2b8b775 100644 --- a/packages/trpc/server/organisation-router/leave-organisation.ts +++ b/packages/trpc/server/organisation-router/leave-organisation.ts @@ -51,6 +51,14 @@ export const leaveOrganisationRoute = authenticatedProcedure throw new AppError(AppErrorCode.NOT_FOUND); } + // The organisation owner cannot leave their own organisation. Ownership must + // be transferred to another member first. + if (organisation.ownerUserId === userId) { + throw new AppError(AppErrorCode.UNAUTHORIZED, { + message: 'You cannot leave an organisation you own. Please transfer ownership first.', + }); + } + const { organisationClaim } = organisation; const inviteCount = organisation.invites.length; diff --git a/packages/trpc/server/organisation-router/resend-organisation-member-invite.ts b/packages/trpc/server/organisation-router/resend-organisation-member-invite.ts index b3c2bdc68..e83c1658a 100644 --- a/packages/trpc/server/organisation-router/resend-organisation-member-invite.ts +++ b/packages/trpc/server/organisation-router/resend-organisation-member-invite.ts @@ -1,7 +1,8 @@ import { ORGANISATION_MEMBER_ROLE_PERMISSIONS_MAP } from '@documenso/lib/constants/organisations'; import { AppError, AppErrorCode } from '@documenso/lib/errors/app-error'; import { sendOrganisationMemberInviteEmail } from '@documenso/lib/server-only/organisation/create-organisation-member-invites'; -import { buildOrganisationWhereQuery } from '@documenso/lib/utils/organisations'; +import { getMemberOrganisationRole } from '@documenso/lib/server-only/team/get-member-roles'; +import { buildOrganisationWhereQuery, isOrganisationRoleWithinUserHierarchy } from '@documenso/lib/utils/organisations'; import { prisma } from '@documenso/prisma'; import { authenticatedProcedure } from '../trpc'; @@ -97,6 +98,21 @@ export const resendOrganisationMemberInvitation = async ({ }); } + const currentUserOrganisationRole = await getMemberOrganisationRole({ + organisationId: organisation.id, + reference: { + type: 'User', + id: userId, + }, + }); + + // A user cannot interact with an invitation that is not within their own hierarchy. + if (!isOrganisationRoleWithinUserHierarchy(currentUserOrganisationRole, invitation.organisationRole)) { + throw new AppError(AppErrorCode.UNAUTHORIZED, { + message: 'You cannot resend an invite for a member with a higher role', + }); + } + await sendOrganisationMemberInviteEmail({ email: invitation.email, token: invitation.token,