Skip to content

Change http response code for missing-legalhold-consent. #1688

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

Merged
fisx merged 6 commits into from
Aug 3, 2021
Merged

Change http response code for missing-legalhold-consent. #1688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e49ec82
Change http response code for `missing-legalhold-consent`.
fisx Jul 30, 2021
1cc46f5
CHANGELOG
fisx Jul 30, 2021
eaf7945
Missed a spot...
fisx Aug 2, 2021
83abb9c
Mark error idioms as deprecated.
pcapriotti Aug 3, 2021
1dade31
hi ci
fisx Aug 3, 2021
8f4d1f2
Merge branch 'develop' into SQSERVICES-603-fix-status-codes
fisx Aug 3, 2021
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.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ Upgrade nginz (#1658)
* New, hardened end-point for changing email (68b4db08)
* Fix: CSV export is missing SCIM external id when SAML is also used (#1608)
* Fix: sso_id field in user record (brig) was not always filled correctly in cassandra (#1334)
* Change http response code for `missing-legalhold-consent` from 412 to 403 (#1688)

## Documentation

Expand Down
3 changes: 2 additions & 1 deletion services/brig/src/Brig/API/Error.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,8 +456,9 @@ propertyManagedByScim prop = Wai.mkError status403 "managed-by-scim" $ "Updating
sameBindingTeamUsers :: Wai.Error
sameBindingTeamUsers = Wai.mkError status403 "same-binding-team-users" "Operation not allowed to binding team users."

-- | See 'Galley.API.Error.missingLegalholdConsent'.
missingLegalholdConsent :: Wai.Error
missingLegalholdConsent = Wai.mkError status412 "missing-legalhold-consent" "Failed to connect to a user or to invite a user to a group because somebody is under legalhold and somebody else has not granted consent."
missingLegalholdConsent = Wai.mkError status403 "missing-legalhold-consent" "Failed to connect to a user or to invite a user to a group because somebody is under legalhold and somebody else has not granted consent."

ownerDeletingSelf :: Wai.Error
ownerDeletingSelf =
Expand Down
2 changes: 1 addition & 1 deletion services/brig/src/Brig/IO/Intra.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -898,7 +898,7 @@ guardLegalhold protectee userClients = do
res <- lift $ galleyRequest PUT req
case Bilge.statusCode res of
200 -> pure ()
412 -> throwE ClientMissingLegalholdConsent
403 -> throwE ClientMissingLegalholdConsent
404 -> pure () -- allow for galley not to be ready, so the set of valid deployment orders is non-empty.
_ -> throwM internalServerError
where
Expand Down
11 changes: 10 additions & 1 deletion services/galley/src/Galley/API/Error.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,17 @@ noLegalHoldDeviceAllocated = mkError status404 "legalhold-no-device-allocated" "
legalHoldCouldNotBlockConnections :: Error
legalHoldCouldNotBlockConnections = mkError status500 "legalhold-internal" "legal hold service: could not block connections when resolving policy conflicts."

-- | See 'Brig.API.Error.missingLegalholdConsent'.
--
-- FUTUREWORK: To avoid this duplication, you could turn this into an `ErrorDescription` and
-- move it to the `ErrorDescription` module in wire-api, where it can then be used from both
-- brig and galley. Also, it makes it easier to add it to swagger, if desired, or even later
-- turn it into a statically checked response with `MultiVerb`.
--
-- For examples, see https://github.com/wireapp/wire-server/pull/1657 or
-- https://github.com/wireapp/wire-server/pull/1657/commits/8e73769d7d4a657fef5fbe9210f885f9ccbcaab6.
missingLegalholdConsent :: Error
missingLegalholdConsent = mkError status412 "missing-legalhold-consent" "Failed to connect to a user or to invite a user to a group because somebody is under legalhold and somebody else has not granted consent."
missingLegalholdConsent = mkError status403 "missing-legalhold-consent" "Failed to connect to a user or to invite a user to a group because somebody is under legalhold and somebody else has not granted consent."
Copy link
Contributor

@pcapriotti pcapriotti Jul 30, 2021

Choose a reason for hiding this comment

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

To avoid this duplication, you could turn this into an ErrorDescription and move it to the ErrorDescription module in wire-api, where it can then be used from both brig and galley. Also, it makes it easier to add it to swagger, if desired, or even later turn it into a statically checked response with MultiVerb.

I've done this for a bunch of errors as part of #1657. This is a typical example: 8e73769. It can be a bit of work though, depending on how often the error is thrown, so feel free to ignore this.

Copy link
Contributor Author

@fisx fisx Jul 30, 2021

Choose a reason for hiding this comment

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

thanks, i'll look into it.

Copy link
Contributor Author

@fisx fisx Jul 30, 2021

Choose a reason for hiding this comment

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

(why is nothing ever easy? :-))

Copy link
Contributor Author

@fisx fisx Aug 3, 2021

Choose a reason for hiding this comment

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

I've solved it like this: 83abb9c

I've made you the commit author @pcapriotti, let me know if that's not ok and I'll change it back.

Copy link
Contributor

@pcapriotti pcapriotti Aug 3, 2021

Choose a reason for hiding this comment

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

Sure, that's fine.

Copy link
Contributor

@pcapriotti pcapriotti Aug 3, 2021

Choose a reason for hiding this comment

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

FYI: #1693 contains the change I was suggesting, since it was useful for another refactoring.


disableSsoNotImplemented :: Error
disableSsoNotImplemented =
Expand Down
16 changes: 8 additions & 8 deletions services/galley/test/integration/API/Teams/LegalHold.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,7 @@ testOldClientsBlockDeviceHandshake = do
-- If user has a client without the ClientSupportsLegalholdImplicitConsent
-- capability then message sending is prevented to legalhold devices.
peerClient <- randomClient peer (someLastPrekeys !! 2)
runit peer peerClient >>= errWith 412 (\err -> Error.label err == "missing-legalhold-consent")
runit peer peerClient >>= errWith 403 (\err -> Error.label err == "missing-legalhold-consent")
upgradeClientToLH peer peerClient
runit peer peerClient >>= errWith 412 (\(_ :: Msg.ClientMismatch) -> True)

Expand Down Expand Up @@ -928,8 +928,8 @@ testNoConsentBlockOne2OneConv connectFirst teamPeer approveLH testPendingConnect
const 201 === statusCode
else do
void doEnableLH
postConnection legalholder peer !!! do testResponse 412 (Just "missing-legalhold-consent")
postConnection peer legalholder !!! do testResponse 412 (Just "missing-legalhold-consent")
postConnection legalholder peer !!! do testResponse 403 (Just "missing-legalhold-consent")
postConnection peer legalholder !!! do testResponse 403 (Just "missing-legalhold-consent")

data GroupConvAdmin
= LegalholderIsAdmin
Expand Down Expand Up @@ -1044,7 +1044,7 @@ testGroupConvInvitationHandlesLHConflicts inviteCase = do
assertNotConvMember peer convId
InviteAlsoNonConsenters -> do
API.Util.postMembers userWithConsent (List1.list1 legalholder [peer2]) convId
>>= errWith 412 (\err -> Error.label err == "missing-legalhold-consent")
>>= errWith 403 (\err -> Error.label err == "missing-legalhold-consent")

testNoConsentCannotBeInvited :: HasCallStack => TestM ()
testNoConsentCannotBeInvited = do
Expand Down Expand Up @@ -1081,11 +1081,11 @@ testNoConsentCannotBeInvited = do
liftIO $ assertEqual "approving should change status" UserLegalHoldEnabled userStatus

API.Util.postMembers userLHNotActivated (List1.list1 peer2 []) convId
>>= errWith 412 (\err -> Error.label err == "missing-legalhold-consent")
>>= errWith 403 (\err -> Error.label err == "missing-legalhold-consent")

localdomain <- viewFederationDomain
API.Util.postQualifiedMembers userLHNotActivated ((Qualified peer2 localdomain) :| []) convId
>>= errWith 412 (\err -> Error.label err == "missing-legalhold-consent")
>>= errWith 403 (\err -> Error.label err == "missing-legalhold-consent")

testCannotCreateGroupWithUsersInConflict :: HasCallStack => TestM ()
testCannotCreateGroupWithUsersInConflict = do
Expand Down Expand Up @@ -1120,7 +1120,7 @@ testCannotCreateGroupWithUsersInConflict = do
liftIO $ assertEqual "approving should change status" UserLegalHoldEnabled userStatus

createTeamConvAccessRaw userLHNotActivated tid [peer2, legalholder] (Just "corp + us") Nothing Nothing Nothing (Just roleNameWireMember)
>>= errWith 412 (\err -> Error.label err == "missing-legalhold-consent")
>>= errWith 403 (\err -> Error.label err == "missing-legalhold-consent")

data TestClaimKeys
= TCKConsentMissing
Expand Down Expand Up @@ -1169,7 +1169,7 @@ testClaimKeys testcase = do
TCKConsentAndNewClients -> good
where
good = testResponse 200 Nothing
bad = testResponse 412 (Just "missing-legalhold-consent")
bad = testResponse 403 (Just "missing-legalhold-consent")

let fetchKeys :: ClientId -> TestM ()
fetchKeys legalholderLHDevice = do
Expand Down
4 changes: 2 additions & 2 deletions services/galley/test/integration/API/Teams/LegalHold/DisabledByDefault.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,7 +752,7 @@ testOldClientsBlockDeviceHandshake = do
-- If user has a client without the ClientSupportsLegalholdImplicitConsent
-- capability then message sending is prevented to legalhold devices.
peerClient <- randomClient peer (someLastPrekeys !! 2)
runit peer peerClient >>= errWith 412 (\err -> Error.label err == "missing-legalhold-consent")
runit peer peerClient >>= errWith 403 (\err -> Error.label err == "missing-legalhold-consent")
upgradeClientToLH peer peerClient
runit peer peerClient >>= errWith 412 (\(_ :: Msg.ClientMismatch) -> True)

Expand Down Expand Up @@ -803,7 +803,7 @@ testClaimKeys testcase = do
TCKConsentAndNewClients -> good
where
good = testResponse 200 Nothing
bad = testResponse 412 (Just "missing-legalhold-consent")
bad = testResponse 403 (Just "missing-legalhold-consent")

let fetchKeys :: ClientId -> TestM ()
fetchKeys legalholderLHDevice = do
Expand Down