Allow choosing a role when creating a team member; fix role change events. #573

fisx · 2019-01-16T18:01:08Z

For a summary, read this commit-by-commit, starting with the commit messages.

- add optional `Role` field to `InvitationRequest`. - add 'Permissions' field to `Invitation` (schema change in brig-cassandra). - default to `RoleMember` if no role / permissions are given. - move `instance Cql Permissions` to galley-types.

fisx · 2019-01-16T21:44:21Z

I get this when I run integration tests locally:

Brig API Integration
  user
    account
      put /self - 200:                             FAIL (0.74s)
        test/integration/API/User/Account.hs:494:
        User not in results for query: "dogbert"

Interestingly, I also get this error on develop. I have no idea how it can be related to this PR. Will investigate tomorrow.

If CI passes here, consider this my private problem.

fisx · 2019-01-17T05:24:05Z

ya, private problem... perhaps if you run the tests often enough on the same state, it somehow goes bad. not sure it's worth exploring, or how to explore it? if we can't think of anything i'll just restart docker-ephemeral, and expect this to go away.

This reverts commit a414f6e.

This reverts commit 9faf37f.

fisx · 2019-01-17T10:10:27Z

@jschaul You can probably skip reading the two commits from last night that their reverts. I left that in for my own sanity and because non-destructive updates are nice.

fisx · 2019-01-17T10:12:47Z

(background: clients need a way to get the list of all team members so they know who they can talk to, and they want to be able to extract permissions from that end-point where accessible to avoid extra communication rounds, so my earlier attempt was too simple.)

(oh, and i will also push a small swagger update in a minute.)

jschaul · 2019-01-17T11:37:22Z

libs/galley-types/src/Galley/Types/Teams.hs

        ]
+
+#ifdef WITH_CQL
+instance Cql.Cql Permissions where


What's your rationale for moving the CQL instances to the galley-types package? Since brig is referring to permissions directly now (so it's used by brig and galley), wouldn't types-common be a better place to move it to? (it also already has a dependency/feature flag on cql).

not sure which place makes most sense given the related types, but sure, i can move it. will do!

what module should i move this to, then? i can't think of any way to move it that i would prefer over the current code here, but i'm happy to implement a concrete suggestion.

Data.Misc ? Or Data.<some new module>.

I decided I'm opposed to this change for now. It becomes more and more obvious that brig and galley aren't really two different services and should be merged. Until we solve that issue, I want to keep the galley types in their proper place, and not spread them out over Data.Galley and Galley.Types.Team.

Following the above logic of service types dependencies, can we put this (Permissions and Role) under Data.Role on types-common? Even if galley and brig do get merged, at least until then let's keep things consistent;

Note that the whole invitation should and could be part of galley - the reason we decided against it was because all the email logic/templating/etc. was part of brig

jschaul · 2019-01-17T14:00:46Z

libs/galley-types/src/Galley/Types/Teams/Swagger.hs

        description "The permissions this user has in the given team \
            \ (only visible with permission `GetMemberPermissions`)."
+        optional  -- not optional in the type, but in the json instance.  (in
+                  -- servant, we could probably just add a helper type for this.)


This is not very nice. For other cases where the API-exposed json doesn't match the database-stored type, we created two separate data types that reflect that: the outside TeamMember data type could have an optional permission field, whereas the inside TeamMember doesn't.

that's not something i broke in this PR, though. it's always been not very nice, i just fixed the swagger. i it's ok with you, i'll leave a TODO.

jschaul · 2019-01-17T14:05:36Z

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

            throwE $ DuplicateUserKey uk
-        let minvmeta :: Maybe (UserId, UTCTimeMillis)
-            minvmeta = (, inCreatedAt inv) <$> inCreatedBy inv
+        let minvmeta :: (Maybe (UserId, UTCTimeMillis), Team.Permissions)


why is userId and time optional, while permissions are not here? That's a little strange.

userid is not always applicable (e.g. SCIM), so that has to be optional.

time and userid have just turned up together like that. that could probably be changed, but that would increase the metadata storage further, and i don't see any advantage.

permissions are decided during creation of the Invitation. it's likely i could keep the Maybe Role here and decide it in galley during team user creation, but why? (i guess this has to do with your idea about admins getting removed from the system and the posthumusly causing member creations, but i'm still not entirely convinced.)

jschaul · 2019-01-17T14:08:49Z

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

    addUserToTeamSSO account tid ident = do
        let uid = userId (accountUser account)
-        added <- lift $ Intra.addTeamMember uid tid Nothing
+        added <- lift $ Intra.addTeamMember uid tid (Nothing, Team.rolePermissions Team.RoleMember)


You're setting the default here (RoleMember) in brig. Wouldn't it be good to keep the defaults of team permissions to galley? Galley handles all the permission logic so far. I.e. you could pass (Nothing, Nothing) here, and galley could assign the current default of RoleMember.

ok, that answers my previous comment. i'm convinced. :-)

jschaul · 2019-01-17T16:34:45Z

libs/brig-types/src/Brig/Types/Team/Invitation.hs


 data Invitation = Invitation
    { inTeam       :: !TeamId
+    , inPerms      :: !Permissions


This has to be a Maybe - existing invitations don't have this field in the database. We need to keep backwards compatibility with existing invitations.

even more convinced!

jschaul · 2019-01-17T16:44:44Z

services/brig/test/integration/API/Team.hs

+        inv <- decodeBody =<< postInvitation brig tid owner invite
+        registerInvite inv aliceEmail
+
+    do


I don't understand why this nested do is necessary or useful?

scoping. the are local variables that are declared twice, and i don't want to think of two different names.

jschaul · 2019-01-17T16:45:59Z

services/brig/test/integration/API/Team.hs

+testInvitationRoles brig galley = do
+    (owner, tid) <- createUserWithTeam brig galley
+
+    let registerInvite :: Invitation -> Email -> Http UserId


to keep the style of the code around, could you move this to a top-level function instead of an inner let?

There are no other top-level helpers in this module, no? So it would just make this small function harder to find, and otherwise at best solve an aesthetic problem. I would like ot keep this self-contained.

jschaul · 2019-01-17T16:52:04Z

services/brig/test/integration/API/Team.hs

        const 403 === statusCode
        const (Just "too-many-team-invitations") === fmap Error.label . decodeBody

+-- | Admins can invite collaborators, but not owners.


Do we already have a test checking that members/collaborators cannot invite?

no, but that not specific to the changes in this PR.

I will add a few more tests in a separate PR, though.

jschaul · 2019-01-17T16:57:07Z

services/brig/test/integration/API/Team.hs

+        aliceEmail <- randomEmail
+        let invite = InvitationRequest aliceEmail (Name "Alice") Nothing (Just Team.RoleAdmin)
+        inv <- decodeBody =<< postInvitation brig tid owner invite
+        registerInvite inv aliceEmail


(style-opinion/consistency) - I find the nested blocks, which are nested purely for style reasons, unnecessary and confusing, and would prefer:

aliceEmail <- randomEmail let invite = InvitationRequest aliceEmail (Name "Alice") Nothing (Just Team.RoleAdmin) inv <- decodeBody =<< postInvitation brig tid owner invite alice <- registerInvite inv aliceEmail

Empty lines and/or comments -- invite alice as an admin would provide enough separation between the different parts of the test, without extra indentation.

i don't know how to answer this. what i do is introduce smaller, self-contained scopes that do not leak noise into the other scopes. so how can it possibly be anything but easier to read?

can you motivate your objection better than "it's hard to read"? (i don't want to spend a lot of time on this, but i had to defeind this at least once.)

FWIW I am mildly in favor of self-contained scopes, though for "leak noise" reasons more than for readability reasons.

I would find this particular one more readable, too, if alice in alice <- do had a type annotation.

jschaul · 2019-01-17T17:00:40Z

services/brig/test/integration/API/Team.hs

        const 400 === statusCode
        const (Just "invalid-invitation-code") === fmap Error.label . decodeBody
- where
-    invite email = InvitationRequest email (Name "Bob") Nothing


Can you motivate the where to let style change, or revert it? (Most of brig/galley tends to use let for things of kind *, and where for things of kind * -> .... That's of course arbitrary, but it would be nice to not change code style too much within the same service)

motivation is that it's a let in 30 other places in the same module, with almost exactly the same line. i changed it for consistency.

neongreen · 2019-01-17T21:54:56Z

libs/galley-types/src/Galley/Types/Teams.hs


 isTeamOwner :: TeamMember -> Bool
 isTeamOwner tm = fullPermissions == (tm^.permissions)
+    -- (this should be reimplemented in terms of 'rolePermissions'.)


TODO/FUTUREWORK?

i changed my mind. i'll remove this comment, it's more distracting than helpful.

neongreen · 2019-01-17T21:56:59Z

libs/brig-types/src/Brig/Types/Team/Invitation.hs

+        [ "email"        .= irEmail i
+        , "inviter_name" .= irName i
+        , "locale"       .= irLocale i
+        ] <> maybe [] (\role -> ["role" .= role]) (irRole i)


We have (#) for this. It's used like this: https://github.com/wireapp/wire-server/blob/develop/libs/brig-types/src/Brig/Types/Common.hs#L266-L271

However, locale is also a Maybe and we're happy to encode a null in this case, which clients might be relying upon. So in this particular case we can't use #. Ughh.

i've been wondering about this. i'll just add it as null then. thanks for the # hint anyway!

neongreen · 2019-01-17T21:58:59Z

libs/galley-types/src/Galley/Types/Teams.hs

+hasCopyPermission :: TeamMember -> Perm -> Bool
+hasCopyPermission tm p = p `Set.member` (tm^.permissions.copy)
+
 -- Note [team roles]


This note might need some updating now.

neongreen · 2019-01-17T22:00:16Z

libs/galley-types/src/Galley/Types/Teams.hs

+instance ToJSON TeamMember where
+    toJSON = teamMemberJson (const True)
+
+teamMemberJson :: (TeamMember -> Bool) -> TeamMember -> Value


I'd like a haddock saying what (TeamMember -> Bool) is for.

neongreen · 2019-01-17T22:10:51Z

services/brig/src/Brig/Team/Util.hs

+  where
+    check :: Maybe TeamMember -> Bool
+    check (Just m) = hasPermission m AddTeamMember &&
+                     and (hasCopyPermission m <$> (Set.toList $ inviteePerms ^. self))


Hm.

Why?

Is it the same as checking that self == copy?

does 279146c answer this?

neongreen · 2019-01-17T22:12:47Z

services/brig/test/integration/API/Team.hs

+        aliceEmail <- randomEmail
+        let invite = InvitationRequest aliceEmail (Name "Alice") Nothing (Just Team.RoleAdmin)
+        inv <- decodeBody =<< postInvitation brig tid owner invite
+        registerInvite inv aliceEmail


FWIW I am mildly in favor of self-contained scopes, though for "leak noise" reasons more than for readability reasons.

I would find this particular one more readable, too, if alice in alice <- do had a type annotation.

@jschaul

this is essential for backwards-compatibility, thanks @jschaul!

tiago-loureiro · 2019-01-18T12:40:46Z

libs/brig-types/src/Brig/Types/Team/Invitation.hs

 instance FromJSON Invitation where
    parseJSON = withObject "invitation" $ \o ->
        Invitation <$> o .: "team"
+                   <*> o .: "role"


This should be a .:? instead

tiago-loureiro · 2019-01-18T12:45:53Z

libs/brig-types/src/Brig/Types/Team/Invitation.hs

 import Data.Aeson
 import Data.Id
 import Data.Json.Util
+import Galley.Types.Teams (Role)


Let's please not make a service types package depend on another service types package; it's just a matter of time until galley-types depends on brig-types and then we're stuck with circular dependencies.

If we need to share types, let's add them to the types-common package (FWIW, I see that galley-types currently depends on gundeck-types and we should aim to get rid of that in the same fashion).

tiago-loureiro · 2019-01-18T12:52:12Z

libs/galley-types/src/Galley/Types/Teams.hs

        ]
+
+#ifdef WITH_CQL
+instance Cql.Cql Permissions where


Following the above logic of service types dependencies, can we put this (Permissions and Role) under Data.Role on types-common? Even if galley and brig do get merged, at least until then let's keep things consistent;

Note that the whole invitation should and could be part of galley - the reason we decided against it was because all the email logic/templating/etc. was part of brig

fisx · 2019-01-18T13:16:25Z

All tests pass on my machine as of 727d8ee.

How about this: I will clean up the {gundeck,galley,brig}-types entanglement, but i'll do it in another PR?

tiago-loureiro · 2019-01-18T13:51:23Z

How about this: I will clean up the {gundeck,galley,brig}-types entanglement, but i'll do it in another PR?

👍

tiago-loureiro · 2019-01-18T13:53:32Z

libs/galley-types/src/Galley/Types/Teams.hs

+rolePermissions role = Permissions p p  where p = rolePerms role
+
+rolePerms :: Role -> Set Perm
+rolePerms RoleOwner = rolePerms RoleAdmin <> Set.fromList


If we want to introduce roles now, we should make sure that role Owner = fullPermissions or something like this; I am afraid that otherwise we may very well add a new permission and forget to add it (types won't help us there)

since you already approved: yes, but different PR! :-)

(won't be a visible change outside of the wire services. also need to think about the options for a minute.)

tiago-loureiro · 2019-01-18T14:05:29Z

services/brig/src/Brig/Team/DB.hs


 insertInvitation :: MonadClient m
                 => TeamId
+                 -> Maybe Role


Btw, just a small note here: we could certainly change the insertInvitation to accept Role and the inviter: UserId, since those will always exist after this deployment;

We still need those Maybes on the read path but wondering if we can already ensure they get written during the creation of an invitation (you could also change the FromJSON instance of InvitationRequest to default to member, we do that in some places). Given that invitations only live on our DB for 2 weeks in production, the read path could even be changed after that.

Not necessary to change it, just food for thought, not sure what the general opinion about this topic is.

Role: agreed (even though it doesn't seem important to me). UserId: what about SCIM?

Yes, please let's not have UserId. Ideally we should have UserId | ScimTokenId for auditabilibilibility purposes, though then we might also need to think in the future about deactivating SCIM tokens instead of deleting them.

While I'm at it, we might also want to store info about who created which SCIM token. Auditabilibilibility!

btw i feel that adding application logic to aeson parsers is generally a bad idea (and @jschaul seems to agree with me, see swagger remark above). adding a default role is admittedly a corner case, but then i would at least use instance Default Role where def = RoleMember.

My bad, forget about the UserId

tiago-loureiro

Other that what was commented already 👍

fisx · 2019-01-18T14:08:43Z

spar tests fail on ci, but not on my machine. any ideas how this could be possible? it's not aws vs. fake-aws, spar does not do any of those tests.

fisx added 10 commits January 16, 2019 19:23

Cleanup

001a816

Introduce Role datatype.

109a049

Test

ea9329c

Fix

76c9f31

Do not push member update events to unprivileged users.

c1967e4

Push member update events to updated members.

691aceb

Only answer authorized team member get requests.

9faf37f

Fix test.

f52efbe

Refactor: TeamMember is much simpler now.

a414f6e

fisx changed the title ~~[WIP] allow choosing a role when creating a team member; fix role change events.~~ Allow choosing a role when creating a team member; fix role change events. Jan 16, 2019

fisx changed the title ~~Allow choosing a role when creating a team member; fix role change events.~~ [WIP] Allow choosing a role when creating a team member; fix role change events. Jan 16, 2019

fisx force-pushed the fisx-hack-invitations branch from b38ef0e to a414f6e Compare January 16, 2019 21:43

fisx changed the title ~~[WIP] Allow choosing a role when creating a team member; fix role change events.~~ Allow choosing a role when creating a team member; fix role change events. Jan 16, 2019

fisx added 4 commits January 17, 2019 10:31

Revert "Refactor: TeamMember is much simpler now."

6f9f633

This reverts commit a414f6e.

Revert "Only answer authorized team member get requests."

041ba67

This reverts commit 9faf37f.

Include perms for self in member get requests.

a3c9afb

Add aeson instances for TeamMember{,List}.

646c73f

fisx requested a review from jschaul January 17, 2019 10:10

fisx added 2 commits January 17, 2019 11:14

Fix: swagger.

df4cdc0

Fixup

94a4b66

fisx mentioned this pull request Jan 17, 2019

App-level websocket pongs. #561

Merged

2 tasks

jschaul reviewed Jan 17, 2019

View reviewed changes

neongreen reviewed Jan 17, 2019

View reviewed changes

fisx added 2 commits January 18, 2019 10:08

Fix: store optional Role in Invitation.

4125673

this is essential for backwards-compatibility, thanks @jschaul!

Cleanup

279146c

fisx added 5 commits January 18, 2019 11:03

Cleanup

63bb653

Aeson instances: render maybe as null, not omitteed fields.

a7c4eee

Cleanup

6f78bad

Fixup a7c4eee

1dd1aec

Fixup a7c4eee

430496d

tiago-loureiro reviewed Jan 18, 2019

View reviewed changes

Fixup 4125673

727d8ee

fisx mentioned this pull request Jan 18, 2019

[WIP] disentangle types packages #574

Closed

tiago-loureiro reviewed Jan 18, 2019

View reviewed changes

tiago-loureiro approved these changes Jan 18, 2019

View reviewed changes

fisx merged commit 20a1b74 into develop Jan 18, 2019

fisx deleted the fisx-hack-invitations branch January 18, 2019 14:25

This was referenced Jan 18, 2019

Make role-perm mapping more robust to change. #576

Merged

Release/2019-01-24 #596

Merged

Allow choosing a role when creating a team member; fix role change events. #573

Allow choosing a role when creating a team member; fix role change events. #573

Uh oh!

Conversation

fisx commented Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fisx commented Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fisx commented Jan 17, 2019

Uh oh!

fisx commented Jan 17, 2019

Uh oh!

fisx commented Jan 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jschaul Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisx commented Jan 16, 2019 •

edited

Loading

fisx commented Jan 16, 2019 •

edited

Loading

jschaul Jan 17, 2019 •

edited

Loading

tiago-loureiro Jan 18, 2019 •

edited

Loading