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 onboarding flow again #1213

Merged
merged 132 commits into from
Oct 27, 2020
Merged

SCIM onboarding flow again #1213

merged 132 commits into from
Oct 27, 2020

Conversation

fisx
Copy link
Contributor

@fisx fisx commented Sep 25, 2020 •
edited
Loading

@fisx fisx changed the title SCIM onboarding flow again [skip ci] SCIM onboarding flow again Sep 25, 2020
@fisx fisx force-pushed the fisx/scim-onboarding-flow-again branch from 5da39fc to 4418bee Compare September 25, 2020 12:28
@CLAassistant
Copy link

CLAassistant commented Oct 5, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@fisx fisx force-pushed the fisx/scim-onboarding-flow-again branch from bd26d3d to 3bfc508 Compare October 6, 2020 11:38
fisx
fisx commented Oct 12, 2020
View reviewed changes
libs/wire-api/src/Wire/API/User.hs Outdated Show resolved Hide resolved
services/brig/src/Brig/API/Public.hs Outdated Show resolved Hide resolved
services/brig/src/Brig/API/User.hs Outdated Show resolved Hide resolved
@smatting smatting force-pushed the fisx/scim-onboarding-flow-again branch from 1468b26 to d5c7850 Compare October 12, 2020 14:33
fisx
fisx commented Oct 13, 2020
View reviewed changes
libs/wire-api/src/Wire/API/User.hs Outdated Show resolved Hide resolved
services/brig/src/Brig/API/Public.hs Outdated Show resolved Hide resolved
services/brig/src/Brig/Data/User.hs Outdated Show resolved Hide resolved
services/spar/src/Spar/Intra/Brig.hs Show resolved Hide resolved
services/spar/src/Spar/Intra/Brig.hs Outdated Show resolved Hide resolved
fisx and others added 17 commits October 13, 2020 14:27
@fisx
Add new AccountStatus constructor, rename all others.
5d28a92 
The renaming helps seeing all places where we may or may not want to
change the application logic.
@fisx
Revert the renaming (squash this after PR review).
16bc18d
@fisx
Resolve TODOs, fixups.
2511593
@fisx
When creating users via SCIM, use invitation logic.
151d261 
Additional quirk: also add a user with the 'PendingInvitation' status
we introduced earlier.  This way, both SCIM-GET and team settings will
show a user (resp. pending invitation), but no active user will
confuse any other part of the system.
@fisx
...
474627e
@smatting @fisx
Add CreateUserNoVerifyViaScim param newAccount and handler
e894bc9
@fisx
maybe of interest, but does not compile.
3c6a759
@fisx
Update integration test.
58f4f94
@fisx
...
8c8330d
@fisx
...
be8b56a
@smatting @fisx
Remove viaScim param; Add NewTeamMemberScimInvitation
29a25a8
@smatting @fisx
... from discussion Mo afternoon, details below
1ff06f8 
- Refactor isNewUserInvitedViaScim
- Apply suggestions from code review
- Disallow NewTeamMemberScimInvitation in NewUserPublic
- Refactor: rename isNewUserCreatedViaScim -> isNewUserInvitedViaScim
- Send welcome mail for scim invited users
@fisx
Cleanup
aabf6f2
@fisx
Undo change noise.
34cacb9
@fisx
ormolu.
ed6dce5
@smatting @fisx
Suggestions: no welcome mail + comment
1f718d6 
Co-authored-by: fisx <mf@zerobuzz.net>
@fisx
Haddocks.
f29a2bd
@fisx fisx force-pushed the fisx/scim-onboarding-flow-again branch from 367c5fc to f29a2bd Compare October 13, 2020 12:27
smatting
smatting previously approved these changes Oct 23, 2020
View reviewed changes
@fisx
Copy link
Contributor Author

fisx commented Oct 23, 2020

I see no brig integration tests. Please add tests for the new endpoints in brig:

* GET /i/users?ids=ABC&includePendingInvitations=true / false

* the new invitation create/get endpoints introduced in brig.

We're testing all of this, both happy and sad paths. It's admittedly hard to find, though: 6b60d7f

@fisx
Copy link
Contributor Author

fisx commented Oct 23, 2020 •
edited
Loading

I think we're still missing some bits & pieces...

The following database functions in Brig.Data.User read table brig.user, and thus may get confused about the new PendingInvitation status:

  • lookupAccount
  • lookupAccounts
  • lookupUser
  • lookupUsers
  • lookupName
  • lookupLocale
  • lookupPassword
  • lookupStatus
  • lookupRichInfo
  • lookupUserTeam
  • lookupUsersTeam
  • lookupServiceUsers
  • lookupServiceUsersForTeam

There is also ES index refresh, but we have that covered. No other mentions of the table, so with this, we should be ok.

  • will the users in status PendingInvitation, and the active users created when scim-triggered invitations are accepted, have ManagedByWire ManagedByScim set?

@fisx fisx dismissed smatting’s stale review October 23, 2020 18:16

see my last comment.

@fisx
Copy link
Contributor Author

fisx commented Oct 26, 2020

will the users in status PendingInvitation, and the active users created when scim-triggered invitations are accepted, have ManagedByWire ManagedByScim set?

just fixed that in my last comment.

@fisx
Copy link
Contributor Author

fisx commented Oct 26, 2020

  • lookupPassword - all calls are safe (only account deletion, password reset, login).
  • lookupStatus - this has to return PendingInvitation for spar to work. all uses are safe.
  • lookupRichInfo - different table, not touched through NewUserScimInvitation
  • lookupUserTeam - already safe to use for auth (left a comment)
  • lookupUsersTeam - dead code
  • lookupServiceUsers - different c*-table
  • lookupServiceUsersForTeam - different c*-table

@smatting
Copy link
Contributor

smatting commented Oct 26, 2020 •
edited
Loading

  • When checking for calls to lookupAccount I noticed isPendingInvitation Brig/User/Auth.hs:251 would probably yield False. It's used in login via resolveLoginId, which will throw an incorrect exception. Is this a hypothetical unhappy flow?:
    1. user is created via scim with password
    2. user tries to login without accepting the invitation first. She receives wrong LoginError valueLoginFailed when instead she should receive LoginPendingActivation value
  • When checking for calls to lookupAccount I noticed activateKey Brig/Data/Activation.hs:87 would probably throwE invalidCode. It looks like this function is used in Brig.API.createUser via activateWithCurrency and activate, but from the comments it looks like scim-invited users flow won't call these functions

@fisx
Copy link
Contributor Author

fisx commented Oct 26, 2020

... It's used in login via resolveLoginId, which will throw an incorrect exception. Is this a hypothetical unhappy flow?

Not entirely hypothetical, so good catch & good fix!

... activateWithCurrency and activate, but from the comments it looks like scim-invited users flow won't call these functions

I agree.

@fisx
Copy link
Contributor Author

fisx commented Oct 27, 2020

CI problems are unrelated; integration tests passed locally.

@fisx fisx merged commit f989d78 into develop Oct 27, 2020
@fisx fisx deleted the fisx/scim-onboarding-flow-again branch October 27, 2020 08:38
@fisx fisx mentioned this pull request Oct 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jschaul jschaul jschaul left review comments

@smatting smatting smatting 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.
4 participants
@fisx @CLAassistant @smatting @jschaul