[WPB-20053] Consolidate brig/galley api access effects from spar into wire-subsystems. by fisx · Pull Request #5189 · wireapp/wire-server

fisx · 2026-04-16T11:51:07Z

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

Checklist

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

Copilot

Pull request overview

This PR consolidates Spar’s Brig/Galley API access effects by migrating the effect definitions + RPC interpreters into wire-subsystems, and updates Spar to use Wire.BrigAPIAccess / Wire.GalleyAPIAccess throughout.

Changes:

Removed Spar-local Spar.Sem.BrigAccess / Spar.Sem.GalleyAccess (and their HTTP interpreters) and switched Spar to the wire-subsystems equivalents.
Added/extended Wire.BrigAPIAccess and Wire.GalleyAPIAccess with SCIM/SAML- and team-member-related operations, plus RPC implementations.
Updated Spar unit + integration tests, and adjusted build files (.cabal, default.nix) and changelog.

Reviewed changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
services/spar/test/Test/Spar/Scim/UserSpec.hs	Updates unit tests to use `BrigAPIAccess` mock instead of `Spar.Sem.BrigAccess`.
services/spar/test/Test/Spar/Saml/IdPSpec.hs	Switches mocks to `BrigAPIAccess`/`GalleyAPIAccess`; enables required feature config mock defaults.
services/spar/test-integration/Util/Core.hs	Moves `getAccount` import to `Wire.BrigAPIAccess`.
services/spar/test-integration/Test/Spar/Scim/UserSpec.hs	Updates integration tests to call `Wire.BrigAPIAccess` functions.
services/spar/test-integration/Test/Spar/Intra/BrigSpec.hs	Adjusts delete-user test to match new `deleteUser` return type/behavior.
services/spar/test-integration/Test/Spar/APISpec.hs	Updates integration test calls from old Brig effect to `Wire.BrigAPIAccess`.
services/spar/src/Spar/Sem/Utils.hs	Removes obsolete HTTP runner utilities after migrating to RPC interpreters in wire-subsystems.
services/spar/src/Spar/Sem/GalleyAccess/Http.hs	Deletes Spar-local Galley HTTP interpreter (migrated to wire-subsystems).
services/spar/src/Spar/Sem/GalleyAccess.hs	Deletes Spar-local Galley effect (migrated to wire-subsystems).
services/spar/src/Spar/Sem/BrigAccess/Http.hs	Deletes Spar-local Brig HTTP interpreter (migrated to wire-subsystems).
services/spar/src/Spar/Sem/BrigAccess.hs	Deletes Spar-local Brig effect (migrated to wire-subsystems).
services/spar/src/Spar/Scim/User.hs	Rewires SCIM user logic to use `Wire.BrigAPIAccess` / `Wire.GalleyAPIAccess`.
services/spar/src/Spar/Scim/Auth.hs	Switches SCIM token/auth logic to new access effects + BrigApp helpers.
services/spar/src/Spar/Scim.hs	Updates SCIM API effect constraints to `BrigAPIAccess`/`GalleyAPIAccess`.
services/spar/src/Spar/Intra/Galley.hs	Removes Spar-local Galley client module (now handled via wire-subsystems RPC).
services/spar/src/Spar/Intra/BrigApp.hs	Adds Spar-specific helpers (`ensureReAuthorised`, permission checks, SSO enabled check) using wire-subsystems effects.
services/spar/src/Spar/Intra/Brig.hs	Removes Spar-local Brig client module (now handled via wire-subsystems RPC).
services/spar/src/Spar/CanonicalInterpreter.hs	Updates canonical interpreter stack to interpret `BrigAPIAccess`/`GalleyAPIAccess` via wire-subsystems RPC.
services/spar/src/Spar/App.hs	Updates core Spar logic to depend on `BrigAPIAccess`/`GalleyAPIAccess`.
services/spar/src/Spar/API.hs	Updates API handlers to use `BrigAPIAccess`/`GalleyAPIAccess` and BrigApp helper functions.
services/spar/spar.cabal	Removes Spar-local modules and adds needed deps for updated tests (`data-default`).
services/spar/default.nix	Adds `data-default` dependency for Spar tests/build.
libs/wire-subsystems/wire-subsystems.cabal	Adds `cookie` dependency (needed by Brig RPC SSO cookie parsing).
libs/wire-subsystems/test/unit/Wire/MockInterpreters/GalleyAPIAccess.hs	Extends mock interpreter to acknowledge new Galley operations.
libs/wire-subsystems/src/Wire/GalleyAPIAccess/Rpc.hs	Implements RPC handlers for `UpdateTeamMember` and `IsEmailValidationEnabledTeam`.
libs/wire-subsystems/src/Wire/GalleyAPIAccess.hs	Adds new Galley effect constructors for team-member update and email-validation feature check.
libs/wire-subsystems/src/Wire/BrigAPIAccess/Rpc.hs	Adds RPC implementations for SCIM/SAML-related Brig operations migrated from Spar.
libs/wire-subsystems/src/Wire/BrigAPIAccess.hs	Extends Brig effect with SCIM/SAML user-management operations migrated from Spar.
libs/wire-subsystems/default.nix	Adds `cookie` dependency for wire-subsystems build.
changelog.d/5-internal/WPB-0000-consolidate-brig_galley-api-access-effects-from-spar-into-wire-subsystems	Adds changelog entry for the consolidation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

battermann

There is at least one place where error handling is different from before. E.g. cases of 4xx now lead to 500, which is a contract change, and therefore a breaking API change. We should carefully check this for each endpoint. I think this mostly applies to where there was a rethrow before.

battermann · 2026-04-20T11:00:02Z

+    -- * SAML / SCIM user management
+    createSAML,
+    createNoSAML,
+    updateEmail,
+    getAccount,
+    getAccountByHandle,
+    getByEmail,
+    setName,
+    setHandle,
+    setManagedBy,
+    setSSOId,
+    setRichInfo,
+    setLocale,
+    getRichInfo,
+    checkHandleAvailable,
+    ssoLogin,
+    getStatus,
+    getStatusMaybe,
+    setStatus,
+    getDefaultUserLocale,
+    checkAdminGetTeamId,
+    sendSAMLIdPChangedEmail,


It's not really the responsibility of this PR, but we could decide to export everything implicitly like e.g. BrigAPIAccess does? I do not see any advantage in explicit exports here or for the Subsystem modules in general.

battermann · 2026-04-20T11:29:01Z

+            newUserSparLocale = mLocale,
+            newUserSparRole = role
+          }
+  resp <- brigRequest $ method POST . path "/i/users/spar" . json newUser . expect2xx


Checking the original Intra.Brig.createBrigUserSAML there we rethrow in case of a non 201/201 status code, so 4xx surfice as such to the caller. Now the expect2xx throw a StatusCodeException and bubbles up to the caller probably as a generic 500 error. This is a regression IMO.

rethrow sneakily always throws a 500 error. it contains the error from the failing service, but only in the message. obviously the name is horrible, but luckily i was able to remove it now.

there are still some changes in error details like 503 instead of 500, or different message field. also this change reduces logging on info level a lot, but that's intentional.

good you made me take another look, though, thanks! any other concerns?

battermann

There are still places where we return generic 500 instead of forwarding 4xx errors. I commented on some, but I haven't checked all endpoints.

battermann · 2026-04-22T14:36:50Z

+  resp <- brigRequest $ method POST . path "/i/users/spar" . json newUser
+  if statusCode resp `elem` [200, 201]
+    then userId . selfUser <$> decodeBodyOrThrow @SelfProfile "brig" resp
+    else throw $ ParseException "brig" ("Unexpected status " <> show (statusCode resp) <> " from POST /i/users/spar")


I think we should rethrow the original error. ParseException will always cause 500 internal server error, IIRC.

battermann · 2026-04-22T14:37:00Z

+        . json newUser
+  if statusCode resp `elem` [200, 201]
+    then userId <$> decodeBodyOrThrow @User "brig" resp
+    else throw $ ParseException "brig" ("Unexpected status " <> show (statusCode resp) <> " from POST /i/teams/invitations")


same as above

battermann · 2026-04-22T14:39:01Z

+      case getHeader "Set-Cookie" resp of
+        Nothing -> throw $ ParseException "brig" "Missing Set-Cookie header in SSO login response"
+        Just cky -> pure $ parseSetCookie cky
+    n -> throw $ ParseException "brig" ("Unexpected status " <> show n <> " from brig SSO login")


We should rethrow here, too, instead of generic 500. At least 404 is a valid/expected response, AFAICT.

battermann · 2026-04-22T14:40:44Z

+  resp <- getStatusRaw uid
+  case statusCode resp of
+    200 -> fromAccountStatusResp <$> decodeBodyOrThrow @AccountStatusResp "brig" resp
+    _ -> throw $ ParseException "brig" ("Unexpected status " <> show (statusCode resp) <> " from GET /i/users/status")


Here also, this does not forward 404 or any 4xx error.

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

fisx force-pushed the WPB-0000-consolidate-brig_galley-api-access-effects-from-spar-into-wire-subsystems branch 3 times, most recently from ec57f74 to 2368e57 Compare April 16, 2026 16:32

fisx requested a review from Copilot April 16, 2026 16:33

Copilot started reviewing on behalf of fisx April 16, 2026 16:34 View session

Copilot AI reviewed Apr 16, 2026

View reviewed changes

Comment thread libs/wire-subsystems/src/Wire/BrigAPIAccess/Rpc.hs

fisx mentioned this pull request Apr 17, 2026

[WPB-20053] De-duplicate {brig,galley} api access effects from spar into wire-subsystems. #4840

Closed

2 tasks

fisx changed the title ~~[WPB-0000] Consolidate brig/galley api access effects from spar into wire-subsystems.~~ [WPB-20053] Consolidate brig/galley api access effects from spar into wire-subsystems. Apr 17, 2026

fisx force-pushed the WPB-0000-consolidate-brig_galley-api-access-effects-from-spar-into-wire-subsystems branch 2 times, most recently from 9bb61f3 to 198c84d Compare April 20, 2026 07:09

fisx marked this pull request as ready for review April 20, 2026 07:09

fisx requested review from a team as code owners April 20, 2026 07:09

battermann requested changes Apr 20, 2026

View reviewed changes

fisx added 8 commits April 21, 2026 09:26

Changelog.

a0e7784

Move {Brig,Galley]APIAccess effects from spar to wire-subsystems.

61e9677

Nit-pick.

716609c

Fix translation issues.

1b1b53e

make sanitize--pr

9032762

Better http error when throttling kicks in.

64622cd

Remove explicit exports from Wire.BrigAPIAccess.

cea01d4

Fix: undo behavioral divergence in moved code.

9e4cb9d

fisx force-pushed the WPB-0000-consolidate-brig_galley-api-access-effects-from-spar-into-wire-subsystems branch from 198c84d to 9e4cb9d Compare April 21, 2026 07:55

Rm dead code.

d2a7d4f

fisx requested a review from battermann April 21, 2026 08:41

ormolu.

074fdb6

battermann requested changes Apr 22, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WPB-20053] Consolidate brig/galley api access effects from spar into wire-subsystems.#5189

[WPB-20053] Consolidate brig/galley api access effects from spar into wire-subsystems.#5189
fisx wants to merge 10 commits intodevelopfrom
WPB-0000-consolidate-brig_galley-api-access-effects-from-spar-into-wire-subsystems

fisx commented Apr 16, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

battermann left a comment

Uh oh!

battermann Apr 20, 2026

Uh oh!

battermann Apr 20, 2026

Uh oh!

fisx Apr 21, 2026

Uh oh!

battermann left a comment

Uh oh!

battermann Apr 22, 2026

Uh oh!

battermann Apr 22, 2026

Uh oh!

battermann Apr 22, 2026

Uh oh!

battermann Apr 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

fisx commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

battermann left a comment

Choose a reason for hiding this comment

Uh oh!

battermann Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

battermann Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

fisx Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

battermann left a comment

Choose a reason for hiding this comment

Uh oh!

battermann Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

battermann Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

battermann Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

battermann Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fisx commented Apr 16, 2026 •

edited

Loading