Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Enhance SCIM payload handling and attribute creation #18427

Merged
merged 2 commits into from
Jan 3, 2025

Conversation

hariombalhara
Copy link
Member

@hariombalhara hariombalhara commented Dec 31, 2024

  • fix: the issue where a user update from Okta didn't sync attributes
  • fix: When creating a new attribute, OptionGroup was getting saved as regular Option itself.
  • Ignore core user attributes( as defined in the SCIM spec) from attributes syncing

Detailed Changes:

  • Updated getAttributesFromScimPayload to accept directoryId and ignore core attributes as defined in the SCIM spec.
  • Modified handleUserEvents to pass directoryId when calling getAttributesFromScimPayload.
  • Improved test cases for getAttributesFromScimPayload to reflect the new parameter and behavior.
  • Refactored attribute creation logic in attributes-create-view.tsx to streamline attribute submission.
  • Updated attribute creation schema to include optional isGroup property for options.
  • Introduced utility function getOptionsWithValidContains to ensure valid sub-option references during attribute creation.

These changes improve the robustness of user event handling and attribute management in the application.

What does this PR do?

  • Fixes #XXXX (GitHub issue number)
  • Fixes CAL-XXXX (Linear issue number - should be visible at the bottom of the GitHub issue description)

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • N/A I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Create an attribute with option group
  • Try syncing the attribute from Okta for an already synced user.

Copy link

vercel bot commented Dec 31, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Dec 31, 2024 10:18am
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Dec 31, 2024 10:18am

@graphite-app graphite-app bot requested a review from a team December 31, 2024 06:18
@keithwillcode keithwillcode added the core area: core, team members only label Dec 31, 2024
@dosubot dosubot bot added the enterprise area: enterprise, audit log, organisation, SAML, SSO label Dec 31, 2024
@keithwillcode keithwillcode added the enterprise area: enterprise, audit log, organisation, SAML, SSO label Dec 31, 2024
@hariombalhara hariombalhara changed the base branch from okta-sync-fixes to main December 31, 2024 06:19
Copy link

graphite-app bot commented Dec 31, 2024

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (12/31/24)

1 reviewer was added to this PR based on Keith Williams's automation.

Copy link
Contributor

github-actions bot commented Dec 31, 2024

E2E results are ready!

@hariombalhara hariombalhara force-pushed the okta-attributes-sync-fixes branch from 67dd2d8 to 63013d4 Compare December 31, 2024 07:16
- 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.
- 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.
@@ -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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was causing the update to not sync attribues as the user is already a part of org.

org,
directoryId,
});
if (UserRepository.isAMemberOfOrganization({ user, organizationId })) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still allow attributes sync for existing user.

@@ -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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using common utility

@@ -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() })),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was causing a groupOption to become regular option when created during the attribute creation itself.

@hariombalhara hariombalhara self-assigned this Dec 31, 2024
Copy link
Contributor

@hbjORbj hbjORbj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@hariombalhara hariombalhara merged commit 727f7df into main Jan 3, 2025
44 checks passed
@hariombalhara hariombalhara deleted the okta-attributes-sync-fixes branch January 3, 2025 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core area: core, team members only enterprise area: enterprise, audit log, organisation, SAML, SSO ready-for-e2e
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants