Allow SCIM without saml #1200

fisx · 2020-09-02T14:53:15Z

Fixes https://github.com/zinfra/backend-issues/issues/1365, https://github.com/zinfra/backend-issues/issues/1572

documentation

Before this PR, 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.

That's the gist of it!

- use internal end-points - return 'UserAccount' (contains status) instead of 'User'. - more transparent error handling.

fisx · 2020-09-03T20:22:52Z

TODO:

/wire-server/libs/wire-api/src/Wire/API/User/Identity.hs:256:9: error: [-Wpartial-fields, -Werror=partial-fields] (and two more)
f88559d
any important test cases that I should add?

Also remove two deprecated combinators from Spar.Intra.Brig.

fisx

I re-read everything except Spar.Scim.User (which is admittedly one of the more important modules). Some comments below.

fisx · 2020-09-04T07:51:59Z

libs/wire-api/src/Wire/API/User.hs

-        \ 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"


This change just makes the error message more specific.

fisx · 2020-09-04T07:58:30Z

libs/wire-api/src/Wire/API/User/Identity.hs

+      Text
+      -- An XML blob specifying the user's ID on the identity provider's side.
+      Text
+  | UserScimExternalId


This is for the case when there is no idp, but externalId is an email address. We should probably store this as an Email, too, non-email externalIds without SAML IdP cannot work (how would the user authenticate?).

At least we could leave this comment in the code.

fisx · 2020-09-04T08:05:17Z

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

@@ -173,6 +175,10 @@ sitemap = do
      .&. accept "application" "json"
      .&. jsonRequest @UserSSOId

+  delete "/i/users/:uid/sso-id" (continue deleteSSOIdH) $


I think we're not using this after all, but I'm tempted to keep it.

fisx · 2020-09-04T08:06:40Z

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

@@ -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


conn is the device that has triggered the event, and doesn't need to receive it. In the case of SCIM, this is Nothing.

fisx · 2020-09-04T08:08:04Z

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

@@ -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


I think that this is how you delete cells from rows in cassandra? By just writing Nothing?

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.

fisx · 2020-09-04T08:11:28Z

services/spar/src/Spar/App.hs

-validateEmailIfExists :: UserId -> SAML.UserRef -> Spar ()
-validateEmailIfExists uid (SAML.UserRef _ nameid) = case nameid ^. SAML.nameID of
-  UNameIDEmail email -> do
-    Intra.isEmailValidationEnabledUser uid >>= \case


This function has gone away, I forget why.

fisx · 2020-09-04T09:33:20Z

services/spar/src/Spar/Scim/Auth.hs

+                }
+        wrapMonadClient $ Data.insertScimToken token info
+        pure $ CreateScimTokenResponse token info
+
  case idps of


This is where all the changes in the PR begin.

fisx · 2020-09-04T10:28:05Z

services/spar/test-integration/Test/Spar/Intra/BrigSpec.hs

      pendingWith "or deactivated?  we should decide what we want here."
+
+  describe "getBrigUser" $ do


these tests aren't really improving the coverage, but it seemed easier to debug stuff by focussing on this smaller test.

fisx · 2020-09-04T11:11:24Z

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

@@ -130,20 +196,20 @@ instance MonadSparToBrig m => MonadSparToBrig (ReaderT r m) where
 createBrigUser ::
  (HasCallStack, MonadSparToBrig m) =>
  -- | SSO identity
-  SAML.UserRef ->
+  ValidExternalId ->


This is one of the more meaningful changes in the PR. Above are helpers inspired by this new type, below I mostly re-aligned functions, imrpoved error handling a little, and called better (more internal) end-points.

fisx · 2020-09-04T11:22:08Z

also, this just failed (non-deterministically?):


  test-integration/Test/Spar/Scim/UserSpec.hs:660:10: 
  1) Spar.Scim.User, GET /Users, doesn't list deleted users
       predicate failed on: User {userId = 7b85c17c-30c7-4499-a27e-7920c34b215a, userIdentity = Just (SSOIdentity (UserSSOId "<Issuer xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\" xmlns:samla=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:samlm=\"urn:oasis:names:tc:SAML:2.0:metadata\" xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\" xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\">https://issuer.net/_23776545-901f-48e8-a044-e2e40d21d77a</Issuer>" "<NameID xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\" xmlns:samla=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:samlm=\"urn:oasis:names:tc:SAML:2.0:metadata\" xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\" Format=\"urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress\" xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\">[email protected]</NameID>") Nothing Nothing), userDisplayName = Name {fromName = "Scim User #3066607"}, userPict = Pict {fromPict = []}, userAssets = [], userAccentId = ColourId {fromColourId = 0}, userDeleted = False, userLocale = en, userService = Nothing, userHandle = Just (Handle {fromHandle = "scimuser_3066607"}), userExpire = Nothing, userTeam = Just b53ce0d8-9032-460c-b6ae-fb15a8a3436e, userManagedBy = ManagedByScim}

  To rerun use: --match "/Spar.Scim.User/GET /Users/doesn't list deleted users/"

Randomized with seed 469431066

arianvp · 2020-09-04T16:16:17Z

services/spar/test-integration/Test/Spar/DataSpec.hs

+            Data.getSAMLUser
+            undefined -- could be @Data.lookupScimExternalId@, but we don't hit that path.
+            veid
+      Left _email -> undefined -- runSparCass . Data.lookupScimExternalId . fromEmail $ _email


Why is this case undefined? Because it never happens? can we do an fromRight instead?

yes, it never happens. it's perfectly reasonable to allow this case, and the comment two lines above says how it could be implemented, but i felt that it's better to not write dead code, but rather make it clear where we don't walk.

feel free to turn it into a fromRight if you think it's an improvement.

arianvp · 2020-09-04T16:25:00Z

services/spar/test-integration/Test/Spar/Scim/UserSpec.hs

+    -- TODO: yes, we should just test for error labels consistently, i know...
+    const (Just "Setting user passwords is not supported for security reasons.") =~= responseBody
+
+testCreateUserNoIdP :: TestSpar ()


The description of this test says it checks tht the email validation link is sent; but i dont see this being tested here.
How are we making sure this codepath is indeed taken?

Scrap that ;I can't read :)

arianvp · 2020-09-04T16:25:51Z

services/spar/test-integration/Test/Spar/Scim/UserSpec.hs

+    aFewTimes (runSpar $ Intra.getBrigUser userid) isJust
+      >>= maybe (error "could not find user in brig") pure
+  brigUser `userShouldMatch` WrappedScimStoredUser scimStoredUser
+  liftIO $ userEmail brigUser `shouldBe` Nothing


Why is the email Nothing ? I would expect it to be the email with which the user is being registered through scim? Wasn't that the point of this PR?

Nevermind scrap that; I see if comes later in the test

arianvp · 2020-09-04T16:30:27Z

It would be nice if we could document the new behaviour in the scim documentation; and also update the PR description to explain what exactly this PR implements. Especially because we changed designs like 3 times.

From what I understand the gist is that we send email activation emails upon inviting the user through scim; and modified brig to make users not ephemeral.

fisx · 2020-09-04T18:49:28Z

It would be nice if we could document the new behaviour in the scim documentation; and also update the PR description to explain what exactly this PR implements. Especially because we changed designs like 3 times.

From what I understand the gist is that we send email activation emails upon inviting the user through scim; and modified brig to make users not ephemeral.

Yes. I added a few more ideas to the description of this PR. We should probably move them to the docs somewhere, but I don't know where exactly. Ah, wait, I know! I'll make another commit. :)

arianvp · 2020-09-07T15:11:55Z

docs/reference/spar-braindump.md

+
+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.


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

arianvp · 2020-09-07T17:22:03Z

CI is currently failing because this PR uses Scim.util.deletUser which has a bug (it expects 200 whilst it should expect 204). Easy fix

tiago-loureiro · 2020-09-08T13:45:42Z

docs/reference/spar-braindump.md

+
+**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]'}`.


Suggested change

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]'}`.

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]'}`.

The comment was right. I just think this helper function wasn't used before

arianvp

Approved.

No nits on the code; but still a big question-mark on whether this is how we want to have this feature at all. I don't think it's very user-friendly and I do not think it's going to save us time in the long run. However, it will unblock us for now. Which is good. This should be discussed in a retro.

fisx added 6 commits September 2, 2020 13:36

New brig internal end-points.

ead3e5d

Support for email/password-authenticated scim users.

0ae0455

Cleanup

1bf9d9c

Cleanup

c936365

Fix: spar's notion of brig's api.

2f5ec95

- use internal end-points - return 'UserAccount' (contains status) instead of 'User'. - more transparent error handling.

Fix: allow UserSSOId in brig to carry scim external ids.

1f64197

fisx force-pushed the fisx/allow-scim-without-saml branch from 8c4172f to c3534c3 Compare September 2, 2020 21:46

fisx added 11 commits September 3, 2020 10:29

Fix: UserSSOId parsing in spar.

09a2755

Cleanup

7131b1a

Fix: brig internal api in spar.

8d767be

Fix: store RichInfo in brig if it changes.

f38d535

Fix: do not support setting passwords.

1bfb2f9

Fix: re-align error message in test.

4907be8

Fix: remove redundant test.

011795f

Fix: test.

3fcb32b

Fix: test.

82c920e

Fix: update sso_id in brig correctly.

0025e9b

Fix: do not pull users with email pending validation.

7dd4434

fisx force-pushed the fisx/allow-scim-without-saml branch from c3534c3 to 8fd50df Compare September 3, 2020 12:05

fisx added 2 commits September 3, 2020 15:40

Refactor: functions for handler-, email-based scim user lookup.

fb9c9be

Give externalIds that are emails their own lookup table in spar.

ea229b8

fisx force-pushed the fisx/allow-scim-without-saml branch from 8fd50df to e4523d1 Compare September 3, 2020 15:25

fisx added 4 commits September 3, 2020 20:14

Fix: misc

91fca93

Fix: misc

0e2e480

Comment.

fe92678

...

f88559d

fisx force-pushed the fisx/allow-scim-without-saml branch from f189c96 to f88559d Compare September 3, 2020 20:00

fisx added 2 commits September 4, 2020 09:41

Add missing test.

dc00e06

Fix compiler warning.

77056b0

fisx added 6 commits September 4, 2020 09:47

Fix: check if email address is available (even without idp).

c9897aa

nit-picks.

1ba1b96

Fix: scim-delete if there is no saml idp.

37131ee

Also remove two deprecated combinators from Spar.Intra.Brig.

Refactor: reduce integration test setup time.

11fe7d2

Fix: do not use email address as saml subject if no uref is found.

0f3edd4

Fix: test.

0ff6166

fisx commented Sep 4, 2020

View reviewed changes

fisx added 2 commits September 4, 2020 16:41

Fix: test.

1a2907f

Fix: test.

a5b87d8

arianvp reviewed Sep 4, 2020

View reviewed changes

fisx marked this pull request as ready for review September 4, 2020 18:34

arianvp reviewed Sep 7, 2020

View reviewed changes

Add at least a few lines of docs.

344080a

arianvp force-pushed the fisx/allow-scim-without-saml branch from 1cf250c to 344080a Compare September 8, 2020 12:55

tiago-loureiro reviewed Sep 8, 2020

View reviewed changes

Assert that deleteUser returns 204

9d90058

The comment was right. I just think this helper function wasn't used before

arianvp self-requested a review September 9, 2020 11:24

arianvp approved these changes Sep 9, 2020

View reviewed changes

arianvp merged commit ee47bbc into develop Sep 9, 2020

arianvp deleted the fisx/allow-scim-without-saml branch September 9, 2020 11:57

jschaul mentioned this pull request Oct 5, 2020

Release 2020 10 05 #1214

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow SCIM without saml #1200

Allow SCIM without saml #1200

fisx commented Sep 2, 2020 •

edited

Loading

fisx commented Sep 3, 2020 •

edited

Loading

fisx left a comment

fisx Sep 4, 2020

fisx Sep 4, 2020

fisx Sep 4, 2020

fisx Sep 4, 2020

fisx Sep 4, 2020

fisx Sep 4, 2020

fisx Sep 4, 2020

fisx Sep 4, 2020

fisx Sep 4, 2020

fisx Sep 4, 2020

fisx commented Sep 4, 2020

arianvp Sep 4, 2020 •

edited

Loading

fisx Sep 4, 2020

arianvp Sep 4, 2020

arianvp Sep 4, 2020

arianvp Sep 4, 2020

arianvp Sep 4, 2020

arianvp commented Sep 4, 2020

fisx commented Sep 4, 2020

arianvp Sep 7, 2020

arianvp commented Sep 7, 2020

tiago-loureiro Sep 8, 2020

arianvp left a comment •

edited

Loading

		pendingWith "or deactivated? we should decide what we want here."

		describe "getBrigUser" $ do


		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.


		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]'}`.

Allow SCIM without saml #1200

Allow SCIM without saml #1200

Conversation

fisx commented Sep 2, 2020 • edited Loading

documentation

fisx commented Sep 3, 2020 • 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

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 Sep 4, 2020

arianvp Sep 4, 2020 • 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

arianvp commented Sep 4, 2020

fisx commented Sep 4, 2020

Choose a reason for hiding this comment

arianvp commented Sep 7, 2020

Choose a reason for hiding this comment

arianvp left a comment • edited Loading

Choose a reason for hiding this comment

fisx commented Sep 2, 2020 •

edited

Loading

fisx commented Sep 3, 2020 •

edited

Loading

arianvp Sep 4, 2020 •

edited

Loading

arianvp left a comment •

edited

Loading