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.

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

fix: Make user removal more resilient #47896

Merged
merged 2 commits into from
Oct 7, 2024
Merged

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Sep 11, 2024

Summary

Currently there is a problem if an exception is thrown in User::delete, because at that point the user is already removed from the backend, but not all data is deleted.

There is no way to recover from this state, as the user is gone no information is available anymore. This means the data is still available on the server but can not removed by any API anymore.

The solution here is to first set a flag and backup the user home, this can be used to recover failed user deletions in a way the delete can be re-tried.

Discussion

I am not 100% sure this is the best way, but as out API needs real user instances to delete user data, I think we need to go with the "FailedUsersBackend".
I tested this with injecting various exceptions in random places in the User::delete method and then let the background job cleanup the state. In my test cases this worked as expected.

Checklist

@susnux susnux force-pushed the fix/resiliant-user-removal branch 2 times, most recently from e58b96c to b714280 Compare September 11, 2024 14:51
@susnux susnux marked this pull request as ready for review September 11, 2024 15:00
@susnux susnux added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 11, 2024
@susnux
Copy link
Contributor Author

susnux commented Sep 11, 2024

@blizzz I think you got a point here, we need to cover the case where backend->deleteUser threw an exception.
(if it returned we are always in a defined state)

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

Not tested, but looking ok

lib/private/User/FailedUsersBackend.php Outdated Show resolved Hide resolved
lib/private/User/User.php Outdated Show resolved Hide resolved
lib/private/User/User.php Outdated Show resolved Hide resolved
lib/private/User/User.php Outdated Show resolved Hide resolved
lib/private/User/BackgroundJobs/CleanupDeletedUsers.php Outdated Show resolved Hide resolved
@come-nc
Copy link
Contributor

come-nc commented Sep 12, 2024

I like the idea, not sure I like the fact that it runs daily.

@susnux
Copy link
Contributor Author

susnux commented Sep 19, 2024

I like the idea, not sure I like the fact that it runs daily.

It simply skips if there is nothing to do, otherwise it only does things in the maintenance window.

@susnux
Copy link
Contributor Author

susnux commented Sep 24, 2024

/backport to stable30

@susnux
Copy link
Contributor Author

susnux commented Sep 24, 2024

/backport to stable29

@susnux
Copy link
Contributor Author

susnux commented Sep 24, 2024

/backport to stable28

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

I still disagree that this should run daily.
It should be a repair-step. We have ton of other repair steps or occ commands to fix problems, non of them run daily.

Currently there is a problem if an exception is thrown in `User::delete`,
because at that point the user is already removed from the backend,
but not all data is deleted.

There is no way to recover from this state, as the user is gone no information is available anymore.
This means the data is still available on the server but can not removed by any API anymore.

The solution here is to first set a flag and backup the user home,
this can be used to recover failed user deletions in a way the delete can be re-tried.

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux
Copy link
Contributor Author

susnux commented Sep 26, 2024

I still disagree that this should run daily.
It should be a repair-step. We have ton of other repair steps or occ commands to fix problems, non of them run daily.

But it is not expensive, yet it ensures that you are compliant with data protection laws as deleted users are really deleted.

@susnux susnux requested a review from come-nc September 26, 2024 19:13
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Ok, legal requirement is a good reason to run it daily.

@susnux susnux merged commit 3095a92 into master Oct 7, 2024
173 checks passed
@susnux susnux deleted the fix/resiliant-user-removal branch October 7, 2024 09:33
@susnux
Copy link
Contributor Author

susnux commented Oct 7, 2024

/backport to stable30

@susnux
Copy link
Contributor Author

susnux commented Oct 7, 2024

/backport to stable29

@susnux
Copy link
Contributor Author

susnux commented Oct 7, 2024

/backport to stable28

Copy link

backportbot bot commented Oct 7, 2024

The backport to stable28 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable28
git pull origin stable28

# Create the new backport branch
git checkout -b backport/47896/stable28

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 16833aff d57a2dd4

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/47896/stable28

Error: Failed to create pull request: Validation Failed: {"resource":"PullRequest","code":"custom","message":"A pull request already exists for nextcloud:backport/47896/stable28."}


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

Copy link

backportbot bot commented Oct 7, 2024

The backport to stable29 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable29
git pull origin stable29

# Create the new backport branch
git checkout -b backport/47896/stable29

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 16833aff d57a2dd4

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/47896/stable29

Error: Failed to create pull request: Validation Failed: {"resource":"PullRequest","code":"custom","message":"A pull request already exists for nextcloud:backport/47896/stable29."}


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

Copy link

backportbot bot commented Oct 7, 2024

The backport to stable30 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable30
git pull origin stable30

# Create the new backport branch
git checkout -b backport/47896/stable30

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 16833aff d57a2dd4

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/47896/stable30

Error: Failed to create pull request: Validation Failed: {"resource":"PullRequest","code":"custom","message":"A pull request already exists for nextcloud:backport/47896/stable30."}


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

@provokateurin
Copy link
Member

Please include #48610 in the backports @susnux

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User data not deleted if User::delete failed
5 participants