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 */