Keep track of who is allowed to modify users (we vs. SCIM) #602

neongreen · 2019-01-29T20:21:34Z

No description provided.

fisx

looks good to me so far!

libs/brig-types/src/Brig/Types/Common.hs

neongreen · 2019-01-31T23:21:42Z

Changes in this PR

New field managed_by for all users, with two possible options ("wire" and "scim"). Users with "scim" can't be edited from Brig, but can be edited from Spar. The managed_by field is exposed in the /self profile so that client apps can look at it and decide whether to allow changes to the profile, or not.
New event ("managed_by was changed").
New internal endpoint PUT /i/users/:uid/managed-by, intended to be used by Spar to "claim" a user who was not provisioned via SCIM but who will from now on be managed by SCIM.
Some reshuffling around UserEvent (no visible changes but I would like for whoever is familiar with that code to take a look).

Planned changes in future PRs

This PR keeps track of managed_by but doesn't enforce it. To enforce it I plan to create endpoints PUT /i/self, PUT /i/self/email, PUT /i/self/phone, ... and use them to make changes from Spar. Public endpoints would be checking managed_by first.
New error type thrown by public endpoints (403 "read-only").
New endpoint POST /scim/user-ownership/:uid <Bool> to convert an existing user into a SCIM user or relinquish the ownership. Relinquish. That's a nice word.

neongreen · 2019-01-31T23:25:44Z

Also pay attention to what exactly counts as a profile change:

-- | Who controls changes to the user profile.
data ManagedBy
      -- | The profile can be changed in-app; user doesn't show up via SCIM at all.
    = ManagedByWire
      -- | The profile can only be changed via SCIM, with several exceptions:
      --
      --  1. User properties can still be set (because they are used internally by clients).
      --  2. Password can be changed by the user (SCIM doesn't support setting passwords yet).
      --  3. The user can still be deleted normally (SCIM doesn't support deleting users yet).
      --
      -- There are some other things that SCIM can't do yet, like setting accent IDs, but they
      -- are not essential, unlike e.g. passwords.
    | ManagedBySCIM

I'm not sure whether these are reasonable exceptions.

neongreen · 2019-01-31T23:27:11Z

Also, it's not too late to change any of the names.

fisx · 2019-02-01T09:22:24Z

To enforce it I plan to create endpoints PUT /i/self, PUT /i/self/email, PUT /i/self/phone, ... and use them to make changes from Spar. Public endpoints would be checking managed_by first.

i would also check that the internal end-points only work on users managed by scim.

fisx · 2019-02-01T10:11:49Z

libs/brig-types/src/Brig/Types/Swagger.hs

+managedBy = string $ enum
+    [ "wire"
+    , "scim"
+    ]


Nice, i didn't know this when i wrote the swagger DataType for Role, will fix.

Is the following better (automatically keeps in sync with changes in the type) or worse (convoluted and hard to read)? If better, I will change this everywhere in the code-base in a separate PR.

dataTypeFromBoundedEnum :: forall a proxy. (Bounded a, Enum a, Aeson.ToJSON a) => proxy a -> DataType dataTypeFromBoundedEnum _ = string . enum $ unpack . Aeson.encode <$> [(minBound :: a)..] role :: DataType role = dataTypeFromBoundedEnum (Proxy @Role)

I'd say it's better as long as dataTypeFromBoundedEnum has a haddock explaining that the Aeson instance is used.

I would also prefer to use AllowAmbiguousTypes and dataTypeFromBoundedEnum @Role, but this isn't important.

services/spar/src/Spar/Intra/Brig.hs

neongreen · 2019-02-01T13:43:51Z

i would also check that the internal end-points only work on users managed by scim.

This is reasonable in the context of this PR, but I would say that internal endpoints should try to be general-purpose tools unless there are severe reasons to do otherwise. Strong coupling between services (this is in Spar because Brig needs it, this is in Brig because Spar needs it, etc) makes it harder to reason about them in isolation, and Brig is already hard to reason about.

(Sidenote: ideally I'd have a separation between user-facing services – e.g. Captain and Spar – and everything else. User-facing services don't care about database states and whatever, internal services don't question orders coming down the chain of command. No sure if anybody else would agree with this vision though.)

fisx · 2019-02-01T14:45:30Z

(Sidenote: ideally I'd have a separation between user-facing services – e.g. Captain and Spar – and everything else. User-facing services don't care about database states and whatever, internal services don't question orders coming down the chain of command. No sure if anybody else would agree with this vision though.)

Sounds possibly reasonable. I would do that after fusing spar and galley, though.

neongreen · 2019-02-01T14:50:22Z

I would do that after fusing spar and galley, though.

You mean Brig and Galley?

jschaul · 2019-02-01T14:50:36Z

libs/brig-types/src/Brig/Types/Common.hs

+data ManagedBy
+      -- | The profile can be changed in-app; user doesn't show up via SCIM at all.
+    = ManagedByWire
+      -- | The profile can only be changed via SCIM, with several exceptions:


Can you define the profile? which attributes are part of it, which ones are not?

How do you intend to resolve the inconsistencies happening when at some point SCIM supports more things, but users have changed these already in Wire. Would those user-made changes then simply be reverted? (That's not very nice?).

The profile = all user-editable attributes.

"properties" aren't a part of that because we (the backend) don't even know what is stored there. It looks like it's mostly per-client settings and therefore not something SCIM should ever be bothered with: https://github.com/wireapp/wire-webapp/blob/dev/app/script/properties/PropertiesType.js#L22-L41

I foresee no inconsistencies. The only user-visible thing we allow changing is passwords, but SCIM only works for SSO-enabled users who (IIRC) don't have passwords anyway. Also users will (should) be notified when their password is changed via SCIM, so it's not like they will have something reverted silently.

(I will add an expanded version of this comment to the docs.)

jschaul · 2019-02-01T14:56:13Z

services/brig/brig.cabal

@@ -293,6 +293,7 @@ executable brig-schema
        V53
        V54
        V55
+        V56


V57. Sorry!

jschaul · 2019-02-01T14:59:15Z

services/spar/src/Spar/Scim.hs

@@ -246,6 +246,8 @@ createValidScimUser (ValidScimUser user uref handl mbName) = do
    -- Set user handle on brig (which can't be done during user creation yet).
    -- TODO: handle errors better here?
    lift $ Intra.Brig.setHandle buid handl
+    -- Flag the user as SCIM-managed
+    lift $ Intra.Brig.setManagedBy buid ManagedBySCIM


Couldn't this be set already at the time a user in brig is created, rather than afterwards with another, separate cassandra request?

Yeah, initially I went that way but then reverted the change. I don't remember what was the reason for reverting it :/

I will try it out again and maybe I'll remember.

@jschaul if you're still working, can you look at this one?

(I pushed a commit that does that change)

Note: setManagedBy is currently unused, but will be used in the next PR.

fisx · 2019-02-01T16:17:51Z

I would do that after fusing spar and galley, though.

You mean Brig and Galley?

yes, sorry. don't fuse spar! :)

ChrisPenner · 2019-02-05T14:44:08Z

libs/brig-types/src/Brig/Types/Common.hs

+    deriving (Eq, Show, Bounded, Enum)
+
+instance FromJSON ManagedBy where
+    parseJSON = withText "ManagedBy" $ \case


Is it reasonable to pull out a helper:

typeName :: forall a. Typeable a => String typeName = show $ typeRep @a

And use something like

withText (typeName @ManagedBy)

So we get a compile-time error here if the type is renamed?

I suppose this should all be covered by tests anyways; probably not worth it 🤷‍♂️

I don't think typeName will compile without a Proxy argument, but then it would work, and I am in favor of the idea.

@fisx We would need AllowAmbiguousTypes for it to compile; not sure if the team has opinions on that particular extension or not 🤷‍♂️ It's really only to avoid typing Proxy everywhere haha; so not a big deal either way and perhaps more confusing to new folks :P

I personally find it to be fine for small helpers like this

@neongreen Or am I misunderstanding the purpose of this string; does it need to match the type name exactly or is it actually just for error messages?

I don't think typeName will compile without a Proxy argument

It will if you enable AllowAmbiguousTypes (which you should)

This said, the type name doesn't really matter. It only shows up in Aeson error messages and I'm not sure that deviating from this "standard" way of writing JSON instances is worth it.

ChrisPenner · 2019-02-05T15:07:41Z

libs/brig-types/src/Brig/Types/User.hs

@@ -246,6 +252,7 @@ data NewUser = NewUser
    , newUserLocale         :: !(Maybe Locale)
    , newUserPassword       :: !(Maybe PlainTextPassword)
    , newUserExpiresIn      :: !(Maybe ExpiresIn)
+    , newUserManagedBy      :: !(Maybe ManagedBy)


Just curious; do we tend to make all data strict? I presume it makes sense in most (if not all) cases; has anyone considered using a global StrictData pragma?

do we tend to make all data strict

Yes.

has anyone considered using a global StrictData pragma?

I recall considering it when I first got here. Then I got scared of enabling it and decided not to spend more time on figuring out whether it's safe or not. I don't have any specific concerns in mind, though.

@neongreen It would either speed things up; slow things down, or neither haha; tough to say which without trying it.

I'm just slightly worried that we might be relying on laziness in some place or other.

It's certainly possible; but if that's true the result will be either that the integration tests fail; or that we discover some place where we use laziness unexpectedly; If we do require it in some place we can explicitly denote it with ~ to make it lazy again;

Granted, this maybe just takes time that nobody has; just curious :)

neongreen · 2019-02-06T03:30:43Z

@jschaul did you look at the commit that addresses "couldn't this be set already at the time a user in brig is created"? It's not small enough for me to comfortably say "yeah this code has been reviewed".

45448d6

fisx reviewed Jan 29, 2019

View reviewed changes

libs/brig-types/src/Brig/Types/Common.hs Show resolved Hide resolved

neongreen force-pushed the readonly branch 2 times, most recently from c315ed0 to 4d67156 Compare January 31, 2019 23:01

neongreen changed the title ~~[WIP] Keep track of who is allowed to modify users (we vs. SCIM)~~ Keep track of who is allowed to modify users (we vs. SCIM) Jan 31, 2019

fisx approved these changes Feb 1, 2019

View reviewed changes

jschaul reviewed Feb 1, 2019

View reviewed changes

Artyom Kazak added 7 commits February 4, 2019 13:16

Add the managed_by field

5e99fbe

Add a roundtrip test for ManagedBy and fix compilation

8f379ad

Set managedBy in SCIM; remove UserLocaleUpdated

20d63fc

Comment

ad780fd

Fix an unrelated bug

3ee1c22

Fix a typo

a476669

Let the NewUser endpoing set managedBy

45448d6

neongreen force-pushed the readonly branch from 4d67156 to 45448d6 Compare February 4, 2019 17:28

Expand a comment

15e7c12

ChrisPenner reviewed Feb 5, 2019

View reviewed changes

Typo

8833a64

Artyom Kazak added 2 commits February 6, 2019 16:02

SCIM -> Scim

da5164f

CI

fb98a5c

neongreen merged commit 11dfe00 into develop Feb 6, 2019

neongreen deleted the readonly branch February 6, 2019 17:30

This was referenced Feb 14, 2019

Release/2019-02-14 #629

Closed

Release/2019-02-14 #630

Closed

neongreen mentioned this pull request Feb 18, 2019

Release/2019-02-18 #634

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep track of who is allowed to modify users (we vs. SCIM) #602

Keep track of who is allowed to modify users (we vs. SCIM) #602

neongreen commented Jan 29, 2019

fisx left a comment

neongreen commented Jan 31, 2019

neongreen commented Jan 31, 2019 •

edited

Loading

neongreen commented Jan 31, 2019

fisx commented Feb 1, 2019

fisx Feb 1, 2019

neongreen Feb 1, 2019

neongreen commented Feb 1, 2019

fisx commented Feb 1, 2019

neongreen commented Feb 1, 2019

jschaul Feb 1, 2019

neongreen Feb 1, 2019

neongreen Feb 1, 2019

jschaul Feb 1, 2019

jschaul Feb 1, 2019

neongreen Feb 1, 2019

neongreen Feb 4, 2019

neongreen Feb 4, 2019

neongreen Feb 4, 2019

fisx commented Feb 1, 2019

ChrisPenner Feb 5, 2019

fisx Feb 5, 2019

ChrisPenner Feb 5, 2019 •

edited

Loading

ChrisPenner Feb 5, 2019

neongreen Feb 5, 2019

neongreen Feb 5, 2019

ChrisPenner Feb 5, 2019

neongreen Feb 5, 2019

ChrisPenner Feb 5, 2019

neongreen Feb 5, 2019

ChrisPenner Feb 5, 2019

neongreen commented Feb 6, 2019

@@ @@ -293,6 +293,7 @@ executable brig-schema @@
                       V53
                       V54
                       V55
+                      V56

Keep track of who is allowed to modify users (we vs. SCIM) #602

Keep track of who is allowed to modify users (we vs. SCIM) #602

Conversation

neongreen commented Jan 29, 2019

fisx left a comment

Choose a reason for hiding this comment

neongreen commented Jan 31, 2019

Changes in this PR

Planned changes in future PRs

neongreen commented Jan 31, 2019 • edited Loading

neongreen commented Jan 31, 2019

fisx commented Feb 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neongreen commented Feb 1, 2019

fisx commented Feb 1, 2019

neongreen commented Feb 1, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisx commented Feb 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisPenner Feb 5, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neongreen commented Feb 6, 2019

neongreen commented Jan 31, 2019 •

edited

Loading

ChrisPenner Feb 5, 2019 •

edited

Loading