WPB-21385: Fix /teams/:tid/members for non-searchable/stealth users #4826

eyeinsky · 2025-10-24T14:19:01Z

Checklist

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

Copilot

Pull Request Overview

This PR modifies the /teams/:tid/members endpoint to filter team members based on their searchability status. Non-admin users will now only see searchable team members, while admins continue to see all team members regardless of searchability.

Added filtering logic to exclude non-searchable users from team member listings for non-admin users
Updated API documentation to reflect the new TeamMemberNotFound error
Added integration tests to verify filtering behavior for both regular users and admins

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
services/galley/src/Galley/API/Teams.hs	Implements filtering of non-searchable team members for non-admin users by fetching user data from Brig and checking searchability
libs/wire-api/src/Wire/API/Routes/Public/Galley/TeamMember.hs	Adds `TeamMemberNotFound` error to the API route definition
integration/test/Test/Search.hs	Adds test coverage for both regular user (filtered) and admin (unfiltered) access to team members

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

Copilot · 2025-10-27T15:58:19Z

services/galley/src/Galley/API/Teams.hs

+          else do
+            -- if user isn't an admin, filter by whether team member is searchable
+            pwsResults' <- flip filterM (pwsResults pws) $ \tm -> do
+              u <- noteS @'TeamMemberNotFound =<< E.getUser (tm ^. userId) -- fetch user from Brig


This implementation makes an individual Brig API call for each team member during filtering, which could result in an N+1 query pattern. For large teams, this could significantly impact performance. Consider batching the user fetches or implementing a bulk user lookup API to retrieve searchability for all team members in a single request.

Sadly this is true, the right way to do it would be to perform a join on the database side, but Cassandra doesn't do joins. Will need to wait for the postgres migration to reach users and team members first.

Yes, but we could get the searchable attributes in a batch instead of each single user.

BrigAPIAccess has GetUsers :: [UserId] -> BrigAPIAccess m [User] which we could use here.

Yup, the getUser was just a wrapper around GetUsers!

Right, but we should get all users of the page at once, not make a request for every single one.

Copilot · 2025-10-27T15:58:19Z

services/galley/src/Galley/API/Teams.hs

+          else do
+            -- if user isn't an admin, filter by whether team member is searchable
+            pwsResults' <- flip filterM (pwsResults pws) $ \tm -> do
+              u <- noteS @'TeamMemberNotFound =<< E.getUser (tm ^. userId) -- fetch user from Brig


The error TeamMemberNotFound is misleading in this context. The team member exists (they're in the team), but their corresponding user record in Brig cannot be found. This error will be confusing for API consumers who see a team member listed but get a TeamMemberNotFound error. Consider using a more specific error like UserNotFound or handling the missing user case differently (e.g., filtering them out silently or logging a warning).

I partly agree. I think we should just skip the users that we cannot find in brig. But we should not return an error. This has nothing to do with the searchable attribute, if the searching user has the SetMemberSearchable permission, we also do not throw this error (because we do not even care if the user exists in brig or not...)

Hmm, UserNotFound is a Brig error but we are in Galley. I think we can't rethrow the Brig's error either as the getUser queries an internal endpoint -- should we add a UserNotFound to Galley as well?

Just skip the user and do not add them to the search restult.

eyeinsky · 2025-10-27T16:01:06Z

While CI is green and there is an test section for not getting stealth users when a regular team member is searching, then might need to keep an eye on how long do the requests with this "backend filtering" approach take.

battermann

I think we have to change the following:

get users from brig in a batch to determine the searchable attribute
do not throw an error if use does not exist, just do not include them in the result

what I don't like is that filtering is applied post-fetch per page, so returned page sizes may be smaller than requested. this may be necessary, but we need to make a note in the docs somewhere. (e.g. it is possible that we get an empty page with has_more=true, which is very surprising to the caller)

battermann · 2025-10-27T15:58:44Z

integration/test/Test/Search.hs

+    assertBool "/teams/:tid/members returns only searchable users to regular team member"
+      $ Set.fromList foundUids
+      == Set.fromList searchableUsers'Uids


I would use shouldMatchSet here. Is there a specific reason to use assertBool?

battermann · 2025-10-27T16:06:06Z

services/galley/src/Galley/API/Teams.hs

+          else do
+            -- if user isn't an admin, filter by whether team member is searchable
+            pwsResults' <- flip filterM (pwsResults pws) $ \tm -> do
+              u <- noteS @'TeamMemberNotFound =<< E.getUser (tm ^. userId) -- fetch user from Brig


Yes, but we could get the searchable attributes in a batch instead of each single user.

battermann · 2025-10-27T16:08:29Z

services/galley/src/Galley/API/Teams.hs

+          else do
+            -- if user isn't an admin, filter by whether team member is searchable
+            pwsResults' <- flip filterM (pwsResults pws) $ \tm -> do
+              u <- noteS @'TeamMemberNotFound =<< E.getUser (tm ^. userId) -- fetch user from Brig


I partly agree. I think we should just skip the users that we cannot find in brig. But we should not return an error. This has nothing to do with the searchable attribute, if the searching user has the SetMemberSearchable permission, we also do not throw this error (because we do not even care if the user exists in brig or not...)

…archable fetch users in batch

use shouldMatchSet

…archable make sanitize-pr

…archable add comment on why we do this

adamlow-wire · 2025-10-27T17:20:17Z

services/galley/src/Galley/API/Teams.hs

+                uids = map (^. userId) pwsResults0
+            users <- E.getUsers uids
+            let searchableUsers'Uids = map (qUnqualified . U.userQualifiedId) $ filter U.userSearchable users
+                pwsResults1 = filter (\tm -> (tm ^. userId) `elem` searchableUsers'Uids) pwsResults0


Wouldn’t this end up doing a nested lookup for every item, basically O(n²), since elem scans the whole list each time? Would using Set be a better option (imho)?

Even better, you can zip users and pwsResults0, filter once, then extract the results.

@pcapriotti Do you think this is alright: https://github.com/wireapp/wire-server/pull/4826/files#diff-231e4a1faa3508d94466458a77868b8b06786b7a9eec42af70301a975e772939R498-R502 , I compare but then return a Nothing. As I assume these users are supposed to all exist, should I throw instead?

akshaymankar · 2025-10-28T08:15:16Z

services/galley/src/Galley/API/Teams.hs

+    then do
+      pws :: PageWithState TeamMember <- E.listTeamMembers @CassandraPaging tid mState mLimit
+      pws' <-
+        if member `hasPermission` SetMemberSearchable


I think we can make this endpoint filter no matter who is looking, because we confirmed this is only used in the webapp and not team-settings. My understanding is that product doesn't want the stealth users to be found in the webapp at all.

because we confirmed this is only used in the webapp and not team-settings.

Correction, it's used in all clients (mobile too). But the point stands.

…archable

…archable make sanitize-pr

…archable rewrap

…archable we don't throw this anymore

eyeinsky · 2025-10-28T09:18:58Z

@akshaymankar Ready for another round!

battermann

Do we not need a changelog? (if it is part of another ticket that already has a changelog, we should at least add it there)

battermann · 2025-10-28T09:30:09Z

services/galley/src/Galley/API/Teams.hs

+      let f user tm =
+            if qUnqualified (U.userQualifiedId user) == tm ^. userId && U.userSearchable user
+              then Just tm
+              else Nothing -- throw here instead?


Can we remove the comment?

battermann · 2025-10-28T09:41:55Z

services/galley/src/Galley/API/Teams.hs

+            if qUnqualified (U.userQualifiedId user) == tm ^. userId && U.userSearchable user
+              then Just tm
+              else Nothing -- throw here instead?
+      pure $ toTeamMembersPage member $ pws {pwsResults = catMaybes $ zipWith f users pwsResults0}


Is the zipWith robust enough? What if we get the result in the wrong order or if a user is missing from the response?

Good point. I was assuming getUsers returned exactly the requested users in the same order. If we can't make that assumption, then we need to index them as a map or use a set for filtering like @adamlow-wire suggested.

…archable - use a lookup for users - log not found users as error

battermann

LGTM, only a nit pick

battermann · 2025-10-28T10:22:54Z

services/galley/src/Galley/API/Teams.hs

+      let notFoundUserUids = lefts results
+      unless (null notFoundUserUids) $
+        P.err $
+          Log.field "targets" (toByteString $ show $ map toByteString notFoundUserUids)


Is it necessary to do toByteString twice?

I don't really know, but found such an example elsewhere. Looking deeper, it seems that the field value is freeform text then I guess I could just show it?

services/galley/src/Galley/API/Teams.hs

Co-authored-by: Leif Battermann <[email protected]>

…archable

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

eyeinsky added 2 commits October 27, 2025 14:38

Add test for GET /teams/:tid/members

b4339ac

Filter users in GET /teams/:tid/member by whether user is searchable

4f322e9

eyeinsky force-pushed the ml/WPB-21385-non-searchable-team-members-endpoint branch from cea52ae to 4f322e9 Compare October 27, 2025 14:12

eyeinsky changed the title ~~WIP /teams/:tid/members searchable fix~~ WPB-21385: Fix /teams/:tid/members for non-searchable/stealth users Oct 27, 2025

eyeinsky marked this pull request as ready for review October 27, 2025 15:40

eyeinsky requested a review from a team as a code owner October 27, 2025 15:40

battermann requested a review from Copilot October 27, 2025 15:57

Copilot AI reviewed Oct 27, 2025

View reviewed changes

battermann requested changes Oct 27, 2025

View reviewed changes

eyeinsky added 4 commits October 27, 2025 18:23

fixup! Filter users in GET /teams/:tid/member by whether user is se…

6f99bae

…archable fetch users in batch

fixup! Add test for GET /teams/:tid/members

46331df

use shouldMatchSet

fixup! Filter users in GET /teams/:tid/member by whether user is se…

f0a9bed

…archable make sanitize-pr

fixup! Filter users in GET /teams/:tid/member by whether user is se…

88a13a1

…archable add comment on why we do this

adamlow-wire reviewed Oct 27, 2025

View reviewed changes

akshaymankar reviewed Oct 28, 2025

View reviewed changes

eyeinsky added 5 commits October 28, 2025 10:46

fixup! Filter users in GET /teams/:tid/member by whether user is se…

0f5f4f0

…archable

fixup! Add test for GET /teams/:tid/members

703e0b5

fixup! Filter users in GET /teams/:tid/member by whether user is se…

dc99a08

…archable make sanitize-pr

fixup! Filter users in GET /teams/:tid/member by whether user is se…

1b519de

…archable rewrap

fixup! Filter users in GET /teams/:tid/member by whether user is se…

74f8b9e

…archable we don't throw this anymore

battermann reviewed Oct 28, 2025

View reviewed changes

Add changelog entry

bb61275

eyeinsky requested review from a team as code owners October 28, 2025 09:40

battermann reviewed Oct 28, 2025

View reviewed changes

fixup! Filter users in GET /teams/:tid/member by whether user is se…

717168c

…archable - use a lookup for users - log not found users as error

battermann approved these changes Oct 28, 2025

View reviewed changes

battermann reviewed Oct 28, 2025

View reviewed changes

services/galley/src/Galley/API/Teams.hs Outdated Show resolved Hide resolved

eyeinsky and others added 3 commits October 28, 2025 14:06

Update services/galley/src/Galley/API/Teams.hs

2cb4a0a

Co-authored-by: Leif Battermann <[email protected]>

fixup! Filter users in GET /teams/:tid/member by whether user is se…

2c867d9

…archable

fixup! Filter users in GET /teams/:tid/member by whether user is se…

1e72da1

…archable

eyeinsky merged commit 02ed33e into develop Oct 28, 2025
9 checks passed

eyeinsky deleted the ml/WPB-21385-non-searchable-team-members-endpoint branch October 28, 2025 13:06

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-21385: Fix /teams/:tid/members for non-searchable/stealth users #4826

WPB-21385: Fix /teams/:tid/members for non-searchable/stealth users #4826

Uh oh!

Conversation

eyeinsky commented Oct 24, 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

Reviewed Changes

Uh oh!

Copilot AI Oct 27, 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!

Copilot AI Oct 27, 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!

eyeinsky commented Oct 27, 2025

Uh oh!

battermann left a comment

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!

Choose a reason for hiding this comment

Uh oh!

eyeinsky commented Oct 28, 2025

Uh oh!

battermann left a comment

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!

battermann left a comment

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!

Reviewers

Assignees

Labels

Projects

eyeinsky commented Oct 24, 2025 •

edited

Loading