Block and kick users in case of LH no_consent conflict (1:1 convs). #1507

fisx · 2021-05-14T10:12:09Z

https://wearezeta.atlassian.net/browse/SQSERVICES-429

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

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

services/galley/src/Galley/Data.hs

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

- for i in $(echo Relation Accepted Blocked Pending Ignored Sent Cancelled MissingLegalholdConsent); do find ./tools/ ./libs/ ./services/ -type f -name '*.hs' -exec perl -i -pe 's/\b'$i'_'\''/'$i'/g' {} \;; done - drop the `StripSuffix` in the schema generation

fisx

I just found a button for showing changes since last review, so here is an empty review. :)

there is a benchmark that's quite reassuring. I don't think we need to worry about http response times here.

libs/brig-types/src/Brig/Types/Connection.hs

libs/brig-types/test/unit/Test/Brig/Roundtrip.hs

fisx · 2021-05-25T10:54:43Z

Ready to merge IMO (given ci agrees), but I can't approve, it's my PR. :)

libs/brig-types/src/Brig/Types/Connection.hs

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

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

jschaul · 2021-05-25T20:30:57Z

services/galley/src/Galley/Data.hs

@@ -309,6 +310,11 @@ userTeams u =
  map runIdentity
    <$> retry x1 (query Cql.selectUserTeams (params Quorum (Identity u)))

+usersTeams :: MonadClient m => [UserId] -> m (Map UserId TeamId)
+usersTeams uids = do
+  pairs <- retry x1 (query Cql.selectUsersTeams (params Quorum (Identity uids)))


Larger IN queries (I know there's currently chunks of 32 in use; but still it's not ideal even at 32) lead to performance problems. See https://lostechies.com/ryansvihla/2014/09/22/cassandra-query-patterns-not-using-the-in-query-for-multiple-partitions/

Preferred way to do this lookup here is to use pooledMapConcurrentlyN (https://hoogle.zinfra.io/file/root/.stack/snapshots/x86_64-linux/e2cc9ab01ac828ffb6fe45a45d38d7ca6e672fb9fe95528498b990da673c5071/8.8.4/doc/unliftio-0.2.13/UnliftIO-Async.html#v:pooledMapConcurrentlyN) (e.g. with 8 threads). Then you can re-use the existing selectOneUserTeam.

Okay to do this in a follow-up PR if you prefer (but please do this change before this all goes live).

jschaul · 2021-05-25T20:32:41Z

services/galley/src/Galley/Data/Queries.hs

@@ -135,6 +135,9 @@ selectUserTeamsIn = "select team from user_team where user = ? and team in ? ord
 selectUserTeamsFrom :: PrepQuery R (UserId, TeamId) (Identity TeamId)
 selectUserTeamsFrom = "select team from user_team where user = ? and team > ? order by team"

+selectUsersTeams :: PrepQuery R (Identity [UserId]) (UserId, TeamId)
+selectUsersTeams = "select user, team from user_team where user in ?"


As stated in the other comment, this query should be removed if possible.

jschaul · 2021-05-25T20:41:51Z

services/galley/test/integration/API/Teams/LegalHold.hs

-  -- tracked here: https://wearezeta.atlassian.net/browse/SQSERVICES-429
-  pure ()
+-- If LH is activated for other user in 1:1 conv, 1:1 conv is blocked
+testNoConsentBlockOne2OneConv :: HasCallStack => Bool -> Bool -> Bool -> Bool -> TestM ()


I admit it's a little hard to read this helper function as there are too many boolean branches here to figure out in one's head if this makes sense or not. I would personally prefer a bit more code duplication and less if/else branching on booleans. I can't properly review this helper function and the associated tests. Glancing at it, it looks okay, but there might be a glaring reverted condition here that I missed. Perhaps someone else could review this particular function and the associated tests? Maybe @smatting ?

This is a nitpick though and this can (or not) be done also as a follow-up cleanup.

Stefan has paired excessively with me on this test, but I'll fix the boolean blindness in a separate PR anyway.

services/galley/test/integration/API/Teams/LegalHold.hs

Co-authored-by: jschaul <jschaul@users.noreply.github.com>

fisx · 2021-05-26T07:52:58Z

I hope I fixed all the errors here: 4aef9e9

(Copied from #1507, which has meanwhile been reverted in #1549.)

* Import removeTeam to Galley.API.LegalHold without import cycles. (will be needed shortly.) * iterateConversations. (Copied from #1507, which has meanwhile been reverted in #1549.) * Make removeMember device id argument optional. * handleGroupConvPolicyConflicts [wip] * test stub * mapcon * fix bug in handleGroupConvPolicyConflicts * add 2 test cases: who is admin? who gets removed? * Add group conv logic [wip] * move lh helpers to utils * fixup add members * fixup tests * use Conversation from galley to include self * implement anyLHConsentMissing * Move check*Event funs from Teams to Utils * update first * adjust test to new setting & adjust case when peer is admin * hi ci * Add checks for leave events * add testNoConsentCannotBeInvited * add failing test case * Add guards (test succeeds) * refactor: rename guard functions * comment wording * anyLegalholdActivated: make true to the name * adjust testNoConsentRemoveFromGroupConv * inviting conflicting users: adjust and rename test * remove futurework (wont do) and add comments * remove test marker * spell out the tests * add FUTUREWORKs for test structure * comment wording * add test for v2 endpoint * add additional safeguard * Cleanup. * Add two missing test case stubs. * Eliminate redundant negation in function name. * Boolean blindness. only the first line changed, rest is ormolu noise. * Fix call side. * Changed my mind about whether boolean blindness is a good thing. * Fixup. * Cleanup (move source comment to inner block). * Tweak detail. * Remove dubious claim. #1595 (comment) * Simplify. * Move code around. * LH consent: guarantee that all conflicting conv members are removed. * Taking back e16a0bc. (I thought I had spotted a difference in database load, but I think i was wrong, and the original code is more straight-forward.) * Fixup. * Change test cases & descriptions. * fix syntax * All -> Some (explanation in message) Why 1) Because "Some" is the negation of "None" 2) This is consistent with testGroup "Legalhold is activated for user A in a group conversation" * add failing test case * Refactor: factor out new fn getLHStatusForUsers * Update business logic * Remove NoConsentingAdmins test case * Add test case: mixed invitees * Refactor: group tests regarding invite together * assertion (it's non-trivial, but easy enough to convince ourselves that it'll pass fine, so no error handling needed.) * Language. Co-authored-by: Matthias Fischmann <mf@zerobuzz.net>

fisx force-pushed the SQSERVICES-402-consent branch from b743a45 to a45d554 Compare May 14, 2021 21:03

fisx force-pushed the SQSERVICES-402-block-and-kick branch from 3150fa3 to 4641849 Compare May 14, 2021 21:04

fisx marked this pull request as draft May 14, 2021 21:05

Base automatically changed from SQSERVICES-402-consent to develop May 17, 2021 14:46

fisx force-pushed the SQSERVICES-402-block-and-kick branch 2 times, most recently from c6fda81 to 1794cdd Compare May 17, 2021 19:31

smatting force-pushed the SQSERVICES-402-block-and-kick branch from 53c48f8 to 203de03 Compare May 19, 2021 16:04

fisx commented May 20, 2021

View reviewed changes

services/galley/src/Galley/API/LegalHold.hs Outdated Show resolved Hide resolved

fisx force-pushed the SQSERVICES-402-block-and-kick branch 2 times, most recently from 729234a to d3bfc33 Compare May 23, 2021 11:11

fisx and others added 11 commits May 23, 2021 17:24

...

2ced774

fix incorrect test env

9117e7e

block accepted connections

716c358

work on integration tests.

6672735

Catch lh policy conflicts when creating connections.

04a6795

Introduce 'Relation' state "blocked for missing legalhold consent".

2486b37

...

7a14260

...

34ee2a8

...

51b4b63

I don't know!

6b5b8ed

fisx force-pushed the SQSERVICES-402-block-and-kick branch from d3bfc33 to 6b5b8ed Compare May 23, 2021 19:41

...

d25fd7d

fisx commented May 24, 2021

View reviewed changes

fisx and others added 5 commits May 24, 2021 09:03

...

7baccdd

Better failure messages in test helpers.

906b08d

updateConnectionInternal.

a1b8429

fix compilation

572e575

Implement LH block/unblock for connections

56a8b14

De-prioritize TODO.

788345d

there is a benchmark that's quite reassuring. I don't think we need to worry about http response times here.

fisx commented May 25, 2021

View reviewed changes

libs/brig-types/src/Brig/Types/Connection.hs Show resolved Hide resolved

libs/brig-types/test/unit/Test/Brig/Roundtrip.hs Show resolved Hide resolved

fisx marked this pull request as ready for review May 25, 2021 10:46

fisx changed the title ~~Block and kick users in case of LH no_consent conflict. [skip ci]~~ Block and kick users in case of LH no_consent conflict. May 25, 2021

fisx changed the title ~~Block and kick users in case of LH no_consent conflict.~~ Block and kick users in case of LH no_consent conflict (1:1 convs). May 25, 2021

Document implicit json instances.

127eef9

Fixup.

2cddef4

jschaul approved these changes May 25, 2021

View reviewed changes

Hi CI

d35ea14

This comment has been minimized.

Sign in to view

fisx changed the title ~~Block and kick users in case of LH no_consent conflict (1:1 convs).~~ [skip ci] Block and kick users in case of LH no_consent conflict (1:1 convs). May 26, 2021

fisx and others added 5 commits May 26, 2021 07:49

Update services/brig/src/Brig/API/Connection.hs

690b67a

Co-authored-by: jschaul <jschaul@users.noreply.github.com>

Process review feedback.

da45b70

Fix: granting consent.

d28161e

Refactor: mv function to top-level scope.

08750d4

Fix: (again) changing LH state.

4aef9e9

fisx changed the title ~~[skip ci] Block and kick users in case of LH no_consent conflict (1:1 convs).~~ Block and kick users in case of LH no_consent conflict (1:1 convs). May 26, 2021

fisx merged commit 0f32ae1 into develop May 26, 2021

fisx deleted the SQSERVICES-402-block-and-kick branch May 26, 2021 08:59

This was referenced May 26, 2021

Fix slow cassandra query. #1530

Merged

Fix: Connection type when unblocking after LH #1549

Merged

fisx added a commit that referenced this pull request May 30, 2021

iterateConversations.

258b6b0

(Copied from #1507, which has meanwhile been reverted in #1549.)

fisx added a commit that referenced this pull request May 30, 2021

iterateConversations.

7a86d24

(Copied from #1507, which has meanwhile been reverted in #1549.)

smatting pushed a commit that referenced this pull request May 30, 2021

iterateConversations.

886bb46

(Copied from #1507, which has meanwhile been reverted in #1549.)

fisx added a commit that referenced this pull request May 31, 2021

iterateConversations.

945c34d

(Copied from #1507, which has meanwhile been reverted in #1549.)

smatting pushed a commit that referenced this pull request Jun 9, 2021

iterateConversations.

e9dade4

(Copied from #1507, which has meanwhile been reverted in #1549.)

smatting mentioned this pull request Jun 11, 2021

Handle LH policy conflicts in group conversations #1595

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block and kick users in case of LH no_consent conflict (1:1 convs). #1507

Block and kick users in case of LH no_consent conflict (1:1 convs). #1507

fisx commented May 14, 2021 •

edited

Loading

fisx left a comment

fisx commented May 25, 2021

jschaul May 25, 2021

jschaul May 25, 2021

jschaul May 25, 2021 •

edited

Loading

fisx May 26, 2021

This comment has been minimized.

fisx commented May 26, 2021

Block and kick users in case of LH no_consent conflict (1:1 convs). #1507

Block and kick users in case of LH no_consent conflict (1:1 convs). #1507

Conversation

fisx commented May 14, 2021 • edited Loading

fisx left a comment

Choose a reason for hiding this comment

fisx commented May 25, 2021

jschaul May 25, 2021

Choose a reason for hiding this comment

jschaul May 25, 2021

Choose a reason for hiding this comment

jschaul May 25, 2021 • edited Loading

Choose a reason for hiding this comment

fisx May 26, 2021

Choose a reason for hiding this comment

This comment has been minimized.

fisx commented May 26, 2021

fisx commented May 14, 2021 •

edited

Loading

jschaul May 25, 2021 •

edited

Loading