From b5eca351585d91496cbeb3a0c75507db1b3785a8 Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 17 Aug 2026 20:01:26 +0200 Subject: [PATCH] List what actually blocks a user being deleted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The user view has a panel answering "what would break if this user were removed". It has to agree with what happens when somebody presses delete, and it did not. Six foreign keys reference `User` with no `ON DELETE`, which is `RESTRICT`, so any of them stops the removal: `Account.userId` / `userEditId`, `AccountHistory.userId` / `userEditId`, `Notification.userId` and `PublicLink.userId`. The panel covered the accounts and the public links — and, informatively, the account and group memberships, which cascade away and block nothing — but not the account history and not the notifications. So an administrator retiring somebody reads a panel listing nothing that prevents the removal, presses delete, and is refused: the person once edited an account, or has a single unread notification. Neither is visible anywhere, and neither is something they can act on without being told about it. The history is grouped by account rather than listed per revision — it keeps a row for every change ever made, so a long-lived account would otherwise fill the panel with identical-looking entries. The name is aggregated because the account may have been renamed since, and may not exist any more. The `switch` assigning an icon per kind became a `match`, which is one assignment instead of one per kind, and takes the PHPStan baseline entry for that dynamic property from four occurrences to one. `UserDeletionBlockersTest` runs against a real database, since `IntegrationTestCase` mocks the database away and a test that inserts a history row would be asking a mock whether it was reported. Each case asserts both halves — the panel reports it *and* the delete is really refused — because either alone can pass while this is broken. A fourth case is the control: a user nothing refers to is reported as unused and really can be deleted. The existing unit test counted bind values, so it stayed green while two of the six blocking relations were missing from the query entirely. --- phpstan.baseline.neon | 2 +- .../In/Web/Controllers/User/UserViewBase.php | 23 +- .../Adapter/Out/User/Repositories/User.php | 68 ++- .../User/UserDeletionBlockersTest.php | 394 ++++++++++++++++++ .../In/Web/Controllers/User/UserUsageTest.php | 4 + .../Out/User/Repositories/UserTest.php | 48 ++- 6 files changed, 518 insertions(+), 21 deletions(-) create mode 100644 tests/Integration/Application/User/UserDeletionBlockersTest.php diff --git a/phpstan.baseline.neon b/phpstan.baseline.neon index eb50532a0..2eb6f0253 100644 --- a/phpstan.baseline.neon +++ b/phpstan.baseline.neon @@ -1986,7 +1986,7 @@ parameters: - message: '#^Access to an undefined property SP\\Domain\\Common\\Models\\Simple\:\:\$icon\.$#' identifier: property.notFound - count: 4 + count: 1 path: src/Infrastructure/Adapter/In/Web/Controllers/User/UserViewBase.php - diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/User/UserViewBase.php b/src/Infrastructure/Adapter/In/Web/Controllers/User/UserViewBase.php index 6db4495c5..13adbb212 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/User/UserViewBase.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/User/UserViewBase.php @@ -132,19 +132,16 @@ protected function setViewData(?int $userId = null, bool $isView = false): void 'usage', array_map( static function ($value) { - switch ($value->ref) { - case 'Account': - $value->icon = 'description'; - break; - case 'UserGroup': - $value->icon = 'group'; - break; - case 'PublicLink': - $value->icon = 'link'; - break; - default: - $value->icon = 'info_outline'; - } + // One assignment rather than one per kind: a kind the map does not know + // still gets an icon, so a row is never rendered without one. + $value->icon = match ($value->ref) { + 'Account' => 'description', + 'AccountHistory' => 'history', + 'Notification' => 'notifications', + 'PublicLink' => 'link', + 'UserGroup' => 'group', + default => 'info_outline', + }; return $value; }, diff --git a/src/Infrastructure/Adapter/Out/User/Repositories/User.php b/src/Infrastructure/Adapter/Out/User/Repositories/User.php index 20e230892..6f7683250 100644 --- a/src/Infrastructure/Adapter/Out/User/Repositories/User.php +++ b/src/Infrastructure/Adapter/Out/User/Repositories/User.php @@ -28,9 +28,11 @@ use Exception; use JsonException; use SP\Domain\Account\Models\Account as AccountModel; +use SP\Domain\Account\Models\AccountHistory as AccountHistoryModel; use SP\Domain\Account\Models\AccountToUser as AccountToUserModel; use SP\Domain\Account\Models\PublicLink as PublicLinkModel; use SP\Domain\Client\Models\Client as ClientModel; +use SP\Domain\Notification\Models\Notification as NotificationModel; use SP\Domain\Common\Models\Simple; use SP\Domain\Core\Dtos\ItemSearchDto; use SP\Domain\Core\Exceptions\ConstraintException; @@ -588,6 +590,19 @@ public function getUserEmailById(array $ids): QueryResult /** * Returns the usage of the given user's id * + * This is what the user view shows as "what would break if this user were removed", so it has + * to cover the relations that actually stop the removal. Six foreign keys reference `User` + * without `ON DELETE`, which is `RESTRICT`: `Account.userId` / `userEditId`, + * `AccountHistory.userId` / `userEditId`, `Notification.userId` and `PublicLink.userId`. Two + * of those — the account history and the notifications — were missing here, so the panel could + * show nothing that blocks a delete while the delete was blocked all the same: having once + * edited an account, or having a single unread notification, is enough. The administrator was + * left with a refusal and a panel that disagreed with it. + * + * The account memberships and group memberships listed alongside them cascade away and do not + * block anything; they are shown because they are still worth knowing about before removing + * somebody. + * * @param int $id * * @return QueryResult @@ -691,11 +706,62 @@ public function getUsageForUser(int $id): QueryResult ), '"PublicLink" AS ref' ] + ) + // One row per account rather than per revision: the history keeps a row for + // every change ever made, and an account edited fifty times would otherwise + // fill the panel with fifty identical-looking entries. The name is taken from + // one of the recorded revisions, which is why it is aggregated — the account + // may have been renamed since, and may not exist at all any more. + ->unionAll() + ->from(AccountHistoryModel::TABLE) + ->innerJoin( + ClientModel::TABLE, + sprintf('%s.id = %s.clientId', ClientModel::TABLE, AccountHistoryModel::TABLE) + ) + ->where( + sprintf( + '%s.userId = :userId5 OR %s.userEditId = :userEditId2', + AccountHistoryModel::TABLE, + AccountHistoryModel::TABLE + ) + ) + ->groupBy([sprintf('%s.accountId', AccountHistoryModel::TABLE)]) + ->cols( + [ + sprintf('%s.accountId as id', AccountHistoryModel::TABLE), + sprintf( + 'CONCAT(MAX(%s.name), "(", MAX(%s.name), ")") AS name', + AccountHistoryModel::TABLE, + ClientModel::TABLE + ), + '"AccountHistory" AS ref' + ] + ) + ->unionAll() + ->from(NotificationModel::TABLE) + ->where(sprintf('%s.userId = :userId6', NotificationModel::TABLE)) + ->cols( + [ + sprintf('%s.id as id', NotificationModel::TABLE), + sprintf('%s.component AS name', NotificationModel::TABLE), + '"Notification" AS ref' + ] ), 'Items' ) ->orderBy(['Items.ref']) - ->bindValues(['userId1' => $id, 'userEditId' => $id, 'userId2' => $id, 'userId3' => $id, 'userId4' => $id]); + ->bindValues( + [ + 'userId1' => $id, + 'userEditId' => $id, + 'userId2' => $id, + 'userId3' => $id, + 'userId4' => $id, + 'userId5' => $id, + 'userEditId2' => $id, + 'userId6' => $id, + ] + ); return $this->db->runQuery(QueryData::build($query)); } diff --git a/tests/Integration/Application/User/UserDeletionBlockersTest.php b/tests/Integration/Application/User/UserDeletionBlockersTest.php new file mode 100644 index 000000000..8743dcbbb --- /dev/null +++ b/tests/Integration/Application/User/UserDeletionBlockersTest.php @@ -0,0 +1,394 @@ +root = FileSystem::buildPath( + sys_get_temp_dir(), + 'syspass-user-blockers-' . bin2hex(random_bytes(6)) + ); + $this->configPath = FileSystem::buildPath($this->root, 'config'); + + foreach ([$this->configPath, $this->cachePath(), $this->tmpPath(), $this->backupPath()] as $dir) { + if (!mkdir($dir, 0777, true) && !is_dir($dir)) { + self::fail(sprintf('Directory "%s" was not created', $dir)); + } + } + + file_put_contents( + FileSystem::buildPath($this->configPath, 'config.xml'), + getResource('config', 'config.xml') + ); + + $this->pdo = getDbHandler()->getConnection(); + } + + protected function tearDown(): void + { + FileSystem::rmdirRecursive($this->root); + + parent::tearDown(); + } + + /** + * A user who appears only in an account's history is reported as being in use — and really + * cannot be deleted. + * + * The two halves are asserted together on purpose. Either one alone can pass while the feature + * is broken: the panel could list something that does not block, or the delete could be blocked + * by something the panel never mentions. That second case is exactly what this fixes. + */ + #[Test] + public function anAccountsHistoryKeepsAUserAndTheViewSaysSo(): void + { + $userId = $this->createUser(); + $accountId = $this->giveTheUserAHistoryRow($userId); + + $usage = $this->usageFor($userId); + + self::assertContains( + 'AccountHistory', + array_column($usage, 'ref'), + 'the user is named in an account history row, which blocks their removal, and the ' + . 'panel that lists what would break does not mention it' + ); + + $entry = self::entryFor($usage, 'AccountHistory'); + + self::assertSame($accountId, (int)$entry['id'], 'the entry must point at the account it came from'); + self::assertNotEmpty($entry['name'], 'an entry with no name tells the administrator nothing'); + + $this->assertTheDeleteIsRefused($userId); + } + + /** + * The same for a notification, which is the cheaper way to become undeletable: one unread + * message is enough, and nothing about a notification suggests it is holding an account open. + */ + #[Test] + public function aNotificationKeepsAUserAndTheViewSaysSo(): void + { + $userId = $this->createUser(); + $notificationId = $this->giveTheUserANotification($userId); + + $usage = $this->usageFor($userId); + + self::assertContains( + 'Notification', + array_column($usage, 'ref'), + 'a single notification blocks the removal and the panel does not mention it' + ); + + $entry = self::entryFor($usage, 'Notification'); + + self::assertSame($notificationId, (int)$entry['id']); + self::assertSame('Test Component', $entry['name'], 'the entry names the notification component'); + + $this->assertTheDeleteIsRefused($userId); + } + + /** + * An account edited many times is one entry, not one per revision. + * + * The history keeps a row for every change ever made to an account, so without the grouping a + * long-lived account fills the panel with entries that all say the same thing. + */ + #[Test] + public function anAccountEditedRepeatedlyIsListedOnce(): void + { + $userId = $this->createUser(); + $accountId = $this->giveTheUserAHistoryRow($userId); + + $this->giveTheUserAHistoryRow($userId, $accountId); + $this->giveTheUserAHistoryRow($userId, $accountId); + + $entries = array_filter($this->usageFor($userId), static fn(array $e) => $e['ref'] === 'AccountHistory'); + + self::assertCount(1, $entries, 'three revisions of one account must be one entry, not three'); + } + + /** + * A user nothing refers to is reported as unused, and really can be deleted. + * + * The counterpart to the tests above: a panel that reported everybody as blocked would satisfy + * them all and be useless. + */ + #[Test] + public function aUserNothingRefersToIsNotReportedAndCanBeDeleted(): void + { + $userId = $this->createUser(); + + self::assertSame([], $this->usageFor($userId), 'nothing refers to this user'); + + $this->buildContainer()->get(UserService::class)->delete($userId); + + $statement = $this->pdo->prepare('SELECT COUNT(*) FROM `User` WHERE `id` = :id'); + $statement->execute(['id' => $userId]); + + self::assertSame(0, (int)$statement->fetchColumn(), 'the user must actually be gone'); + } + + /** + * @return array + */ + private function usageFor(int $userId): array + { + return array_map( + static fn(object $row) => ['ref' => $row->ref, 'name' => (string)$row->name, 'id' => (int)$row->id], + $this->buildContainer()->get(UserService::class)->getUsageForUser($userId) + ); + } + + /** + * @param array $usage + * + * @return array{ref: string, name: string, id: int} + */ + private static function entryFor(array $usage, string $ref): array + { + foreach ($usage as $entry) { + if ($entry['ref'] === $ref) { + return $entry; + } + } + + self::fail(sprintf('No %s entry in the usage list', $ref)); + } + + private function assertTheDeleteIsRefused(int $userId): void + { + try { + $this->buildContainer()->get(UserService::class)->delete($userId); + } catch (ConstraintException) { + return; + } + + self::fail('The database was expected to refuse the delete, and did not'); + } + + /** + * A history row naming this user as its author, on a new account id unless one is given. + */ + private function giveTheUserAHistoryRow(int $userId, ?int $accountId = null): int + { + $accountId ??= random_int(100000, 999999); + + $statement = $this->pdo->prepare( + 'INSERT INTO `AccountHistory` (`accountId`, `userId`, `userGroupId`, `userEditId`, `name`, + `login`, `pass`, `key`, `notes`, `mPassHash`, `categoryId`, `clientId`, + `dateAdd`, `isModify`, `isDeleted`) + VALUES (:accountId, :userId, :groupId, :userId2, :name, :login, :pass, :key, :notes, + :mPassHash, :categoryId, :clientId, NOW(), 0, 0)' + ); + + $statement->execute( + [ + 'accountId' => $accountId, + 'userId' => $userId, + 'groupId' => self::GROUP_ID, + 'userId2' => $userId, + 'name' => 'Retired Account', + 'login' => 'someone', + 'pass' => '', + 'key' => '', + 'notes' => '', + 'mPassHash' => '', + 'categoryId' => $this->anExistingId('Category'), + 'clientId' => $this->anExistingId('Client'), + ] + ); + + return $accountId; + } + + private function giveTheUserANotification(int $userId): int + { + $statement = $this->pdo->prepare( + 'INSERT INTO `Notification` (`type`, `component`, `description`, `date`, `checked`, `userId`) + VALUES (:type, :component, :description, :date, 0, :userId)' + ); + + $statement->execute( + [ + 'type' => 'Test', + 'component' => 'Test Component', + 'description' => 'Something happened', + 'date' => time(), + 'userId' => $userId, + ] + ); + + return (int)$this->pdo->lastInsertId(); + } + + private function anExistingId(string $table): int + { + $id = $this->pdo->query(sprintf('SELECT `id` FROM `%s` ORDER BY `id` LIMIT 1', $table))?->fetchColumn(); + + self::assertNotFalse($id, sprintf('The fixtures provide no %s to attach history to', $table)); + + return (int)$id; + } + + private function createUser(): int + { + $dic = $this->buildContainer(); + + // Kept short on purpose: UserProfile.name is varchar(45). + $profileId = $dic->get(UserProfileService::class)->create( + (new UserProfileModel(['name' => 'Blockers ' . bin2hex(random_bytes(4))])) + ->dehydrate(new ProfileData(['accView' => true])) + ); + + return $dic->get(UserService::class)->createWithMasterPass( + new UserModel( + [ + 'name' => 'Deletion Blockers Test User', + 'login' => 'blockers_' . bin2hex(random_bytes(4)), + 'userGroupId' => self::GROUP_ID, + 'userProfileId' => $profileId, + 'isAdminApp' => false, + 'isAdminAcc' => false, + 'isDisabled' => false, + 'isChangePass' => false, + 'isLdap' => false, + ] + ), + 'Blockers!' . bin2hex(random_bytes(4)), + self::MASTER_PASS + ); + } + + private function buildContainer(): ContainerInterface + { + $_ENV['CONFIG_PATH'] = $this->configPath; + + try { + $coreDefinitions = CoreDefinitions::getDefinitions(REAL_APP_ROOT, 'cli'); + } finally { + unset($_ENV['CONFIG_PATH']); + } + + $coreDefinitions['paths'] = array_map( + fn(array $path) => match ($path[0]) { + Path::CACHE => [Path::CACHE, $this->cachePath()], + Path::TMP => [Path::TMP, $this->tmpPath()], + Path::BACKUP => [Path::BACKUP, $this->backupPath()], + default => $path, + }, + $coreDefinitions['paths'] + ); + + $moduleDefinitions = FileSystem::require( + FileSystem::buildPath(REAL_APP_ROOT, 'src', 'Infrastructure', 'Adapter', 'In', 'Cli', 'module.php') + ); + + $builder = new ContainerBuilder(); + $builder->addDefinitions( + DomainDefinitions::getDefinitions(), + $coreDefinitions, + $moduleDefinitions, + [DbStorageHandler::class => getDbHandler()] + ); + + return $builder->build(); + } + + private function cachePath(): string + { + return FileSystem::buildPath($this->root, 'cache'); + } + + private function tmpPath(): string + { + return FileSystem::buildPath($this->root, 'tmp'); + } + + private function backupPath(): string + { + return FileSystem::buildPath($this->root, 'backup'); + } +} diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/User/UserUsageTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/User/UserUsageTest.php index 4ea34f8df..49e2bf8b1 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/User/UserUsageTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/User/UserUsageTest.php @@ -66,6 +66,8 @@ public function eachKindOfUseGetsItsOwnIcon() ['ref' => 'Account', 'name' => 'An account', 'id' => 1], ['ref' => 'UserGroup', 'name' => 'A group', 'id' => 2], ['ref' => 'PublicLink', 'name' => 'A link', 'id' => 3], + ['ref' => 'AccountHistory', 'name' => 'A retired account', 'id' => 4], + ['ref' => 'Notification', 'name' => 'A notification', 'id' => 5], ] ); } @@ -139,6 +141,8 @@ private function outputCheckerIcons(string $output): void self::assertStringContainsString('mdl-list__item-icon">description', $html, 'an account'); self::assertStringContainsString('mdl-list__item-icon">group', $html, 'a group'); self::assertStringContainsString('mdl-list__item-icon">link', $html, 'a public link'); + self::assertStringContainsString('mdl-list__item-icon">history', $html, "an account's history"); + self::assertStringContainsString('mdl-list__item-icon">notifications', $html, 'a notification'); } private function outputCheckerFallbackIcon(string $output): void diff --git a/tests/Unit/Infrastructure/Adapter/Out/User/Repositories/UserTest.php b/tests/Unit/Infrastructure/Adapter/Out/User/Repositories/UserTest.php index 871a9eb98..74a536911 100644 --- a/tests/Unit/Infrastructure/Adapter/Out/User/Repositories/UserTest.php +++ b/tests/Unit/Infrastructure/Adapter/Out/User/Repositories/UserTest.php @@ -62,6 +62,8 @@ class UserTest extends UnitaryTestCase private MockObject|DatabaseInterface $database; /** + * Every placeholder is the user being asked about. + * * @throws ConstraintException * @throws QueryException */ @@ -74,18 +76,52 @@ public function testGetUsageForUser() self::callback(static function (QueryData $queryData) { $params = $queryData->getQuery()->getBindValues(); - return count($params) === 5 - && $params['userId1'] === 100 - && $params['userEditId'] === 100 - && $params['userId2'] === 100 - && $params['userId3'] === 100 - && $params['userId4'] === 100; + return count($params) === 8 + && count(array_unique($params)) === 1 + && array_values($params)[0] === 100; }) ); $this->user->getUsageForUser(100); } + /** + * The query covers every relation that stops a user being deleted. + * + * Counting bind values does not: the previous version of the test above passed while the + * account history and the notifications — two of the six `RESTRICT` foreign keys onto `User` — + * were absent from the query entirely, so the view could report nothing blocking a delete that + * the database then refused. + * + * @throws ConstraintException + * @throws QueryException + */ + public function testGetUsageForUserCoversEveryRelationThatBlocksADelete() + { + $statement = null; + + $this->database + ->expects($this->once()) + ->method('runQuery') + ->with( + self::callback(static function (QueryData $queryData) use (&$statement) { + $statement = $queryData->getQuery()->getStatement(); + + return true; + }) + ); + + $this->user->getUsageForUser(100); + + foreach (['Account', 'AccountHistory', 'Notification', 'PublicLink'] as $table) { + self::assertStringContainsString( + sprintf('`%s`', $table), + (string)$statement, + sprintf('%s references User without ON DELETE, so it blocks a delete and must be reported', $table) + ); + } + } + /** * @throws SPException */