WPB-18190: Add route to delete collaborator from team #4694

blackheaven · 2025-07-30T16:01:43Z

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

Checklist

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

PS: I did not see #4685, sorry

blackheaven · 2025-07-30T16:11:57Z

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

I'm not sure how to handle this one.

I have tried to move services/galley/src/Galley/Effects/ConversationStore.hs in lib/wire-subsystems, but Cassandra queries and many data-types are in Galley.

I'm also not sure how to list the O2O conversations of a (TeamId, UserId).

I would be inclined to create a dedicate effects, with one operation and a Cassandra interpreter, having queries in the same file.

WDYT?

blackheaven · 2025-08-04T09:10:46Z

libs/wire-subsystems/src/Wire/ConversationsStore/Cassandra.hs

This one is failing with:

[[email protected]] E, IO Exception occurred, message=ResponseError {reHost = datacenter1:rack1:127.0.0.1:9042, reTrace = Nothing, reWarn = [], reCause = Invalid "unconfigured table conversation"}, request=abef79a6-8f9f-41b3-b674-036e4216db63 [[email protected]] E, request=abef79a6-8f9f-41b3-b674-036e4216db63, code=500, label=server-error, "Server Error"

I feel like I'm in another key space, can I have some hints about it?

pcapriotti

Looks good overall, but I'm not sure about the logic for deleting conversations. See comments below.

pcapriotti · 2025-08-06T06:11:53Z

services/galley/src/Galley/ConversationsSubsystem.hs

I don't think this is enough. This would leave the conversation in the database, as well as member entries. It's probably better to reuse the conversation store here, which already has a deleteConversation method.

pcapriotti · 2025-08-06T06:20:46Z

services/galley/src/Galley/ConversationsSubsystem.hs

This doesn't look right to me, but maybe I'm not understanding something. In my understanding, we should:

delete all one2one conversations with unconnected team members

remove ourselves from team conversations.

The code here seems to only look for team conversations that were created by the collaborator, and then delete them. I can't see anything about one2one conversations. Regardless, I don't think it matters who created the conversation. When a collaboration is ended, the collaborator should simply be kicked from team conversations.

I have deleteConversation for One2One conversations type, and remove user-conversation relationship otherwise, let me know what you think.

blackheaven · 2025-08-07T08:51:19Z

I cannot reproduce the error, which seems not related to my changes.

blackheaven · 2025-08-15T14:19:28Z

@battermann I have made the change to rely on the same mechanisms behind rmUser, feel free to have a look.

Thanks.

changelog.d/2-features/WPB-18190

battermann · 2025-08-13T15:05:49Z

integration/test/Test/TeamCollaborators.hs

I think we should create a few team and 1:1 conversations and verify that the user is removed.

We should also verify that the user still exists and is still member of other non-team conersations etc.

Sorry, I have missed it:

yes, I would create a couple of conversations, team group conv, 1:1 conv, non-team conv, and then check membership after the collaborator is removed

TODO: find how to create team and non-team convs

@battermann I did not managed to create non-team conversation becoming one, but the remaining edge cases are checked let me know what you think.

battermann · 2025-08-19T11:41:11Z

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

I am not sure about the naming convention here. Why the prefix Internal?

Mainly because we do not check permissions in it, should I drop it?

Ah got it. It's fine then.

pcapriotti

I'm still not convinced that the removal logic is correct.

we shouldn't delete all 1-1 conversations indiscriminately, but just the ones with unconnected team members. I'm thinking of cases where a user A was connected with team member B, then A is added as a collaborator of B's team, then removed. We don't want to delete the conversation between A and B in that case.
I don't think we should be manually making db queries. That makes it harder to check that we're not breaking any invariants. Instead, we should use internal APIs and/or low-level helper functions that are shared with the rest of the code, even if that makes things less efficient.

pcapriotti · 2025-08-22T05:59:41Z

services/galley/src/Galley/ConversationsSubsystem.hs

Same issue as in #4708: we shouldn't use IN queries.

pcapriotti · 2025-08-22T06:00:43Z

services/galley/src/Galley/ConversationsSubsystem.hs

Same here. Instead, I think we should use the existing pagination internal API, like we do when deleting a user.

dropped, however, I'm not sur what you are referring to, there is nothing more in rmUser.

battermann · 2025-09-08T10:10:37Z

services/brig/src/Brig/Team/API.hs

    :<|> Named @"get-team-size" (\uid tid -> lift . liftSem $ teamSizePublic uid tid)
    :<|> Named @"accept-team-invitation" (\luid req -> lift $ liftSem $ acceptTeamInvitation luid req.password req.code)
    :<|> Named @"add-team-collaborator" (\zuid tid (NewTeamCollaborator uid perms) -> lift . liftSem $ createTeamCollaborator zuid uid tid perms)
+    :<|> Named @"remove-team-collaborator" (\zuid tid uid -> lift . liftSem $ GalleyAPIAccess.removeTeamMember zuid uid tid)


We should move this handler to galley, so that we do not need to make an extra unnecessary call from brig to galley.

blackheaven · 2025-09-10T16:00:11Z

@battermann I have reworked the the design, I still have to rework the tests

battermann · 2025-09-15T15:02:34Z

libs/wire-api/src/Wire/API/Routes/Public/Galley/TeamMember.hs

+    :<|> Named
+           "remove-team-collaborator"
+           ( Summary "Remove a collaborator from the team."
+               :> From 'V11


should be V12 by now

updated, thanks.

battermann · 2025-09-15T15:05:46Z

libs/wire-api/src/Wire/API/Team/Member.hs

  | NewTeamCollaborator
  | JoinRegularConversations
  | CreateApp
+  | RemoveTeamCollaborator


Is this used, I can't find it

It is in removeTeamCollaborator, the handler.

battermann · 2025-09-15T15:09:27Z

integration/test/Test/TeamCollaborators.hs

+      defProteus {team = Just team, qualifiedUsers = [alice, bob]}
+      >>= getJSON 201
+
+  withWebSockets [owner, alice] $ \[wsOwner, wsAlice] -> do


should the collaborator themselves, also get an event?

battermann · 2025-09-15T15:11:04Z

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

+            )
+          . (.status)
+  uncheckedDeleteTeamMember lusr Nothing tid rusr toNotify
+  internalRemoveTeamCollaborator rusr tid


should we send the collaborator remove event here?

added, thanks.

battermann · 2025-09-15T15:11:20Z

libs/wire-api/src/Wire/API/Event/Team.hs

  | EdConvDelete ConvId
  | EdCollaboratorAdd UserId [CollaboratorPermission]
  | EdAppCreate UserId
+  | EdCollaboratorRemove UserId


this is not sent, is it?

added, thanks.

battermann · 2025-09-15T15:31:37Z

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

-                  EdReasonDeleted
-              )
-              def
+        case () of


this is a funny construct, why not match on the cnvmType value directly?

done, thanks

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

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

blackheaven commented Jul 30, 2025

View reviewed changes

blackheaven commented Aug 4, 2025

View reviewed changes

blackheaven force-pushed the gdifolco/WPB-18190_collaborator-delete-endpoint branch from 8e873f3 to 78aea4e Compare August 5, 2025 12:32

blackheaven marked this pull request as ready for review August 5, 2025 12:34

blackheaven requested review from a team as code owners August 5, 2025 12:35

pcapriotti reviewed Aug 6, 2025

View reviewed changes

blackheaven requested a review from pcapriotti August 6, 2025 15:35

blackheaven force-pushed the gdifolco/WPB-18190_collaborator-delete-endpoint branch from 83eb7a1 to c1f5acf Compare August 6, 2025 16:54

blackheaven force-pushed the gdifolco/WPB-18190_collaborator-delete-endpoint branch from c1f5acf to 0969d6d Compare August 13, 2025 16:05

blackheaven self-assigned this Aug 14, 2025

battermann reviewed Aug 19, 2025

View reviewed changes

blackheaven force-pushed the gdifolco/WPB-18190_collaborator-delete-endpoint branch from c5e0a60 to 30c92d0 Compare August 19, 2025 15:29

blackheaven requested a review from battermann August 20, 2025 14:45

pcapriotti reviewed Aug 22, 2025

View reviewed changes

blackheaven requested a review from pcapriotti August 22, 2025 14:18

blackheaven force-pushed the gdifolco/WPB-18190_collaborator-delete-endpoint branch 4 times, most recently from 5eb2959 to c510050 Compare September 3, 2025 12:16

blackheaven force-pushed the gdifolco/WPB-18190_collaborator-delete-endpoint branch from 741ffd8 to 0ec3937 Compare September 5, 2025 16:06

battermann reviewed Sep 8, 2025

View reviewed changes

battermann reviewed Sep 15, 2025

View reviewed changes

blackheaven force-pushed the gdifolco/WPB-18190_collaborator-delete-endpoint branch from f7bee64 to 1c458bf Compare September 17, 2025 08:55

blackheaven and others added 26 commits September 24, 2025 10:48

fix: pass around changes to trigger notifications

c217801

fix: add tests assertions HasCallStack

869c0ab

fix: could it finally work?

cc93c00

fix: filter duplicated convs

a845443

fix: ormolu

0b1bc4a

fix: comment

2e08048

fix: ormolu

f71e5e4

fix: rebase

d6c5c6f

refactor: move to galley

485bbcf

fix: condition

8921bbc

fix: endpoint is on galley now

87646c5

fix: add forgot collaborator deletion

f9e0b31

fix: add debug logs

3614fb6

fix: ormolu

97371d0

fix: add debug logs

152359e

refactor: drop only conversations in teams

c3cc262

fix: drop debug logs

6be8485

fix: working

5239163

refactor: Leif feedback

c585a07

fix: send collaborator removal event to all members

14c857a

refactor: code review

8368ee6

fix: ormolu

2c4b431

fix: drop canthrow

c711d68

fix: tests

a721475

fix: ormolu

d8b1e27

Update libs/wire-api/src/Wire/API/Routes/Public/Galley/TeamMember.hs

9986c2f

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

blackheaven force-pushed the gdifolco/WPB-18190_collaborator-delete-endpoint branch from 6ab2eaa to 9986c2f Compare September 24, 2025 08:49

blackheaven merged commit f957075 into develop Sep 24, 2025
8 checks passed

blackheaven deleted the gdifolco/WPB-18190_collaborator-delete-endpoint branch September 24, 2025 10:16

battermann pushed a commit that referenced this pull request Sep 26, 2025

WPB-18190: Add route to delete collaborator from team (#4694)

e1bef67

WPB-18190: Add route to delete collaborator from team #4694

WPB-18190: Add route to delete collaborator from team #4694

Uh oh!

Conversation

blackheaven commented Jul 30, 2025

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcapriotti 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!

blackheaven commented Aug 7, 2025

Uh oh!

blackheaven commented Aug 15, 2025

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!

Choose a reason for hiding this comment

Uh oh!

pcapriotti 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!

blackheaven commented Sep 10, 2025

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!

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!

Uh oh!