Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WPB-5695 Enforce group conversation permission for external partner role #3788

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/3-bug-fixes/WPB-5695
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Enforce external partner permissions on the backend
1 change: 1 addition & 0 deletions integration/integration.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ library
Test.Conversation
Test.Demo
Test.Errors
Test.ExternalPartner
Test.Federation
Test.Federator
Test.MessageTimer
Expand Down
1 change: 1 addition & 0 deletions integration/test/API/Galley.hs
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,7 @@ getConversationCode user conv mbZHost = do
& maybe id zHost mbZHost
)

-- https://staging-nginz-https.zinfra.io/v5/api/swagger-ui/#/default/put_conversations__cnv_domain___cnv__name
changeConversationName ::
(HasCallStack, MakesValue user, MakesValue conv, MakesValue name) =>
user ->
Expand Down
2 changes: 1 addition & 1 deletion integration/test/SetupHelpers.hs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ deleteUser :: (HasCallStack, MakesValue user) => user -> App ()
deleteUser user = bindResponse (API.Brig.deleteUser user) $ \resp -> do
resp.status `shouldMatchInt` 200

-- | returns (user, team id)
-- | returns (owner, team id, members)
createTeam :: (HasCallStack, MakesValue domain) => domain -> Int -> App (Value, String, [Value])
createTeam domain memberCount = do
res <- createUser domain def {team = True}
Expand Down
82 changes: 82 additions & 0 deletions integration/test/Test/ExternalPartner.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
{-# OPTIONS_GHC -Wno-ambiguous-fields #-}

-- This file is part of the Wire Server implementation.
--
-- Copyright (C) 2023 Wire Swiss GmbH <opensource@wire.com>
--
-- This program is free software: you can redistribute it and/or modify it under
-- the terms of the GNU Affero General Public License as published by the Free
-- Software Foundation, either version 3 of the License, or (at your option) any
-- later version.
--
-- This program is distributed in the hope that it will be useful, but WITHOUT
-- ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
-- FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
-- details.
--
-- You should have received a copy of the GNU Affero General Public License along
-- with this program. If not, see <https://www.gnu.org/licenses/>.

module Test.ExternalPartner where

import API.Galley
import GHC.Stack
import MLS.Util
import SetupHelpers
import Testlib.Prelude

testExternalPartnerPermissions :: HasCallStack => App ()
testExternalPartnerPermissions = do
(owner, tid, u1 : u2 : u3 : _) <- createTeam OwnDomain 4

partner <- createTeamMemberWithRole owner tid "partner"

-- a partner should not be able to create conversation with 2 additional users or more
void $ postConversation partner (defProteus {team = Just tid, qualifiedUsers = [u1, u2]}) >>= getJSON 403

do
-- a partner can create a one to one conversation with a user from the same team
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this creating a group conversation? I understand the idea of the team 1:1 conversation, but here we're not really creating a 1:1 conversation, are we? That it is a 1:1 conversation is a concept in Wire clients.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly.

conv <- postConversation partner (defProteus {team = Just tid, qualifiedUsers = [u1]}) >>= getJSON 201

-- they should not be able to add another team member to the one to one conversation
bindResponse (addMembers partner conv def {users = [u2]}) $ \resp -> do
resp.status `shouldMatchInt` 403

-- the other member in the conversation gets deleted
deleteUser u1

-- now they still should not be able to add another member
bindResponse (addMembers partner conv def {users = [u2]}) $ \resp -> do
resp.status `shouldMatchInt` 403

do
-- also an external partner cannot add someone to a conversation, even if it is empty
conv <- postConversation partner (defProteus {team = Just tid}) >>= getJSON 201
bindResponse (addMembers partner conv def {users = [u3]}) $ \resp -> do
resp.status `shouldMatchInt` 403

testExternalPartnerPermissionsMls :: HasCallStack => App ()
testExternalPartnerPermissionsMls = do
-- external partners should not be able to create (MLS) conversations
(owner, tid, _) <- createTeam OwnDomain 2
bobExt <- createTeamMemberWithRole owner tid "partner"
bobExtClient <- createMLSClient def bobExt
bindResponse (postConversation bobExtClient defMLS) $ \resp -> do
resp.status `shouldMatchInt` 403

testExternalPartnerPermissionMlsOne2One :: HasCallStack => App ()
testExternalPartnerPermissionMlsOne2One = do
(owner, tid, alice : _) <- createTeam OwnDomain 2
bobExternal <- createTeamMemberWithRole owner tid "partner"
void $ getMLSOne2OneConversation alice bobExternal >>= getJSON 200

testExternalPartnerPermissionsConvName :: HasCallStack => App ()
testExternalPartnerPermissionsConvName = do
(owner, tid, u1 : _) <- createTeam OwnDomain 2

partner <- createTeamMemberWithRole owner tid "partner"

conv <- postConversation partner (defProteus {team = Just tid, qualifiedUsers = [u1]}) >>= getJSON 201

bindResponse (changeConversationName partner conv "new name") $ \resp -> do
resp.status `shouldMatchInt` 403
6 changes: 3 additions & 3 deletions libs/galley-types/src/Galley/Types/Teams.hs
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,9 @@ rolePerms RoleAdmin =
rolePerms RoleMember =
rolePerms RoleExternalPartner
<> Set.fromList
[ DoNotUseDeprecatedDeleteConversation,
DoNotUseDeprecatedAddRemoveConvMember,
DoNotUseDeprecatedModifyConvName,
[ DeleteConversation,
AddRemoveConvMember,
ModifyConvName,
GetMemberPermissions
]
rolePerms RoleExternalPartner =
Expand Down
29 changes: 19 additions & 10 deletions libs/wire-api/src/Wire/API/Team/Permission.hs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ serviceWhitelistPermissions =
Set.fromList
[ AddTeamMember,
RemoveTeamMember,
DoNotUseDeprecatedAddRemoveConvMember,
AddRemoveConvMember,
SetTeamData
]

Expand All @@ -127,11 +127,20 @@ serviceWhitelistPermissions =
-- | Team-level permission. Analog to conversation-level 'Action'.
data Perm
= CreateConversation
| DoNotUseDeprecatedDeleteConversation -- NOTE: This gets now overruled by conv level checks
| -- NOTE: This may get overruled by conv level checks in case those are more restrictive
-- We currently cannot get rid of this team-level permission in favor of the conv-level action
-- because it is used for e.g. for the team role 'RoleExternalPartner'
DeleteConversation
| AddTeamMember
| RemoveTeamMember
| DoNotUseDeprecatedAddRemoveConvMember -- NOTE: This gets now overruled by conv level checks
| DoNotUseDeprecatedModifyConvName -- NOTE: This gets now overruled by conv level checks
| -- NOTE: This may get overruled by conv level checks in case those are more restrictive
-- We currently cannot get rid of this team-level permission in favor of the conv-level action
-- because it is used for e.g. for the team role 'RoleExternalPartner'
AddRemoveConvMember
| -- NOTE: This may get overruled by conv level checks in case those are more restrictive
-- We currently cannot get rid of this team-level permission in favor of the conv-level action
-- because it is used for e.g. for the team role 'RoleExternalPartner'
ModifyConvName
| GetBilling
| SetBilling
| SetTeamData
Expand Down Expand Up @@ -159,11 +168,11 @@ intToPerms n =

permToInt :: Perm -> Word64
permToInt CreateConversation = 0x0001
permToInt DoNotUseDeprecatedDeleteConversation = 0x0002
permToInt DeleteConversation = 0x0002
permToInt AddTeamMember = 0x0004
permToInt RemoveTeamMember = 0x0008
permToInt DoNotUseDeprecatedAddRemoveConvMember = 0x0010
permToInt DoNotUseDeprecatedModifyConvName = 0x0020
permToInt AddRemoveConvMember = 0x0010
permToInt ModifyConvName = 0x0020
permToInt GetBilling = 0x0040
permToInt SetBilling = 0x0080
permToInt SetTeamData = 0x0100
Expand All @@ -174,11 +183,11 @@ permToInt SetMemberPermissions = 0x1000

intToPerm :: Word64 -> Maybe Perm
intToPerm 0x0001 = Just CreateConversation
intToPerm 0x0002 = Just DoNotUseDeprecatedDeleteConversation
intToPerm 0x0002 = Just DeleteConversation
intToPerm 0x0004 = Just AddTeamMember
intToPerm 0x0008 = Just RemoveTeamMember
intToPerm 0x0010 = Just DoNotUseDeprecatedAddRemoveConvMember
intToPerm 0x0020 = Just DoNotUseDeprecatedModifyConvName
intToPerm 0x0010 = Just AddRemoveConvMember
intToPerm 0x0020 = Just ModifyConvName
intToPerm 0x0040 = Just GetBilling
intToPerm 0x0080 = Just SetBilling
intToPerm 0x0100 = Just SetTeamData
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -233,11 +233,11 @@ testObject_Event_team_18 =
{ _self =
fromList
[ CreateConversation,
DoNotUseDeprecatedDeleteConversation,
DeleteConversation,
AddTeamMember,
RemoveTeamMember,
DoNotUseDeprecatedAddRemoveConvMember,
DoNotUseDeprecatedModifyConvName,
AddRemoveConvMember,
ModifyConvName,
GetBilling,
SetBilling,
SetTeamData,
Expand All @@ -249,11 +249,11 @@ testObject_Event_team_18 =
_copy =
fromList
[ CreateConversation,
DoNotUseDeprecatedDeleteConversation,
DeleteConversation,
AddTeamMember,
RemoveTeamMember,
DoNotUseDeprecatedAddRemoveConvMember,
DoNotUseDeprecatedModifyConvName,
AddRemoveConvMember,
ModifyConvName,
GetBilling,
GetMemberPermissions,
SetMemberPermissions,
Expand All @@ -275,20 +275,20 @@ testObject_Event_team_19 =
( Permissions
{ _self =
fromList
[ DoNotUseDeprecatedDeleteConversation,
[ DeleteConversation,
RemoveTeamMember,
DoNotUseDeprecatedAddRemoveConvMember,
DoNotUseDeprecatedModifyConvName,
AddRemoveConvMember,
ModifyConvName,
GetBilling,
SetBilling,
GetMemberPermissions,
GetTeamConversations
],
_copy =
fromList
[ DoNotUseDeprecatedDeleteConversation,
[ DeleteConversation,
RemoveTeamMember,
DoNotUseDeprecatedAddRemoveConvMember,
AddRemoveConvMember,
GetBilling,
SetBilling,
GetMemberPermissions,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@ import Imports (Maybe (Just, Nothing), fromJust)
import Wire.API.Team.Member (NewTeamMember, mkNewTeamMember)
import Wire.API.Team.Permission
( Perm
( AddTeamMember,
( AddRemoveConvMember,
AddTeamMember,
CreateConversation,
DeleteConversation,
DeleteTeam,
DoNotUseDeprecatedAddRemoveConvMember,
DoNotUseDeprecatedDeleteConversation,
DoNotUseDeprecatedModifyConvName,
GetBilling,
GetMemberPermissions,
GetTeamConversations,
ModifyConvName,
RemoveTeamMember,
SetBilling,
SetMemberPermissions,
Expand Down Expand Up @@ -63,13 +63,13 @@ testObject_NewTeamMember_team_2 =
{ _self =
fromList
[ CreateConversation,
DoNotUseDeprecatedDeleteConversation,
DeleteConversation,
AddTeamMember,
RemoveTeamMember,
DoNotUseDeprecatedAddRemoveConvMember,
DoNotUseDeprecatedModifyConvName
AddRemoveConvMember,
ModifyConvName
],
_copy = fromList [DoNotUseDeprecatedDeleteConversation, DoNotUseDeprecatedAddRemoveConvMember]
_copy = fromList [DeleteConversation, AddRemoveConvMember]
}
)
( Just
Expand All @@ -85,8 +85,8 @@ testObject_NewTeamMember_team_3 =
( Permissions
{ _self =
fromList
[CreateConversation, DoNotUseDeprecatedDeleteConversation, RemoveTeamMember, GetBilling, DeleteTeam],
_copy = fromList [CreateConversation, DoNotUseDeprecatedDeleteConversation, GetBilling]
[CreateConversation, DeleteConversation, RemoveTeamMember, GetBilling, DeleteTeam],
_copy = fromList [CreateConversation, DeleteConversation, GetBilling]
}
)
( Just
Expand Down Expand Up @@ -124,7 +124,7 @@ testObject_NewTeamMember_team_6 =
( Permissions
{ _self =
fromList
[CreateConversation, DoNotUseDeprecatedDeleteConversation, GetBilling, SetTeamData, SetMemberPermissions],
[CreateConversation, DeleteConversation, GetBilling, SetTeamData, SetMemberPermissions],
_copy = fromList [CreateConversation, GetBilling]
}
)
Expand All @@ -141,7 +141,7 @@ testObject_NewTeamMember_team_7 =
( Permissions
{ _self =
fromList
[AddTeamMember, RemoveTeamMember, DoNotUseDeprecatedModifyConvName, GetTeamConversations, DeleteTeam],
[AddTeamMember, RemoveTeamMember, ModifyConvName, GetTeamConversations, DeleteTeam],
_copy = fromList [AddTeamMember]
}
)
Expand All @@ -156,8 +156,8 @@ testObject_NewTeamMember_team_8 =
mkNewTeamMember
(Id (fromJust (UUID.fromString "00000008-0000-0003-0000-000200000003")))
( Permissions
{ _self = fromList [DoNotUseDeprecatedModifyConvName],
_copy = fromList [DoNotUseDeprecatedModifyConvName]
{ _self = fromList [ModifyConvName],
_copy = fromList [ModifyConvName]
}
)
( Just
Expand Down Expand Up @@ -193,7 +193,7 @@ testObject_NewTeamMember_team_11 =
mkNewTeamMember
(Id (fromJust (UUID.fromString "00000006-0000-0005-0000-000000000002")))
( Permissions
{ _self = fromList [CreateConversation, DoNotUseDeprecatedModifyConvName, SetTeamData],
{ _self = fromList [CreateConversation, ModifyConvName, SetTeamData],
_copy = fromList []
}
)
Expand All @@ -215,8 +215,8 @@ testObject_NewTeamMember_team_13 =
mkNewTeamMember
(Id (fromJust (UUID.fromString "00000002-0000-0004-0000-000600000001")))
( Permissions
{ _self = fromList [AddTeamMember, DoNotUseDeprecatedAddRemoveConvMember, SetTeamData, GetTeamConversations],
_copy = fromList [AddTeamMember, DoNotUseDeprecatedAddRemoveConvMember, GetTeamConversations]
{ _self = fromList [AddTeamMember, AddRemoveConvMember, SetTeamData, GetTeamConversations],
_copy = fromList [AddTeamMember, AddRemoveConvMember, GetTeamConversations]
}
)
Nothing
Expand All @@ -228,7 +228,7 @@ testObject_NewTeamMember_team_14 =
( Permissions
{ _self =
fromList
[CreateConversation, DoNotUseDeprecatedDeleteConversation, DoNotUseDeprecatedModifyConvName, GetBilling],
[CreateConversation, DeleteConversation, ModifyConvName, GetBilling],
_copy = fromList []
}
)
Expand Down Expand Up @@ -291,8 +291,8 @@ testObject_NewTeamMember_team_19 =
mkNewTeamMember
(Id (fromJust (UUID.fromString "00000004-0000-0005-0000-000100000008")))
( Permissions
{ _self = fromList [DoNotUseDeprecatedDeleteConversation, RemoveTeamMember, SetBilling, SetMemberPermissions],
_copy = fromList [DoNotUseDeprecatedDeleteConversation, SetBilling]
{ _self = fromList [DeleteConversation, RemoveTeamMember, SetBilling, SetMemberPermissions],
_copy = fromList [DeleteConversation, SetBilling]
}
)
Nothing
Expand All @@ -305,8 +305,8 @@ testObject_NewTeamMember_team_20 =
{ _self =
fromList
[ AddTeamMember,
DoNotUseDeprecatedAddRemoveConvMember,
DoNotUseDeprecatedModifyConvName,
AddRemoveConvMember,
ModifyConvName,
SetBilling,
GetMemberPermissions,
GetTeamConversations
Expand Down
Loading
Loading