Feature/legalhold tokens #761

jschaul · 2019-05-20T15:34:25Z

(see also note in 2483ac1)

Adds new token types to:

zauth
libzauth
nginx-auth-module

token generation and storage in brig

implement legal hold user/access token login /i/legalhold/login
implement legal hold token refresh /legalhold/access
throw error in the edge case where /i/legalhold/login is used to create a session legal hold token (not a possible code path in the current implementation but possible type wise - see TODO undefined in current changeset
integration test in legal hold tests: legal hold login leads to a lu cookie and a la access token.
integration tests: legal hold login cookie can be refreshed and revoked (many of the same tests as done here
make use of /i/legalhold-login (instead of current /i/sso-login)

end-to-end test using ACL, nginz route configuration

adjust deploy/services-demo/conf/nginz/zauth_acl.txt (as well as the other 2 configuration places) to allow the routes needed by the legal hold service
manual or integration test to check the change in nginx-auth-module is working (So far there are no tests testing nginz - except the smoketests) : 1) creating a token by talking to brig directly under /i/legalhold/login and 2) making a request to nginz using that token on a route allowed by the zauth_acl and making sure nginz is allowing a valid legalhold token through.
Pick one of two options to deal with token refresh endpoint:

~~keep /access and /legalhold/access separate (in which case /legalhold/access needs to be mapped to brig in the nginx configurations (in all 3 places)~~
merge /access and /legalhold/access endpoints (in which case nginz config doesn't need to be adjusted - the nginz config change in this PR can be undone), instead the types on that endpoint need to probably be Either UserToken LegalHoldUserToken (see e.g. https://github.com/wireapp/wire-server/blob/develop/services/brig/src/Brig/API.hs#L1216-L1217 an example Either endpoint).

fix galley tests with the new requirement that legalhold users = team users
logout & cookie cleanup test

Finally, after merging and deploying, ensure legal hold service can reach all its necessary routes (make sure all nginx configs are adjusted and possibly /access to changed to /legalhold/access in the legal hold service (depending on the choice in the task above)

…so work

fisx

I took a quick look. It's a lot of boilerplate, but simple (which is good) and sound (also good) as far as I can tell.

fisx · 2019-05-21T09:31:39Z

libs/zauth/src/Data/ZAuth/Token.hs

+-- B = Bot
+-- P = Provider
+-- LA = LegalHold Access (Used as short-lived token for LegalHold Service)
+-- LU = LegalHold User (Used as a cookie for LegalHold Service to refresh access tokens)


now i'm wondering if LD (legalhold device) isn't a better name. less symmetrical, but more accurate. no strong opinion, though.

you could also move the list from this comment into the declaration:

data Type = A --^ Access (...) | B --^ ... ...

you could also move ...

thanks!

now i'm wondering if LD (legalhold device) ...

I prefer the symmetrical name, I think.

The problem was that there was nothing in the context of 'newAccessToken' that would tell the type checker what the type of (the type variables in the type of) 'Nothing' should be.

fisx

(more comments coming up...)

services/brig/src/Brig/User/API/Auth.hs

fisx

looks good! done with everything but the tests. i'll wait for you to confirm they are done, then i'll review them.

services/brig/src/Brig/User/Auth.hs

services/brig/src/Brig/API.hs

Co-Authored-By: fisx <mf@zerobuzz.net>

(also allow passing, and check the password)

fisx · 2019-08-30T11:11:47Z

services/brig/test/integration/API/User/Auth.hs

+        const 403 === statusCode
+
+    putLegalHoldEnabled tid LegalHoldEnabled galley -- enable it for this team
+    _rs <- legalHoldLogin brig (LegalHoldLogin alice (Just defPassword) Nothing) PersistentCookie


Fail again here at the moment LH is enabled for team, but not for member?

That should be tested as part of galley's legal hold tests (if it's not, it should get added there, but it's not related to this PR)

fisx · 2019-08-30T11:16:26Z

services/brig/test/integration/API/User/Auth.hs

+        t = decodeToken rs
+    -- ensure nginz allows refresh at /access
+    _rs <- post (n . path "/access" . cookie c . header "Authorization" ("Bearer " <> (toByteString' t))) <!! do
+        const 200 === statusCode


I think this is the line that the comment in 114 refers to. You could move the comment here (not tested):

Suggested change

const 200 === statusCode

assertEqual "/access (requires that private key on nginz and public key on brig match)"

(const 200) statusCode

The Bilge.assert and the HUnit.assert don't work well together, so the above doesn't compile, but I can do this:

_rs <- get (n . path "/clients" . header "Authorization" ("Bearer " <> (toByteString' t))) liftIO $ assertEqual "Ensure nginz is started. Ensure nginz and brig share the same private/public zauth keys. Ensure ACL file is correct." 200 (statusCode _rs)

Oops, I thought there was an assertEqual in hunit and one in bilge. You could you https://hoogle.zinfra.io/file/wire-server/.stack-work/install/x86_64-linux/custom-wire-1.1-G6NVtOVxqIB2/8.4.3/doc/bilge-0.22.0/Bilge-Assert.html#v:assertTrue, though?

services/brig/test/integration/API/User/Auth.hs

* Add json logging and logging (#828) (#836) * Add logs for team operations * Add logging for legalhold * Json logging (#836) * Allow to set log format The old netstrings option is now deprecated, use logFormat instead. logFormat takes presedence of netstrings if both are set. If none are set, log format defaults to Plain * Implement json renderer for tinylog * Specialize the error cases on conversation lookup. (#841) Getting a conversation you left will now give 403 access-denied, not 404 not-found. * Feature Flags in galley options. (#825) * Feature flags in galley options file. * Honour sso feature flag. * Honour legalhold feature flag (test). * Honour legalhold feature flag (handler). * Disable all LH tests if feature is disabled. * Check options settings for feature flag. * Avoid catch-all pattern. Catch-all patterns are not future-proof if constructors for new falvors of "disabled" are added.

This reverts commit b01fd8c.

services/brig/src/Brig/IO/Intra.hs

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

Conflicts: services/brig/src/Brig/User/Auth.hs services/brig/src/Brig/User/Auth/Cookie.hs services/brig/test/integration/API/User/Auth.hs

fisx

Some remaining open questions (besides the new comments in the code), for our call:

I would like to be walked through the rust code. The changes look sound, but I'm not very confident I haven't overlooked anything in my review.

I would also like to go through how disabeling LH on a per-team or per-instance level affects teams that already have LH devices running.

Do these need to be resolved?:

git grep -Hn -B2 -A2 TODO services/galley/src/Galley/API/LegalHold.hs
services/galley/src/Galley/API/LegalHold.hs-199-    LHService.confirmLegalHold clientId tid uid legalHoldAuthToken
services/galley/src/Galley/API/LegalHold.hs-200-    LegalHoldData.setUserLegalHoldStatus tid uid UserLegalHoldEnabled
services/galley/src/Galley/API/LegalHold.hs:201:    -- TODO: send event at this point (see also:
services/galley/src/Galley/API/LegalHold.hs-202-    -- https://github.com/wireapp/wire-server/pull/802#pullrequestreview-262280386)
services/galley/src/Galley/API/LegalHold.hs-203-    pure empty
--
services/galley/src/Galley/API/LegalHold.hs-238-        LHService.removeLegalHold tid uid
services/galley/src/Galley/API/LegalHold.hs-239-        LegalHoldData.setUserLegalHoldStatus tid uid UserLegalHoldDisabled
services/galley/src/Galley/API/LegalHold.hs:240:        -- TODO: send event at this point (see also: related TODO in this module in
services/galley/src/Galley/API/LegalHold.hs-241-        -- 'approveDevice' and
services/galley/src/Galley/API/LegalHold.hs-242-        -- https://github.com/wireapp/wire-server/pull/802#pullrequestreview-262280386)

fisx · 2019-09-05T09:01:55Z

services/brig/test/integration/API/User/Auth.hs

+    let mtoken = (fromByteString (cookie_value ck)) :: Maybe (ZAuth.Token u)
+    assertBool "cookie token exists" (isJust mtoken)
+    let (Just token) = mtoken
+    assertBool "cookie token has correct type" ((token^.ZAuth.header.ZAuth.typ) == ZAuth.zauthType @u)


very nit-picky stuff, leading to a question that maybe genuinely interesting in the end:

I feel strongly we should require operators be surrounded spaces in our coding style. This is a good case to motivate this rule: the double-use of . for module qualification and function composition is very hard to read.

The error message from pattern match failure in line 875 is just as good as the one from the assertion in 874, so this code can be simplified.

I personally like type information best where a name is bound (ie., introduced).

Too many parens. :)

So this could be rewritten:

let Just (token :: ZAuth.Token u) = fromByteString (cookie_value ck) tokentype = ZAuth.zauthType @u assertBool "cookie token has correct type" $ token ^. ZAuth.header . ZAuth.typ == tokentype

... and then I still have one more question: Can token really be parsed if it fails the subsequent assertion? Should it?

fisx · 2019-09-05T09:13:43Z

services/galley/src/Galley/Intra/Client.hs

@@ -52,16 +53,16 @@ notifyClientsAboutLegalHoldRequest requesterUid targetUid lastPrekey' = do
                . json (LegalHoldClientRequest requesterUid lastPrekey')
                . expect2xx

-getLegalHoldAuthToken :: UserId -> Galley OpaqueAuthToken
-getLegalHoldAuthToken uid = do
+getLegalHoldAuthToken :: UserId -> Maybe PlainTextPassword -> Galley OpaqueAuthToken


I am right that this is because some team members may have a password, and of course we want that password to be provided as an extra safety hook if a user approves a LH device, and we just forgot about that in the earlier part of the implementation?

fisx

after a last round of pairing over the code, we agreed that it's done! (the remaining open questions are unrelated to this PR.)

jschaul added 7 commits May 20, 2019 15:14

[zauth] New Token types

a462c38

creation helpers

b4a3788

[libzauth] new token types

f265dd0

[libzauth-c] token types

118dc92

[nginx-module-zauth] new token types

a6f4036

[libzauth] ACL - test path names with more than a single character al…

d6055ee

…so work

nginz routes WIP

babb385

fisx reviewed May 21, 2019

View reviewed changes

jschaul added 7 commits May 27, 2019 11:10

PR feedback

de4deee

[brig] more boilerplate

deb9ad6

WIP

f6c9d88

WIP undefined

70024ce

type class for UserToken ?

8a6d3da

make it compile

5a6740b

stub internal endpoint

cc476c5

fisx mentioned this pull request May 31, 2019

Approve LH device for user #768

Merged

8 tasks

jschaul added 8 commits July 8, 2019 16:29

default token timeout config values for LH

927f37d

WIP typeclass

23d2bfa

WIP typeclass 2

a2fdb34

WIP...

5a66a0c

compiles

2b17ef1

add timeout to typeclass

a32499c

rename

22e4383

add unrelated files to gitignore

ee96844

jschaul force-pushed the feature/legalhold-tokens branch from 4cf7ac9 to dc50482 Compare July 10, 2019 17:38

typeclassify more functions

b069d27

jschaul force-pushed the feature/legalhold-tokens branch from dc50482 to b069d27 Compare July 10, 2019 18:34

jschaul and others added 3 commits July 10, 2019 20:35

ambiguousness

60ebed5

Fix type error.

7f3e42c

The problem was that there was nothing in the context of 'newAccessToken' that would tell the type checker what the type of (the type variables in the type of) 'Nothing' should be.

add TODOs, start looking at integration test compilation issues...

6e858f2

jschaul added 4 commits August 28, 2019 20:38

PR feedback: Simplify Error Types

ef2a57a

missing import

24c0900

Merge branch 'develop' into feature/legalhold-tokens

fcec60a

add FUTUREWORK comment

ec2fd89

fisx reviewed Aug 29, 2019

View reviewed changes

services/brig/src/Brig/User/API/Auth.hs Show resolved Hide resolved

services/brig/src/Brig/User/API/Auth.hs Show resolved Hide resolved

fisx reviewed Aug 29, 2019

View reviewed changes

services/brig/src/Brig/User/Auth.hs Outdated Show resolved Hide resolved

services/brig/src/Brig/User/Auth.hs Outdated Show resolved Hide resolved

services/brig/src/Brig/User/Auth.hs Outdated Show resolved Hide resolved

services/brig/src/Brig/API.hs Outdated Show resolved Hide resolved

jschaul and others added 6 commits August 29, 2019 19:25

Update services/brig/src/Brig/User/Auth.hs

209e36f

Co-Authored-By: fisx <mf@zerobuzz.net>

comment

4660dfa

reauthenticate when creating legalHold Login

606d249

(also allow passing, and check the password)

brig integration tests now always depend on nginz

f1260f5

test legalhold login requires a password

94533cb

Add logout test

2edf37b

fisx reviewed Aug 30, 2019

View reviewed changes

services/brig/test/integration/API/User/Auth.hs Show resolved Hide resolved

jschaul and others added 6 commits August 30, 2019 16:09

PR feedback: comments

290528c

fix galley tests

0f19035

debug...

b01fd8c

use internal API to avoid unnecessary permission checks; fix galley test

5f46f6e

Revert "debug..."

172d6db

This reverts commit b01fd8c.

fisx reviewed Sep 3, 2019

View reviewed changes

services/brig/src/Brig/IO/Intra.hs Show resolved Hide resolved

services/galley/test/integration/API/Teams/LegalHold.hs Outdated Show resolved Hide resolved

fisx and others added 2 commits September 3, 2019 10:06

Merge branch 'develop' into feature/legalhold-tokens

85a2f80

Conflicts: services/brig/src/Brig/User/Auth.hs services/brig/src/Brig/User/Auth/Cookie.hs services/brig/test/integration/API/User/Auth.hs

Hi CI

acb9913

fisx reviewed Sep 5, 2019

View reviewed changes

fisx approved these changes Sep 5, 2019

View reviewed changes

PR feedback, add FUTUREWORK comment

eac6a4b

jschaul merged commit fdda052 into develop Sep 5, 2019

jschaul deleted the feature/legalhold-tokens branch September 5, 2019 17:18

This was referenced Sep 11, 2019

Change feature flags from boolean to custom enum types. #850

Merged

Release 2019 09 16 #858

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/legalhold tokens #761

Feature/legalhold tokens #761

jschaul commented May 20, 2019 •

edited

Loading

fisx left a comment

fisx May 21, 2019

fisx May 21, 2019

jschaul May 27, 2019

fisx left a comment

fisx left a comment

fisx Aug 30, 2019

jschaul Aug 30, 2019

fisx Aug 30, 2019

jschaul Aug 30, 2019

fisx Aug 30, 2019

fisx left a comment

fisx Sep 5, 2019

fisx Sep 5, 2019

fisx left a comment

	const 200 === statusCode
	assertEqual "/access (requires that private key on nginz and public key on brig match)"
	(const 200) statusCode

Feature/legalhold tokens #761

Feature/legalhold tokens #761

Conversation

jschaul commented May 20, 2019 • edited Loading

fisx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisx left a comment

Choose a reason for hiding this comment

fisx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisx left a comment

Choose a reason for hiding this comment

jschaul commented May 20, 2019 •

edited

Loading