[SQSERVICES-1067] Address user account consistency issue when SCIM provisioning crashes by elland · Pull Request #2526 · wireapp/wire-server

elland · 2022-06-30T11:01:10Z

This should prevent issues when creating a user fails midway, and we end up with dangling data that needs to be manually deleted in order to enable user creation.

As part of this effort, we're taking steps to break down createUser into multiple, use-case specific functions, which take more specific arguments to what it actually needs.

I've left a few checkmarks below unchecked, but I'll be getting those done soon, it shouldn't affect review.

Special attention should be paid to the new NewUserSpar record and its usage as well as a change in the createSAML functions. Also, this adds a new internal endpoint, POST i/users/spar, to disambiguate from the (hopefully soon to be legacy) i/users.

Checklist

The PR Title explains the impact of the change.
The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
changelog.d contains the following bits of information (details):
- A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

elland · 2022-06-30T11:01:33Z

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

Organising code.

elland · 2022-06-30T11:02:44Z

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

Less is more :)

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

fisx

I'll read the rest later.

libs/wire-api/src/Wire/API/Routes/Internal/Brig.hs

fisx · 2022-07-07T14:29:25Z

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

In fact I've been thinking about turning that off globally. I don't think this hint is ever incredibly helpful, it just ranges between harmless and obnoxious-yet-harmless.

Yeah, it's certainly contentious.

libs/wire-api/src/Wire/API/Routes/Internal/Brig.hs

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

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

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

mdimjasevic

There are several things I would change and you can find comments inlined.

I wasn't able to grasp the logic behind your proposed change so it would be great if you can walk me through the PR in a call.

mdimjasevic · 2022-07-18T12:31:19Z

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

What's the need for making the list explicit?

Oh I think this was HLS being too proactive.

mdimjasevic · 2022-07-18T14:29:50Z

libs/wire-api/src/Wire/API/Routes/Internal/Brig.hs

You haven't addressed this TODO note.

mdimjasevic · 2022-07-18T14:31:05Z

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

If possible, I'd away the generic Text type here and use/create a type specific to external IDs.

Same. Since I just moved this code from somewhere else, I didn't want to make this PR even larger than it already was by refactoring this + all the call sites.

mdimjasevic · 2022-07-18T14:35:12Z

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

I believe there's an easier way to specify an endpoint, i.e., without having to write AsUnion instances. Perhaps if you look at the Galley API routes module, you should find inspiration there.

Also, to me it doesn't sound like these HTTP-specific types belong to the module.

Let's see about improving that in our call later 👍

mdimjasevic · 2022-07-18T14:36:14Z

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

Why have left and right for errors? It's not wrong, but it looks unusual. Errors are typically kept on the left, and the right side is kept for happy case values.

This is only used to make working with schema profunctor easier.

I didn't want to create a custom Either isomorphism, on top of the one already created to handle these two different error types, just to not re-use Either, when… either already exists and has the upside of already being supported in schema profunctor.

mdimjasevic · 2022-07-19T08:02:42Z

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

There's mapLeft, or alternatively you could use first.

mdimjasevic · 2022-07-19T08:06:04Z

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

An unhandled TODO note.

mdimjasevic · 2022-08-01T10:25:41Z

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

This looks like withExceptT.

mdimjasevic

Just take care of remaining TODOs and that's it! Thanks for the walk-through during the call!

checked that all review comments have been taken care of.

elland commented Jun 30, 2022

View reviewed changes

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

Copy link

Contributor Author

elland Jun 30, 2022

Choose a reason for hiding this comment

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

Organising code.

elland commented Jun 30, 2022

View reviewed changes

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

Copy link

Contributor Author

elland Jun 30, 2022

Choose a reason for hiding this comment

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

Less is more :)

elland commented Jun 30, 2022

View reviewed changes

services/spar/src/Spar/Scim/User.hs Outdated Show resolved Hide resolved

elland force-pushed the clean-up-failed-user-provisioning branch from 5d1534c to 026897b Compare July 4, 2022 08:14

elland temporarily deployed to cachix July 4, 2022 08:14 Inactive

elland force-pushed the clean-up-failed-user-provisioning branch from 026897b to 84459b1 Compare July 4, 2022 08:14

elland temporarily deployed to cachix July 4, 2022 08:14 Inactive

elland mentioned this pull request Jul 4, 2022

Added more examples to Schema Profuctor docs. #2529

Merged

2 tasks

elland force-pushed the clean-up-failed-user-provisioning branch from 84459b1 to 7d68e62 Compare July 6, 2022 07:45

elland temporarily deployed to cachix July 6, 2022 07:45 Inactive

elland force-pushed the clean-up-failed-user-provisioning branch from 7d68e62 to df5ce4f Compare July 6, 2022 07:46

elland temporarily deployed to cachix July 6, 2022 07:46 Inactive

fisx reviewed Jul 7, 2022

View reviewed changes

elland force-pushed the clean-up-failed-user-provisioning branch from df5ce4f to 13c0351 Compare July 12, 2022 14:38

elland temporarily deployed to cachix July 13, 2022 07:20 Inactive

elland marked this pull request as ready for review July 13, 2022 11:58

supersven self-requested a review July 18, 2022 10:30

supersven previously requested changes Jul 18, 2022

View reviewed changes

mdimjasevic reviewed Jul 19, 2022

View reviewed changes

elland temporarily deployed to cachix August 1, 2022 09:10 Inactive

mdimjasevic reviewed Aug 1, 2022

View reviewed changes

services/brig/src/Brig/API/User.hs Outdated

Copy link

Contributor

mdimjasevic Aug 1, 2022

Choose a reason for hiding this comment

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

This looks like withExceptT.

elland reacted with heart emoji

mdimjasevic approved these changes Aug 1, 2022

View reviewed changes

elland temporarily deployed to cachix August 1, 2022 12:06 Inactive

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 1, 2022

elland added 5 commits August 1, 2022 14:53

Spin off SAML Spar user creation from createUser flow.

f0d190d

Deleted commented out code.

8428fed

Re-enabled code for the missing flows.

371836b

Add new return type with combined errors for user creation.

2917d46

Renamed+unified paths for user creation w/ Spar.

dca675c

elland added 4 commits August 1, 2022 14:53

Format.

e4399cd

Addressed PR comments.

55fc5ad

Addressed PR comments pt 2

b20b395

Added changelog.

c69989e

elland force-pushed the clean-up-failed-user-provisioning branch from dbaa8e4 to c69989e Compare August 1, 2022 14:58

elland temporarily deployed to cachix August 1, 2022 14:58 Inactive

elland merged commit 669ee9c into develop Aug 2, 2022

elland deleted the clean-up-failed-user-provisioning branch August 2, 2022 07:16

supersven mentioned this pull request Aug 16, 2022

Release 2022-08-16 - (expected chart version 4.22.0) #2614

Merged

Comments

Conversation

elland commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fisx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elland Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdimjasevic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdimjasevic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

elland commented Jun 30, 2022 •

edited

Loading

elland Jul 11, 2022 •

edited

Loading