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