fix: Don't re-add a user to auto groups while they are being deleted - #96
Merged
Conversation
Deleting a user removes them from every group before deleting the user record, so each removal fires UserRemovedEvent while userExists() is still true. The modification hook then put the user straight back into the auto groups, and that row outlived the user record — leaving a membership in oc_group_user for a user that no longer exists. Nextcloud logs "Found one enabled account that is removed from its backend, but still exists in Nextcloud database" for each such row on every user listing. On the instance where this was found, an hourly API consumer was producing 11 of them per run, and the accounts had been deleted through the GUI months apart. The existing userExists() guard cannot catch this: it was written for CleanupDeletedUsers, where the user really is gone by the time the event fires. During an interactive deletion the user still exists. So the deletion is now tracked from BeforeUserDeletedEvent, which fires before the group removals begin, and the hook bails out for that uid. The flag is never cleared, deliberately. PHP rebuilds the container every request, so it cannot grow over time, and clearing it on UserDeletedEvent would only be correct if every group removal fires before that event — if one fires after, the bug is back. A stale entry costs nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jo23sh
force-pushed
the
fix/no-readd-during-user-deletion
branch
from
September 1, 2026 06:12
4c4e9dc to
e1041de
Compare
This file contains hidden or 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
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.
The bug
Deleting a user removes them from every group before deleting the user record, so each removal fires
UserRemovedEventwhileuserExists()is stilltrue. The modification hook then puts the user straight back into the auto groups — and that row outlives the user record, leaving a membership inoc_group_userfor a user that no longer exists.Nextcloud logs this for every such row, on every user listing:
Found on an instance where an hourly API consumer was producing 11 of them per run. The accounts had been deleted through the GUI, months apart, and were gone from
oc_users,oc_accountsandoc_preferences— only the group rows remained, all in the configured auto group.Why the existing guard misses it
f5458a2added auserExists()check, but it was written forCleanupDeletedUsers, where the user really is gone by the time the event fires. During an interactive deletion the user still exists, so the guard passes.Dropping
UserRemovedEventisn't an option — putting someone back when an admin removes them by hand is whatmodification_hookis for.The fix
Track the deletion from
BeforeUserDeletedEvent, which fires before the group removals begin, and bail out for that uid.The flag is never cleared, deliberately: PHP rebuilds the container every request, so it cannot grow over time, and clearing it on
UserDeletedEventwould only be correct if every group removal fires before that event — if one fires after, the bug is back. A stale entry costs nothing.Tests
testUserBeingDeletedIsIgnoredWhileStillPresent— the case the old guard cannot covertestOtherUsersAreUnaffectedByAPendingDeletion— the flag is per uid, not a global switchOnly
php -lwas run locally (no PHP on the machine this was written on); relying on CI for phpunit.