From 6f656305f8a315166dd0812dcc67ba6d42f9cfc2 Mon Sep 17 00:00:00 2001 From: Hariom Date: Tue, 31 Dec 2024 12:50:01 +0530 Subject: [PATCH 1/2] feat: Enhance SCIM user attribute handling and sync process - Introduced a new PR_TODO.md file to track ongoing tasks and fixes. - Updated `getAttributesFromScimPayload` to accept a directoryId and ignore core user attributes during syncing. - Modified `handleUserEvents` to pass directoryId when syncing custom attributes. - Improved attribute creation logic to handle unique options and added support for optional isGroup property in attribute options. - Added utility function `getOptionsWithValidContains` to ensure valid sub-options in attribute options. - Updated tests to reflect changes in attribute handling and ensure proper functionality. This commit addresses issues with user attribute syncing and enhances the overall attribute management process. --- .../getAttributesFromScimPayload.test.ts | 28 ++++---- .../dsync/lib/getAttributesFromScimPayload.ts | 60 +++++++++++++--- .../features/ee/dsync/lib/handleUserEvents.ts | 68 +++++++++---------- .../attributes/attributes-create-view.tsx | 9 +-- .../viewer/attributes/create.handler.ts | 8 ++- .../viewer/attributes/create.schema.ts | 2 +- .../routers/viewer/attributes/edit.handler.ts | 24 +------ .../server/routers/viewer/attributes/utils.ts | 26 +++++++ 8 files changed, 135 insertions(+), 90 deletions(-) create mode 100644 packages/trpc/server/routers/viewer/attributes/utils.ts diff --git a/packages/features/ee/dsync/lib/__tests__/getAttributesFromScimPayload.test.ts b/packages/features/ee/dsync/lib/__tests__/getAttributesFromScimPayload.test.ts index 4c1cdcac5ccfdb..76610e786fe1c1 100644 --- a/packages/features/ee/dsync/lib/__tests__/getAttributesFromScimPayload.test.ts +++ b/packages/features/ee/dsync/lib/__tests__/getAttributesFromScimPayload.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it } from "vitest"; import getAttributesFromScimPayload from "../getAttributesFromScimPayload"; +const directoryId = "xxx-xxx-xxx-xxx"; describe("getAttributesFromScimPayload", () => { it("should return empty object for unsupported events", () => { const event = { @@ -10,7 +11,7 @@ describe("getAttributesFromScimPayload", () => { data: { raw: { schemas: [] } }, } as DirectorySyncEvent; - const result = getAttributesFromScimPayload(event); + const result = getAttributesFromScimPayload({ event, directoryId }); expect(result).toEqual({}); }); @@ -28,7 +29,7 @@ describe("getAttributesFromScimPayload", () => { }, } as DirectorySyncEvent; - const result = getAttributesFromScimPayload(event); + const result = getAttributesFromScimPayload({ event, directoryId }); expect(result).toEqual({ department: "Engineering", title: "Software Engineer", @@ -48,7 +49,7 @@ describe("getAttributesFromScimPayload", () => { }, } as DirectorySyncEvent; - const result = getAttributesFromScimPayload(event); + const result = getAttributesFromScimPayload({ event, directoryId }); expect(result).toEqual({ skills: ["JavaScript", "TypeScript", "React"], }); @@ -68,7 +69,7 @@ describe("getAttributesFromScimPayload", () => { }, } as DirectorySyncEvent; - const result = getAttributesFromScimPayload(event); + const result = getAttributesFromScimPayload({ event, directoryId }); expect(result).toEqual({ department: "Engineering", }); @@ -87,7 +88,7 @@ describe("getAttributesFromScimPayload", () => { }, } as DirectorySyncEvent; - const result = getAttributesFromScimPayload(event); + const result = getAttributesFromScimPayload({ event, directoryId }); expect(result).toEqual({}); }); @@ -108,7 +109,7 @@ describe("getAttributesFromScimPayload", () => { }, } as DirectorySyncEvent; - const result = getAttributesFromScimPayload(event); + const result = getAttributesFromScimPayload({ event, directoryId }); expect(result).toEqual({ department: "Engineering", location: "Remote", @@ -128,13 +129,13 @@ describe("getAttributesFromScimPayload", () => { }, } as DirectorySyncEvent; - const result = getAttributesFromScimPayload(event); + const result = getAttributesFromScimPayload({ event, directoryId }); expect(result).toEqual({ department: "Engineering", }); }); - it("should extract from core namespace as well.", () => { + it("should extract from core namespace as well ignoring the core attributes as defined in the SCIM spec.", () => { const event = { event: "user.created", data: { @@ -152,13 +153,14 @@ describe("getAttributesFromScimPayload", () => { }, } as DirectorySyncEvent; - const result = getAttributesFromScimPayload(event); + const result = getAttributesFromScimPayload({ event, directoryId }); expect(result).toEqual({ - userName: "kush@acme.com", - displayName: "Kush", territory: "XANAM", - externalId: "00ulb1kpy4EMATtuS5d7", - groups: [], + // Core Attributes won't be there - It avoids unnecessary warnings about attributes not defined in cal.com + // userName: "kush@acme.com", + // displayName: "Kush", + // externalId: "00ulb1kpy4EMATtuS5d7", + // groups: [], }); }); }); diff --git a/packages/features/ee/dsync/lib/getAttributesFromScimPayload.ts b/packages/features/ee/dsync/lib/getAttributesFromScimPayload.ts index ebd9ed9c52fd97..e674aeb6c5d2a0 100644 --- a/packages/features/ee/dsync/lib/getAttributesFromScimPayload.ts +++ b/packages/features/ee/dsync/lib/getAttributesFromScimPayload.ts @@ -1,5 +1,6 @@ import type { DirectorySyncEvent } from "@boxyhq/saml-jackson"; +import { DIRECTORY_IDS_TO_LOG } from "@calcom/lib/constants"; import logger from "@calcom/lib/logger"; import { safeStringify } from "@calcom/lib/safeStringify"; @@ -7,6 +8,32 @@ const log = logger.getSubLogger({ prefix: ["getAttributesFromScimPayload"] }); type ScimUserAttributeName = string; type ScimUserAttributeValue = string | string[]; +const coreSchemaUrn = "urn:ietf:params:scim:schemas:core:2.0:User"; +//It avoids unnecessary warnings about attributes not defined in cal.com +const coreUserAttributesToIgnore = [ + "userName", + "name", + "displayName", + "emails", + "active", + "externalId", + "id", + "groups", + "meta", + "locale", + "password", + "phoneNumbers", + "photos", + "profileUrl", + "timezone", + "title", + "addresses", + "entitlements", + "ims", + "roles", + "x509Certificates", +]; + /** * event.data.raw has this format * { @@ -45,9 +72,13 @@ type ScimUserAttributeValue = string | string[]; * "segment": "SMB" * } */ -function getAttributesFromScimPayload( - event: DirectorySyncEvent -): Record { +function getAttributesFromScimPayload({ + event, + directoryId, +}: { + event: DirectorySyncEvent; + directoryId: string; +}): Record { const scimUserAttributes: Record = {}; if (event.event !== "user.created" && event.event !== "user.updated") { @@ -57,10 +88,11 @@ function getAttributesFromScimPayload( const raw = event.data.raw; raw.schemas.forEach((schema: unknown) => { - if (schema === "urn:ietf:params:scim:schemas:core:2.0:User") { + if (schema === coreSchemaUrn) { // Core schema has payload in the root const { schemas: _schemas, ...namespaceData } = raw; - collectAttributes(namespaceData); + + collectAttributes({ data: namespaceData, ignoreList: coreUserAttributesToIgnore }); return; } const namespaceName = schema; @@ -77,15 +109,27 @@ function getAttributesFromScimPayload( return; } - collectAttributes(namespaceData); + collectAttributes({ data: namespaceData }); }); - log.debug("Collected attributes", `Attributes: ${safeStringify(scimUserAttributes)}`); + const shouldLog = DIRECTORY_IDS_TO_LOG.includes(directoryId); + if (shouldLog) { + console.log("Collected Attributes:", `${safeStringify(scimUserAttributes)}`); + } return scimUserAttributes; - function collectAttributes(data: Record) { + function collectAttributes({ + data, + ignoreList = [], + }: { + data: Record; + ignoreList?: string[]; + }) { Object.entries(data).forEach(([customAttributeName, value]) => { + if (ignoreList.includes(customAttributeName)) { + return; + } if (!value) { log.warn( "getAttributesFromScimPayload", diff --git a/packages/features/ee/dsync/lib/handleUserEvents.ts b/packages/features/ee/dsync/lib/handleUserEvents.ts index 7ab022ff68f49b..9db8791ff2ec0f 100644 --- a/packages/features/ee/dsync/lib/handleUserEvents.ts +++ b/packages/features/ee/dsync/lib/handleUserEvents.ts @@ -45,7 +45,7 @@ async function syncCustomAttributesToUser({ return; } - const customAttributes = getAttributesFromScimPayload(event); + const customAttributes = getAttributesFromScimPayload({ event, directoryId }); await assignValueToUserInOrgBulk({ orgId: org.id, userId: user.id, @@ -69,11 +69,6 @@ const handleUserEvents = async (event: DirectorySyncEvent, organizationId: numbe select: dSyncUserSelect, }); - // User is already a part of that org - if (user && UserRepository.isAMemberOfOrganization({ user, organizationId }) && eventData.active) { - return; - } - const translation = await getTranslation(user?.locale || "en", "common"); const org = await getTeamOrThrow(organizationId); @@ -84,36 +79,37 @@ const handleUserEvents = async (event: DirectorySyncEvent, organizationId: numbe if (user) { if (eventData.active) { - // If data.active is true then provision the user into the org - const addedUser = await inviteExistingUserToOrg({ - user: user as UserWithMembership, - org, - translation, - }); - - await sendExistingUserTeamInviteEmails({ - currentUserName: user.username, - currentUserTeamName: org.name, - existingUsersWithMemberships: [ - { - ...addedUser, - profile: null, - }, - ], - language: translation, - isOrg: true, - teamId: org.id, - isAutoJoin: true, - currentUserParentTeamName: org?.parent?.name, - orgSlug: org.slug, - }); - - await syncCustomAttributesToUser({ - event, - userEmail, - org, - directoryId, - }); + if (UserRepository.isAMemberOfOrganization({ user, organizationId })) { + await syncCustomAttributesToUser({ + event, + userEmail, + org, + directoryId, + }); + } else { + // If data.active is true then provision the user into the org + const addedUser = await inviteExistingUserToOrg({ + user: user as UserWithMembership, + org, + translation, + }); + await sendExistingUserTeamInviteEmails({ + currentUserName: user.username, + currentUserTeamName: org.name, + existingUsersWithMemberships: [ + { + ...addedUser, + profile: null, + }, + ], + language: translation, + isOrg: true, + teamId: org.id, + isAutoJoin: true, + currentUserParentTeamName: org?.parent?.name, + orgSlug: org.slug, + }); + } } else { // If data.active is false then remove the user from the org await removeUserFromOrg({ diff --git a/packages/features/ee/organizations/pages/settings/attributes/attributes-create-view.tsx b/packages/features/ee/organizations/pages/settings/attributes/attributes-create-view.tsx index cc699b7dc1d36d..0916c468f3f45a 100644 --- a/packages/features/ee/organizations/pages/settings/attributes/attributes-create-view.tsx +++ b/packages/features/ee/organizations/pages/settings/attributes/attributes-create-view.tsx @@ -38,13 +38,10 @@ function CreateAttributesPage() { } onSubmit={(values) => { - // Create set of attributes to get unique values - const uniqueAttributes = new Set(values.options.map((option) => option.value)); + const { attrName, ...rest } = values; mutation.mutate({ - name: values.attrName, - type: values.type, - isLocked: values.isLocked, - options: Array.from(uniqueAttributes).map((value) => ({ value })), + ...rest, + name: attrName, }); }} /> diff --git a/packages/trpc/server/routers/viewer/attributes/create.handler.ts b/packages/trpc/server/routers/viewer/attributes/create.handler.ts index 5ba65a80814a42..76709a62d0d925 100644 --- a/packages/trpc/server/routers/viewer/attributes/create.handler.ts +++ b/packages/trpc/server/routers/viewer/attributes/create.handler.ts @@ -7,6 +7,7 @@ import { TRPCError } from "@trpc/server"; import type { TrpcSessionUser } from "../../../trpc"; import type { ZCreateAttributeSchema } from "./create.schema"; +import { getOptionsWithValidContains } from "./utils"; type GetOptions = { ctx: { @@ -28,8 +29,8 @@ const createAttributesHandler = async ({ input, ctx }: GetOptions) => { } const slug = slugify(input.name); - const options = input.options.map((v) => v.value); - const optionsWithoutDuplicates = Array.from(new Set(options)); + const uniqueOptions = getOptionsWithValidContains(input.options); + const typeHasOptions = typesWithOptions.includes(input.type); let attributes: Attribute; @@ -55,10 +56,11 @@ const createAttributesHandler = async ({ input, ctx }: GetOptions) => { // TEXT/NUMBER don't have options if (typeHasOptions) { await prisma.attributeOption.createMany({ - data: optionsWithoutDuplicates.map((value) => ({ + data: uniqueOptions.map(({ value, isGroup }) => ({ attributeId: attributes.id, value, slug: slugify(value), + isGroup, })), }); } diff --git a/packages/trpc/server/routers/viewer/attributes/create.schema.ts b/packages/trpc/server/routers/viewer/attributes/create.schema.ts index 505e6d530744fa..65bea228c24b85 100644 --- a/packages/trpc/server/routers/viewer/attributes/create.schema.ts +++ b/packages/trpc/server/routers/viewer/attributes/create.schema.ts @@ -4,7 +4,7 @@ export const createAttributeSchema = z.object({ name: z.string(), type: z.enum(["TEXT", "NUMBER", "SINGLE_SELECT", "MULTI_SELECT"]), isLocked: z.boolean().optional(), - options: z.array(z.object({ value: z.string() })), + options: z.array(z.object({ value: z.string(), isGroup: z.boolean().optional() })), }); export type ZCreateAttributeSchema = z.infer; diff --git a/packages/trpc/server/routers/viewer/attributes/edit.handler.ts b/packages/trpc/server/routers/viewer/attributes/edit.handler.ts index 3a601e68cf2a36..0d69ae1e92bc6e 100644 --- a/packages/trpc/server/routers/viewer/attributes/edit.handler.ts +++ b/packages/trpc/server/routers/viewer/attributes/edit.handler.ts @@ -5,6 +5,7 @@ import { TRPCError } from "@trpc/server"; import type { TrpcSessionUser } from "../../../trpc"; import type { ZEditAttributeSchema } from "./edit.schema"; +import { getOptionsWithValidContains } from "./utils"; type GetOptions = { ctx: { @@ -115,29 +116,6 @@ const editAttributesHandler = async ({ input, ctx }: GetOptions) => { return attributes; }; -/** - * Ensures that contains has no non-existent sub-options - */ -function getOptionsWithValidContains(options: ZEditAttributeSchema["options"]) { - return options.map(({ contains, ...option }) => { - if (!contains) - return { - ...option, - contains: [], - }; - const possibleSubOptions = options - .filter((option) => !option.isGroup) - .filter((option): option is typeof option & { id: string } => option.id !== undefined); - - const possibleSubOptionsIds = possibleSubOptions.map((option) => option.id); - - return { - ...option, - contains: contains.filter((subOptionId) => possibleSubOptionsIds.includes(subOptionId)), - }; - }); -} - async function validateOptionsBelongToAttribute( options: ZEditAttributeSchema["options"], attributeId: string diff --git a/packages/trpc/server/routers/viewer/attributes/utils.ts b/packages/trpc/server/routers/viewer/attributes/utils.ts new file mode 100644 index 00000000000000..cb03934f5c21a2 --- /dev/null +++ b/packages/trpc/server/routers/viewer/attributes/utils.ts @@ -0,0 +1,26 @@ +/** + * Ensures that contains has no non-existent sub-options + */ +export function getOptionsWithValidContains< + T extends { id?: string; value: string; contains?: string[]; isGroup?: boolean } +>(options: T[]): T[] { + return options + .filter((obj, index, self) => index === self.findIndex((t) => t.value === obj.value)) + .map(({ contains, ...option }) => { + if (!contains) + return { + ...option, + contains: [] as string[], + } as T; + const possibleSubOptions = options + .filter((option) => !option.isGroup) + .filter((option): option is typeof option & { id: string } => option.id !== undefined); + + const possibleSubOptionsIds = possibleSubOptions.map((option) => option.id); + + return { + ...option, + contains: contains.filter((subOptionId) => possibleSubOptionsIds.includes(subOptionId)), + } as T; + }); +} From 45615b19204e7dca61339adbaadce22b894e8749 Mon Sep 17 00:00:00 2001 From: Hariom Date: Tue, 31 Dec 2024 15:47:44 +0530 Subject: [PATCH 2/2] test: Add unit tests for getOptionsWithValidContains utility function - Introduced a new test file for the getOptionsWithValidContains function. - Added tests to verify the removal of duplicate options, initialization of empty contains arrays, filtering of non-existent sub-options, and handling of empty options arrays. - Ensured comprehensive coverage of the utility's functionality to enhance reliability and maintainability. --- .../routers/viewer/attributes/utils.test.ts | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 packages/trpc/server/routers/viewer/attributes/utils.test.ts diff --git a/packages/trpc/server/routers/viewer/attributes/utils.test.ts b/packages/trpc/server/routers/viewer/attributes/utils.test.ts new file mode 100644 index 00000000000000..3bc03b8423209e --- /dev/null +++ b/packages/trpc/server/routers/viewer/attributes/utils.test.ts @@ -0,0 +1,55 @@ +import { describe, it, expect } from "vitest"; + +import { getOptionsWithValidContains } from "./utils"; + +type Option = { + id?: string; + value: string; + contains?: string[]; + isGroup?: boolean; +}; + +describe("getOptionsWithValidContains", () => { + it("should remove duplicate options based on value", () => { + const options: Option[] = [ + { id: "1", value: "option1" }, + { id: "2", value: "option1" }, // Duplicate value + { id: "3", value: "option2" }, + ]; + + const result = getOptionsWithValidContains(options); + expect(result).toHaveLength(2); + expect(result[0].value).toBe("option1"); + expect(result[1].value).toBe("option2"); + }); + + it("should initialize empty contains array if not provided", () => { + const options: Option[] = [ + { id: "1", value: "option1" }, + { id: "2", value: "option2" }, + ]; + + const result = getOptionsWithValidContains(options); + expect(result[0].contains).toEqual([]); + expect(result[1].contains).toEqual([]); + }); + + it("should filter out non-existent sub-options from contains array", () => { + const options: Option[] = [ + { id: "1", value: "option1", contains: ["2", "3", "nonexistent"] }, + { id: "2", value: "option2" }, + { id: "3", value: "option3" }, + ]; + + const result = getOptionsWithValidContains(options); + expect(result[0].contains).toEqual(["2", "3"]); + expect(result[1].contains).toEqual([]); + expect(result[2].contains).toEqual([]); + }); + + it("should handle empty options array", () => { + const options: Option[] = []; + const result = getOptionsWithValidContains(options); + expect(result).toEqual([]); + }); +});