WPB-23434: Support SCIM PATCH of multi-valued emails attribute - #5419
Open
blackheaven wants to merge 5 commits into
Open
WPB-23434: Support SCIM PATCH of multi-valued emails attribute#5419blackheaven wants to merge 5 commits into
blackheaven wants to merge 5 commits into
Conversation
zebot
added
the
ok-to-test
Implement value-path PATCH (e.g. `emails[type eq "work"].value`) for the `emails` multi-valued attribute in hscim's applyUserOperation, which Entra and other IdPs use to provision user emails. Any value-path target was previously rejected with "can not lens into multi-valued attributes yet". Supports the value/type/primary sub-attributes and whole-entry paths; other multi-valued attributes remain unsupported. Also fix email propagation in Spar: an email-only PATCH does not change externalId, so updateVsuUref (the only update-path route to validateEmail -> Brig.updateEmail, gated on externalId change) never ran, and GET reconstructs the email from Brig -- so the change was a silent no-op. Add an explicit email-diff validateEmail in updateValidScimUser, gated on externalId being unchanged to avoid double-validation with updateVsuUref.
blackheaven
force-pushed
the
gdifolco/WPB-23434-scim-patch
branch
from
August 5, 2026 07:37
a0fb7f7 to
e4acc08
Compare
akshaymankar
requested changes
Aug 5, 2026
akshaymankar
left a comment
akshaymankar
left a comment
Member
There was a problem hiding this comment.
Looks good overall. Two things I would request:
- Document somewhere that Entra is acting wrong or fix
applyUserOperationso it doesn't rewriteAddtoReplacein case of multi-values and complex attributes. - The integration test should be in the new suite.
Other comments are nits.
Comment on lines
+386
to
+393
| -- NOTE on "create on absent": RFC 7644 搂3.5.2.3 says a @Replace@ value-path | ||
| -- that matches nothing is a no-op. Entra, however, emits an @Add@ (rewritten to | ||
| -- @Replace@ in 'applyUserOperation') against @emails[type eq "work"].value@ to | ||
| -- provision the address, expecting the entry to be created if absent. Every | ||
| -- mainstream SCIM client/validator expects this create-on-absent behaviour for | ||
| -- the email value-path, so we deviate from the RFC here: when the filter is | ||
| -- @type eq <s>@ and no entry matches, we append | ||
| -- @Email { typ = Just s, value = newVal, primary = Nothing }@. |
Member
There was a problem hiding this comment.
Should we fix applyUserOperation so we don't rewrite Add as Replace? There's a comment on it which talks about not doing the rewrite for complex and multi-values.
Comment on lines
+395
to
+397
| -- | The 'Filter' embedded in a 'ValuePath'. | ||
| valuePathFilter :: ValuePath -> Filter | ||
| valuePathFilter (ValuePath _ flt) = flt |
Member
There was a problem hiding this comment.
nit: we should change ValuePath so it has named record fields.
Comment on lines
+163
to
+169
| it "creates a work email when none matches (Entra payload)" $ do | ||
| let Right p = emailValuePath | ||
| operation = Operation Replace (Just p) (Just (String "x@y.com")) | ||
| result = User.applyPatch (mkUser []) (PatchOp [operation]) | ||
| result `shouldSatisfy` isRight | ||
| let Right patched = result | ||
| emails patched `shouldBe` [mkEmail "work" "x@y.com"] |
Member
There was a problem hiding this comment.
Can you please link to some documentation from Entra where it says entra is sending Replace instead of Add? Or is this just an artefact of applyUserOperation?
Comment on lines
+2092
to
+2115
| it "can update a user's email via the multi-valued 'emails' value-path" $ do | ||
| (tok, (_, tid, _idp)) <- registerIdPAndScimToken | ||
| -- Disable email verification so the patched email is activated directly, | ||
| -- without a separate activation step. | ||
| setSamlEmailValidation tid Feature.FeatureStatusDisabled | ||
| newEmail <- randomEmail | ||
| user <- randomScimUser | ||
| storedUser <- createUser tok user | ||
| let userid = scimUserId storedUser | ||
| let Right p = PatchOp.parsePath userSchemas "emails[type eq \"work\"].value" | ||
| operation = | ||
| PatchOp.Operation | ||
| PatchOp.Replace | ||
| (Just p) | ||
| (Just (toJSON (fromEmail newEmail))) | ||
| _ <- patchUser tok userid (PatchOp.PatchOp [operation]) | ||
| -- the email propagated all the way to Brig and is reflected on a fresh GET | ||
| eventually $ do | ||
| storedUser'' <- getUser tok userid | ||
| liftIO $ | ||
| Scim.Email.scimEmailsToEmailAddress | ||
| (Scim.User.emails (Scim.value (Scim.thing storedUser''))) | ||
| `shouldBe` Just newEmail | ||
| checkEmail userid (Just newEmail) |
Member
There was a problem hiding this comment.
This must be written in the new integration test suite. This test suite is deprecated.
Handle Add on emails[...] value-path explicitly instead of rewriting to Replace: whole-entry Add appends (concat semantics) while sub-attribute Add delegates to the Replace path. Give ValuePath named record fields and drop the redundant valuePathFilter helper. Correct the create-on-absent NOTE to cite Entra's documented Add behaviour. Move the end-to-end email PATCH test to the new integration suite (patchScimUser helper + testSparPatchEmailValuePath) and remove the deprecated copy. Add hscim unit tests for the Add behaviour.
blackheaven
force-pushed
the
gdifolco/WPB-23434-scim-patch
branch
from
August 5, 2026 10:52
1715cd6 to
709b260
Compare
spar never persists the SCIM email 'type': synthesizeScimUser returns every email with typ=Nothing, so a value-path filter emails[type eq "work"] can never match an existing email. The original test created a user WITH an email, so the Add hit create-on-absent and appended a second entry; scimEmailsToEmailAddress (pickFirst) then returned the old email, so no email diff fired and Brig was never updated. Mirror the proven deprecated test instead: create an email-less SAML user, disable validateSAMLemails so the provisioned email auto-activates, and assert the Add on emails[type eq "work"].value provisions a work email that propagates to Brig. (Add delegates to the Replace path for the .value sub-attribute, so this is behaviour-equivalent to the prior Replace test.)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://wearezeta.atlassian.net/browse/WPB-23434
Checklist
changelog.d