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

Legalhold: dynamic whitelisted teams & whitelist-teams-and-implicit-consent feature in tests #1557

Merged
merged 75 commits into from
Jun 2, 2021

Conversation

smatting
Copy link
Contributor

@smatting smatting commented May 31, 2021

Changes to production code

This PR removes the legalHoldTeamsWhitelist galley config item and introduces a new internal endpoint /i/legalhold/whitelisted-teams to manage the set of whitelisted teams (the underlying table is galley.legalhold_whitelisted). Since the whitelisted teams are needed on every team member lookup this list is cached: at the beginning of every request the list is fetched and stored in an ioref in galley's environment.

The API is

  • PUT /i/legalhold/whitelisted-teams/{tid} to whitelist a team
  • GET /i/legalhold/whitelisted-teams which returns a list of team uuids as a JSON array
  • There is no way to remove team from the whitelist.

Changes to integration tests (CI + local development)

This PR updates the galley configuration for 1) CI and and for 2) local development:

legalhold: whitelist-teams-and-implicit-consent (was legalhold: disabled-by-default before)

All legalhold integration tests written for the disabled-by-default feature flag are moved to module LegalHold.DisabledByDefault (was LegalHold before). The tests configured to run only when legalhold: disabled-by-default is set, otherwise they pass without running.

All legalhold integration tests in the LegalHold module are updated so that they specify the behaviour under the whitelist-teams-and-implicit-consent feature flag. The tests configured to run only when legalhold: whitelist-teams-and-implicit-consent is set, otherwise they pass without running.

This PR also adapts some of brig's integration tests to work under the assumption that legalhold: whitelist-teams-and-implicit-consent is set.

Notes

Note: Running galley with legalhold: whitelist-teams-and-implicit-consent configuration changes the error codes of some endpoints, namely:

DELETE /teams/{tid}/legalhold/setttings

  • this always returns 403 with label legalhold-disable-unimplemented.
    It's not possible to deactivate legalhold for a team.

POST /teams/{tid}/legalhold/setttings

  • returns 412 with label legalhold-unavailable when team is not whitelisted
    (is 403 legalhold-not-enabled when lh is not enabled for the team
    with legalhold: disabled-by-default galley config
    )

POST /teams/{tid}/legalhold/{uid}

  • returns 403 with label legalhold-not-enabled when user has not given consent
    (is 403 with label operation-denied
    or 409 with label legalhold-no-consent
    with legalhold: disabled-by-default galley config)

PUT /teams/{tid}/legalhold/{uid}/approve

  • returns 403 with label access-denied when user has not given consent
    (is 403 legalhold-not-enabled with label
    with legalhold: disabled-by-default galley config)

These API differences are not introduced by this PR.

@smatting smatting force-pushed the SQSERVICES-490-dynamic-whitelisting branch from d191c64 to b1c9d03 Compare May 31, 2021 17:32
fisx added 15 commits May 31, 2021 20:45
The default for this should actually be "permanently disabled", but
either way that's an unrelated change.
This function was fine before this branch.  Enabeling LH for team is
in normal operation (disabled-by-default) what whitelisting is in
implicit-consent mode.  The two are mutually exclusive.
The end-point already is internal, and has to be by design.  It may be
easier for the operator to be able to update the table *before*
turning it on; I certainly can't think of a reason why it they should
be forbidden to do it.
@fisx fisx changed the title Legalhold: dynamic whitelisted teams [skip ci] Legalhold: dynamic whitelisted teams May 31, 2021
@fisx fisx marked this pull request as ready for review May 31, 2021 21:28
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

I've deactivated the test suite so you can deploy this on staging to make some progress with QA and the team.

Ideas for getting the test suite back on track:

  • Don't use deleteLHWhitelistTeam, but deleteTeam. The latter is implicit in withTeam. (See my commits and the comments that come with them.)
  • Don't remove grantConsent*, but check the galley server config. If LH is disabled-by-default, use grantConsent* (those tests did work until before this PR); if it is whitelist-implicit-consent, use putLHWhitelistTeam. Remember to use withTeam if you do that.
  • Where the behavior differs, branch on feature flag again. Again, the old behavior should work already, the new may need adjusting.

I am still hoping all of the necessary changes will be quite local, but I can't see that very clearly yet.

@smatting smatting marked this pull request as draft June 1, 2021 08:46
@smatting smatting marked this pull request as ready for review June 1, 2021 16:36
@smatting smatting changed the title Legalhold: dynamic whitelisted teams Legalhold: dynamic whitelisted teams & whitelist-teams-and-implicit-consent feature in test Jun 1, 2021
@smatting smatting changed the title Legalhold: dynamic whitelisted teams & whitelist-teams-and-implicit-consent feature in test Legalhold: dynamic whitelisted teams & whitelist-teams-and-implicit-consent feature in tests Jun 1, 2021
smatting and others added 9 commits June 1, 2021 18:57
These should still pass if galley.integration.yaml sets legalhold to
`disabled-by-default` again.
underscoring names that we want to keep is still a bit rancid, but at
least this way we'll get warnings about new things we introduce
accidentally.
@fisx
Copy link
Contributor

fisx commented Jun 1, 2021

I sometimes get the tests to pass now locally, sometimes this happens:

     handshake between LH device and user with old clients is blocked:                                   FAIL                                                                                                                                                         
        Exception: HttpExceptionRequest Request {                                                                                                                                                                                                                      
          host                 = "127.0.0.1"                                                                                                                                                           
          port                 = 8085                                                                                                                                                                  
          secure               = False                                                                                                                                                                                                                                 
          requestHeaders       = [("Content-Type","application/json")]                                                                                                                                 
          path                 = "i/teams/54dcbff8-73ba-496e-9740-b523c24f9268/features/legalhold"                                                                                                     
          queryString          = ""                                                                                                                                                                                                                                    
          method               = "PUT"                                                                                                                                                                                                                                           proxy                = Nothing                                                                                                                                                                                                                               
          rawBody              = False                                                                                                                                                                                                                                 
          redirectCount        = 10                                                                                                                                                                    
          responseTimeout      = ResponseTimeoutDefault                                                                                                                                                
          requestVersion       = HTTP/1.1                                                                                                                                                 
        }                                                                                                                                                                                                                                                              
         (StatusCodeException (Response {responseStatus = Status {statusCode = 403, statusMessage = "Forbidden"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Tue, 01 Jun 2021 18:54:31 GMT"),("Server","Warp/3.3.13"),("Co
ntent-Type","application/json")], responseBody = (), responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}) "{\"code\":403,\"message\":\"legal hold is enabled for teams via server config and cannot be changed here\",\"label\":\"legalhold-whitelist
ed-only\"}")                                                                                                                                              
      User cannot fetch prekeys of LH users if consent is missing:                                        [brig] W, logger=cassandra.brig, Server warning: Read 0 live rows and 1347 tombstone cells for query SELECT * FROM brig_test.users_pending_activation WHERE  
LIMIT 10000 (see tombstone_warn_threshold)                                                                                                                                                                                                                             
FAIL                                                                                                                                                                                                                                                                   
        Exception: HttpExceptionRequest Request {                                                                                                                                                      
          host                 = "127.0.0.1"                                                                                                                                                           
          port                 = 8085                                                                                                                                                     
          secure               = False                                                                                                                                                                                                                                 
          requestHeaders       = [("Content-Type","application/json")]                                                                                                                                 
          path                 = "i/teams/6974b45f-6e86-4a90-992d-9d4aad5e5e3e/features/legalhold"                                                                                                     
          queryString          = ""                                                                                                                                                                                                                                    
          method               = "PUT"
          proxy                = Nothing
          rawBody              = False
          redirectCount        = 10
          responseTimeout      = ResponseTimeoutDefault
          requestVersion       = HTTP/1.1
        }
         (StatusCodeException (Response {responseStatus = Status {statusCode = 403, statusMessage = "Forbidden"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Tue, 01 Jun 2021 18:54:32 GMT"),("Server","Warp/3.3.13"),("Co
ntent-Type","application/json")], responseBody = (), responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}) "{\"code\":403,\"message\":\"legal hold is enabled for teams via server config and cannot be changed here\",\"label\":\"legalhold-whitelist
ed-only\"}")
      User cannot fetch prekeys of LH users: if user has old client:                                      FAIL
        Exception: HttpExceptionRequest Request {
          host                 = "127.0.0.1"
          port                 = 8085
          secure               = False
          requestHeaders       = [("Content-Type","application/json")]
          path                 = "i/teams/63b26d54-fb75-4045-aeed-4197faf80711/features/legalhold"
          queryString          = ""
          method               = "PUT"
          proxy                = Nothing
          rawBody              = False
          redirectCount        = 10
          responseTimeout      = ResponseTimeoutDefault
          requestVersion       = HTTP/1.1
        }
         (StatusCodeException (Response {responseStatus = Status {statusCode = 403, statusMessage = "Forbidden"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Tue, 01 Jun 2021 18:54:32 GMT"),("Server","Warp/3.3.13"),("Co
ntent-Type","application/json")], responseBody = (), responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}) "{\"code\":403,\"message\":\"legal hold is enabled for teams via server config and cannot be changed here\",\"label\":\"legalhold-whitelist
ed-only\"}")
      User can fetch prekeys of LH users if consent is given and user has only new clients:               FAIL
        Exception: HttpExceptionRequest Request {
          host                 = "127.0.0.1"
          port                 = 8085
          secure               = False
          requestHeaders       = [("Content-Type","application/json")]
          path                 = "i/teams/766c308c-faac-421c-bdff-1c3345239c28/features/legalhold"
          queryString          = ""
          method               = "PUT"
          proxy                = Nothing
          rawBody              = False
          redirectCount        = 10
          responseTimeout      = ResponseTimeoutDefault
          requestVersion       = HTTP/1.1
        }
         (StatusCodeException (Response {responseStatus = Status {statusCode = 403, statusMessage = "Forbidden"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Tue, 01 Jun 2021 18:54:32 GMT"),("Server","Warp/3.3.13"),("Co
ntent-Type","application/json")], responseBody = (), responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}) "{\"code\":403,\"message\":\"legal hold is enabled for teams via server config and cannot be changed here\",\"label\":\"legalhold-whitelist
ed-only\"}")

I'd try to get the call stack next. (The way we create this error should be banned, it's been costing me so much time! Or I should just figure out which one it is, and avoid it.)

charts/nginz/values.yaml Outdated Show resolved Hide resolved
@smatting smatting merged commit aa6d9ee into develop Jun 2, 2021
@smatting smatting deleted the SQSERVICES-490-dynamic-whitelisting branch June 2, 2021 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants