From 06d8b98b153cd5357ab99fcfa165772bbb5995e4 Mon Sep 17 00:00:00 2001 From: Hariom Balhara Date: Tue, 7 Jan 2025 17:43:19 +0530 Subject: [PATCH] fix: 404 on the public form page for a sub-team form when the owner itself was removed from the org (#18495) * wip * fix: 404 for a form of a member that has been removed. - Moved existing authorization check to isAuthorizedToViewFormOnOrgDomain function to handle form access control - Add comprehensive test suite for org domain authorization scenarios - Refactor getServerSideProps to use the new authorization function - Improve code organization by separating domain-specific authorization logic Test Coverage: - Non-org domain access - Org member's form access - Sub-team form access within org - Access denial for non-org members/teams --- .../server/lib/router/getServerSideProps.ts | 6 +- .../pages/routing-link/getServerSideProps.ts | 40 +------ .../lib/isAuthorizedToViewForm.test.ts | 113 ++++++++++++++++++ .../lib/isAuthorizedToViewForm.ts | 46 +++++++ 4 files changed, 167 insertions(+), 38 deletions(-) create mode 100644 packages/features/routing-forms/lib/isAuthorizedToViewForm.test.ts create mode 100644 packages/features/routing-forms/lib/isAuthorizedToViewForm.ts diff --git a/apps/web/server/lib/router/getServerSideProps.ts b/apps/web/server/lib/router/getServerSideProps.ts index a79c31e4f3..ecf9f6d08a 100644 --- a/apps/web/server/lib/router/getServerSideProps.ts +++ b/apps/web/server/lib/router/getServerSideProps.ts @@ -11,10 +11,10 @@ import { handleResponse } from "@calcom/app-store/routing-forms/lib/handleRespon import { findMatchingRoute } from "@calcom/app-store/routing-forms/lib/processRoute"; import { substituteVariables } from "@calcom/app-store/routing-forms/lib/substituteVariables"; import { getFieldResponseForJsonLogic } from "@calcom/app-store/routing-forms/lib/transformResponse"; -import { isAuthorizedToViewTheForm } from "@calcom/app-store/routing-forms/pages/routing-link/getServerSideProps"; import { getUrlSearchParamsToForward } from "@calcom/app-store/routing-forms/pages/routing-link/getUrlSearchParamsToForward"; import type { FormResponse } from "@calcom/app-store/routing-forms/types/types"; import { orgDomainConfig } from "@calcom/features/ee/organizations/lib/orgDomains"; +import { isAuthorizedToViewFormOnOrgDomain } from "@calcom/features/routing-forms/lib/isAuthorizedToViewForm"; import logger from "@calcom/lib/logger"; import { RoutingFormRepository } from "@calcom/lib/server/repository/routingForm"; import { TRPCError } from "@calcom/trpc/server"; @@ -70,7 +70,9 @@ export const getServerSideProps = async function getServerSideProps(context: Get }; timeTaken.profileEnrichment = performance.now() - profileEnrichmentStart; - if (!isAuthorizedToViewTheForm({ user: formWithUserProfile.user, currentOrgDomain })) { + if ( + !isAuthorizedToViewFormOnOrgDomain({ user: formWithUserProfile.user, currentOrgDomain, team: form.team }) + ) { return { notFound: true, }; diff --git a/packages/app-store/routing-forms/pages/routing-link/getServerSideProps.ts b/packages/app-store/routing-forms/pages/routing-link/getServerSideProps.ts index 56fb024746..a0671d861e 100644 --- a/packages/app-store/routing-forms/pages/routing-link/getServerSideProps.ts +++ b/packages/app-store/routing-forms/pages/routing-link/getServerSideProps.ts @@ -1,44 +1,10 @@ import { orgDomainConfig } from "@calcom/features/ee/organizations/lib/orgDomains"; -import type { Prisma } from "@calcom/prisma/client"; -import { userMetadata } from "@calcom/prisma/zod-utils"; +import { isAuthorizedToViewFormOnOrgDomain } from "@calcom/features/routing-forms/lib/isAuthorizedToViewForm"; import type { AppGetServerSidePropsContext, AppPrisma } from "@calcom/types/AppGetServerSideProps"; import { enrichFormWithMigrationData } from "../../enrichFormWithMigrationData"; import { getSerializableForm } from "../../lib/getSerializableForm"; -export function isAuthorizedToViewTheForm({ - user, - currentOrgDomain, -}: { - user: { - username: string | null; - metadata: Prisma.JsonValue; - movedToProfileId: number | null; - profile: { - organization: { slug: string | null; requestedSlug: string | null } | null; - }; - id: number; - }; - currentOrgDomain: string | null; -}) { - const formUser = { - ...user, - metadata: userMetadata.parse(user.metadata), - }; - const orgSlug = formUser.profile.organization?.slug ?? formUser.profile.organization?.requestedSlug ?? null; - - if (!currentOrgDomain) { - // If not on org domain, let's allow serving any form belong to any organization so that even if the form owner is migrate to an organization, old links for the form keep working - return true; - } else if (currentOrgDomain !== orgSlug) { - // If on org domain, - // We don't serve the form that is of another org - // We don't serve the form that doesn't belong to any org - return false; - } - return true; -} - export const getServerSideProps = async function getServerSideProps( context: AppGetServerSidePropsContext, prisma: AppPrisma @@ -108,7 +74,9 @@ export const getServerSideProps = async function getServerSideProps( user: await UserRepository.enrichUserWithItsProfile({ user: form.user }), }; - if (!isAuthorizedToViewTheForm({ user: formWithUserProfile.user, currentOrgDomain })) { + if ( + !isAuthorizedToViewFormOnOrgDomain({ user: formWithUserProfile.user, currentOrgDomain, team: form.team }) + ) { return { notFound: true, }; diff --git a/packages/features/routing-forms/lib/isAuthorizedToViewForm.test.ts b/packages/features/routing-forms/lib/isAuthorizedToViewForm.test.ts new file mode 100644 index 0000000000..bd931a1054 --- /dev/null +++ b/packages/features/routing-forms/lib/isAuthorizedToViewForm.test.ts @@ -0,0 +1,113 @@ +import { describe, it, expect } from "vitest"; + +import { isAuthorizedToViewFormOnOrgDomain } from "./isAuthorizedToViewForm"; + +const _createUser = (overrides = {}) => ({ + username: "testuser", + metadata: {}, + movedToProfileId: null, + id: 1, + ...overrides, +}); + +/** + * Creates a regular user without organization membership + */ +const createRegularUser = (overrides = {}) => ({ + ..._createUser(overrides), + profile: { + organization: null, + }, +}); + +/** + * Creates a user that is a member of an organization + */ +const createOrgMemberUser = ({ + orgSlug, + requestedSlug, +}: { + orgSlug: string; + requestedSlug: string | null; +}) => ({ + ..._createUser({ + profile: { + organization: { slug: orgSlug, requestedSlug: requestedSlug }, + }, + }), +}); + +const _createTeam = (overrides = {}) => ({ + parent: null, + ...overrides, +}); + +/** + * Creates a regular team without organization association + */ +const createRegularTeam = (overrides = {}) => _createTeam(overrides); + +/** + * Creates a sub-team that belongs to an organization + */ +const createSubTeam = (orgSlug: string, overrides = {}) => + _createTeam({ + parent: { + slug: orgSlug, + }, + ...overrides, + }); + +describe("isAuthorizedToViewFormOnOrgDomain", () => { + it("should allow viewing any form (user or team form) when not on org domain", () => { + const result = isAuthorizedToViewFormOnOrgDomain({ + user: createRegularUser(), + currentOrgDomain: null, + team: createRegularTeam(), + }); + expect(result).toBe(true); + }); + + it("should allow viewing org member's form when user belongs to the current org domain", () => { + const result = isAuthorizedToViewFormOnOrgDomain({ + user: createOrgMemberUser({ orgSlug: "test-org", requestedSlug: null }), + currentOrgDomain: "test-org", + }); + expect(result).toBe(true); + }); + + it("should allow viewing sub-team form when the sub team belongs to the current org domain", () => { + const result = isAuthorizedToViewFormOnOrgDomain({ + user: createRegularUser(), + currentOrgDomain: "test-org", + team: createSubTeam("test-org"), + }); + expect(result).toBe(true); + }); + + it("should deny viewing form when on org domain but neither user nor sub team belongs to it", () => { + const result = isAuthorizedToViewFormOnOrgDomain({ + user: createOrgMemberUser("different-org"), + currentOrgDomain: "test-org", + team: createSubTeam("another-org"), + }); + expect(result).toBe(false); + }); + + it("should handle undefined team parameter on org domain", () => { + const result = isAuthorizedToViewFormOnOrgDomain({ + user: createRegularUser(), + currentOrgDomain: "test-org", + team: undefined, + }); + expect(result).toBe(false); + }); + + it("should allow access when user has pending org membership request", () => { + const result = isAuthorizedToViewFormOnOrgDomain({ + user: createOrgMemberUser({ orgSlug: "test-org", requestedSlug: "test-org" }), + currentOrgDomain: "test-org", + }); + expect(result).toBe(true); + }); +}); diff --git a/packages/features/routing-forms/lib/isAuthorizedToViewForm.ts b/packages/features/routing-forms/lib/isAuthorizedToViewForm.ts new file mode 100644 index 0000000000..f337a5ed04 --- /dev/null +++ b/packages/features/routing-forms/lib/isAuthorizedToViewForm.ts @@ -0,0 +1,46 @@ +import type { Prisma } from "@calcom/prisma/client"; +import { userMetadata } from "@calcom/prisma/zod-utils"; + +type FormUser = { + username: string | null; + metadata: Prisma.JsonValue; + movedToProfileId: number | null; + profile: { + organization: { slug: string | null; requestedSlug: string | null } | null; + }; + id: number; +}; + +type FormTeam = { + parent: { + slug: string | null; + } | null; +} | null; + +export function isAuthorizedToViewFormOnOrgDomain({ + user, + currentOrgDomain, + team, +}: { + user: FormUser; + currentOrgDomain: string | null; + team?: FormTeam; +}) { + const formUser = { + ...user, + metadata: userMetadata.parse(user.metadata), + }; + const orgSlug = formUser.profile.organization?.slug ?? formUser.profile.organization?.requestedSlug ?? null; + const teamOrgSlug = team?.parent?.slug ?? null; + + if (!currentOrgDomain) { + // If not on org domain, let's allow serving any form belong to any organization so that even if the form owner is migrate to an organization, old links for the form keep working + return true; + } else if (currentOrgDomain === orgSlug || currentOrgDomain === teamOrgSlug) { + // If on org domain, allow if: + // 1. The form belongs to a user who is part of the organization (orgSlug matches) + // 2. The form belongs to a team that is part of the organization (teamOrgSlug matches) + return true; + } + return false; +}