WPB-21769: SCIM group search returning members #4859

eyeinsky · 2025-11-17T11:41:48Z

WPB-21769: scim group search should return member lists, not just the metadata.

Return a list of members from Spar's /scim/v2/Groups?filter=... with group members included in the search result.

Checklist

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

Copilot

Pull Request Overview

This PR implements support for returning group members in SCIM group search results. Previously, the /scim/v2/Groups?filter=... endpoint only returned group metadata; now it includes the full member lists in the response.

Key Changes

Introduced parameterized UserGroupPage_ type to support both metadata-only (UserGroupPage) and full group data with members (UserGroupPageWithMembers)
Added new getUserGroupsWithMembers database function that performs a LEFT JOIN to fetch members alongside group metadata
Updated the SCIM subsystem to use UserGroupPageWithMembers and removed the workaround that was converting groups to empty member lists

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`libs/wire-api/src/Wire/API/UserGroup/Pagination.hs`	Refactored `UserGroupPage` into parameterized `UserGroupPage_` type to support both metadata-only and full group representations
`libs/wire-subsystems/src/Wire/UserGroupStore.hs`	Added `GetUserGroupsWithMembers` effect to the store interface
`libs/wire-subsystems/src/Wire/UserGroupStore/Postgres.hs`	Implemented `getUserGroupsWithMembers` with SQL query using LEFT JOIN and GROUP BY to fetch members; refactored common helper functions
`libs/wire-subsystems/src/Wire/UserGroupSubsystem.hs`	Updated `GetGroupsInternal` effect to return `UserGroupPageWithMembers`
`libs/wire-subsystems/src/Wire/UserGroupSubsystem/Interpreter.hs`	Changed internal implementation to call `getUserGroupsWithMembers`
`libs/wire-subsystems/src/Wire/ScimSubsystem/Interpreter.hs`	Removed workaround that was creating empty member lists; now uses full group data directly
`libs/wire-subsystems/src/Wire/BrigAPIAccess.hs`	Updated `GetGroupsInternal` API to return `UserGroupPageWithMembers`
`libs/wire-subsystems/src/Wire/BrigAPIAccess/Rpc.hs`	Updated RPC implementation to return `UserGroupPageWithMembers`
`libs/wire-api/src/Wire/API/Routes/Internal/Brig.hs`	Updated internal API route type signature to return `UserGroupPageWithMembers`
`services/brig/src/Brig/API/Internal.hs`	Updated handler to return `UserGroupPageWithMembers`
`integration/test/Test/Spar.hs`	Refactored test to verify search functionality; removed unused import; renamed test function
`integration/test/API/Spar.hs`	Moved `mkScimGroup` and `mkScimUser` helper functions from test to API module for reusability
`changelog.d/2-features/search-scim-groups-members`	Added changelog entry documenting the feature

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

Copilot · 2025-11-21T07:19:11Z

libs/wire-subsystems/src/Wire/UserGroupStore/Postgres.hs

+    toUserGroup (Id -> id_, name', managedBy', createdAt', members', Just . fromIntegral -> membersCount)  = do
+      name <- userGroupNameFromText name'
+      managedBy <- parseManagedBy managedBy'
+      let createdAt = toUTCTimeMillis createdAt'
+          channels = Nothing
+          channelsCount = Nothing
+          members = Identity (fmap Id members' :: Vector UserId)
+      pure $ UserGroup_ {..}


[nitpick] The membersCount variable is bound in the pattern match but never used in the UserGroup_ construction. The membersCount field is set using the length of the members' vector on line 268, but the SQL query already computes this count. However, using V.length members' is correct and more reliable than using the SQL count, so this is acceptable. Consider removing the unused membersCount binding from the pattern match to avoid confusion.

Hmm, this is a good point. length is O(1) on vectors as well, so it's not recounting the items either (as it would in a list).

Copilot · 2025-11-21T07:19:11Z

libs/wire-subsystems/src/Wire/UserGroupStore/Postgres.hs

+        "g.managed_by :: int",
+        "g.created_at :: timestamptz",
+        "coalesce(array_agg(gm.user_id), array[]::uuid[]) :: uuid[]",
+        "count(*) :: int"


The SQL query has a semantic issue. It uses count(*) which counts the number of joined rows (including NULL members from the LEFT JOIN), not the actual number of members. This will return incorrect counts:

For groups with members: count will match the number of members

For groups without members: count will incorrectly return 1 instead of 0 (due to the LEFT JOIN creating one row with NULL member)

Consider using count(gm.user_id) instead, which only counts non-NULL values and will correctly return 0 for groups without members.

Suggested change

"count(*) :: int"

"count(gm.user_id) :: int"

This is absolutely correct! 2622dc2

Copilot · 2025-11-21T07:19:12Z

libs/wire-subsystems/src/Wire/UserGroupStore/Postgres.hs

+  clause1 "team_id" "=" req.team :
+  case req.searchString of
+    Just name -> [ like "name" name ]


The WHERE clause uses unqualified column names (team_id, name) but the query uses table aliases (g, gm). While this may work in PostgreSQL when there's no ambiguity, it's better to qualify these columns with the g. prefix for consistency and clarity (e.g., g.team_id, g.name).

Suggested change

clause1 "team_id" "=" req.team :

case req.searchString of

Just name -> [ like "name" name ]

clause1 "g.team_id" "=" req.team :

case req.searchString of

Just name -> [ like "g.name" name ]

If the helpers where these functions are used, were used only on one of the main queries then qualifying would work, but in the other user group query the alias is different, made the alias the same 22a9c88.

Copilot · 2025-11-21T07:19:12Z

integration/test/Test/Spar.hs

+  createdGroup2 <- respGroup2.json
+  createdGroup3 <- respGroup3.json
+
+  -- Test geting a single SCIM group by id


Typo in comment: "geting" should be "getting".

Suggested change

-- Test geting a single SCIM group by id

-- Test getting a single SCIM group by id

Yup! a1ff47d

Copilot · 2025-11-21T07:19:12Z

libs/wire-subsystems/src/Wire/UserGroupStore/Postgres.hs

+        "g.name :: text",
+        "g.managed_by :: int",
+        "g.created_at :: timestamptz",
+        "coalesce(array_agg(gm.user_id), array[]::uuid[]) :: uuid[]",


The array_agg(gm.user_id) function will create an array with a single NULL element [NULL] for groups with no members (due to the LEFT JOIN), not an empty array. This will be coalesced to [NULL] instead of [].

Consider filtering out NULLs explicitly: coalesce(array_agg(gm.user_id) FILTER (WHERE gm.user_id IS NOT NULL), array[]::uuid[]) to ensure groups without members get an empty array.

Suggested change

"coalesce(array_agg(gm.user_id), array[]::uuid[]) :: uuid[]",

"coalesce(array_agg(gm.user_id) FILTER (WHERE gm.user_id IS NOT NULL), array[]::uuid[]) :: uuid[]",

battermann · 2025-11-21T07:24:57Z

integration/test/Test/Spar.hs

+  allThreeResp <- filterScimUserGroup OwnDomain tok $ Just "displayName co \"group\""
+  (allThreeResp.json %. "Resources" & asList) `shouldMatchSet` [createdGroup1, createdGroup2, createdGroup3]


It's better to use bindResponse which show better logs in case of failure.

battermann · 2025-11-21T07:25:17Z

integration/test/Test/Spar.hs

+  justTwo <- filterScimUserGroup OwnDomain tok $ Just "displayName co \"another group\""
+  (justTwo.json %. "Resources" & asList) `shouldMatchSet` [createdGroup2, createdGroup3]


Better use bindResponse, see above

battermann · 2025-11-21T07:29:26Z

libs/wire-subsystems/src/Wire/UserGroupStore/Postgres.hs

+getUserGroupsWithMembers req = runTransaction TxSessions.ReadCommitted TxSessions.Read $ do
+  groups :: [UserGroup] <- Tx.statement () $ refineResult (mapM toUserGroup) $ buildStatement query rows
+  count <- getUserGroupCount req
+  return $ UserGroupPage groups count


The convention is to use pure. I wouldn't care so much if my linter would mark this red... :)

battermann · 2025-11-21T07:31:22Z

libs/wire-subsystems/src/Wire/UserGroupStore/Postgres.hs

+        ],
+      "from user_group g",
+      "left join user_group_member gm on g.id = gm.user_group_id"
+      ] <> [where_ (groupMatchIdName req <> groupPaginationWhereClause req)] <> map literal [


No need to map over a singleton list.

battermann · 2025-11-21T07:33:29Z

libs/wire-subsystems/src/Wire/UserGroupStore/Postgres.hs

+      "from user_group g",
+      "left join user_group_member gm on g.id = gm.user_group_id"
+      ] <> [where_ (groupMatchIdName req <> groupPaginationWhereClause req)] <> map literal [
+      "group by g.team_id, g.id"


The query selects more columns than in the group by. Not sure how this can work.

It is because the fields named in the group by parts determine the other non-aggregated columns (name, managed_by, ..), and the rest (the user IDs) are aggregated.

ok, I see, thanks!

no return

don't map over singleton list

Use `bindResponse`

add test case for empty group

make sanitize-pr

eyeinsky · 2025-11-21T14:19:39Z

@battermann I think I've now addressed all of the comments!

Also, Copilot did make some good SQL catches!

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

eyeinsky force-pushed the ml/WPB-21769--scim-group-search-users branch from ddc6aca to 4986d67 Compare November 19, 2025 16:56

eyeinsky added 5 commits November 20, 2025 14:20

Abstract UserGroupPage

5d2a07b

Adjust test to expect members

86c8c70

Return user groups with members from Brig (stub implementation)

0a1a700

Refactor getUserGroups (no functional changes)

19cbb29

Implement getUserGroupsWithMembers

c35f033

eyeinsky force-pushed the ml/WPB-21769--scim-group-search-users branch from 4986d67 to c35f033 Compare November 20, 2025 13:13

Add changelog entry

b295edf

eyeinsky marked this pull request as ready for review November 20, 2025 14:50

eyeinsky requested review from a team as code owners November 20, 2025 14:50

blackheaven approved these changes Nov 20, 2025

View reviewed changes

battermann requested a review from Copilot November 21, 2025 07:15

Copilot started reviewing on behalf of battermann November 21, 2025 07:15 View session

Copilot finished reviewing on behalf of battermann November 21, 2025 07:18

Copilot AI reviewed Nov 21, 2025

View reviewed changes

battermann requested changes Nov 21, 2025

View reviewed changes

eyeinsky added 11 commits November 21, 2025 11:23

fixup! Implement getUserGroupsWithMembers

2622dc2

fixup! Implement getUserGroupsWithMembers

22a9c88

fixup! Implement getUserGroupsWithMembers

a1ff47d

fixup! Implement getUserGroupsWithMembers

6cc6c87

fixup! Implement getUserGroupsWithMembers

2bb3963

fixup! Implement getUserGroupsWithMembers

7f44f27

no return

fixup! Implement getUserGroupsWithMembers

7271395

don't map over singleton list

fixup! Adjust test to expect members

236e9a6

Use `bindResponse`

fixup! Adjust test to expect members

8387e21

add test case for empty group

fixup! Implement getUserGroupsWithMembers

d2b0c6e

make sanitize-pr

fixup! Abstract UserGroupPage

713e835

make sanitize-pr

eyeinsky added 3 commits November 21, 2025 15:41

fixup! Return user groups with members from Brig (stub implementation)

abf57d8

make sanitize-pr

fixup! Return user groups with members from Brig (stub implementation)

22e9044

fixup! Adjust test to expect members

7f7d14b

battermann approved these changes Nov 21, 2025

View reviewed changes

Add mock interpreter

51ca01e

	-- Test geting a single SCIM group by id
	-- Test getting a single SCIM group by id

	"coalesce(array_agg(gm.user_id), array[]::uuid[]) :: uuid[]",
	"coalesce(array_agg(gm.user_id) FILTER (WHERE gm.user_id IS NOT NULL), array[]::uuid[]) :: uuid[]",

		allThreeResp <- filterScimUserGroup OwnDomain tok $ Just "displayName co \"group\""
		(allThreeResp.json %. "Resources" & asList) `shouldMatchSet` [createdGroup1, createdGroup2, createdGroup3]

		justTwo <- filterScimUserGroup OwnDomain tok $ Just "displayName co \"another group\""
		(justTwo.json %. "Resources" & asList) `shouldMatchSet` [createdGroup2, createdGroup3]

WPB-21769: SCIM group search returning members #4859

Are you sure you want to change the base?

WPB-21769: SCIM group search returning members #4859

Conversation

eyeinsky commented Nov 17, 2025 • 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

Key Changes

Reviewed Changes

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

eyeinsky Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eyeinsky commented Nov 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eyeinsky commented Nov 17, 2025 •

edited

Loading

eyeinsky Nov 21, 2025 •

edited

Loading