From d30fb213d2515d162ebaba489d353271bb55993e Mon Sep 17 00:00:00 2001 From: Hariom Balhara Date: Thu, 4 Apr 2024 10:18:41 +0530 Subject: [PATCH] fix: Add adminReview option to allow impersonating org members (#14275) * added layouts for moving teams step * added admin section for org creation step * extended org creation to also move existing teams * wip * wip * wip * further changes * Add checkout for org in onboarding * Fix ts errors * Self review feedback * Self review addressed * Fix unit tests * Fix ts error * Seans feedback addressed * feat: fix correct accounts pending * fix: unit tests for new invite member permissions * tests: org admin onboarding tests for existing user * tests: Inital user self serve flow * chore: update teamAndUserFixture to create X amount of teams * chore: add testId to card actionButton * test: add test-Id to continue or checkout button * tests: add tests for migrating existing teams * feat: match new designs * fix: isAdminCheck * chore: fix pricing copy * fix: flacky tests * Fix tests? * More test fixes * Check all checkboxes * Fix type error * Fix failing test and typescript issues * Fix unpaid org allowing auto-add users * Add self-serve flag * Skip tests * Add adminReview option * Fixes * Add server checks for impersonation * Improved error * feat: extract - and disalow updating from org uset list if not reviewed --------- Co-authored-by: Peer Richelsen Co-authored-by: Alex van Andel Co-authored-by: Peer Richelsen Co-authored-by: sean-brydon Co-authored-by: Joe Au-Yeung Co-authored-by: Joe Au-Yeung <65426560+joeauyeung@users.noreply.github.com> --- apps/web/public/static/locales/en/common.json | 3 ++ .../lib/ImpersonationProvider.ts | 8 +++-- .../lib/ensureOrganizationIsReviewed.ts | 22 +++++++++++++ .../pages/settings/admin/AdminOrgPage.tsx | 22 +++++++------ .../components/UserTable/UserListTable.tsx | 8 ++--- .../lib/server/repository/organization.ts | 31 +++++++++++++++++++ .../migration.sql | 5 +++ packages/prisma/schema.prisma | 4 +++ packages/prisma/zod-utils.ts | 1 + .../organizations/adminUpdate.handler.ts | 1 + .../viewer/organizations/create.handler.ts | 5 +-- .../viewer/organizations/list.handler.ts | 2 ++ .../organizations/updateUser.handler.ts | 3 ++ turbo.json | 1 + 14 files changed, 98 insertions(+), 18 deletions(-) create mode 100644 packages/features/ee/organizations/lib/ensureOrganizationIsReviewed.ts create mode 100644 packages/prisma/migrations/20240401034329_add_admin_review_org/migration.sql diff --git a/apps/web/public/static/locales/en/common.json b/apps/web/public/static/locales/en/common.json index 938665e17c..efd4f87716 100644 --- a/apps/web/public/static/locales/en/common.json +++ b/apps/web/public/static/locales/en/common.json @@ -2362,5 +2362,8 @@ "lock_org_users_eventtypes_description": "Prevent members from creating their own event types.", "cookie_consent_checkbox": "I consent to our privacy policy and cookie usage", "make_a_call": "Make a Call", + "review": "Review", + "reviewed": "Reviewed", + "unreviewed": "Unreviewed", "ADD_NEW_STRINGS_ABOVE_THIS_LINE_TO_PREVENT_MERGE_CONFLICTS": "↑↑↑↑↑↑↑↑↑↑↑↑↑ Add your new strings above here ↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑" } diff --git a/packages/features/ee/impersonation/lib/ImpersonationProvider.ts b/packages/features/ee/impersonation/lib/ImpersonationProvider.ts index ddfa575241..3d364dbf6d 100644 --- a/packages/features/ee/impersonation/lib/ImpersonationProvider.ts +++ b/packages/features/ee/impersonation/lib/ImpersonationProvider.ts @@ -3,6 +3,7 @@ import type { Session } from "next-auth"; import CredentialsProvider from "next-auth/providers/credentials"; import { z } from "zod"; +import { ensureOrganizationIsReviewed } from "@calcom/ee/organizations/lib/ensureOrganizationIsReviewed"; import { getSession } from "@calcom/features/auth/lib/getSession"; import { ProfileRepository } from "@calcom/lib/server/repository/profile"; import prisma from "@calcom/prisma"; @@ -281,7 +282,9 @@ const ImpersonationProvider = CredentialsProvider({ ); } - if (!teamId) throw new Error("You do not have permission to do this."); + await ensureOrganizationIsReviewed(session?.user.org?.id); + + if (!teamId) throw new Error("Error-teamNotFound: You do not have permission to do this."); // Check session const sessionUserFromDb = await prisma.user.findUnique({ @@ -312,7 +315,7 @@ const ImpersonationProvider = CredentialsProvider({ }); if (sessionUserFromDb?.teams.length === 0 || impersonatedUser.teams.length === 0) { - throw new Error("You do not have permission to do this."); + throw new Error("Error-UserHasNoTeams: You do not have permission to do this."); } // We find team by ID so we know there is only one team in the array @@ -329,6 +332,7 @@ const ImpersonationProvider = CredentialsProvider({ }); export default ImpersonationProvider; + async function findProfile(returningUser: { id: number; username: string | null }) { const allOrgProfiles = await ProfileRepository.findAllProfilesForUserIncludingMovedUser({ id: returningUser.id, diff --git a/packages/features/ee/organizations/lib/ensureOrganizationIsReviewed.ts b/packages/features/ee/organizations/lib/ensureOrganizationIsReviewed.ts new file mode 100644 index 0000000000..dc1eca04c7 --- /dev/null +++ b/packages/features/ee/organizations/lib/ensureOrganizationIsReviewed.ts @@ -0,0 +1,22 @@ +import { OrganizationRepository } from "@calcom/lib/server/repository/organization"; + +/** + * It assumes that a user can only impersonate the members of the organization he is logged in to. + * Note: Ensuring that one organization's member can't impersonate other organization's member isn't the job of this function + */ +export async function ensureOrganizationIsReviewed(loggedInUserOrgId: number | undefined) { + if (loggedInUserOrgId) { + const org = await OrganizationRepository.findByIdIncludeOrganizationSettings({ + id: loggedInUserOrgId, + }); + + if (!org) { + throw new Error("Error-OrgNotFound: You do not have permission to do this."); + } + + if (!org.organizationSettings?.isAdminReviewed) { + // If the org is not reviewed, we can't allow impersonation + throw new Error("Error-OrgNotReviewed: You do not have permission to do this."); + } + } +} diff --git a/packages/features/ee/organizations/pages/settings/admin/AdminOrgPage.tsx b/packages/features/ee/organizations/pages/settings/admin/AdminOrgPage.tsx index 2acb0a8dcb..8d332f282d 100644 --- a/packages/features/ee/organizations/pages/settings/admin/AdminOrgPage.tsx +++ b/packages/features/ee/organizations/pages/settings/admin/AdminOrgPage.tsx @@ -78,14 +78,13 @@ function AdminOrgTable() {
{t("organization")} {t("owner")} - {t("verified")} + {t("reviewed")} {t("dns_configured")} {t("published")} {t("edit")}
- {data.map((org) => ( @@ -105,10 +104,10 @@ function AdminOrgTable() {
- {!org.organizationSettings?.isOrganizationVerified ? ( - {t("unverified")} + {!org.organizationSettings?.isAdminReviewed ? ( + {t("unreviewed")} ) : ( - {t("verified")} + {t("reviewed")} )}
@@ -134,14 +133,17 @@ function AdminOrgTable() {
{ - verifyMutation.mutate({ - orgId: org.id, + updateMutation.mutate({ + id: org.id, + organizationSettings: { + isAdminReviewed: true, + }, }); }, icon: "check" as const, diff --git a/packages/features/users/components/UserTable/UserListTable.tsx b/packages/features/users/components/UserTable/UserListTable.tsx index a2b3ed7901..b98159114c 100644 --- a/packages/features/users/components/UserTable/UserListTable.tsx +++ b/packages/features/users/components/UserTable/UserListTable.tsx @@ -108,7 +108,7 @@ function reducer(state: State, action: Action): State { export function UserListTable() { const { data: session } = useSession(); - const { data: currentMembership } = trpc.viewer.organizations.listCurrent.useQuery(); + const { data: org } = trpc.viewer.organizations.listCurrent.useQuery(); const { data: teams } = trpc.viewer.organizations.getTeams.useQuery(); const tableContainerRef = useRef(null); const [state, dispatch] = useReducer(reducer, initialState); @@ -126,9 +126,8 @@ export function UserListTable() { placeholderData: keepPreviousData, } ); - const totalDBRowCount = data?.pages?.[0]?.meta?.totalRowCount ?? 0; - const adminOrOwner = currentMembership?.user.role === "ADMIN" || currentMembership?.user.role === "OWNER"; + const adminOrOwner = org?.user.role === "ADMIN" || org?.user.role === "OWNER"; const domain = orgBranding?.fullDomain ?? WEBAPP_URL; const memorisedColumns = useMemo(() => { @@ -265,7 +264,8 @@ export function UserListTable() { const permissionsForUser = { canEdit: permissionsRaw.canEdit && user.accepted && !isSelf, canRemove: permissionsRaw.canRemove && !isSelf, - canImpersonate: user.accepted && !user.disableImpersonation && !isSelf, + canImpersonate: + user.accepted && !user.disableImpersonation && !isSelf && !!org?.canAdminImpersonate, canLeave: user.accepted && isSelf, canResendInvitation: permissionsRaw.canResendInvitation && !user.accepted, }; diff --git a/packages/lib/server/repository/organization.ts b/packages/lib/server/repository/organization.ts index 73e4366b8c..5657b1b4e8 100644 --- a/packages/lib/server/repository/organization.ts +++ b/packages/lib/server/repository/organization.ts @@ -6,6 +6,12 @@ import { MembershipRole } from "@calcom/prisma/enums"; import { createAProfileForAnExistingUser } from "../../createAProfileForAnExistingUser"; +const orgSelect = { + id: true, + name: true, + slug: true, + logoUrl: true, +}; export class OrganizationRepository { static async createWithOwner({ orgData, @@ -15,6 +21,7 @@ export class OrganizationRepository { name: string; slug: string; isOrganizationConfigured: boolean; + isOrganizationAdminReviewed: boolean; autoAcceptEmail: string; seats: number | null; pricePerSeat: number | null; @@ -33,6 +40,7 @@ export class OrganizationRepository { ...(!IS_TEAM_BILLING_ENABLED ? { slug: orgData.slug } : {}), organizationSettings: { create: { + isAdminReviewed: orgData.isOrganizationAdminReviewed, isOrganizationVerified: true, isOrganizationConfigured: orgData.isOrganizationConfigured, orgAutoAcceptEmail: orgData.autoAcceptEmail, @@ -65,4 +73,27 @@ export class OrganizationRepository { }); return { organization, ownerProfile }; } + + static async findById({ id }: { id: number }) { + return prisma.team.findUnique({ + where: { + id, + isOrganization: true, + }, + select: orgSelect, + }); + } + + static async findByIdIncludeOrganizationSettings({ id }: { id: number }) { + return prisma.team.findUnique({ + where: { + id, + isOrganization: true, + }, + select: { + ...orgSelect, + organizationSettings: true, + }, + }); + } } diff --git a/packages/prisma/migrations/20240401034329_add_admin_review_org/migration.sql b/packages/prisma/migrations/20240401034329_add_admin_review_org/migration.sql new file mode 100644 index 0000000000..7fbafea1ee --- /dev/null +++ b/packages/prisma/migrations/20240401034329_add_admin_review_org/migration.sql @@ -0,0 +1,5 @@ +-- AlterTable +ALTER TABLE "OrganizationSettings" ADD COLUMN "isAdminReviewed" BOOLEAN NOT NULL DEFAULT false; + +-- User written Query: Update OrganizationSettings table to mark all existing organizations as reviewed as all organizations are created by ADMIN only so far. +UPDATE "OrganizationSettings" SET "isAdminReviewed" = true; diff --git a/packages/prisma/schema.prisma b/packages/prisma/schema.prisma index 036a94a4a4..0dcd8a371a 100644 --- a/packages/prisma/schema.prisma +++ b/packages/prisma/schema.prisma @@ -392,9 +392,13 @@ model OrganizationSettings { organization Team @relation(fields: [organizationId], references: [id], onDelete: Cascade) organizationId Int @unique isOrganizationConfigured Boolean @default(false) + // It decides if new organization members can be auto-accepted or not isOrganizationVerified Boolean @default(false) orgAutoAcceptEmail String lockEventTypeCreationForUsers Boolean @default(false) + // It decides if instance ADMIN has reviewed the organization or not. + // It is used to allow super sensitive operations like 'impersonation of Org members by Org admin' + isAdminReviewed Boolean @default(false) dSyncData DSyncData? } diff --git a/packages/prisma/zod-utils.ts b/packages/prisma/zod-utils.ts index 4886b5a468..f69a321d01 100644 --- a/packages/prisma/zod-utils.ts +++ b/packages/prisma/zod-utils.ts @@ -353,6 +353,7 @@ export const orgSettingsSchema = z .object({ isOrganizationVerified: z.boolean().optional(), isOrganizationConfigured: z.boolean().optional(), + isAdminReviewed: z.boolean().optional(), orgAutoAcceptEmail: z.string().optional(), }) .nullable(); diff --git a/packages/trpc/server/routers/viewer/organizations/adminUpdate.handler.ts b/packages/trpc/server/routers/viewer/organizations/adminUpdate.handler.ts index 5410c8a3bd..7b18bb1dbb 100644 --- a/packages/trpc/server/routers/viewer/organizations/adminUpdate.handler.ts +++ b/packages/trpc/server/routers/viewer/organizations/adminUpdate.handler.ts @@ -71,6 +71,7 @@ export const adminUpdateHandler = async ({ input }: AdminUpdateOptions) => { isOrganizationVerified: organizationSettings?.isOrganizationVerified || existingOrg.organizationSettings?.isOrganizationVerified, + isAdminReviewed: organizationSettings?.isAdminReviewed, orgAutoAcceptEmail: organizationSettings?.orgAutoAcceptEmail || existingOrg.organizationSettings?.orgAutoAcceptEmail, }, diff --git a/packages/trpc/server/routers/viewer/organizations/create.handler.ts b/packages/trpc/server/routers/viewer/organizations/create.handler.ts index 0a974b50b5..871a367786 100644 --- a/packages/trpc/server/routers/viewer/organizations/create.handler.ts +++ b/packages/trpc/server/routers/viewer/organizations/create.handler.ts @@ -34,8 +34,8 @@ const getIPAddress = async (url: string): Promise => { export const createHandler = async ({ input, ctx }: CreateOptions) => { const { slug, name, orgOwnerEmail, seats, pricePerSeat, isPlatform } = input; - - if (!ORG_SELF_SERVE_ENABLED && ctx.user.role !== UserPermissionRole.ADMIN) { + const IS_USER_ADMIN = ctx.user.role === UserPermissionRole.ADMIN; + if (!ORG_SELF_SERVE_ENABLED && !IS_USER_ADMIN) { throw new TRPCError({ code: "FORBIDDEN", message: "Only admins can create organizations" }); } @@ -106,6 +106,7 @@ export const createHandler = async ({ input, ctx }: CreateOptions) => { name, slug, isOrganizationConfigured, + isOrganizationAdminReviewed: IS_USER_ADMIN, autoAcceptEmail, seats: seats ?? null, pricePerSeat: pricePerSeat ?? null, diff --git a/packages/trpc/server/routers/viewer/organizations/list.handler.ts b/packages/trpc/server/routers/viewer/organizations/list.handler.ts index 5ff3bb36fd..f4a6f11fa6 100644 --- a/packages/trpc/server/routers/viewer/organizations/list.handler.ts +++ b/packages/trpc/server/routers/viewer/organizations/list.handler.ts @@ -35,6 +35,7 @@ export const listHandler = async ({ ctx }: ListHandlerInput) => { }, select: { lockEventTypeCreationForUsers: true, + isAdminReviewed: true, }, }); @@ -48,6 +49,7 @@ export const listHandler = async ({ ctx }: ListHandlerInput) => { const metadata = teamMetadataSchema.parse(membership?.team.metadata); return { + canAdminImpersonate: !!organizationSettings?.isAdminReviewed, organizationSettings: { lockEventTypeCreationForUsers: organizationSettings?.lockEventTypeCreationForUsers, }, diff --git a/packages/trpc/server/routers/viewer/organizations/updateUser.handler.ts b/packages/trpc/server/routers/viewer/organizations/updateUser.handler.ts index 17cc5d2fb9..ba03c6992d 100644 --- a/packages/trpc/server/routers/viewer/organizations/updateUser.handler.ts +++ b/packages/trpc/server/routers/viewer/organizations/updateUser.handler.ts @@ -1,5 +1,6 @@ import type { Prisma, PrismaPromise, User, Membership, Profile } from "@prisma/client"; +import { ensureOrganizationIsReviewed } from "@calcom/ee/organizations/lib/ensureOrganizationIsReviewed"; import { checkRegularUsername } from "@calcom/lib/server/checkRegularUsername"; import { isOrganisationAdmin, isOrganisationOwner } from "@calcom/lib/server/queries/organisations"; import { resizeBase64Image } from "@calcom/lib/server/resizeBase64Image"; @@ -42,6 +43,8 @@ export const updateUserHandler = async ({ ctx, input }: UpdateUserOptions) => { if (!(await isOrganisationAdmin(userId, organizationId))) throw new TRPCError({ code: "UNAUTHORIZED" }); + await ensureOrganizationIsReviewed(organizationId); + const isUpdaterAnOwner = await isOrganisationOwner(userId, organizationId); // only OWNER can update the role to OWNER if (input.role === MembershipRole.OWNER && !isUpdaterAnOwner) { diff --git a/turbo.json b/turbo.json index 94e78ce55b..2847c649a3 100644 --- a/turbo.json +++ b/turbo.json @@ -401,6 +401,7 @@ "NEXT_PUBLIC_ENABLE_PROFILE_SWITCHER", "NEXT_PUBLIC_HEAD_SCRIPTS", "NEXT_PUBLIC_BODY_SCRIPTS", + "NEXT_PUBLIC_ORG_SELF_SERVE_ENABLED", "NEXT_PUBLIC_API_V2_ROOT_URL" ] }