fix: Enhance SCIM payload handling and attribute creation (#18427)

* 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.

* 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.

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: [],
     });
   });
 });

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<ScimUserAttributeName, ScimUserAttributeValue> {
+function getAttributesFromScimPayload({
+  event,
+  directoryId,
+}: {
+  event: DirectorySyncEvent;
+  directoryId: string;
+}): Record<ScimUserAttributeName, ScimUserAttributeValue> {
   const scimUserAttributes: Record<ScimUserAttributeName, ScimUserAttributeValue> = {};
 
   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<string, unknown>) {
+  function collectAttributes({
+    data,
+    ignoreList = [],
+  }: {
+    data: Record<string, unknown>;
+    ignoreList?: string[];
+  }) {
     Object.entries(data).forEach(([customAttributeName, value]) => {
+      if (ignoreList.includes(customAttributeName)) {
+        return;
+      }
       if (!value) {
         log.warn(
           "getAttributesFromScimPayload",

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({

packages/features/ee/organizations/pages/settings/attributes/attributes-create-view.tsx

@@ -38,13 +38,10 @@ function CreateAttributesPage() {
         <AttributeForm
           header={<CreateAttributeHeader isPending={mutation.isPending} />}
           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,
             });
           }}
         />

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,
       })),
     });
   }

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<typeof createAttributeSchema>;

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

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([]);
+  });
+});

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;
+    });
+}