fix: Make user removal more resilient #47896
Open
+320
−82
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
susnux
commented
Sep 11, 2024
susnux
commented
Sep 11, 2024
susnux
commented
Sep 11, 2024
susnux
commented
Sep 11, 2024
susnux
commented
Sep 11, 2024
susnux
commented
Sep 11, 2024
susnux
commented
Sep 11, 2024
susnux
commented
Sep 11, 2024
susnux
commented
Sep 11, 2024
susnux
commented
Sep 11, 2024
susnux
commented
Sep 11, 2024
susnux
commented
Sep 11, 2024
susnux
force-pushed
the
fix/resiliant-user-removal
branch
from
4648397
to
6c21dc7
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
lib/private/Authentication/Listeners/UserDeletedFilesCleanupListener.php
Fixed
Show fixed
Hide fixed
susnux
force-pushed
the
fix/resiliant-user-removal
branch
2 times, most recently
from
e58b96c
to
b714280
Compare
susnux
added
3. to review
susnux
requested review from
nickvergessen,
Altahrim,
artonge,
yemkareems and
come-nc
|
@blizzz I think you got a point here, we need to cover the case where |
susnux
commented
Sep 11, 2024
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
susnux
force-pushed
the
fix/resiliant-user-removal
branch
from
01472dc
to
9b70c7e
Compare
artonge
approved these changes
Sep 12, 2024
come-nc
reviewed
Sep 12, 2024
| $this->config->setUserValue($this->uid, 'core', 'deleted.backup-home', $this->getHome()); | ||
|
|
||
| // Try to delete the user on the backend | ||
| $result = $this->backend?->deleteUser($this->uid); |
There was a problem hiding this comment.
In what situation is $this->backend null?
There was a problem hiding this comment.
To be honest: No idea. The backend is not required in the constructor and there are some code places where the user is constructed without any backend.
So two ways:
- Make this null safe as is now
- Or assert the backend is set and fail if not.
|
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. |
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 <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
susnux
force-pushed
the
fix/resiliant-user-removal
branch
from
9b70c7e
to
b7fd9af
Compare
|
/backport to stable30 |
|
/backport to stable29 |
|
/backport to stable28 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
User::deletefailed #47900Summary
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::deletemethod and then let the background job cleanup the state. In my test cases this worked as expected.Checklist