From a47c9c9251b2efd7352f6d6bf7fee809ac463167 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 27 May 2026 14:35:25 +0200 Subject: [PATCH 1/5] Changelog. --- .../WPB-25521-refactor-access-control-rules-for-collaborators | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/5-internal/WPB-25521-refactor-access-control-rules-for-collaborators diff --git a/changelog.d/5-internal/WPB-25521-refactor-access-control-rules-for-collaborators b/changelog.d/5-internal/WPB-25521-refactor-access-control-rules-for-collaborators new file mode 100644 index 0000000000..157e4bc707 --- /dev/null +++ b/changelog.d/5-internal/WPB-25521-refactor-access-control-rules-for-collaborators @@ -0,0 +1 @@ +Refactor access control rules for collaborators. From 144a154a49f4682656e24c48011bb8302bc6234e Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 27 May 2026 14:50:46 +0200 Subject: [PATCH 2/5] Nit-pick. --- .../src/Wire/ConversationSubsystem/CreateInternal.hs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libs/wire-subsystems/src/Wire/ConversationSubsystem/CreateInternal.hs b/libs/wire-subsystems/src/Wire/ConversationSubsystem/CreateInternal.hs index e28425c906..dc219385ff 100644 --- a/libs/wire-subsystems/src/Wire/ConversationSubsystem/CreateInternal.hs +++ b/libs/wire-subsystems/src/Wire/ConversationSubsystem/CreateInternal.hs @@ -638,8 +638,8 @@ checkBindingTeamPermissions :: Sem r (Maybe TeamId) checkBindingTeamPermissions lusr lother tid = do mTeamCollaborator <- internalGetTeamCollaborator tid (tUnqualified lusr) - zusrMembership <- TeamSubsystem.internalGetTeamMember (tUnqualified lusr) tid - case (mTeamCollaborator, zusrMembership) of + mTeamMember <- TeamSubsystem.internalGetTeamMember (tUnqualified lusr) tid + case (mTeamCollaborator, mTeamMember) of (Just collaborator, Nothing) -> guardPerm CollaboratorPermission.ImplicitConnection collaborator (Nothing, mbMember) -> void $ permissionCheck CreateConversation mbMember (Just collaborator, Just member) -> @@ -647,7 +647,7 @@ checkBindingTeamPermissions lusr lother tid = do throwS @OperationDenied TeamStore.getTeamBinding tid >>= \case Just Binding -> do - when (isJust zusrMembership) $ + when (isJust mTeamMember) $ verifyMembership tid (tUnqualified lusr) mOtherTeamCollaborator <- internalGetTeamCollaborator tid (tUnqualified lother) unless (isJust mOtherTeamCollaborator) $ From 8578a11d78738d92c284560ddaa286d9e9dda139 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 27 May 2026 15:06:36 +0200 Subject: [PATCH 3/5] Refactor: split up function. We can compute implicit connections separately from testing the explicit connections, so we can recycle it. --- .../src/Wire/ConversationSubsystem/Util.hs | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/libs/wire-subsystems/src/Wire/ConversationSubsystem/Util.hs b/libs/wire-subsystems/src/Wire/ConversationSubsystem/Util.hs index 78d7019397..d2603a4cad 100644 --- a/libs/wire-subsystems/src/Wire/ConversationSubsystem/Util.hs +++ b/libs/wire-subsystems/src/Wire/ConversationSubsystem/Util.hs @@ -165,28 +165,39 @@ ensureConnectedToLocalsOrSameTeam :: Sem r () ensureConnectedToLocalsOrSameTeam _ [] = pure () ensureConnectedToLocalsOrSameTeam (tUnqualified -> u) uids = do - uTeams <- getUserTeams u + implicitConnections <- getImplicitReachableLocals u uids + ensureConnectedToLocals u (uids \\ implicitConnections) + +getImplicitReachableLocals :: + ( Member TeamStore r, + Member TeamCollaboratorsSubsystem r, + Member TeamSubsystem r + ) => + UserId -> + [UserId] -> + Sem r [UserId] +getImplicitReachableLocals reacher reachees = do + uTeams <- getUserTeams reacher icTeams <- getUserCollaborationTeams icUsers <- getTeamCollaborators uTeams - -- We collect all the relevant uids from same teams as the origin user + -- We collect all the relevant reachees from same teams as the origin user sameTeamUids <- forM (uTeams `union` icTeams) $ \team -> - fmap (view Mem.userId) <$> TeamSubsystem.internalSelectTeamMembers team uids - -- Do not check connections for users that are on the same team - ensureConnectedToLocals u ((uids \\ join sameTeamUids) \\ icUsers) + fmap (view Mem.userId) <$> TeamSubsystem.internalSelectTeamMembers team reachees + pure (join sameTeamUids `union` icUsers) where -- Teams in which the user who wants to reach out is member with -- `ImplicitConnection` permission. getUserCollaborationTeams :: (Member TeamCollaboratorsSubsystem r') => Sem r' [TeamId] getUserCollaborationTeams = gTeam - <$$> (filter (flip hasPermission CollaboratorPermission.ImplicitConnection) <$> internalGetTeamCollaborations u) + <$$> (filter (flip hasPermission CollaboratorPermission.ImplicitConnection) <$> internalGetTeamCollaborations reacher) -- We do not check the permissions of team collaborators if a user tries to -- reach out to them (if they are in the same team.) The reasoning behind -- this is that team collaborators have implicitly agreed to be -- collaborated with. getTeamCollaborators :: (Member TeamCollaboratorsSubsystem r') => [TeamId] -> Sem r' [UserId] - getTeamCollaborators teams = gUser <$$> internalGetTeamCollaboratorsWithIds (Set.fromList teams) (Set.fromList uids) + getTeamCollaborators teams = gUser <$$> internalGetTeamCollaboratorsWithIds (Set.fromList teams) (Set.fromList reachees) -- | Check that the user is connected to everybody else. -- From c1e2fa29901d219f96807fd721ef98e4a78d2570 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 27 May 2026 15:14:49 +0200 Subject: [PATCH 4/5] Refactor: move deprecated code away from the interesting logic. --- .../ConversationSubsystem/CreateInternal.hs | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/libs/wire-subsystems/src/Wire/ConversationSubsystem/CreateInternal.hs b/libs/wire-subsystems/src/Wire/ConversationSubsystem/CreateInternal.hs index dc219385ff..bdfdde6c43 100644 --- a/libs/wire-subsystems/src/Wire/ConversationSubsystem/CreateInternal.hs +++ b/libs/wire-subsystems/src/Wire/ConversationSubsystem/CreateInternal.hs @@ -637,6 +637,7 @@ checkBindingTeamPermissions :: TeamId -> Sem r (Maybe TeamId) checkBindingTeamPermissions lusr lother tid = do + guardTeamBinding mTeamCollaborator <- internalGetTeamCollaborator tid (tUnqualified lusr) mTeamMember <- TeamSubsystem.internalGetTeamMember (tUnqualified lusr) tid case (mTeamCollaborator, mTeamMember) of @@ -645,17 +646,21 @@ checkBindingTeamPermissions lusr lother tid = do (Just collaborator, Just member) -> unless (hasPermission collaborator CollaboratorPermission.ImplicitConnection || hasPermission member CreateConversation) $ throwS @OperationDenied - TeamStore.getTeamBinding tid >>= \case - Just Binding -> do - when (isJust mTeamMember) $ - verifyMembership tid (tUnqualified lusr) - mOtherTeamCollaborator <- internalGetTeamCollaborator tid (tUnqualified lother) - unless (isJust mOtherTeamCollaborator) $ - verifyMembership tid (tUnqualified lother) - pure (Just tid) - Just _ -> throwS @'NonBindingTeam - Nothing -> throwS @'TeamNotFound + when (isJust mTeamMember) $ + verifyMembership tid (tUnqualified lusr) + mOtherTeamCollaborator <- internalGetTeamCollaborator tid (tUnqualified lother) + unless (isJust mOtherTeamCollaborator) $ + verifyMembership tid (tUnqualified lother) + pure (Just tid) where + -- it is unclear why we do this here; it can be removed once we + -- remove binding teams from the code. + guardTeamBinding = do + TeamStore.getTeamBinding tid >>= \case + Just Binding -> pure () + Just _ -> throwS @'NonBindingTeam + Nothing -> throwS @'TeamNotFound + guardPerm p m = if m `hasPermission` p then pure () From f610d6c14e2125ea58e4fa4bda0a4a1b8881ecd4 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 27 May 2026 15:29:19 +0200 Subject: [PATCH 5/5] TODOs --- ...-25521-refactor-access-control-rules-for-collaborators | 2 ++ libs/wire-api/src/Wire/API/Conversation.hs | 1 + .../src/Wire/ConversationSubsystem/CreateInternal.hs | 8 ++++++++ 3 files changed, 11 insertions(+) diff --git a/changelog.d/5-internal/WPB-25521-refactor-access-control-rules-for-collaborators b/changelog.d/5-internal/WPB-25521-refactor-access-control-rules-for-collaborators index 157e4bc707..835189d6b4 100644 --- a/changelog.d/5-internal/WPB-25521-refactor-access-control-rules-for-collaborators +++ b/changelog.d/5-internal/WPB-25521-refactor-access-control-rules-for-collaborators @@ -1 +1,3 @@ Refactor access control rules for collaborators. + +TODO: if the behavior changes in this PR, we need to move this and explain how! (but with some luck, it'll remain a behavior-preserving refactoring job.) diff --git a/libs/wire-api/src/Wire/API/Conversation.hs b/libs/wire-api/src/Wire/API/Conversation.hs index ac954d93d9..85fb09384b 100644 --- a/libs/wire-api/src/Wire/API/Conversation.hs +++ b/libs/wire-api/src/Wire/API/Conversation.hs @@ -1016,6 +1016,7 @@ data NewOne2OneConv = NewOne2OneConv { users :: [UserId], -- | A list of qualified users, which can include some local qualified users -- too. + -- TODO: why is this a list and not a pair, given the name `NewOne2OneConv`? qualifiedUsers :: [Qualified UserId], name :: Maybe (Range 1 256 Text), team :: Maybe ConvTeamInfo diff --git a/libs/wire-subsystems/src/Wire/ConversationSubsystem/CreateInternal.hs b/libs/wire-subsystems/src/Wire/ConversationSubsystem/CreateInternal.hs index bdfdde6c43..800fa1fd8a 100644 --- a/libs/wire-subsystems/src/Wire/ConversationSubsystem/CreateInternal.hs +++ b/libs/wire-subsystems/src/Wire/ConversationSubsystem/CreateInternal.hs @@ -637,6 +637,14 @@ checkBindingTeamPermissions :: TeamId -> Sem r (Maybe TeamId) checkBindingTeamPermissions lusr lother tid = do + -- TODO(fisx): i think this can be futher refactored to use + -- `getImplicitReachableLocals`. i'm also still not sure this logic + -- is sound, need to think about this more! + + -- TODO(fisx): also check the access control logic around + -- adding/removing conv members to/from group convs. can that also + -- be simplified? + guardTeamBinding mTeamCollaborator <- internalGetTeamCollaborator tid (tUnqualified lusr) mTeamMember <- TeamSubsystem.internalGetTeamMember (tUnqualified lusr) tid