fix: Don't re-add a user to auto groups while they are being deleted

CHANGELOG.md

@@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Compatibility up to NC36
 
+### Fixed
+
+- Don't re-add a user to the auto groups while they are being deleted, which left orphaned group memberships behind. (#94)
+
 ## 1.7.2 - 2026-08-04
 
 ### Changed

lib/AppInfo/Application.php

@@ -28,6 +28,7 @@
 use OCP\AppFramework\Bootstrap\IBootContext;
 use OCP\AppFramework\Bootstrap\IBootstrap;
 use OCP\AppFramework\Bootstrap\IRegistrationContext;
+use OCP\User\Events\BeforeUserDeletedEvent;
 use OCP\User\Events\UserCreatedEvent;
 use OCP\User\Events\UserFirstTimeLoggedInEvent;
 use OCP\User\Events\PostLoginEvent;
@@ -48,6 +49,7 @@ public function __construct()
 
 	public function register(IRegistrationContext $context): void
 	{
+		$context->registerEventListener(BeforeUserDeletedEvent::class, AutoGroupsListener::class);
 		$context->registerEventListener(UserCreatedEvent::class, AutoGroupsListener::class);
 		$context->registerEventListener(UserFirstTimeLoggedInEvent::class, AutoGroupsListener::class);
 		$context->registerEventListener(UserAddedEvent::class, AutoGroupsListener::class);

lib/AutoGroupsManager.php

@@ -36,6 +36,23 @@
 
 class AutoGroupsManager
 {
+	/**
+	 * UIDs whose deletion is in flight, as `uid => true`.
+	 *
+	 * Never cleared, and deliberately so. It cannot grow over time: PHP rebuilds
+	 * the container every request, so this dies with the request that filled it,
+	 * and a request normally deletes exactly one user. Only a looped
+	 * `occ user:delete` puts more than one entry in it, at one short string each.
+	 *
+	 * Clearing it on UserDeletedEvent would be the obvious lifecycle, and is the
+	 * riskier option: it is only correct if every group removal fires before that
+	 * event, and if one fires after, the hook this guard exists to stop runs again.
+	 * A stale entry costs nothing — auto-group work is skipped for a user whose
+	 * deletion was abandoned, until the request ends moments later.
+	 *
+	 * @var array<string, true>
+	 */
+	private array $deletingUsers = [];
 
     /**
      * AutoGroupsManager constructor.
@@ -87,6 +104,15 @@ public function __construct(
         }
     }
 
+    /**
+     * Remember that this user is being deleted, so the group removals the deletion
+     * is about to perform are not undone by the auto-group hooks.
+     */
+    public function markUserAsDeleting(string $uid): void
+    {
+        $this->deletingUsers[$uid] = true;
+    }
+
     /**
      * The event handler to check group assignment for a user
      */
@@ -99,6 +125,18 @@ public function addAndRemoveAutoGroups(Event $event): void
         // Get user information
         $user = $event->getUser();
 
+		if (isset($this->deletingUsers[$user->getUID()])) {
+			// Deleting a user removes them from every group first and deletes the
+			// user record afterwards, so each removal fires UserRemovedEvent while
+			// the user still exists — the check below cannot catch it. Re-adding
+			// them here puts a row back into oc_group_user that the rest of the
+			// deletion has already passed, leaving an orphan: a group membership
+			// for a user that no longer exists. Nextcloud then logs "Found one
+			// enabled account that is removed from its backend" for it on every
+			// user listing, forever.
+			return;
+		}
+
 		if (!$this->userManager->userExists($user->getUID())) {
 			// Avoid doing any group manipulation when running inside
 			// OC\User\BackgroundJobs\CleanupDeletedUsers

lib/Listener/AutoGroupsListener.php

@@ -28,6 +28,7 @@
 use OCP\AppFramework\Services\IAppConfig;
 use OCP\EventDispatcher\Event;
 use OCP\EventDispatcher\IEventListener;
+use OCP\User\Events\BeforeUserDeletedEvent;
 use OCP\User\Events\UserCreatedEvent;
 use OCP\User\Events\UserFirstTimeLoggedInEvent;
 use OCP\User\Events\PostLoginEvent;
@@ -38,7 +39,7 @@
 
 use OCA\AutoGroups\AutoGroupsManager;
 
-/** @template-implements IEventListener<UserCreatedEvent|UserFirstTimeLoggedInEvent|UserAddedEvent|UserRemovedEvent|PostLoginEvent|UserLoggedInEvent|BeforeGroupDeletedEvent> */
+/** @template-implements IEventListener<BeforeUserDeletedEvent|UserCreatedEvent|UserFirstTimeLoggedInEvent|UserAddedEvent|UserRemovedEvent|PostLoginEvent|UserLoggedInEvent|BeforeGroupDeletedEvent> */
 class AutoGroupsListener implements IEventListener
 {
     public function __construct(
@@ -49,7 +50,13 @@ public function __construct(
 	#[\Override]
     public function handle(Event $event): void
     {
-        if ($event instanceof UserCreatedEvent || $event instanceof UserFirstTimeLoggedInEvent) {
+        if ($event instanceof BeforeUserDeletedEvent) {
+            // Fired before the deletion removes the user from their groups. Without
+            // this the removals fire UserRemovedEvent, the modification hook puts
+            // the user straight back into the auto groups, and the row survives the
+            // user record — see AutoGroupsManager::addAndRemoveAutoGroups().
+            $this->manager->markUserAsDeleting($event->getUser()->getUID());
+        } elseif ($event instanceof UserCreatedEvent || $event instanceof UserFirstTimeLoggedInEvent) {
             if ($this->appConfig->getAppValueBool('creation_hook', true)) {
                 $this->manager->addAndRemoveAutoGroups($event);
             }

tests/Unit/AutoGroupsManagerTest.php

@@ -25,6 +25,7 @@
 namespace OCA\AutoGroups\Tests\Unit;
 
 use OCP\Group\Events\BeforeGroupDeletedEvent;
+use OCP\Group\Events\UserRemovedEvent;
 use OCP\IUserManager;
 use OCP\User\Events\UserCreatedEvent;
 
@@ -261,6 +262,52 @@ public function testDeletedUserIsIgnored()
         $agm->addAndRemoveAutoGroups($event);
     }
 
+    public function testUserBeingDeletedIsIgnoredWhileStillPresent()
+    {
+        $event = $this->createMock(UserRemovedEvent::class);
+        $event->expects($this->once())
+            ->method('getUser')
+            ->willReturn($this->testUser);
+
+        // The case testDeletedUserIsIgnored() cannot cover. Deleting a user removes
+        // them from every group *before* deleting the user record, so each removal
+        // fires UserRemovedEvent while userExists() is still true — re-adding them
+        // here leaves a group membership behind for a user that then ceases to
+        // exist. Knowing the deletion is in flight is the only thing that
+        // distinguishes this from an admin removing someone by hand, which the
+        // modification hook is supposed to undo.
+        $this->userManager->expects($this->never())->method('userExists');
+        $this->groupManager->expects($this->never())->method('getUserGroupIds');
+        $this->groupManager->expects($this->never())->method('search');
+
+        $agm = $this->createAutoGroupsManager(['autogroup1', 'autogroup2'], ['overridegroup1']);
+        $agm->markUserAsDeleting('testuser');
+        $agm->addAndRemoveAutoGroups($event);
+    }
+
+    public function testOtherUsersAreUnaffectedByAPendingDeletion()
+    {
+        $event = $this->createMock(UserRemovedEvent::class);
+        $event->expects($this->once())
+            ->method('getUser')
+            ->willReturn($this->testUser);
+
+        // The flag is per uid, not a global "a deletion is happening" switch: one
+        // user being deleted must not stop the hooks working for everyone else in
+        // the same request.
+        $this->expectUserExistsCheck(true);
+        $this->groupManager->expects($this->once())
+            ->method('getUserGroupIds')
+            ->willReturn(['autogroup1']);
+        $this->groupManager->expects($this->once())
+            ->method('search')
+            ->willReturn([]);
+
+        $agm = $this->createAutoGroupsManager(['autogroup1'], ['overridegroup1']);
+        $agm->markUserAsDeleting('someone.else');
+        $agm->addAndRemoveAutoGroups($event);
+    }
+
     public function testGroupDeletionPrevented()
     {
         $groupMock = $this->createMock(IGroup::class);