-
Notifications
You must be signed in to change notification settings - Fork 324
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
Keep track of who is allowed to modify users (we vs. SCIM) #602
Changes from all commits
5e99fbe
8f379ad
20d63fc
ad780fd
3ee1c22
a476669
45448d6
15e7c12
8833a64
da5164f
fb98a5c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -386,3 +386,44 @@ codeParser :: String -> (String -> Maybe a) -> Parser a | |
codeParser err conv = do | ||
code <- count 2 anyChar | ||
maybe (fail err) return (conv code) | ||
|
||
----------------------------------------------------------------------------- | ||
-- ManagedBy | ||
|
||
-- | Who controls changes to the user profile (where the profile is defined as "all | ||
-- user-editable, user-visible attributes"). | ||
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 | ||
-- and none of them can be modified via SCIM now or in the future). | ||
-- | ||
-- 2. Password can be changed by the user (SCIM doesn't support setting passwords yet, | ||
-- but currently SCIM only works with SSO-users who don't even have passwords). | ||
-- | ||
-- 3. The user can still be deleted normally (SCIM doesn't support deleting users yet; | ||
-- but it's questionable whether this should even count as a /change/ of a user | ||
-- profile). | ||
-- | ||
-- 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 | ||
deriving (Eq, Show, Bounded, Enum) | ||
|
||
instance FromJSON ManagedBy where | ||
parseJSON = withText "ManagedBy" $ \case | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 🤷♂️ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @fisx We would need I personally find it to be fine for small helpers like this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It will if you enable AllowAmbiguousTypes (which you should) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
"wire" -> pure ManagedByWire | ||
"scim" -> pure ManagedByScim | ||
other -> fail $ "Invalid ManagedBy: " ++ show other | ||
|
||
instance ToJSON ManagedBy where | ||
neongreen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
toJSON = String . \case | ||
ManagedByWire -> "wire" | ||
ManagedByScim -> "scim" | ||
|
||
defaultManagedBy :: ManagedBy | ||
defaultManagedBy = ManagedByWire | ||
|
||
-- NB: when adding new types, please add a roundtrip test to "Test.Brig.Types.Common" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -127,6 +127,10 @@ self = defineModel "Self" $ do | |
property "deleted" bool' $ do | ||
description "Whether the account has been deleted." | ||
optional | ||
property "managed_by" managedBy $ do | ||
description "What is the source of truth for this user; if it's SCIM \ | ||
\then the profile can't be edited via normal means" | ||
optional | ||
|
||
user :: Model | ||
user = defineModel "User" $ do | ||
|
@@ -150,6 +154,12 @@ user = defineModel "User" $ do | |
description "Unique user handle." | ||
optional | ||
|
||
managedBy :: DataType | ||
managedBy = string $ enum | ||
[ "wire" | ||
, "scim" | ||
] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, i didn't know this when i wrote the swagger 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say it's better as long as I would also prefer to use AllowAmbiguousTypes and |
||
|
||
assetType :: DataType | ||
assetType = string $ enum | ||
[ "image" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -127,6 +127,9 @@ data User = User | |
-- ^ Set if the user is ephemeral | ||
, userTeam :: !(Maybe TeamId) | ||
-- ^ Set if the user is part of a binding team | ||
, userManagedBy :: !ManagedBy | ||
-- ^ How is the user profile managed (e.g. if it's via SCIM then the user profile | ||
-- can't be edited via normal means) | ||
} | ||
deriving (Eq, Show) | ||
|
||
|
@@ -139,8 +142,9 @@ userPhone = phoneIdentity <=< userIdentity | |
userSSOId :: User -> Maybe UserSSOId | ||
userSSOId = ssoIdentity <=< userIdentity | ||
|
||
-- | A subset of the data of an existing 'User' | ||
-- that is returned on the API. | ||
-- | A subset of the data of an existing 'User' that is returned on the API and is visible to | ||
-- other users. Each user also has access to their own profile in a richer format -- | ||
-- 'SelfProfile'. | ||
data UserProfile = UserProfile | ||
{ profileId :: !UserId | ||
, profileName :: !Name | ||
|
@@ -170,11 +174,12 @@ instance ToJSON User where | |
# "accent_id" .= userAccentId u | ||
# "deleted" .= (if userDeleted u then Just True else Nothing) | ||
# "locale" .= userLocale u | ||
# "service" .= userService u | ||
# "handle" .= userHandle u | ||
# "service" .= userService u | ||
# "handle" .= userHandle u | ||
# "expires_at" .= userExpire u | ||
# "team" .= userTeam u | ||
# "sso_id" .= userSSOId u | ||
# "managed_by" .= userManagedBy u | ||
# [] | ||
|
||
instance FromJSON User where | ||
|
@@ -192,6 +197,7 @@ instance FromJSON User where | |
<*> o .:? "handle" | ||
<*> o .:? "expires_at" | ||
<*> o .:? "team" | ||
<*> o .:? "managed_by" .!= ManagedByWire | ||
|
||
instance FromJSON UserProfile where | ||
parseJSON = withObject "UserProfile" $ \o -> | ||
|
@@ -215,8 +221,8 @@ instance ToJSON UserProfile where | |
# "assets" .= profileAssets u | ||
# "accent_id" .= profileAccentId u | ||
# "deleted" .= (if profileDeleted u then Just True else Nothing) | ||
# "service" .= profileService u | ||
# "handle" .= profileHandle u | ||
# "service" .= profileService u | ||
# "handle" .= profileHandle u | ||
# "locale" .= profileLocale u | ||
# "expires_at" .= profileExpire u | ||
# "team" .= profileTeam u | ||
|
@@ -246,6 +252,7 @@ data NewUser = NewUser | |
, newUserLocale :: !(Maybe Locale) | ||
, newUserPassword :: !(Maybe PlainTextPassword) | ||
, newUserExpiresIn :: !(Maybe ExpiresIn) | ||
, newUserManagedBy :: !(Maybe ManagedBy) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @neongreen It would either speed things up; slow things down, or neither haha; tough to say which without trying it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm just slightly worried that we might be relying on laziness in some place or other. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Granted, this maybe just takes time that nobody has; just curious :) |
||
} | ||
deriving (Eq, Show) | ||
|
||
|
@@ -322,6 +329,7 @@ instance FromJSON NewUser where | |
newUserExpiresIn <- case (newUserExpires, newUserIdentity) of | ||
(Just _, Just _) -> fail "Only users without an identity can expire" | ||
_ -> return newUserExpires | ||
newUserManagedBy <- o .:? "managed_by" | ||
return NewUser{..} | ||
|
||
instance ToJSON NewUser where | ||
|
@@ -341,6 +349,7 @@ instance ToJSON NewUser where | |
# "password" .= newUserPassword u | ||
# "expires_in" .= newUserExpiresIn u | ||
# "sso_id" .= newUserSSOId u | ||
# "managed_by" .= newUserManagedBy u | ||
# maybe [] jsonNewUserOrigin (newUserOrigin u) | ||
|
||
-- | Fails if email or phone or ssoid are present but invalid | ||
|
@@ -379,19 +388,32 @@ data NewTeamUser = NewTeamMember !InvitationCode -- ^ requires email add | |
| NewTeamMemberSSO !TeamId | ||
deriving (Eq, Show) | ||
|
||
-- | newtype for using in external end-points where setting 'SSOIdentity', 'UUID' is not allowed. | ||
-- ('UUID' is only needed by spar for creating users that it can find again later, after a crash. | ||
-- if there another use case arises, this newtype and the 'FromJSON' instance would have to be | ||
-- refactored.) | ||
newtype NewUserNoSSO = NewUserNoSSO NewUser | ||
-- | We use the same 'NewUser' type for the @\/register@ and @\/i\/users@ endpoints. This | ||
-- newtype is used as request body type for the public @\/register@ endpoint, where only a | ||
-- subset of the 'NewUser' functionality should be allowed. | ||
-- | ||
-- Specifically, we forbid the following: | ||
-- | ||
-- * Setting 'SSOIdentity' (SSO users are created by Spar) | ||
-- | ||
-- * Setting the UUID (only needed so that Spar can find the user if Spar crashes before it | ||
-- finishes creating the user). | ||
-- | ||
-- * Setting 'ManagedBy' (it should be the default in all cases unless Spar creates a | ||
-- SCIM-managed user) | ||
newtype NewUserPublic = NewUserPublic NewUser | ||
deriving (Eq, Show) | ||
|
||
instance FromJSON NewUserNoSSO where | ||
instance FromJSON NewUserPublic where | ||
parseJSON val = do | ||
nu <- parseJSON val | ||
when (isJust $ newUserSSOId nu) $ fail "SSO-managed users are not allowed here." | ||
when (isJust $ newUserUUID nu) $ fail "it is not allowed to provide a UUID for the users here." | ||
pure $ NewUserNoSSO nu | ||
when (isJust $ newUserSSOId nu) $ | ||
fail "SSO-managed users are not allowed here." | ||
when (isJust $ newUserUUID nu) $ | ||
fail "it is not allowed to provide a UUID for the users here." | ||
when (newUserManagedBy nu `notElem` [Nothing, Just ManagedByWire]) $ | ||
fail "only managed-by-Wire users can be created here." | ||
pure $ NewUserPublic nu | ||
|
||
|
||
----------------------------------------------------------------------------- | ||
|
@@ -405,12 +427,16 @@ data UserUpdate = UserUpdate | |
} deriving (Eq, Show) | ||
|
||
newtype LocaleUpdate = LocaleUpdate { luLocale :: Locale } deriving (Eq, Show) | ||
newtype EmailUpdate = EmailUpdate { euEmail :: Email } deriving (Eq, Show) | ||
newtype PhoneUpdate = PhoneUpdate { puPhone :: Phone } deriving (Eq, Show) | ||
newtype HandleUpdate = HandleUpdate { huHandle :: Text } deriving (Eq, Show) | ||
newtype ManagedByUpdate = ManagedByUpdate { mbuManagedBy :: ManagedBy } deriving (Eq, Show) | ||
|
||
newtype EmailUpdate = EmailUpdate { euEmail :: Email } deriving (Eq, Show) | ||
newtype PhoneUpdate = PhoneUpdate { puPhone :: Phone } deriving (Eq, Show) | ||
newtype HandleUpdate = HandleUpdate { huHandle :: Text } deriving (Eq, Show) | ||
newtype EmailRemove = EmailRemove { erEmail :: Email } deriving (Eq, Show) | ||
newtype PhoneRemove = PhoneRemove { prPhone :: Phone } deriving (Eq, Show) | ||
newtype EmailRemove = EmailRemove { erEmail :: Email } deriving (Eq, Show) | ||
newtype PhoneRemove = PhoneRemove { prPhone :: Phone } deriving (Eq, Show) | ||
|
||
-- NB: when adding new types, please also add roundtrip tests to | ||
-- 'Test.Brig.Types.User.roundtripTests' | ||
|
||
instance FromJSON UserUpdate where | ||
parseJSON = withObject "UserUpdate" $ \o -> | ||
|
@@ -455,6 +481,13 @@ instance FromJSON HandleUpdate where | |
instance ToJSON HandleUpdate where | ||
toJSON h = object ["handle" .= huHandle h] | ||
|
||
instance FromJSON ManagedByUpdate where | ||
parseJSON = withObject "managed-by-update" $ \o -> | ||
ManagedByUpdate <$> o .: "managed_by" | ||
|
||
instance ToJSON ManagedByUpdate where | ||
toJSON m = object ["managed_by" .= mbuManagedBy m] | ||
|
||
instance FromJSON EmailRemove where | ||
parseJSON = withObject "email-remove" $ \o -> | ||
EmailRemove <$> o .: "email" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -294,6 +294,7 @@ executable brig-schema | |
V54 | ||
V55 | ||
V56 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. V57. Sorry! |
||
V57 | ||
|
||
build-depends: | ||
base | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,4 +23,3 @@ migration = Migration 56 "Add table to exclude phone number prefixes" $ do | |
, primary key (prefix) | ||
) | ||
|] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
{-# LANGUAGE OverloadedStrings #-} | ||
{-# LANGUAGE QuasiQuotes #-} | ||
|
||
module V57 (migration) where | ||
|
||
import Imports | ||
import Cassandra.Schema | ||
import Text.RawString.QQ | ||
|
||
migration :: Migration | ||
migration = Migration 57 "Add managed_by" $ do | ||
schema' [r| alter table user add managed_by int; |] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I will add an expanded version of this comment to the docs.)