Towards getting scim production-ready #559

fisx · 2018-12-27T16:15:25Z

No description provided.

correct me if i'm wrong, but NewUser has no Handle field, so this necessarily has to happen in two steps, no?

(Before this branch, it sometimes returned Nothing and something threw an exception. This is not much better. Now, all failures to find the user will yield Nothing, not an exception.)

(This should really be fixed in hscim by introducing a constraint class for SCIMHandler.)

neongreen · 2019-01-29T14:27:41Z

services/spar/src/Spar/Data.hs

 --
 -- NB: we can add optional columns in the future and extract parts of the json blob should the need
 -- arise.  For instance, if we want to support different versions of SCIM, we could extract
-- 'SCIM.User.schemas' and, throw an exception if the list of values is not supported, and store it


SCIM.User.schemas is no longer a part of User, though it's still present in JSON.

neongreen · 2019-01-29T14:29:30Z

services/spar/src/Spar/Scim.hs

@@ -120,24 +120,26 @@ apiScim = hoistScim (toServant (Scim.siteServer configuration))
 ----------------------------------------------------------------------------
 -- UserDB

-- | Retrieve 'IdP' from 'ScimTokenInfo' and call 'validateSCIMUser''.


No changes here, only the renaming.

neongreen · 2019-01-29T14:30:00Z

services/spar/src/Spar/Scim.hs


 -- | Map the SCIM data on the spar and brig schemata, and throw errors if
 -- the SCIM data does not comply with the standard / our constraints.
-- See also: 'ValidSCIMUser'.


This comment has been extended.

neongreen · 2019-01-29T14:30:30Z

services/spar/src/Spar/Scim.hs

+-- also ignore this model. Leaving @name@ empty will prevent the confusion that
+-- might appear when somebody tries to set @name@ to some value and the
+-- @displayName@ won't be affected by that change.
+validateScimUser'


No changes again, just renaming.

neongreen · 2019-01-29T14:30:45Z

services/spar/src/Spar/Scim.hs

-        SCIM.badRequest SCIM.InvalidValue (Just "userName is not compliant")
+        Scim.badRequest Scim.InvalidValue (Just "userName is not compliant")
+
+    -- See this function's documentation


We prohibit emails and phone numbers.

neongreen · 2019-01-29T14:40:30Z

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

            let userid = scimUserId storedUser
-            putUser_ (Just tok) (Just userid) user' (env ^. teSpar)


No logic changes.

neongreen · 2019-01-29T14:42:06Z

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

            muserid' <- runSparCass $ Data.getUser (vuser' ^. vsuSAMLUserRef)
            liftIO $ do
                muserid' `shouldBe` Just userid

-        it "maps ValidSCIMUser to Brig.User completely and correctly (including 'SAML.UserRef')." $ do
+        -- See validateScimUser' for why we don't allow this
+        it "doesn't allow setting emails or phone numbers" $ do


This test is new. Git mixed it up quite a bit so you might just want to read the two tests without the diff: https://github.com/wireapp/wire-server/blob/fisx-resolve-scim-todos/services/spar/test-integration/Test/Spar/ScimSpec.hs#L225-L253.

neongreen · 2019-01-29T14:42:22Z

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

@@ -216,10 +273,6 @@ specUsers = describe "operations with users" $ do
        it "sets the 'deleted' flag in brig, and does nothing otherwise." $ do
            pendingWith "really?  how do we destroy the data then, and when?"

-    describe "GET /Meta" $ do  -- TODO: is that the end-point?  it's about the 'getMeta' method.


There is no GET /Meta.

neongreen · 2019-01-29T14:42:50Z

services/spar/test-integration/Util/Scim.hs

    suffix <- cs <$> replicateM 5 (getRandomR ('0', '9'))
-    emails <- replicateM 3 randomSCIMEmail


No emails because no emails.

neongreen · 2019-01-29T14:43:21Z

services/spar/test/Test/Spar/ScimSpec.hs

@@ -0,0 +1,82 @@
+{-# LANGUAGE DataKinds                  #-}


No changes here but Git thinks the file is new. I can't blame it.

fisx · 2019-01-29T15:37:32Z

services/spar/src/Spar/Scim.hs

+-- __Emails and phone numbers:__ we prohibit emails and phone numbers for now,
+-- because we'd like to ensure that only verified emails and phone numbers end
+-- up in our database, and implementing verification requires design decisions
+-- that we haven't made yet.


There is an issue here if we want to support email as the SAML subjectID, but we don't allow to store it. This should at least be mentioned here, and reject it here with a helpful error message, but we can resolve it later.

actually, never mind the link i just gave. scim should know how to create a valid SubjectID, and if that even can be configured, then setting it to "email" is just not an option for now. so the error happens either in scim setup, or setup doesn't talk about this and there is only one way of computing UserRef from the scim schema.

Then we may still want to have a way to turn off implicit user creation.

fisx · 2019-01-29T15:49:19Z

services/spar/src/Spar/Scim.hs

-      then pure Nothing
-      else pure scimuser
+    mbBrigUser <- lift (Intra.Brig.getUser uid)
+    if isJust mbBrigUser && (userTeam =<< mbBrigUser) == Just stiTeam


the isJust is redundant, but if you want to keep it for readability that's fine, too.

Yeah, I want to keep it for readability.

This reverts commit 9220794.

fisx added 19 commits December 29, 2018 12:26

Fix: toSCIMUser location, timestamp attributes.

d7b4215

Cleanup.

ea2d554

TODO: this is the wrong approach.

93201b0

Refactor.

0f342a6

Refactor: construct optional SAML subjectID during validation.

92fbf68

Better validation.

8c18a27

Haddocks.

d1dbc42

Resolve bogus TODO.

de79806

correct me if i'm wrong, but NewUser has no Handle field, so this necessarily has to happen in two steps, no?

Fix: store scim users with all metadata in spar; sync with brig later.

4acb902

...

507057f

...

ddc9cfe

...

bbea887

Cleanup.

237e3d9

Haddocks, test case skeletons.

027b09d

Fix: SCIM.UserDB.get never returned Nothing.

10b45c9

(Before this branch, it sometimes returned Nothing and something threw an exception. This is not much better. Now, all failures to find the user will yield Nothing, not an exception.)

Resolve name clashes (more heavily qualified imports).

a99b822

Refactor: factor SCIM.Types out of SCIM module.

0b1e60a

Refactor: I am going to need this function in a moment.

34883db

Cleanup.

3c0a92b

(This should really be fixed in hscim by introducing a constraint class for SCIMHandler.)

fisx force-pushed the fisx-resolve-scim-todos branch from 18c30b2 to b937347 Compare December 29, 2018 11:26

This comment has been minimized.

Sign in to view

fisx added 2 commits January 7, 2019 15:20

Cleanup.

bc20707

Update scim user.

6948ed0

fisx force-pushed the fisx-resolve-scim-todos branch from b937347 to d711cac Compare January 7, 2019 15:30

fisx added 3 commits January 8, 2019 11:34

More scim integration tests.

d59ba42

Refactor: ValidSCIMUser contains a complete SAML.UserRef now.

3da49f2

More scim integration tests.

b763612

fisx changed the title ~~[WIP] towards getting scim production-ready~~ Towards getting scim production-ready Jan 8, 2019

fisx force-pushed the fisx-resolve-scim-todos branch from d711cac to b763612 Compare January 8, 2019 13:59

Artyom Kazak added 13 commits January 22, 2019 19:54

Merge branch 'develop' into fisx-resolve-scim-todos

b71f36c

Fixes after the merge

298b5e1

More renaming

9cc3e7d

macOS :/

78047cd

macOS 2 :/

2423020

More renaming fixes

428c3b5

Fix comment

532232b

Newest hscim

5e1a4e9

Fix version-related tests

efe9ee1

Remove outdated comment (no more 'schemas')

ecbfd81

FUTUREWORK

e277d91

Forbid emails and phone numbers

9220794

No /Meta endpoint

be2d930

neongreen changed the title ~~[WIP] Towards getting scim production-ready~~ Towards getting scim production-ready Jan 24, 2019

Artyom Kazak added 5 commits January 24, 2019 20:50

Merge branch 'develop' into fisx-resolve-scim-todos

f1e9f5a

Fix compilation

c3e4c67

No more 'fromJust'

a866210

Link to a Github comment

25caddb

Bring the comment back

058baeb

neongreen reviewed Jan 29, 2019

View reviewed changes

Artyom Kazak added 2 commits January 29, 2019 16:44

Add a comment about SCIM user creation flow

570d638

Rewrap comments

aa2021c

fisx commented Jan 29, 2019

View reviewed changes

Artyom Kazak added 2 commits January 29, 2019 17:20

Revert "Forbid emails and phone numbers"

f5256e8

This reverts commit 9220794.

Test phone numbers as well

569a33b

neongreen merged commit f78d5cf into develop Jan 29, 2019

neongreen deleted the fisx-resolve-scim-todos branch January 29, 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

Towards getting scim production-ready #559

Towards getting scim production-ready #559

fisx commented Dec 27, 2018

This comment has been minimized.

This comment has been minimized.

neongreen Jan 29, 2019

neongreen Jan 29, 2019

neongreen Jan 29, 2019

neongreen Jan 29, 2019

neongreen Jan 29, 2019

neongreen Jan 29, 2019

neongreen Jan 29, 2019

neongreen Jan 29, 2019

neongreen Jan 29, 2019

neongreen Jan 29, 2019

fisx Jan 29, 2019

fisx Jan 29, 2019

fisx Jan 29, 2019

neongreen Jan 29, 2019

		let userid = scimUserId storedUser
		putUser_ (Just tok) (Just userid) user' (env ^. teSpar)

		suffix <- cs <$> replicateM 5 (getRandomR ('0', '9'))
		emails <- replicateM 3 randomSCIMEmail

Towards getting scim production-ready #559

Towards getting scim production-ready #559

Conversation

fisx commented Dec 27, 2018

This comment has been minimized.

This comment has been minimized.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment