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.

Sign up for GitHub

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

Jump to bottom

[WPB-10783] Prevent MLS-Legalhold interactions #4245

Merged
merged 6 commits into from
Sep 18, 2024
Merged

[WPB-10783] Prevent MLS-Legalhold interactions #4245

merged 6 commits into from
Sep 18, 2024

Conversation

MangoIV
Copy link
Contributor

@MangoIV MangoIV commented Sep 17, 2024 •
edited by akshaymankar
Loading

Checklist

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

@MangoIV MangoIV marked this pull request as draft September 17, 2024 13:54
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 17, 2024
@MangoIV MangoIV changed the title [feat] add failing test case [WPB-10783] prevent an MLS user from being legalholded Sep 17, 2024
@echoes-hq echoes-hq bot added the echoes/initiative: federation-wire-cloud Activate Federation with MLS on Wire Cloud label Sep 17, 2024
@MangoIV MangoIV force-pushed the wpb-10783 branch 2 times, most recently from 5ee899f to 5ff06d3 Compare September 17, 2024 15:39
@MangoIV MangoIV force-pushed the wpb-10772 branch 2 times, most recently from 54790fd to 1ce6612 Compare September 17, 2024 15:43
@akshaymankar akshaymankar changed the title [WPB-10783] prevent an MLS user from being legalholded [WPB-10783] Prevent an MLS user from being legalholded Sep 18, 2024
@elland elland marked this pull request as ready for review September 18, 2024 11:19
Base automatically changed from wpb-10772 to develop September 18, 2024 11:38
@elland elland changed the title [WPB-10783] Prevent an MLS user from being legalholded [WPB-10783] Prevent MLS-Legalhold interactions Sep 18, 2024
MangoIV
MangoIV commented Sep 18, 2024
View reviewed changes
Copy link
Contributor Author

@MangoIV MangoIV left a comment

Choose a reason for hiding this comment

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

looks good to me. I cannot approve as this is my PR but consider it approved.

integration/test/Test/LegalHold.hs Show resolved Hide resolved
services/galley/src/Galley/API/LegalHold.hs
Comment on lines +398 to +401
disallowIfMLSUser luid = do
void $ iterateConversations luid (toRange (Proxy @500)) $ \convs -> do
when (any (\c -> c.convProtocol /= ProtocolProteus) convs) $ do
throwS @'MLSLegalholdIncompatible
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.

services/galley/src/Galley/API/LegalHold.hs
@@ -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.

@akshaymankar akshaymankar merged commit dee9f3f into develop Sep 18, 2024
10 checks passed
@akshaymankar akshaymankar deleted the wpb-10783 branch September 18, 2024 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akshaymankar akshaymankar akshaymankar approved these changes

Assignees
No one assigned
Labels
echoes/initiative: federation-wire-cloud Activate Federation with MLS on Wire Cloud ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MangoIV @akshaymankar @zebot