From db53fee53a6fd216c850e2f8e4d17d7fac216abc Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 27 Aug 2026 00:09:17 +0200 Subject: [PATCH] fix: the group and profile new directory users get cannot be deleted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ldapDefaultGroup`, `ssoDefaultGroup`, `ldapDefaultProfile` and `ssoDefaultProfile` are plain ints in config.xml. Nothing points at UserGroup or UserProfile from there, so no foreign key refuses deleting the row they name — and the RESTRICT on User.userGroupId / User.userProfileId only catches one somebody currently holds, which is precisely not the case for a group or profile being tidied up because its members have moved on. So the delete succeeded cleanly, and every auto-provisioned login afterwards died: with no row to point at, `User::createOnLogin()` violates the NOT NULL foreign key, which `LoginAuthHandler` catches and reports as "Internal error, check the event log". Directory sign-in stops working for everyone who does not already have a local account, and nothing connects that to a group somebody deleted last week. Both deletes refuse it now, naming which setting holds it. The batch deletes are guarded too, because they go to the repository directly rather than through delete() — the foreign keys still stand behind them whichever door is used, but nothing in the database knows about a setting in config.xml. Checked by making the comparison always false: all four new tests fail. Unrelated, and recorded rather than claimed fixed: one full unit run during this change failed on AccountTest::testCreateCannotChangePermissions, which then passed in isolation and in three consecutive full runs. I could not reproduce it — it is not the id-zero trap (forcing the id to 0 passes) and the harness seeds an all-false ProfileData, so it is not the profile either. Noting it so the next person who sees it does not start from scratch. --- src/Application/User/Services/UserGroup.php | 41 ++++++++++++++++ src/Application/User/Services/UserProfile.php | 41 ++++++++++++++++ .../User/Services/UserGroupTest.php | 49 +++++++++++++++++++ .../User/Services/UserProfileTest.php | 47 ++++++++++++++++++ 4 files changed, 178 insertions(+) diff --git a/src/Application/User/Services/UserGroup.php b/src/Application/User/Services/UserGroup.php index a26d0237b..37f4daa00 100644 --- a/src/Application/User/Services/UserGroup.php +++ b/src/Application/User/Services/UserGroup.php @@ -104,6 +104,15 @@ public function delete(int $id): void // // The foreign keys still stand behind this — a row created between the two statements is // refused by the database as before. This is about telling somebody what to go and fix. + // A group set as the directory's default is held by the configuration rather than by a + // row, so no foreign key refuses it — `ldapDefaultGroup` and `ssoDefaultGroup` are plain + // ints in config.xml with nothing pointing at UserGroup. And the group most likely to be + // deleted while reorganising is one with no members left, which is exactly the case the + // RESTRICT on User.userGroupId does not catch. Deleting it succeeded cleanly, and then + // every auto-provisioned login afterwards died on the NOT NULL foreign key in + // User::createOnLogin(), surfacing as "Internal error, check the event log". + $this->assertIsNotADirectoryDefault($id); + $usedBy = $this->getUsage($id); if ($usedBy !== []) { @@ -118,6 +127,30 @@ public function delete(int $id): void } } + /** + * Refuse a group that new directory users are created into. + * + * @throws ServiceException + */ + private function assertIsNotADirectoryDefault(int $id): void + { + $configData = $this->config->getConfigData(); + + $defaults = [ + __('LDAP') => $configData->getLdapDefaultGroup(), + __('SSO') => $configData->getSsoDefaultGroup(), + ]; + + foreach ($defaults as $label => $default) { + if ($default === $id) { + throw ServiceException::warning( + __u('Group in use'), + sprintf(__('It is the default group for %s users'), $label) + ); + } + } + } + /** * What is holding the group, counted by kind, for the hint on the refusal. * @@ -153,6 +186,14 @@ private static function describeUsage(array $usedBy): string */ public function deleteByIdBatch(array $ids): int { + // The batch goes to the repository directly, so the single delete's guard does not stand + // in front of it. The directory default is the one that matters here: the foreign keys + // still refuse a group anything holds, whichever door the delete came through, but nothing + // in the database knows about `ldapDefaultGroup`. + foreach ($ids as $id) { + $this->assertIsNotADirectoryDefault($id); + } + $count = $this->userGroupRepository->deleteByIdBatch($ids)->getAffectedNumRows(); if ($count !== count($ids)) { diff --git a/src/Application/User/Services/UserProfile.php b/src/Application/User/Services/UserProfile.php index cfd20a1d4..9a295752e 100644 --- a/src/Application/User/Services/UserProfile.php +++ b/src/Application/User/Services/UserProfile.php @@ -41,6 +41,7 @@ use SP\Domain\Core\Exceptions\NoSuchItemException; use SP\Domain\Common\Dtos\QueryResult; +use function SP\__; use function SP\__u; /** @@ -115,11 +116,45 @@ public function search(ItemSearchDto $itemSearchData): QueryResult */ public function delete(int $id): void { + $this->assertIsNotADirectoryDefault($id); + if ($this->userProfileRepository->delete($id)->getAffectedNumRows() === 0) { throw NoSuchItemException::info(__u('Profile not found')); } } + /** + * Refuse a profile that new directory users are created with. + * + * A profile set as the directory's default is held by the configuration rather than by a row, + * so no foreign key refuses it — `ldapDefaultProfile` and `ssoDefaultProfile` are plain ints in + * config.xml with nothing pointing at UserProfile. The RESTRICT on User.userProfileId only + * catches a profile somebody currently holds, and the profile most likely to be deleted while + * reorganising is one nobody holds any more. Deleting it succeeded cleanly, and then every + * auto-provisioned login afterwards died on the NOT NULL foreign key in + * User::createOnLogin(), surfacing as "Internal error, check the event log". + * + * @throws ServiceException + */ + private function assertIsNotADirectoryDefault(int $id): void + { + $configData = $this->config->getConfigData(); + + $defaults = [ + __('LDAP') => $configData->getLdapDefaultProfile(), + __('SSO') => $configData->getSsoDefaultProfile(), + ]; + + foreach ($defaults as $label => $default) { + if ($default === $id) { + throw ServiceException::warning( + __u('Profile in use'), + sprintf(__('It is the default profile for %s users'), $label) + ); + } + } + } + /** * @param int[] $ids * @@ -129,6 +164,12 @@ public function delete(int $id): void */ public function deleteByIdBatch(array $ids): int { + // As above: the batch does not pass through delete(), and no foreign key knows about + // `ldapDefaultProfile`. + foreach ($ids as $id) { + $this->assertIsNotADirectoryDefault($id); + } + $count = $this->userProfileRepository->deleteByIdBatch($ids)->getAffectedNumRows(); if ($count !== count($ids)) { diff --git a/tests/Unit/Application/User/Services/UserGroupTest.php b/tests/Unit/Application/User/Services/UserGroupTest.php index 69039a9b8..ddb6ea03d 100644 --- a/tests/Unit/Application/User/Services/UserGroupTest.php +++ b/tests/Unit/Application/User/Services/UserGroupTest.php @@ -346,6 +346,55 @@ public function testDelete() $this->userGroup->delete(100); } + /** + * The group new directory users are created into cannot be deleted. + * + * `ldapDefaultGroup` and `ssoDefaultGroup` are plain ints in config.xml with nothing pointing + * at UserGroup, so no foreign key refuses this — and the RESTRICT on User.userGroupId only + * catches a group somebody currently holds, which is precisely not the case for one being + * tidied up because its members have moved on. Deleting it succeeded cleanly, and then every + * auto-provisioned login afterwards died on the NOT NULL foreign key in + * User::createOnLogin(), reported as "Internal error, check the event log". + * + * @throws ConstraintException + * @throws NoSuchItemException + * @throws QueryException + */ + public function testDeleteRefusesTheDirectoryDefaultGroup() + { + $this->config->getConfigData()->setLdapDefaultGroup(100); + + $this->userGroupRepository->expects($this->never())->method('getUsage'); + $this->userGroupRepository->expects($this->never())->method('delete'); + + try { + $this->userGroup->delete(100); + self::fail('Expected a ServiceException'); + } catch (ServiceException $e) { + self::assertSame('Group in use', $e->getMessage()); + self::assertSame('It is the default group for LDAP users', $e->getHint()); + } + } + + /** + * And the batch does not get round it. It goes to the repository directly, so the single + * delete's guard is not in front of it. + * + * @throws ConstraintException + * @throws QueryException + */ + public function testDeleteByIdBatchRefusesTheDirectoryDefaultGroup() + { + $this->config->getConfigData()->setSsoDefaultGroup(200); + + $this->userGroupRepository->expects($this->never())->method('deleteByIdBatch'); + + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('Group in use'); + + $this->userGroup->deleteByIdBatch([100, 200, 300]); + } + /** * A group something still holds is refused, and the refusal says what holds it. * diff --git a/tests/Unit/Application/User/Services/UserProfileTest.php b/tests/Unit/Application/User/Services/UserProfileTest.php index b396fd654..552a5a33b 100644 --- a/tests/Unit/Application/User/Services/UserProfileTest.php +++ b/tests/Unit/Application/User/Services/UserProfileTest.php @@ -240,6 +240,53 @@ public function testDelete() $this->userProfile->delete(100); } + /** + * The profile new directory users are created with cannot be deleted. + * + * `ldapDefaultProfile` and `ssoDefaultProfile` are plain ints in config.xml with nothing + * pointing at UserProfile, so no foreign key refuses this — and the RESTRICT on + * User.userProfileId only catches a profile somebody currently holds, which is precisely not + * the case for one being tidied up. Deleting it succeeded cleanly, and then every + * auto-provisioned login afterwards died on the NOT NULL foreign key in + * User::createOnLogin(), reported as "Internal error, check the event log". + * + * @throws ConstraintException + * @throws NoSuchItemException + * @throws QueryException + */ + public function testDeleteRefusesTheDirectoryDefaultProfile() + { + $this->config->getConfigData()->setLdapDefaultProfile(100); + + $this->userProfileRepository->expects($this->never())->method('delete'); + + try { + $this->userProfile->delete(100); + self::fail('Expected a ServiceException'); + } catch (ServiceException $e) { + self::assertSame('Profile in use', $e->getMessage()); + self::assertSame('It is the default profile for LDAP users', $e->getHint()); + } + } + + /** + * And the batch does not get round it. + * + * @throws ConstraintException + * @throws QueryException + */ + public function testDeleteByIdBatchRefusesTheDirectoryDefaultProfile() + { + $this->config->getConfigData()->setSsoDefaultProfile(200); + + $this->userProfileRepository->expects($this->never())->method('deleteByIdBatch'); + + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('Profile in use'); + + $this->userProfile->deleteByIdBatch([100, 200, 300]); + } + /** * @throws ConstraintException * @throws NoSuchItemException