-
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
Allow SCIM without saml #1200
Allow SCIM without saml #1200
Changes from 34 commits
ead3e5d
0ae0455
1bf9d9c
c936365
2f5ec95
1f64197
09a2755
7131b1a
8d767be
f38d535
1bfb2f9
4907be8
011795f
3fcb32b
82c920e
0025e9b
7dd4434
fb9c9be
ea229b8
91fca93
0e2e480
fe92678
f88559d
dc00e06
77056b0
c9897aa
1ba1b96
37131ee
11fe7d2
0f3edd4
0ff6166
1a2907f
a5b87d8
344080a
9d90058
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 |
---|---|---|
|
@@ -21,6 +21,25 @@ documentation answering your questions, look here! | |
- if you want to work on our saml/scim implementation and do not have access to [https://github.com/zinfra/backend-issues/issues?q=is%3Aissue+is%3Aopen+label%3Aspar] and [https://github.com/wireapp/design-specs/tree/master/Single%20Sign%20On], please get in touch with us. | ||
|
||
|
||
## design considerations | ||
|
||
### SCIM without SAML. | ||
|
||
Before https://github.com/wireapp/wire-server/pull/1200, scim tokens could only be added to teams that already had exactly one SAML IdP. Now, we also allow SAML-less teams to have SCIM provisioning. This is an alternative to onboarding via team-settings and produces user accounts that are authenticated with email and password. (Phone may or may not work, but is not officially supported.) | ||
|
||
The way this works is different from team-settings: we don't send invites, but we create active users immediately the moment the SCIM user post is processed. The new thing is that the created user has neither email nor phone nor a SAML identity, nor a password. | ||
|
||
How does this work? | ||
|
||
**email:** If no SAML IdP is present, SCIM user posts must contain an externalId that is an email address. This email address is not added to the newly created user, because it has not been validated. Instead, the flow for changing an email address is triggered in brig: an email is sent to the address containing a validation key, and once the user completes the flow, brig will add the email address to the user. We had to add very little code for this in this PR, it's all an old feature. | ||
|
||
When SCIM user gets are processed, in order to reconstruct the externalId from the user spar is retrieving from brig, we introduce a new json object for the `sso_id` field that looks like this: `{'scim_external_id': '[email protected]'}`. | ||
|
||
In order to find users that have email addresses pending validation, we introduce a new table in spar's cassandra called `scim_external_ids`, in analogy to `user`. We have tried to use brig's internal `GET /i/user&email=...`, but that also finds pending email addresses, and there are corner cases when changing email addresses and waiting for the new address to be validated and the old to be removed... that made this approach seem infeasible. | ||
|
||
**password:** once the user has validated their email address, they need to trigger the "forgot password" flow -- also old code. | ||
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 doesn't sound great? Will the reset-password flow be triggered automatically in any way? Without it; the UX of this approach is not good and I foresee a lot of handholding needed |
||
|
||
|
||
## operations | ||
|
||
### enabling / disabling the sso feature for a team | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -694,10 +694,9 @@ parseNewUserOrigin pass uid ssoid o = do | |
(Nothing, Nothing, Just a, Nothing, Nothing) -> return . Just . NewUserOriginTeamUser $ NewTeamCreator a | ||
(Nothing, Nothing, Nothing, Just _, Just t) -> return . Just . NewUserOriginTeamUser $ NewTeamMemberSSO t | ||
(Nothing, Nothing, Nothing, Nothing, Nothing) -> return Nothing | ||
(_, _, _, _, _) -> | ||
fail $ | ||
"team_code, team, invitation_code, sso_id are mutually exclusive\ | ||
\ and sso_id, team_id must be either both present or both absent." | ||
(_, _, _, Just _, Nothing) -> fail "sso_id, team_id must be either both present or both absent." | ||
(_, _, _, Nothing, Just _) -> fail "sso_id, team_id must be either both present or both absent." | ||
_ -> fail "team_code, team, invitation_code, sso_id, and the pair (sso_id, team_id) are mutually exclusive" | ||
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 change just makes the error message more specific. |
||
case (result, pass, uid) of | ||
(_, _, Just SSOIdentity {}) -> pure result | ||
(Just (NewUserOriginTeamUser _), Nothing, _) -> fail "all team users must set a password on creation" | ||
|
@@ -729,7 +728,8 @@ data NewTeamUser | |
= -- | requires email address | ||
NewTeamMember InvitationCode | ||
| NewTeamCreator BindingNewTeamUser | ||
| NewTeamMemberSSO TeamId | ||
| -- | sso: users with saml credentials and/or created via scim | ||
NewTeamMemberSSO TeamId | ||
deriving stock (Eq, Show, Generic) | ||
deriving (Arbitrary) via (GenericUniform NewTeamUser) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -247,24 +247,34 @@ isValidPhone = either (const False) (const True) . parseOnly e164 | |
-- Morally this is the same thing as 'SAML.UserRef', but we forget the | ||
-- structure -- i.e. we just store XML-encoded SAML blobs. If the structure | ||
-- of those blobs changes, Brig won't have to deal with it, only Spar will. | ||
data UserSSOId = UserSSOId | ||
{ -- | An XML blob pointing to the identity provider that can confirm | ||
-- user's identity. | ||
userSSOIdTenant :: Text, | ||
-- | An XML blob specifying the user's ID on the identity provider's side. | ||
userSSOIdSubject :: Text | ||
} | ||
-- | ||
-- FUTUREWORK: rename the data type to @UserSparId@ (not the two constructors, those are ok). | ||
data UserSSOId | ||
= UserSSOId | ||
-- An XML blob pointing to the identity provider that can confirm | ||
-- user's identity. | ||
Text | ||
-- An XML blob specifying the user's ID on the identity provider's side. | ||
Text | ||
| UserScimExternalId | ||
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 is for the case when there is no idp, but externalId is an email address. We should probably store this as an At least we could leave this comment in the code. |
||
Text | ||
deriving stock (Eq, Show, Generic) | ||
deriving (Arbitrary) via (GenericUniform UserSSOId) | ||
|
||
instance ToJSON UserSSOId where | ||
toJSON (UserSSOId tenant subject) = object ["tenant" .= tenant, "subject" .= subject] | ||
toJSON = \case | ||
UserSSOId tenant subject -> object ["tenant" .= tenant, "subject" .= subject] | ||
UserScimExternalId eid -> object ["scim_external_id" .= eid] | ||
|
||
instance FromJSON UserSSOId where | ||
parseJSON = withObject "UserSSOId" $ \obj -> | ||
UserSSOId | ||
<$> obj .: "tenant" | ||
<*> obj .: "subject" | ||
parseJSON = withObject "UserSSOId" $ \obj -> do | ||
mtenant <- obj .:? "tenant" | ||
msubject <- obj .:? "subject" | ||
meid <- obj .:? "scim_external_id" | ||
case (mtenant, msubject, meid) of | ||
(Just tenant, Just subject, Nothing) -> pure $ UserSSOId tenant subject | ||
(Nothing, Nothing, Just eid) -> pure $ UserScimExternalId eid | ||
_ -> fail "either need tenant and subject, or scim_external_id, but not both" | ||
|
||
-- | If the budget for SMS and voice calls for a phone number | ||
-- has been exhausted within a certain time frame, this timeout | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,7 @@ import Brig.API.Handler | |
import qualified Brig.API.IdMapping as IdMapping | ||
import Brig.API.Types | ||
import qualified Brig.API.User as API | ||
import Brig.API.Util (validateHandle) | ||
import Brig.App | ||
import qualified Brig.Data.User as Data | ||
import Brig.Options hiding (internalEvents, sesQueue) | ||
|
@@ -58,6 +59,7 @@ import Network.Wai.Routing | |
import Network.Wai.Utilities as Utilities | ||
import Network.Wai.Utilities.Response (json) | ||
import Network.Wai.Utilities.ZAuth (zauthConnId, zauthUserId) | ||
import Wire.API.User | ||
import Wire.API.User.RichInfo | ||
|
||
--------------------------------------------------------------------------- | ||
|
@@ -173,6 +175,10 @@ sitemap = do | |
.&. accept "application" "json" | ||
.&. jsonRequest @UserSSOId | ||
|
||
delete "/i/users/:uid/sso-id" (continue deleteSSOIdH) $ | ||
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 think we're not using this after all, but I'm tempted to keep it. |
||
capture "uid" | ||
.&. accept "application" "json" | ||
|
||
put "/i/users/:uid/managed-by" (continue updateManagedByH) $ | ||
capture "uid" | ||
.&. accept "application" "json" | ||
|
@@ -183,6 +189,22 @@ sitemap = do | |
.&. accept "application" "json" | ||
.&. jsonRequest @RichInfoUpdate | ||
|
||
put "/i/users/:uid/handle" (continue updateHandleH) $ | ||
capture "uid" | ||
.&. accept "application" "json" | ||
.&. jsonRequest @HandleUpdate | ||
|
||
put "/i/users/:uid/name" (continue updateUserNameH) $ | ||
capture "uid" | ||
.&. accept "application" "json" | ||
.&. jsonRequest @NameUpdate | ||
|
||
get "/i/users/:uid/rich-info" (continue getRichInfoH) $ | ||
capture "uid" | ||
|
||
head "/i/users/handles/:handle" (continue checkHandleInternalH) $ | ||
capture "handle" | ||
|
||
post "/i/clients" (continue internalListClientsH) $ | ||
accept "application" "json" | ||
.&. jsonRequest @UserSet | ||
|
@@ -433,7 +455,14 @@ addPhonePrefixH (_ ::: req) = do | |
updateSSOIdH :: UserId ::: JSON ::: JsonRequest UserSSOId -> Handler Response | ||
updateSSOIdH (uid ::: _ ::: req) = do | ||
ssoid :: UserSSOId <- parseJsonBody req | ||
success <- lift $ Data.updateSSOId uid ssoid | ||
success <- lift $ Data.updateSSOId uid (Just ssoid) | ||
if success | ||
then return empty | ||
else return . setStatus status404 $ plain "User does not exist or has no team." | ||
|
||
deleteSSOIdH :: UserId ::: JSON -> Handler Response | ||
deleteSSOIdH (uid ::: _) = do | ||
success <- lift $ Data.updateSSOId uid Nothing | ||
if success | ||
then return empty | ||
else return . setStatus status404 $ plain "User does not exist or has no team." | ||
|
@@ -457,6 +486,44 @@ updateRichInfo uid rup = do | |
-- Intra.onUserEvent uid (Just conn) (richInfoUpdate uid ri) | ||
lift $ Data.updateRichInfo uid (RichInfoAssocList richInfo) | ||
|
||
getRichInfoH :: UserId -> Handler Response | ||
getRichInfoH uid = json <$> getRichInfo uid | ||
|
||
getRichInfo :: UserId -> Handler RichInfo | ||
getRichInfo uid = RichInfo . fromMaybe emptyRichInfoAssocList <$> lift (API.lookupRichInfo uid) | ||
|
||
updateHandleH :: UserId ::: JSON ::: JsonRequest HandleUpdate -> Handler Response | ||
updateHandleH (uid ::: _ ::: body) = empty <$ (updateHandle uid =<< parseJsonBody body) | ||
|
||
updateHandle :: UserId -> HandleUpdate -> Handler () | ||
updateHandle uid (HandleUpdate handleUpd) = do | ||
handle <- validateHandle handleUpd | ||
API.changeHandle uid Nothing handle !>> changeHandleError | ||
|
||
updateUserNameH :: UserId ::: JSON ::: JsonRequest NameUpdate -> Handler Response | ||
updateUserNameH (uid ::: _ ::: body) = empty <$ (updateUserName uid =<< parseJsonBody body) | ||
|
||
updateUserName :: UserId -> NameUpdate -> Handler () | ||
updateUserName uid (NameUpdate nameUpd) = do | ||
name <- either (const $ throwStd invalidUser) pure $ mkName nameUpd | ||
let uu = | ||
UserUpdate | ||
{ uupName = Just name, | ||
uupPict = Nothing, | ||
uupAssets = Nothing, | ||
uupAccentId = Nothing | ||
} | ||
lift (Data.lookupUser uid) >>= \case | ||
Just _ -> lift $ API.updateUser uid Nothing uu | ||
Nothing -> throwStd invalidUser | ||
|
||
checkHandleInternalH :: Text -> Handler Response | ||
checkHandleInternalH = | ||
API.checkHandle >=> \case | ||
API.CheckHandleInvalid -> throwE (StdError invalidHandle) | ||
API.CheckHandleFound -> pure $ setStatus status200 empty | ||
API.CheckHandleNotFound -> pure $ setStatus status404 empty | ||
|
||
getContactListH :: JSON ::: UserId -> Handler Response | ||
getContactListH (_ ::: uid) = do | ||
contacts <- lift $ API.lookupContactList uid | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1096,7 +1096,7 @@ instance ToJSON GetActivationCodeResp where | |
updateUserH :: UserId ::: ConnId ::: JsonRequest Public.UserUpdate -> Handler Response | ||
updateUserH (uid ::: conn ::: req) = do | ||
uu <- parseJsonBody req | ||
lift $ API.updateUser uid conn uu | ||
lift $ API.updateUser uid (Just conn) uu | ||
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.
|
||
return empty | ||
|
||
changePhoneH :: UserId ::: ConnId ::: JsonRequest Public.PhoneUpdate -> Handler Response | ||
|
@@ -1177,7 +1177,7 @@ changeHandleH (u ::: conn ::: req) = do | |
changeHandle :: UserId -> ConnId -> Public.HandleUpdate -> Handler () | ||
changeHandle u conn (Public.HandleUpdate h) = do | ||
handle <- API.validateHandle h | ||
API.changeHandle u conn handle !>> changeHandleError | ||
API.changeHandle u (Just conn) handle !>> changeHandleError | ||
|
||
beginPasswordResetH :: JSON ::: JsonRequest Public.NewPasswordReset -> Handler Response | ||
beginPasswordResetH (_ ::: req) = do | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -238,7 +238,7 @@ updateEmail u e = retry x5 $ write userEmailUpdate (params Quorum (e, u)) | |
updatePhone :: UserId -> Phone -> AppIO () | ||
updatePhone u p = retry x5 $ write userPhoneUpdate (params Quorum (p, u)) | ||
|
||
updateSSOId :: UserId -> UserSSOId -> AppIO Bool | ||
updateSSOId :: UserId -> Maybe UserSSOId -> AppIO Bool | ||
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 think that this is how you delete cells from rows in cassandra? By just writing 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. If in doubt, this just for the internal end-point that we're not using any more. We may want to use it in the future, but that's not a strong reason to not remove it again, then this change here would not be necessary. |
||
updateSSOId u ssoid = do | ||
mteamid <- lookupUserTeam u | ||
case mteamid of | ||
|
@@ -549,7 +549,7 @@ userEmailUpdate = "UPDATE user SET email = ? WHERE id = ?" | |
userPhoneUpdate :: PrepQuery W (Phone, UserId) () | ||
userPhoneUpdate = "UPDATE user SET phone = ? WHERE id = ?" | ||
|
||
userSSOIdUpdate :: PrepQuery W (UserSSOId, UserId) () | ||
userSSOIdUpdate :: PrepQuery W (Maybe UserSSOId, UserId) () | ||
userSSOIdUpdate = "UPDATE user SET sso_id = ? WHERE id = ?" | ||
|
||
userManagedByUpdate :: PrepQuery W (ManagedBy, UserId) () | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -143,6 +143,7 @@ executables: | |
- silently | ||
- spar | ||
- stm | ||
- tasty-hunit | ||
- tinylog | ||
- wai | ||
- wai-extra | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
-- This file is part of the Wire Server implementation. | ||
-- | ||
-- Copyright (C) 2020 Wire Swiss GmbH <[email protected]> | ||
-- | ||
-- This program is free software: you can redistribute it and/or modify it under | ||
-- the terms of the GNU Affero General Public License as published by the Free | ||
-- Software Foundation, either version 3 of the License, or (at your option) any | ||
-- later version. | ||
-- | ||
-- This program is distributed in the hope that it will be useful, but WITHOUT | ||
-- ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS | ||
-- FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more | ||
-- details. | ||
-- | ||
-- You should have received a copy of the GNU Affero General Public License along | ||
-- with this program. If not, see <https://www.gnu.org/licenses/>. | ||
|
||
module V10 | ||
( migration, | ||
) | ||
where | ||
|
||
import Cassandra.Schema | ||
import Imports | ||
import Text.RawString.QQ | ||
|
||
migration :: Migration | ||
migration = Migration 10 "Add table for mapping scim external ids to brig user ids" $ do | ||
void $ | ||
schema' | ||
[r| | ||
CREATE TABLE if not exists scim_external_ids | ||
( external text | ||
, user uuid | ||
, primary key (external) | ||
) with compaction = {'class': 'LeveledCompactionStrategy'}; | ||
|] |
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.