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

Allow Legalhold for large teams (>2000) #1546

Merged
merged 9 commits into from
May 29, 2021
Merged

Allow Legalhold for large teams (>2000) #1546

merged 9 commits into from
May 29, 2021

Conversation

smatting
Copy link
Contributor

@smatting smatting commented May 28, 2021 •
edited
Loading

@smatting smatting force-pushed the SQSERVICES-410-lh-large-teams branch 2 times, most recently from 14e3efb to fc77d89 Compare May 28, 2021 14:02
@smatting smatting force-pushed the SQSERVICES-410-lh-large-teams branch from fc77d89 to 49bdb5d Compare May 28, 2021 14:22
@smatting smatting changed the title Allow Legalhold for large teams (>2000) [skip ci] Allow Legalhold for large teams (>2000) May 28, 2021
@smatting smatting force-pushed the SQSERVICES-410-lh-large-teams branch from 49bdb5d to 6b23e7a Compare May 28, 2021 14:22
@fisx
Copy link
Contributor

fisx commented May 28, 2021 

      GET, PUT [/i]?/teams/{tid}/legalhold:                                                                                                     OK (5.94s)
      GET, PUT [/i]?/teams/{tid}/legalhold - too large:                                                                                         FAIL
        Exception: Assertions failed:
         1: 403 =/= 200
         2: Just "too-large-team-for-legalhold" =/= Nothing
        
        Response was:
        
        Response {responseStatus = Status {statusCode = 200, statusMessage = "OK"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Fri, 28 May 2021 15:14:20 GMT"),("Server","Warp/3.3.13"),("Content-Encoding","gzip"),("Content-Type","application/json")], responseBody = Just "{\"status\":\"enabled\"}", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}
        CallStack (from HasCallStack):
          error, called at src/Bilge/Assert.hs:88:5 in bilge-0.22.0-LBx2CWuGwBiGpt6gnLBlsx:Bilge.Assert
          <!!, called at src/Bilge/Assert.hs:106:19 in bilge-0.22.0-LBx2CWuGwBiGpt6gnLBlsx:Bilge.Assert
          !!!, called at test/integration/API/Teams/LegalHold.hs:651:3 in main:API.Teams.LegalHold
      POST /clients:                                                                                                                            OK (0.56s)

@smatting smatting marked this pull request as ready for review May 29, 2021 13:11
fisx
fisx approved these changes May 29, 2021
View reviewed changes
services/galley/src/Galley/API/LegalHold.hs
@@ -142,6 +143,14 @@ removeSettings zusr tid (Public.RemoveLegalHoldSettingsRequest mPassword) = do
void $ permissionCheck ChangeLegalHoldTeamSettings zusrMembership
ensureReAuthorised zusr mPassword
removeSettings' tid
where
assertNotWhitelisting :: Galley ()
Copy link
Contributor

Choose a reason for hiding this comment

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

I want to rename this to assertNoSettingsWhitelist, but I'll try to control myself.

services/galley/test/integration/API/Teams/LegalHold.hs
@@ -18,6 +17,7 @@
--
-- 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/>.
{-# OPTIONS_GHC -Wno-unused-top-binds #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

We can avoid this by wrapping the test lines 156, 157 below into when False rather than removing them completely. But not now.

services/galley/test/integration/API/Teams/LegalHold.hs
\(the 'withLHWhitelist' trick does not work because it does not allow \
\brig to talk to the dynamically spawned galley)."
FeatureLegalHoldDisabledByDefault ->
liftIO $
Copy link
Contributor

Choose a reason for hiding this comment

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

deduplicate this block. (not now)

services/galley/test/integration/API/Teams/LegalHold.hs
(tid, _owner, _others) <- createBindingTeamWithMembers (fanoutLimit + 1)
-- TODO: it is impossible in this test to create teams bigger than the fanout limit.
-- Change the +1 to anything else and look at the logs
(tid, _owner, _others) <- createBindingTeamWithMembers (fanoutLimit + 5)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this TODO. Or why you changed the +1 to +5.

services/galley/test/integration/API/Teams/LegalHold.hs
@@ -667,6 +691,44 @@ testAddTeamUserTooLargeWithLegalhold = do
const 403 === statusCode
const (Just "too-many-members-for-legalhold") === fmap Error.label . responseJsonMaybe

-- | This test only works if you manually adjust the server config files (the 'withLHWhitelist' trick does not work because it does not allow brig to talk to the dynamically spawned galley).
Copy link
Contributor

Choose a reason for hiding this comment

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

The first line of this haddocks is redundant.

fisx
fisx reviewed May 29, 2021
View reviewed changes
services/galley/src/Galley/API/Teams.hs
Comment on lines +885 to +889
limit <- fromIntegral . fromRange <$> fanoutLimit
let withinLimit = teamSize <= limit
view (options . Opts.optSettings . Opts.setFeatureFlags . flagLegalHold) >>= \case
FeatureLegalHoldDisabledPermanently -> pure withinLimit
FeatureLegalHoldDisabledByDefault -> pure withinLimit
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
limit <- fromIntegral . fromRange <$> fanoutLimit
let withinLimit = teamSize <= limit
view (options . Opts.optSettings . Opts.setFeatureFlags . flagLegalHold) >>= \case
FeatureLegalHoldDisabledPermanently -> pure withinLimit
FeatureLegalHoldDisabledByDefault -> pure withinLimit
let withinLimit = (teamSize <=) . fromIntegral . fromRange <$> fanoutLimit
view (options . Opts.optSettings . Opts.setFeatureFlags . flagLegalHold) >>= \case
FeatureLegalHoldDisabledPermanently -> withinLimit
FeatureLegalHoldDisabledByDefault -> withinLimit

@fisx fisx merged commit 26e3392 into develop May 29, 2021
@fisx fisx deleted the SQSERVICES-410-lh-large-teams branch May 29, 2021 18:44
@fisx fisx mentioned this pull request May 31, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fisx fisx fisx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@smatting @fisx