WPB-23434: Support SCIM PATCH of multi-valued emails attribute by blackheaven · Pull Request #5419 · wireapp/wire-server

blackheaven · 2026-08-05T06:56:38Z

https://wearezeta.atlassian.net/browse/WPB-23434

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

akshaymankar

Looks good overall. Two things I would request:

Document somewhere that Entra is acting wrong or fix applyUserOperation so it doesn't rewrite Add to Replace in case of multi-values and complex attributes.
The integration test should be in the new suite.

Other comments are nits.

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.

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.

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

scimEmailsToEmailAddress now returns Either, rejecting (Left) when more than one email is marked primary (RFC 7643 §2.4 violation). spar's validateScimUser' turns the Left into a SCIM 400, covering POST/PUT/PATCH (PATCH re-validates via putUser). Previously spar silently first-wins, masking client-side misconfiguration. Other inputs (zero or one primary) are unchanged.

…test rename)

synthesizeScimUser now echoes type = Just "work" instead of Nothing. spar/brig store a single address with no type, but Entra PATCHes with a value-path filter emails[type eq "work"]; without an echoed type the filter never matched and such PATCHes appended a duplicate email rather than updating the address in place. Echoing the canonical type (RFC 7643 sec 4.1.2) at the single synthesis point makes the filter match across create/GET/PATCH. Adds testSparPatchEmailValuePathInPlace (in-place update), corrects the create-on-absent test comment, and adds a changelog fragment.

Follow-up to 1406a78 (synthesizeScimUser now echoes type = Just "work" instead of Nothing): update all SCIM email test-expectations to expect type = "work", since every GET/PUT/PATCH response now materializes it. integration framework (Test/Spar.hs): add a scimWorkEmail helper and route the 13 full-email-object assertions/inputs that compare to a response through it. PUT inputs never round-tripped to a response are left typeless, preserving coverage that the server synthesizes the type when omitted. spar test-integration: flip the two expected-email constructions (Util/Scim.hs setDefaultRoleAndEmailsIfEmpty, APISpec.hs scim-user-search) from Nothing to Just "work". Test-only; no production code.

akshaymankar

Looks good, but needs some explanation for the eventually use and I just want to be sure if we're changing APIs it is well tested and is very visible in release notes.

akshaymankar · 2026-08-18T13:46:32Z

+with no `type`, the filter never matched, and every such PATCH appended a
+duplicate email instead of updating the address in place.


I thought previously the PATCH just errored? How did it appent a duplicate email?

akshaymankar · 2026-08-18T13:52:47Z

+    res.status `shouldMatchInt` 200
+  -- The provisioned email propagates end-to-end: SCIM GET reflects it and,
+  -- with validation disabled, it is active in Brig.
+  eventually $ do


Please explain the need for this eventually in a comment.

akshaymankar · 2026-08-18T13:59:15Z

+  -- @.value@. The proof is the new value and echoed @type = "work"@ below -- a
+  -- create-on-absent append would be collapsed back to the OLD address by
+  -- 'scimEmailsToEmailAddress', failing the value assertion.
+  eventually $ do


Also this eventually needs an explanation, it would be nice if we try to avoid these, but if something is genuinely async, sure it might make sense, but maybe there is some event (like the team admin getting a team member join event) which can be used for synchronizing.

akshaymankar · 2026-08-18T14:00:37Z

-    resp.json %. "emails" `shouldMatch` (toJSON [object ["value" .= email]])
+    resp.json %. "emails" `shouldMatch` (toJSON [scimWorkEmail email])


Hmm, this looks like an API change, we must add an entry to release notes I think to warn people.
Btw, have we tested this with different setups? I'm scared this will break some customers.

akshaymankar · 2026-08-18T14:04:24Z

+scimEmailsToEmailAddress es
+  | Prelude.length primaries > 1 =
+      Left "More than one email is marked as primary; RFC 7643 §2.4 allows at most one."
+  | otherwise = Right (pickFirst primaries <|> pickFirst es)


nit: IMO this is faster and also looks neater. And we can remove the pickFirst function.

Suggested change

scimEmailsToEmailAddress es

| Prelude.length primaries > 1 =

Left "More than one email is marked as primary; RFC 7643 §2.4 allows at most one."

| otherwise = Right (pickFirst primaries <|> pickFirst es)

scimEmailsToEmailAddress es =

case primaries of

[] -> Right Nothing

[primaryEmail] -> Right . Just . unEmailAddress $ value primaryEmail

_ -> Left "More than one email is marked as primary; RFC 7643 §2.4 allows at most one."

blackheaven requested review from a team as code owners August 5, 2026 06:56

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 5, 2026

blackheaven force-pushed the gdifolco/WPB-23434-scim-patch branch from a0fb7f7 to e4acc08 Compare August 5, 2026 07:37

akshaymankar requested changes Aug 5, 2026

View reviewed changes

Comment thread libs/hscim/src/Web/Scim/Schema/User.hs Outdated

Comment thread libs/hscim/src/Web/Scim/Schema/User.hs Outdated

Comment thread libs/hscim/test/Test/Schema/UserSpec.hs Outdated

Comment thread services/spar/test-integration/Test/Spar/Scim/UserSpec.hs Outdated

blackheaven requested a review from akshaymankar August 5, 2026 10:50

blackheaven force-pushed the gdifolco/WPB-23434-scim-patch branch from 1715cd6 to 709b260 Compare August 5, 2026 10:52

blackheaven mentioned this pull request Aug 11, 2026

SCIM Error Responses Not Compliant with RFC 7644 Section 3.12 #5434

Closed

blackheaven added 7 commits August 18, 2026 15:42

WPB-23434: Resolve SCIM PATCH review feedback (Add->Replace comment, …

c820f08

…test rename)

blackheaven force-pushed the gdifolco/WPB-23434-scim-patch branch from 7a95f96 to f88dea5 Compare August 18, 2026 14:04

akshaymankar requested changes Aug 18, 2026

View reviewed changes

Navigation Menu

Uh oh!

WPB-23434: Support SCIM PATCH of multi-valued emails attribute - #5419

WPB-23434: Support SCIM PATCH of multi-valued emails attribute#5419
blackheaven wants to merge 7 commits into
developfrom
gdifolco/WPB-23434-scim-patch

blackheaven commented Aug 5, 2026

Uh oh!

akshaymankar left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akshaymankar left a comment

Uh oh!

akshaymankar Aug 18, 2026

Uh oh!

akshaymankar Aug 18, 2026

Uh oh!

akshaymankar Aug 18, 2026

Uh oh!

akshaymankar Aug 18, 2026

Uh oh!

akshaymankar Aug 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		with no `type`, the filter never matched, and every such PATCH appended a
		duplicate email instead of updating the address in place.

		resp.json %. "emails" `shouldMatch` (toJSON [object ["value" .= email]])
		resp.json %. "emails" `shouldMatch` (toJSON [scimWorkEmail email])

-scimEmailsToEmailAddress es
-  | Prelude.length primaries > 1 =
-      Left "More than one email is marked as primary; RFC 7643 §2.4 allows at most one."
-  | otherwise = Right (pickFirst primaries <|> pickFirst es)
+scimEmailsToEmailAddress es =
+  case primaries of
+    [] -> Right Nothing
+    [primaryEmail] -> Right . Just . unEmailAddress $ value primaryEmail
+    _ -> Left "More than one email is marked as primary; RFC 7643 §2.4 allows at most one."

Uh oh!

Conversation

blackheaven commented Aug 5, 2026

Checklist

Uh oh!

akshaymankar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akshaymankar left a comment

Choose a reason for hiding this comment

Uh oh!

akshaymankar Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

akshaymankar Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

akshaymankar Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

akshaymankar Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

akshaymankar Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants