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-10783] Prevent MLS-Legalhold interactions #4245

Merged
merged 6 commits into from
Sep 18, 2024
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/2-features/block-lh-for-mls-users
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Deny requests for a legalhold device for users who are part of any MLS conversations
26 changes: 22 additions & 4 deletions integration/test/Test/LegalHold.hs
Original file line number Diff line number Diff line change
Expand Up @@ -906,15 +906,33 @@ testLHDisableBeforeApproval = do
>>= assertStatus 200
getBob'sStatus `shouldMatch` "disabled"

-- ---------
-- WPB-10783
-- ---------
testBlockLHForMLSUsers :: (HasCallStack) => App ()
testBlockLHForMLSUsers = do
-- scenario 1:
-- if charlie is in any MLS conversation, he cannot approve to be put under legalhold
(charlie, tid, []) <- createTeam OwnDomain 1
[charlie1] <- traverse (createMLSClient def) [charlie]
void $ createNewGroup charlie1
void $ createAddCommit charlie1 [charlie] >>= sendAndConsumeCommitBundle

legalholdWhitelistTeam tid charlie >>= assertStatus 200
withMockServer def lhMockApp \lhDomAndPort _chan -> do
postLegalHoldSettings tid charlie (mkLegalHoldSettings lhDomAndPort) >>= assertStatus 201
requestLegalHoldDevice tid charlie charlie `bindResponse` do
assertLabel 409 "mls-legal-hold-not-allowed"
akshaymankar marked this conversation as resolved.
Show resolved Hide resolved

-- ---------
-- WPB-10772
-- ---------

-- | scenario 2.1:
-- charlie first is put under legalhold and after that wants to join an MLS conversation
-- claiming a keypackage of charlie to add them to a conversation should not be possible
testLegalholdThenMLSThirdParty :: (HasCallStack) => App ()
testLegalholdThenMLSThirdParty = do
testBlockClaimingKeyPackageForLHUsers :: (HasCallStack) => App ()
testBlockClaimingKeyPackageForLHUsers = do
(alice, tid, [charlie]) <- createTeam OwnDomain 2
[alice1, charlie1] <- traverse (createMLSClient def) [alice, charlie]
_ <- uploadNewKeyPackage charlie1
Expand All @@ -937,8 +955,8 @@ testLegalholdThenMLSThirdParty = do
-- since he doesn't need to claim his own keypackage to do so, this would succeed
-- we need to check upon group creation if the user is under legalhold and reject
-- the operation if they are
testLegalholdThenMLSSelf :: (HasCallStack) => App ()
testLegalholdThenMLSSelf = do
testBlockCreateMLSConvForLHUsers :: (HasCallStack) => App ()
testBlockCreateMLSConvForLHUsers = do
(alice, tid, [charlie]) <- createTeam OwnDomain 2
[alice1, charlie1] <- traverse (createMLSClient def) [alice, charlie]
_ <- uploadNewKeyPackage alice1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ type LegalHoldAPI =
:> CanThrow 'LegalHoldServiceBadResponse
:> CanThrow 'LegalHoldServiceNotRegistered
:> CanThrow 'LegalHoldCouldNotBlockConnections
:> CanThrow 'MLSLegalholdIncompatible
:> CanThrow 'UserLegalHoldIllegalOperation
:> Description
"This endpoint can lead to the following events being sent:\n\
Expand Down
9 changes: 9 additions & 0 deletions services/galley/src/Galley/API/LegalHold.hs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ import Polysemy.Input
import Polysemy.TinyLog qualified as P
import System.Logger.Class qualified as Log
import Wire.API.Conversation (ConvType (..))
import Wire.API.Conversation.Protocol
import Wire.API.Conversation.Role
import Wire.API.Error
import Wire.API.Error.Galley
Expand Down Expand Up @@ -345,6 +346,7 @@ requestDevice ::
Member (ErrorS 'LegalHoldNotEnabled) r,
Member (ErrorS 'LegalHoldServiceBadResponse) r,
Member (ErrorS 'LegalHoldServiceNotRegistered) r,
Member (ErrorS 'MLSLegalholdIncompatible) r,
Member (ErrorS 'NotATeamMember) r,
Member (ErrorS 'NoUserLegalHoldConsent) r,
Member (ErrorS OperationDenied) r,
Expand Down Expand Up @@ -392,6 +394,12 @@ requestDevice lzusr tid uid = do
lhs@UserLegalHoldDisabled -> RequestDeviceSuccess <$ provisionLHDevice zusr luid lhs
UserLegalHoldNoConsent -> throwS @'NoUserLegalHoldConsent
where
disallowIfMLSUser :: Local UserId -> Sem r ()
disallowIfMLSUser luid = do
void $ iterateConversations luid (toRange (Proxy @500)) $ \convs -> do
when (any (\c -> c.convProtocol /= ProtocolProteus) convs) $ do
throwS @'MLSLegalholdIncompatible
Comment on lines +398 to +401
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
disallowIfMLSUser luid = do
void $ iterateConversations luid (toRange (Proxy @500)) $ \convs -> do
when (any (\c -> c.convProtocol /= ProtocolProteus) convs) $ do
throwS @'MLSLegalholdIncompatible
disallowIfMLSUser luid =
void $ iterateConversations luid (toRange (Proxy @500)) $ \convs ->
when (any (\c -> c.convProtocol /= ProtocolProteus) convs) $
throwS @'MLSLegalholdIncompatible

Copy link
Member

Choose a reason for hiding this comment

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

Gonna leave these too, CI took too long, I'm tired today.


-- Wire's LH service that galley is usually calling here is idempotent in device creation,
-- ie. it returns the existing device on multiple calls to `/init`, like here:
-- https://github.com/wireapp/legalhold/blob/e0a241162b9dbc841f12fbc57c8a1e1093c7e83a/src/main/java/com/wire/bots/hold/resource/InitiateResource.java#L42
Expand All @@ -401,6 +409,7 @@ requestDevice lzusr tid uid = do
-- device at (almost) the same time.
provisionLHDevice :: UserId -> Local UserId -> UserLegalHoldStatus -> Sem r ()
provisionLHDevice zusr luid userLHStatus = do
disallowIfMLSUser luid
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe a small comment would be nice as well, even though I won't insist on it, the code is already quite clear.

Suggested change
disallowIfMLSUser luid
-- currently, legalhold is not compatible with MLS, hence we disallow provisioning
-- a legalhold device for any user who partakes in anything but proteus conversations.
disallowIfMLSUser luid

Copy link
Member

Choose a reason for hiding this comment

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

Took long enough for CI, I think I won't add it. Feel free to create another PR.

(lastPrekey', prekeys) <- requestDeviceFromService luid
-- We don't distinguish the last key here; brig will do so when the device is added
LegalHoldData.insertPendingPrekeys (tUnqualified luid) (unpackLastPrekey lastPrekey' : prekeys)
Expand Down
Loading