Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SCIM cleanup 2 #1172

Merged
merged 23 commits into from
Jul 24, 2020
Merged

SCIM cleanup 2 #1172

merged 23 commits into from
Jul 24, 2020

Conversation

fisx
Copy link
Contributor

@fisx fisx commented Jul 15, 2020 •
edited
Loading

Follow-up to #1169

Fixes https://github.com/zinfra/backend-issues/issues/1006

TODO:

  • Write a comment about why RichInfo has a map and an assoc list and why brig only cares about the assoc list.

@fisx fisx force-pushed the scim-cleanup-2 branch 2 times, most recently from 8921efc to 4b3c25e Compare July 16, 2020 12:15
@fisx
Copy link
Contributor Author

fisx commented Jul 16, 2020

@arianvp and i tried to follow up on the discussion in #1169 about storing time stamps in brig, and in the end decided that it's best to keep the time stamps in spar.scim_user_times. Brig doesn't need this info and must not leak it, and using cassandra write times is tricky to implement, better look at the system time in brig and add it to the haskell type manually. before storing it.

@fisx
Copy link
Contributor Author

fisx commented Jul 16, 2020

I think we should soft-reset the entire branch and go through the changes together one more time @arianvp.

@fisx fisx force-pushed the scim-cleanup-2 branch from 4b3c25e to 4865853 Compare July 16, 2020 15:38
@arianvp
Copy link
Contributor

arianvp commented Jul 20, 2020

@arianvp and i tried to follow up on the discussion in #1169 about storing time stamps in brig, and in the end decided that it's best to keep the time stamps in spar.scim_user_times. Brig doesn't need this info and must not leak it, and using cassandra write times is tricky to implement, better look at the system time in brig and add it to the haskell type manually. before storing it.

Please add this context to the commit message. Even the context here in the comment is incomplete. Also

@fisx fisx changed the title SCIM cleanup 2 [WIP] SCIM cleanup 2 Jul 20, 2020
arianvp
arianvp reviewed Jul 20, 2020
View reviewed changes
services/spar/src/Spar/Scim/User.hs Outdated
@@ -610,6 +605,65 @@ assertHandleNotUsedElsewhere hndl uid = do
unless ((userHandle =<< musr) == Just hndl) $
assertHandleUnused' "userName does not match UserId" hndl uid

-- | Helper function that given a brig user, creates a scim user on the fly or returns
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well; it always creates a scim user on the fly now right? as we dont store them anymore. Comment should reflect that new reality

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still valid

@fisx fisx force-pushed the scim-cleanup-2 branch 7 times, most recently from 50c203c to 0e8edad Compare July 24, 2020 12:31
fisx and others added 8 commits July 24, 2020 14:32
@fisx
Drop table spar.scim_user and obtain most information from brig.
55bf9ac 
- keep timestamps in `spar.scim_user_times`.
- delete db handles for old table and add ones for the new one.
- re-adjust docs.
- re-adjust application logic to get user data from brig.
- fix some tests in hscim, spar-integration.
- simplify ValidScimUser type slightly.
- remove some deprecated scim code.
@fisx
simplify!
99aab12
@fisx
Fix: write access times if they are missing, ...
1bf0566 
... not if the user was not managed by scim.  it's not clear whether
the former is a bug, but it's certainly less straight-forward.
@akshaymankar @fisx
Parse rich-info returned by brig as RichInfoAssocList
9e5aea4
@akshaymankar @fisx
Ensure rich info has at least 1 field in tests
400a6b9 
When rich info doesn't have any fields, some stuff doesn't get properly tested
@akshaymankar @fisx
Move assocListToRichInfo to its rightful place
00f0a57
@akshaymankar @fisx
Write down our regained context about RichInfo
4ed0946
@akshaymankar @fisx
Ensure order of RichInfoAssocList is always maintained
3e0a8a2
fisx added 4 commits July 24, 2020 14:32
@fisx
Fix tests.
be711e7
@fisx
Fix hscim test suite.
9365da9 
There is one less test in hscim now, but that test is already
happening in the spar integration tests.
@fisx
Refactor RichInfo; haddocks.
df63407 
we now store assoc lists everywhere, but do not change behavior
in scim (except for bug fixes).  see haddocks for details.
@fisx
Link to github issue about future work.
d6a2236
@fisx fisx force-pushed the scim-cleanup-2 branch from 0e8edad to d6a2236 Compare July 24, 2020 12:40
@fisx fisx changed the title [WIP] SCIM cleanup 2 SCIM cleanup 2 Jul 24, 2020
@fisx fisx marked this pull request as ready for review July 24, 2020 12:41
arianvp
arianvp reviewed Jul 24, 2020
View reviewed changes
docs/developer/scim/storage.md Outdated
Currently (2019-04-29) we only implement one-way sync – when a user is modified via SCIM, Spar takes care to update data in Brig. However, user data is _not_ updated on the Spar side when it is changed in Brig, and Brig does not yet prohibit changing user data via its API – it relies on clients to be well-behaved and respect `userManagedBy`.
SCIM user data is validated by the spar service and stored as brig users. All fields that wire doesn't care about are silently dropped. `GET /scim/v2/Users` will trigger a lookup in brig, and the data thus obtained is synthesized back into a SCIM record.

Time stamps `created_at` and `last_updated_at` for the SCIM metadata are stored in `spar.scim_user_times`. The are kept in sync with the users that are otherwise stored in brig. (Rationale: we briefly considered using `select writetime(*) from brig.user` for last update and `select writetime(activated) from brig.user` for creation, but this has two drawbacks: (a) it stores data further away from spar where it is needed, and there is a bigger risk of that data going to places where it's not supposed to be; and (b) we don't have the time stamps when storing the record, so the `POST` handler would need to do a database write and a consecutive lookup, or an `insert if not exists`.)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Point (a) doesnt make sense given the timestamp is stored by cassandra; not brig

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's still further away from spar: if ti's in spar-cassandra, it goes from cassandra to spar; if it's in brig cassandra, it goes from cassandra to brig to spar, and brig holds data that it has no business holding.

how can i make this point more clear/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets just remove (a)

arianvp
arianvp reviewed Jul 24, 2020
View reviewed changes
services/spar/src/Spar/Scim/User.hs Outdated
getBrigUser' = MaybeT . lift . Brig.getBrigUser
getUserTeam' = MaybeT . pure . userTeam
brigUser <-
lift (Brig.getBrigUser uid)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't check if the user is in the team anymore? that seems bad

arianvp
arianvp reviewed Jul 24, 2020
View reviewed changes
services/spar/src/Spar/Intra/Brig.hs
resp :: Response (Maybe LBS) <-
call $
method PUT
. path "/self/handle"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grrrrr. but future work?

@fisx fisx requested a review from arianvp July 24, 2020 14:38
@fisx
Copy link
Contributor Author

fisx commented Jul 24, 2020

CI failed for unrelated reasons; integration tests pass locally.

@fisx fisx merged commit 82f1c29 into develop Jul 24, 2020
@fisx fisx deleted the scim-cleanup-2 branch July 24, 2020 18:22
@fisx fisx mentioned this pull request Jul 29, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akshaymankar akshaymankar akshaymankar left review comments

@arianvp arianvp arianvp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fisx @arianvp @akshaymankar