SCIM Cleanup

[fisx]

This PR contains some cleanup work that should lead to fixing https://github.com/zinfra/backend-issues/issues/1006 in another PR.

[fisx]

next up:

• avoid using the vsuUser field of ValidScimUser
• drop the vsuUser field
• drop the spar.scim_user table and obtain all its information from brig on demand.

@arianvp what do you think?

[arianvp]

NeededInfo and ValidScimUser are the same datatype with those changes (which is a good sign I guess)

which means there is some code duplication in synthesizeScimUser we should get rid off

[arianvp]

I'm blind! You already did that. yes bravo. this is the direction I want this to go

[arianvp]

Why still store the SCIM user? synthesiszeScimUser can give a Scim.User for every ValidScimUser

[arianvp]

I guess we will drop that field as a next step right? I am just rubber-ducking to myself. ignore me mostly

[fisx]

These two test cases are wrong now, we need to adjust the expectations:

Failures:

  test-integration/Test/Spar/Scim/UserSpec.hs:792:5: 
  1) Spar.Scim.User, PUT /Users/:id, updates user attributes in scim_user
       expected: User {schemas = [User20,CustomSchema "urn:wire:scim:schemas:profile:1.0",CustomSchema "urn:ietf:params:scim:schemas:extension:wire:1.0:User"], userName = "scimuser_6475384", externalId = Just "scimuser_extid_6475384", name = Nothing, displayName = Just "Scim User #6475384", nickName = Nothing, profileUrl = Nothing, title = Nothing, userType = Nothing, preferredLanguage = Nothing, locale = Nothing, active = Nothing, password = Nothing, emails = [Email {typ = Nothing, value = EmailAddress2 {unEmailAddress = "xbqgopykzpuwzsx@ofqodagitekamdu.com"}, primary = Nothing}], phoneNumbers = [Phone {typ = Nothing, value = Just "+930274719"},Phone {typ = Nothing, value = Just "+790107577193"},Phone {typ = Nothing, value = Just "+3203902848"}], ims = [], photos = [], addresses = [], entitlements = [], roles = [], x509Certificates = [], extra = ScimUserExtra {_sueRichInfo = RichInfo {richInfoMap = fromList [], richInfoAssocList = []}}}
        but got: User {schemas = [User20,CustomSchema "urn:wire:scim:schemas:profile:1.0",CustomSchema "urn:ietf:params:scim:schemas:extension:wire:1.0:User"], userName = "scimuser_6475384", externalId = Just "scimuser_extid_6475384", name = Nothing, displayName = Just "Scim User #6475384", nickName = Nothing, profileUrl = Nothing, title = Nothing, userType = Nothing, preferredLanguage = Nothing, locale = Nothing, active = Nothing, password = Nothing, emails = [], phoneNumbers = [], ims = [], photos = [], addresses = [], entitlements = [], roles = [], x509Certificates = [], extra = ScimUserExtra {_sueRichInfo = RichInfo {richInfoMap = fromList [], richInfoAssocList = []}}}

  To rerun use: --match "/Spar.Scim.User/PUT /Users/:id/updates user attributes in scim_user/"

  test-integration/Test/Spar/Scim/UserSpec.hs:845:5: 
  2) Spar.Scim.User, PUT /Users/:id, works fine when neither name nor handle are changed
I, logger=cassandra.spar, New control connection: datacenter1:rack1:127.0.0.1:9042#<socket: 60>
       expected: User {schemas = [User20,CustomSchema "urn:wire:scim:schemas:profile:1.0",CustomSchema "urn:ietf:params:scim:schemas:extension:wire:1.0:User"], userName = "scimuser_2866857", externalId = Just "scimuser_extid_0003267@example.com", name = Nothing, displayName = Just "Scim User #2866857", nickName = Nothing, profileUrl = Nothing, title = Nothing, userType = Nothing, preferredLanguage = Nothing, locale = Nothing, active = Nothing, password = Nothing, emails = [Email {typ = Nothing, value = EmailAddress2 {unEmailAddress = "ipffagrvmytzinl@lochamnvcbpplbx.com"}, primary = Nothing},Email {typ = Nothing, value = EmailAddress2 {unEmailAddress = "xwpspcqlodmndwx@apcfvrlaslmieej.com"}, primary = Nothing},Email {typ = Nothing, value = EmailAddress2 {unEmailAddress = "idrarhiufnodcok@yldtbscmkqlbnzf.com"}, primary = Nothing}], phoneNumbers = [Phone {typ = Nothing, value = Just "+1230498667092"},Phone {typ = Nothing, value = Just "+619088579"},Phone {typ = Nothing, value = Just "+27322336786"}], ims = [], photos = [], addresses = [], entitlements = [], roles = [], x509Certificates = [], extra = ScimUserExtra {_sueRichInfo = RichInfo {richInfoMap = fromList [], richInfoAssocList = []}}}
        but got: User {schemas = [User20,CustomSchema "urn:wire:scim:schemas:profile:1.0",CustomSchema "urn:ietf:params:scim:schemas:extension:wire:1.0:User"], userName = "scimuser_2866857", externalId = Just "scimuser_extid_0003267@example.com", name = Nothing, displayName = Just "Scim User #2866857", nickName = Nothing, profileUrl = Nothing, title = Nothing, userType = Nothing, preferredLanguage = Nothing, locale = Nothing, active = Nothing, password = Nothing, emails = [], phoneNumbers = [], ims = [], photos = [], addresses = [], entitlements = [], roles = [], x509Certificates = [], extra = ScimUserExtra {_sueRichInfo = RichInfo {richInfoMap = fromList [], richInfoAssocList = []}}}

  To rerun use: --match "/Spar.Scim.User/PUT /Users/:id/works fine when neither name nor handle are changed/"

Randomized with seed 818899458

Finished in 59.4049 seconds
234 examples, 2 failures, 44 pending

[fisx]

I ran into my first non-trivial issue half-way through removing spar.scim_user: We need creation and last-update time for every user. It is mandatory in the standard. We currently store creation times in brig.invitation and brig.team_invitation. I think for the purposes of SCIM, we can ignore brig.invitation`.

I propose to:

1. Add two fields to User and store them in brig.user: created_at, last_updated_at. Both have optional UTCTime values.
2. If a team user is created, both created_at and last_updated_at are set to the created_at value from the team invitation.
3. if spar creates a user via scim, or pulls a user from brig that has missing time stamps, it fills in the gaps with the current time.
4. If spar updates a user, it sets the last_updated_at field accordingly.

[arianvp]

If a team user is created, both created_at and last_updated_at are set to the created_at value from the team invitation.

I'm pretty sure cassandra exposes these things for all rows natively.

select writetime(*) from brig.user

[tiago-loureiro]

If a team user is created, both created_at and last_updated_at are set to the created_at value from the team invitation.

I'm pretty sure cassandra exposes these things for all rows natively.

select writetime(*) from brig.user

You cannot do that unfortunately; you can only get the writetime of fields that are not part of the primary key; so last_updated_at can be inferred from the other fields (just take the max, I think we do sth similar for conflict version on ES) but created_at cannot - one can assume the created_at to be the writetime of the activated field on brig though since I am pretty sure we never set it to false

[arianvp]

Why this new code? what was wrong with toExternalId' ? It's not clear from context; not commit message, why this code was added to me

[arianvp]

this might be a bit pedantic; and question of philosophy on testing.

Is the property we want to test here "spar distributes synthesizeUser over responses" or "Spar keeps these specific fields only" ?

Reason I ask is that my expected workflow would be to change the test to expect the new field; get a failure, then implement the new feature. However with the way we formulate the test now; that isn't possible as the test will magically pass.

That's ok; but just wondering whether coupling what we're testing with the implementation is what we want here? It probably is?

[fisx]

this is a very valid point. i was wondering the same thing, wanna try duplicating the work in spar and see how ugly it gets?