Skip to content

Conversation

@supersven
Copy link
Contributor

@supersven supersven commented Aug 22, 2022

This story gave me some headache: I tried to make things better without adding new fields/tables or expensive queries / indexes. And, had to keep in mind that data, e.g. email addresses or handles may be reused by another account.

Changes:

  • brig accounts are now deleted in a synchronous call (/i/users/:uid/ensure-deleted). This way the SCIM caller should at least get an error when something in brig went wrong.
  • The endpoint can be re-called to ensure that a brig account is deleted. If there is still dangling data around, it re-runs the deletion.
  • Some data may be re-used; e.g. handles or email addresses. These are deleted first. We cannot access them (without expensive queries / indexes) after the user tombstone has been written. So, the tombstone is written after they were deleted. (It has been like that before, however it's an important detail to keep / understand.)
  • spar depends on user data from brig (veid) to delete it's user data. To ensure this is available when needed, spar data is deleted before brig data.

All this could have been much nicer without the query restrictions of Cassandra. 🤔

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.
  • If HTTP endpoint paths have been added or renamed, or feature configs have changed, the endpoint / config-flag checklist (see Wire-employee only backend wiki page) has been followed. (Discussed in Backend channel.)
  • 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.
    • If internal end-points have been added or changed: which services have to be deployed in a specific order?

@supersven supersven temporarily deployed to cachix August 22, 2022 05:59 Inactive
@supersven supersven temporarily deployed to cachix August 22, 2022 05:59 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 22, 2022
@supersven supersven temporarily deployed to cachix August 22, 2022 09:08 Inactive
@supersven supersven temporarily deployed to cachix August 22, 2022 09:08 Inactive
@supersven supersven temporarily deployed to cachix August 22, 2022 09:45 Inactive
@supersven supersven temporarily deployed to cachix August 22, 2022 09:45 Inactive
@supersven supersven temporarily deployed to cachix August 23, 2022 17:10 Inactive
@supersven supersven temporarily deployed to cachix August 23, 2022 17:10 Inactive
@supersven supersven changed the title WIP: fix failed SCIM deletions Make deletions via SCIM more stable Aug 29, 2022
@supersven supersven requested a review from fisx August 29, 2022 09:00
@supersven supersven force-pushed the sventennie/fix-failed-scim-deletions branch from 9ec4ca1 to 6f9e26f Compare August 29, 2022 09:10
@supersven supersven temporarily deployed to cachix August 29, 2022 09:10 Inactive
@supersven supersven temporarily deployed to cachix August 29, 2022 09:10 Inactive
@supersven supersven temporarily deployed to cachix August 29, 2022 14:26 Inactive
@supersven supersven temporarily deployed to cachix August 29, 2022 14:26 Inactive
@supersven supersven marked this pull request as ready for review August 29, 2022 14:27
Copy link
Contributor

@elland elland left a comment

Choose a reason for hiding this comment

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

Looks good. I have a few nitpicky concerns about function names but that's not too important.

@fisx fisx temporarily deployed to cachix August 31, 2022 11:18 Inactive
@fisx fisx temporarily deployed to cachix August 31, 2022 11:18 Inactive
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

I've made two commits simplifying things, but I think there is more:

  • can't we just modify the old Delete effect instead of removing it and adding a different one? semantics is still the same, no?
  • similarly, can we modify the internal delete end-point in brig that was used before, rather than adding a new one?

while i was changing this, i found another instance where users are deleted on brig (namely when the idp is deleted and all the accounts in it are deleted with it). have you intentionally not updated that? why?

@supersven
Copy link
Contributor Author

can't we just modify the old Delete effect instead of removing it and adding a different one? semantics is still the same, no?

@fisx , yes, your two commits regarding this seem to be perfectly reasonable. 👍

similarly, can we modify the internal delete end-point in brig that was used before, rather than adding a new one?

This question follows from your changes in spar (previously it was used by the function you deleted). It sounds like a good idea. 👍
As there's no response body in delete "/i/users/:uid", this change should be fine. (Operationally, the call would become synchronous instead of asynchronous. However, I cannot think of any issues regarding this.)

Speaking of synchronous vs. asynchronous: Haven you seen that the prior behavior was asynchronous (with an event) and this is now synchronous? Is that fine with you?

while i was changing this, i found another instance where users are deleted on brig (namely when the idp is deleted and all the accounts in it are deleted with it). have you intentionally not updated that? why?

I think I wasn't brave enough and a bit overwhelmed by the implicit relationships of side-effects. So, I kept my change set non invasive. Next time, I should be braver... 😄

@supersven supersven temporarily deployed to cachix August 31, 2022 14:22 Inactive
@supersven supersven temporarily deployed to cachix August 31, 2022 14:22 Inactive
@supersven supersven temporarily deployed to cachix September 1, 2022 15:32 Inactive
@supersven supersven temporarily deployed to cachix September 1, 2022 15:32 Inactive
@supersven supersven temporarily deployed to cachix September 1, 2022 16:56 Inactive
@supersven supersven temporarily deployed to cachix September 1, 2022 16:56 Inactive
@supersven supersven temporarily deployed to cachix September 1, 2022 17:09 Inactive
@supersven supersven temporarily deployed to cachix September 1, 2022 17:09 Inactive
@supersven supersven force-pushed the sventennie/fix-failed-scim-deletions branch from 236a703 to 25d7236 Compare September 7, 2022 14:31
@supersven supersven temporarily deployed to cachix September 7, 2022 14:31 Inactive
@supersven supersven temporarily deployed to cachix September 7, 2022 14:31 Inactive
@supersven supersven temporarily deployed to cachix September 7, 2022 14:40 Inactive
@supersven supersven temporarily deployed to cachix September 7, 2022 14:40 Inactive
@supersven supersven merged commit 7a5e718 into develop Sep 8, 2022
@supersven supersven deleted the sventennie/fix-failed-scim-deletions branch September 8, 2022 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants