WPB-20056: Add support for SCIM managed UserGroup deletion #4833

blackheaven · 2025-10-31T22:15:28Z

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

Checklist

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

Copilot

Pull Request Overview

This pull request implements support for deleting SCIM-managed user groups through the SCIM API, enabling proper lifecycle management of groups created via SCIM provisioning.

Adds a new scimDeleteUserGroup function to delete SCIM-managed groups with proper validation
Implements deleteGroupManaged internal API endpoint to handle deletion based on managedBy field
Adds comprehensive unit tests for both successful deletions and failure scenarios

Reviewed Changes

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

Show a summary per file

File	Description
services/spar/src/Spar/Scim/Group.hs	Implements the SCIM group deletion handler, replacing the previous `undefined` stub
services/spar/src/Spar/Error.hs	Adds error mapping for internal errors during SCIM group operations
services/brig/src/Brig/API/Internal.hs	Adds internal API endpoint for deleting managed groups
libs/wire-subsystems/src/Wire/UserGroupSubsystem/Interpreter.hs	Implements `deleteGroupManagedImpl` with `managedBy` validation and notification logic
libs/wire-subsystems/src/Wire/UserGroupSubsystem.hs	Adds `DeleteGroupManaged` operation to the subsystem interface
libs/wire-subsystems/src/Wire/ScimSubsystem/Interpreter.hs	Implements SCIM deletion logic with proper error handling for not found and permission errors
libs/wire-subsystems/src/Wire/ScimSubsystem.hs	Adds `ScimDeleteUserGroup` operation to the SCIM subsystem interface
libs/wire-subsystems/src/Wire/BrigAPIAccess/Rpc.hs	Implements RPC call to brig's internal delete endpoint
libs/wire-subsystems/src/Wire/BrigAPIAccess.hs	Adds `DeleteGroupManaged` to the Brig API access interface
libs/wire-api/src/Wire/API/User/Profile.hs	Adds HTTP API data serialization support for `ManagedBy` type
libs/wire-api/src/Wire/API/Routes/Internal/Brig.hs	Defines the internal API route for managed group deletion
libs/wire-api/src/Wire/API/Error/Brig.hs	Adds `UserGroupManagedByMismatch` error type for validation failures
libs/wire-subsystems/test/unit/Wire/ScimSubsystem/InterpreterSpec.hs	Adds comprehensive test coverage for SCIM group deletion scenarios
libs/wire-subsystems/test/unit/Wire/UserGroupSubsystem/InterpreterSpec.hs	Minor formatting cleanup in test setup
integration/test/API/Spar.hs	Minor formatting cleanup in test helper function

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

services/spar/src/Spar/Error.hs

battermann

I would like to have an integration test.
Are brig errors corrected mapped to SCIM errors? (Integration tests might show)

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

libs/wire-subsystems/src/Wire/UserGroupSubsystem/Interpreter.hs

libs/wire-api/src/Wire/API/User/Profile.hs

libs/wire-subsystems/src/Wire/ScimSubsystem/Interpreter.hs

libs/wire-subsystems/src/Wire/UserGroupSubsystem/Interpreter.hs

libs/wire-subsystems/test/unit/Wire/ScimSubsystem/InterpreterSpec.hs

libs/wire-api/src/Wire/API/Routes/Internal/Brig.hs

fisx · 2025-11-12T09:35:08Z

libs/wire-subsystems/test/unit/Wire/ScimSubsystem/InterpreterSpec.hs

+  [User] ->
+  Map TeamId [TeamMember] ->
+  Sem AllDependencies a ->
+  Either UGS.UserGroupSubsystemError (Either ScimSubsystemError a)


please don't involve user group subsystem here, this is supposed to work on spar, so the interpreter should use brig internal api instead.

the errors returned from there should be translated into errors from the hscim library wrapped in ScimSubsystemError.

(i want to remove ScimSubsystemError and replace it with ScimError, but in a different PR.)

I disagree, in tests I need to check it throws.

sorry, you're right

fisx · 2025-11-12T09:40:38Z

services/spar/src/Spar/Error.hs

        Scim.notFound "group members" badIds
      ScimSubsystemScimGroupWithNonScimMembers badIds ->
        Scim.badRequest Scim.InvalidValue (Just $ "These users are not \"managed_by\" = \"scim\": " <> renderIds badIds)
+      ScimSubsystemInternalError _ ->


i think there is also a scim error for that ("serverError" or something?), so you can use ScimSubsystemError here as well.

I'm not sure to understand, it's defined as:

serverError :: -- | Error details Text -> ScimError serverError details = ScimError { schemas = [Error20], status = Status 500, scimType = Nothing, detail = pure details }

please ignore me, still making up my mind about errors... 🙈

libs/wire-subsystems/test/unit/Wire/ScimSubsystem/InterpreterSpec.hs

fisx · 2025-11-12T14:41:42Z

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

 decodeBodyOrThrow ctx r = either (throw . ParseException ctx) pure (responseJsonEither r)

-- | Get statuses of all connections between two groups of users (the usual
+-- \| Get statuses of all connections between two groups of users (the usual


is this a typo? anyway this should be haddocks documentation.

It's the formater

ormolu? hum, it seems wrong, but i don't want to fight ormolu... 👍

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

libs/wire-subsystems/src/Wire/UserGroupSubsystem/Interpreter.hs

integration/test/Test/Spar.hs

fisx · 2025-11-13T09:12:00Z

integration/test/Test/Spar.hs

-  (owner, tid, []) <- createTeam OwnDomain 1
-  tok <- createScimToken owner def >>= getJSON 200 >>= (%. "token") >>= asString
+  (owner, tid, _) <- createTeam OwnDomain 1
+  tok <- createScimTokenV6 owner def >>= \resp -> resp.json %. "token" >>= asString


hum, didn't the other work? if so we should probably fix it, right? (probably not in this PR, though)

I have applied #4833 (comment) should I revert it?

fisx · 2025-11-13T09:15:50Z

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

 decodeBodyOrThrow ctx r = either (throw . ParseException ctx) pure (responseJsonEither r)

-- | Get statuses of all connections between two groups of users (the usual
+-- \| Get statuses of all connections between two groups of users (the usual


ormolu? hum, it seems wrong, but i don't want to fight ormolu... 👍

fisx · 2025-11-13T09:18:01Z

libs/wire-subsystems/test/unit/Wire/ScimSubsystem/InterpreterSpec.hs

+  [User] ->
+  Map TeamId [TeamMember] ->
+  Sem AllDependencies a ->
+  Either UGS.UserGroupSubsystemError (Either ScimSubsystemError a)


sorry, you're right

fisx · 2025-11-13T09:23:56Z

services/spar/src/Spar/Error.hs

        Scim.notFound "group members" badIds
      ScimSubsystemScimGroupWithNonScimMembers badIds ->
        Scim.badRequest Scim.InvalidValue (Just $ "These users are not \"managed_by\" = \"scim\": " <> renderIds badIds)
+      ScimSubsystemInternalError _ ->


please ignore me, still making up my mind about errors... 🙈

fisx · 2025-11-13T11:57:53Z

libs/wire-subsystems/src/Wire/BrigAPIAccess.hs

yes, you win, this is at least as nice as what we had before! :)

blackheaven requested a review from a team as a code owner October 31, 2025 22:15

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Oct 31, 2025

blackheaven force-pushed the gdifolco/WPB-20056-scim-group-delete branch 2 times, most recently from 195c665 to 8684fb6 Compare October 31, 2025 22:55

blackheaven force-pushed the WPB-20053-scim-groups-create-2 branch from 277dac2 to 638da63 Compare November 1, 2025 11:36

blackheaven requested review from a team as code owners November 1, 2025 11:36

blackheaven force-pushed the gdifolco/WPB-20056-scim-group-delete branch from 8684fb6 to 9a76805 Compare November 1, 2025 11:37

eyeinsky force-pushed the WPB-20053-scim-groups-create-2 branch 3 times, most recently from 42d669b to 797766b Compare November 5, 2025 12:48

fisx force-pushed the WPB-20053-scim-groups-create-2 branch 2 times, most recently from c5929df to 69310c6 Compare November 10, 2025 07:55

blackheaven changed the base branch from WPB-20053-scim-groups-create-2 to ml/WPB-20054-scim-get-group November 10, 2025 18:10

blackheaven force-pushed the gdifolco/WPB-20056-scim-group-delete branch 2 times, most recently from 438c361 to b233796 Compare November 10, 2025 20:37

fisx force-pushed the ml/WPB-20054-scim-get-group branch from 0b04971 to e93e1a8 Compare November 11, 2025 08:32

blackheaven force-pushed the gdifolco/WPB-20056-scim-group-delete branch 3 times, most recently from 1c6bac3 to eef24c5 Compare November 11, 2025 18:05

battermann requested a review from Copilot November 12, 2025 08:36

Copilot started reviewing on behalf of battermann November 12, 2025 08:36 View session

Copilot finished reviewing on behalf of battermann November 12, 2025 08:38

Copilot AI reviewed Nov 12, 2025

View reviewed changes

services/spar/src/Spar/Error.hs Show resolved Hide resolved

blackheaven force-pushed the gdifolco/WPB-20056-scim-group-delete branch from e1d7099 to 154f9e8 Compare November 12, 2025 09:28

battermann requested changes Nov 12, 2025

View reviewed changes

fisx reviewed Nov 12, 2025

View reviewed changes

integration/test/Test/Spar.hs Outdated Show resolved Hide resolved

Base automatically changed from ml/WPB-20054-scim-get-group to develop November 12, 2025 16:54

blackheaven force-pushed the gdifolco/WPB-20056-scim-group-delete branch 4 times, most recently from e4e4b9a to 37e2ad2 Compare November 13, 2025 07:52

blackheaven requested review from battermann and fisx November 13, 2025 08:03

fisx approved these changes Nov 13, 2025

View reviewed changes

fisx reviewed Nov 13, 2025

View reviewed changes

libs/wire-subsystems/src/Wire/BrigAPIAccess.hs

Copy link

Contributor

fisx Nov 13, 2025

Choose a reason for hiding this comment

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

yes, you win, this is at least as nice as what we had before! :)

fisx approved these changes Nov 13, 2025

View reviewed changes

battermann approved these changes Nov 13, 2025

View reviewed changes

blackheaven force-pushed the gdifolco/WPB-20056-scim-group-delete branch from d2504a8 to 66b2313 Compare November 13, 2025 15:08

WPB-20056: Add support for SCIM managed UserGroup deletion

91e928a

blackheaven force-pushed the gdifolco/WPB-20056-scim-group-delete branch from 66b2313 to 91e928a Compare November 13, 2025 16:03

Hi CI

fbd43ff

blackheaven merged commit 08dfa28 into develop Nov 14, 2025
10 checks passed

blackheaven deleted the gdifolco/WPB-20056-scim-group-delete branch November 14, 2025 07:49

This was referenced Nov 26, 2025

Release 2025-11-26 - (expected chart version 5.24.0) #4875

Closed

Release 2025-11-26 - (expected chart version 5.24.0) #4876

Merged

WPB-20056: Add support for SCIM managed UserGroup deletion #4833

WPB-20056: Add support for SCIM managed UserGroup deletion #4833

Uh oh!

Conversation

blackheaven commented Oct 31, 2025 • edited by fisx 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

blackheaven commented Oct 31, 2025 •

edited by fisx

Loading