Skip to content

fix(directory-sync): parse SCIM PATCH op case-insensitively so Entra group changes aren't dropped - #4070

Open
byrmsh wants to merge 1 commit into
ory:mainfrom
byrmsh:fix/scim-patch-op-casing
Open

fix(directory-sync): parse SCIM PATCH op case-insensitively so Entra group changes aren't dropped#4070
byrmsh wants to merge 1 commit into
ory:mainfrom
byrmsh:fix/scim-patch-op-casing

Conversation

@byrmsh

@byrmsh byrmsh commented Jul 13, 2026

Copy link
Copy Markdown

Problem

parseGroupOperation and parseUserPatchRequest (npm/src/directory-sync/scim/utils.ts) compare the SCIM PATCH op against lowercase literals (op === 'add', 'remove', 'replace').

Microsoft Entra ID, unless the tenant enables the aadOptscim062020 feature flag, sends PascalCase op values. Microsoft documents this with verbatim request samples, e.g. a group member add:

{ "op": "Add", "path": "members", "value": [ { "value": "u1091" } ] }

and a member remove uses "op": "Remove". See "Known issues with SCIM 2.0 protocol compliance" (Microsoft Learn): https://learn.microsoft.com/en-us/entra/identity/app-provisioning/application-provisioning-config-problem-scim-compatibility

Because "Add" !== "add", group operations fall through to { action: 'unknown' }. In DirectoryGroups.patch() (npm/src/directory-sync/scim/DirectoryGroups.ts) an unknown action matches none of the addGroupMember / removeGroupMember / updateGroupName branches, so it is silently discarded: the request still returns 200, no group.user_added webhook is emitted, and the membership is never recorded. Group provisioning from a default-configured Entra tenant simply does not work.

The same mismatch affects user PATCH: parseUserPatchRequest gates attribute removal on op === 'remove', so Entra's "op": "Remove" is not recognized as a removal and is instead applied as a set.

The aadOptscim062020 flag that emits lowercase ops is opt-in and, per the same doc, "currently doesn't work with on-demand provisioning", so real Entra directories hit this by default.

Fix

Normalize op to lowercase before comparison in both functions. This is loss-less: every valid SCIM (RFC 7644) / JSON Patch (RFC 6902) op token is already lowercase, so normalizing can only broaden matching, never reinterpret a valid op. Optional chaining (operation.op?.toLowerCase()) keeps a malformed op-less operation degrading to its existing default ({ action: 'unknown' } / an attribute set) instead of throwing.

Added a unit test in npm/test/dsync/scim-utils.test.ts covering PascalCase and lowercase group ops (plus the op-less case) and PascalCase user removals.

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

SCIM group and user PATCH parsers now normalize operation verbs to lowercase. Tests cover PascalCase and lowercase operations, removal sentinels, replacement values, and malformed group operations.

Changes

SCIM patch parsing

Layer / File(s) Summary
Normalize patch operation verbs
npm/src/directory-sync/scim/utils.ts, npm/test/dsync/scim-utils.test.ts
Group and user PATCH operations are parsed case-insensitively, including Microsoft Entra PascalCase verbs. Tests verify action mapping, removal handling, replacement values, and missing-operation behavior.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: case-insensitive SCIM PATCH op parsing for Entra.
Description check ✅ Passed It includes problem, fix, motivation, and testing details, but omits the issue reference and most template checklist/type sections.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@deepakprabhakara
deepakprabhakara requested review from pi1814 and removed request for pi1814 July 14, 2026 09:44
@deepakprabhakara

Copy link
Copy Markdown
Collaborator

Thanks @byrmsh, the team will review your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants