From cd16c45f4362da6b488178f0eeefab225fe56454 Mon Sep 17 00:00:00 2001 From: blaipr Date: Wed, 26 Aug 2026 21:12:58 +0200 Subject: [PATCH 1/2] fix: a preset naming a deleted user does not break account saving MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fixed permission preset keeps the users and groups it shares new accounts with inside a serialized blob. No foreign key reaches in there — the one on ItemPreset covers the preset's own scope columns, not its contents — so a user or group named in a preset can be deleted with nothing to stop it, and nothing to say it mattered. It matters at the next save. AccountToUser and AccountToUserGroup *do* have foreign keys on those ids, so the insert fails with error 1452, inside the transaction Account::create() and update() run in, and the whole save rolls back. Not for one person and not once: every account create and edit by anybody the preset resolves to, until an administrator works out which preset holds the stale id and edits it. The message they get says "Referenced record not found", which does not point at a preset. The ids are filtered to the ones that still exist before they are applied. What is left of the preset is applied; what has been deleted is dropped. getExistingIds() is new on the user and group repositories, and answers with plain ids rather than rows: Simple declares no properties — every read goes through the model's outer-property bag — so returning rows means either static analysis cannot see the column or the reads have to go through array access at every call site. It is one query per list, skipped entirely when the list is empty, and it runs only for a fixed permission preset that names somebody. Checked by dropping the filter on one of the four lists: the new test fails with the stale id still being handed to the insert. The test needed the existence stub to consult a per-test list of deleted ids rather than being re-stubbed in the test itself — the first stub registered is the one that answers, so a per-test override of a setUp stub silently does nothing, which is how the first version of this test passed against the unfixed code. --- .../Account/Services/AccountPreset.php | 64 +++++++++++++++-- src/Domain/User/Ports/UserGroupRepository.php | 11 +++ src/Domain/User/Ports/UserRepository.php | 11 +++ .../Adapter/Out/User/Repositories/User.php | 28 ++++++++ .../Out/User/Repositories/UserGroup.php | 28 ++++++++ .../Account/Services/AccountPresetTest.php | 71 ++++++++++++++++++- 6 files changed, 207 insertions(+), 6 deletions(-) diff --git a/src/Application/Account/Services/AccountPreset.php b/src/Application/Account/Services/AccountPreset.php index 6c7ae594a..a098c9bfb 100644 --- a/src/Application/Account/Services/AccountPreset.php +++ b/src/Application/Account/Services/AccountPreset.php @@ -36,6 +36,12 @@ use SP\Domain\Core\Exceptions\ConstraintException; use SP\Domain\Core\Exceptions\QueryException; use SP\Domain\Core\Exceptions\SPException; +use SP\Domain\User\Ports\UserRepository; +use SP\Domain\User\Models\UserGroup as UserGroupModel; +use SP\Domain\User\Models\User as UserModel; +use SP\Domain\User\Ports\UserGroupRepository; +use SP\Domain\Common\Dtos\QueryResult; +use SP\Domain\Common\Models\Simple; use SP\Domain\ItemPreset\Models\AccountPermission; use SP\Domain\ItemPreset\Models\ItemPreset as ItemPresetModel; use SP\Domain\ItemPreset\Models\Password; @@ -50,6 +56,8 @@ final class AccountPreset extends Service implements AccountPresetService { /** * @param ItemPresetService $itemPresetService + * @param UserRepository $userRepository + * @param UserGroupRepository $userGroupRepository */ public function __construct( Application $application, @@ -57,7 +65,9 @@ public function __construct( private readonly AccountToUserGroupRepository $accountToUserGroupRepository, private readonly AccountToUserRepository $accountToUserRepository, private readonly ConfigDataInterface $configData, - private readonly PasswordValidator $passwordValidator + private readonly PasswordValidator $passwordValidator, + private readonly UserRepository $userRepository, + private readonly UserGroupRepository $userGroupRepository ) { parent::__construct($application); } @@ -165,10 +175,21 @@ public function addPresetPermissions(int $accountId): void if ($accountPermission !== null) { $userData = $this->context->getUserData(); - $usersView = array_diff($accountPermission->getUsersView(), [$userData->id]); - $usersEdit = array_diff($accountPermission->getUsersEdit(), [$userData->id]); - $userGroupsView = array_diff($accountPermission->getUserGroupsView(), [$userData->userGroupId]); - $userGroupsEdit = array_diff($accountPermission->getUserGroupsEdit(), [$userData->userGroupId]); + // Only ids that still exist. The preset carries them inside a serialized blob, and + // no foreign key reaches in there — the one on ItemPreset covers the preset's own + // scope columns, not its contents. So a user or group named in a fixed preset can + // be deleted with nothing to stop it, and the next account anybody in that + // preset's scope saved failed on the foreign key these ids do have, inside the + // transaction, rolling the whole save back. Every account create and edit for + // those people, until an administrator worked out which preset to edit. + $usersView = $this->existingUsers(array_diff($accountPermission->getUsersView(), [$userData->id])); + $usersEdit = $this->existingUsers(array_diff($accountPermission->getUsersEdit(), [$userData->id])); + $userGroupsView = $this->existingUserGroups( + array_diff($accountPermission->getUserGroupsView(), [$userData->userGroupId]) + ); + $userGroupsEdit = $this->existingUserGroups( + array_diff($accountPermission->getUserGroupsEdit(), [$userData->userGroupId]) + ); if (!empty($usersView)) { $this->accountToUserRepository->addByType($accountId, $usersView); @@ -188,4 +209,37 @@ public function addPresetPermissions(int $accountId): void } } } + + /** + * @param int[] $ids + * + * @return int[] + * @throws ConstraintException + * @throws QueryException + */ + private function existingUsers(array $ids): array + { + if (empty($ids)) { + return []; + } + + return $this->userRepository->getExistingIds($ids); + } + + /** + * @param int[] $ids + * + * @return int[] + * @throws ConstraintException + * @throws QueryException + */ + private function existingUserGroups(array $ids): array + { + if (empty($ids)) { + return []; + } + + return $this->userGroupRepository->getExistingIds($ids); + } + } diff --git a/src/Domain/User/Ports/UserGroupRepository.php b/src/Domain/User/Ports/UserGroupRepository.php index 0ff4fae56..f73f3b945 100644 --- a/src/Domain/User/Ports/UserGroupRepository.php +++ b/src/Domain/User/Ports/UserGroupRepository.php @@ -88,6 +88,17 @@ public function getById(int $id): QueryResult; */ public function getAll(): QueryResult; + /** + * Which of the given ids still exist + * + * @param int[] $ids + * + * @return int[] + * @throws ConstraintException + * @throws QueryException + */ + public function getExistingIds(array $ids): array; + /** * Deletes all the items for given ids * diff --git a/src/Domain/User/Ports/UserRepository.php b/src/Domain/User/Ports/UserRepository.php index 704fd7336..889e5d239 100644 --- a/src/Domain/User/Ports/UserRepository.php +++ b/src/Domain/User/Ports/UserRepository.php @@ -168,6 +168,17 @@ public function getUserEmail(): QueryResult; */ public function getUserEmailById(array $ids): QueryResult; + /** + * Which of the given ids still exist + * + * @param int[] $ids + * + * @return int[] + * @throws ConstraintException + * @throws QueryException + */ + public function getExistingIds(array $ids): array; + /** * Returns the usage of the given user's id * diff --git a/src/Infrastructure/Adapter/Out/User/Repositories/User.php b/src/Infrastructure/Adapter/Out/User/Repositories/User.php index 39ac6ec28..03bd750bd 100644 --- a/src/Infrastructure/Adapter/Out/User/Repositories/User.php +++ b/src/Infrastructure/Adapter/Out/User/Repositories/User.php @@ -566,6 +566,34 @@ public function getUserEmail(): QueryResult return $this->db->runQuery(QueryData::build($query)->setMapClassName(UserModel::class)); } + /** + * Which of the given ids still exist + * + * @param int[] $ids + * + * @return int[] + * @throws ConstraintException + * @throws QueryException + */ + public function getExistingIds(array $ids): array + { + if (empty($ids)) { + return []; + } + + $query = $this->queryFactory + ->newSelect() + ->cols(['id']) + ->from(UserModel::TABLE) + ->where('id IN (:ids)', ['ids' => $ids]); + + $result = $this->db->runQuery(QueryData::build($query)->setMapClassName(Simple::class)); + + // Array access rather than ->id: Simple declares no properties, every read goes through + // the model's outer-property bag, and static analysis cannot see through that. + return array_map(static fn(Simple $row): int => (int)$row['id'], $result->getDataAsArray()); + } + /** * Return the email of the given user's id * diff --git a/src/Infrastructure/Adapter/Out/User/Repositories/UserGroup.php b/src/Infrastructure/Adapter/Out/User/Repositories/UserGroup.php index 0720e7fde..8715f1adf 100644 --- a/src/Infrastructure/Adapter/Out/User/Repositories/UserGroup.php +++ b/src/Infrastructure/Adapter/Out/User/Repositories/UserGroup.php @@ -195,6 +195,34 @@ public function getByName(string $name): QueryResult return $this->db->runQuery(QueryData::buildWithMapper($query, UserGroupModel::class)); } + /** + * Which of the given ids still exist + * + * @param int[] $ids + * + * @return int[] + * @throws ConstraintException + * @throws QueryException + */ + public function getExistingIds(array $ids): array + { + if (empty($ids)) { + return []; + } + + $query = $this->queryFactory + ->newSelect() + ->cols(['id']) + ->from(UserGroupModel::TABLE) + ->where('id IN (:ids)', ['ids' => $ids]); + + $result = $this->db->runQuery(QueryData::build($query)->setMapClassName(Simple::class)); + + // Array access rather than ->id: Simple declares no properties, every read goes through + // the model's outer-property bag, and static analysis cannot see through that. + return array_map(static fn(Simple $row): int => (int)$row['id'], $result->getDataAsArray()); + } + /** * Returns all the items * diff --git a/tests/Unit/Application/Account/Services/AccountPresetTest.php b/tests/Unit/Application/Account/Services/AccountPresetTest.php index 303a69413..f9d7f67f0 100644 --- a/tests/Unit/Application/Account/Services/AccountPresetTest.php +++ b/tests/Unit/Application/Account/Services/AccountPresetTest.php @@ -46,6 +46,11 @@ use SP\Domain\Common\Validators\ValidatorInterface; use SP\Tests\Support\Generators\AccountDataGenerator; use SP\Tests\Support\Generators\ItemPresetDataGenerator; +use SP\Domain\User\Ports\UserRepository; +use SP\Domain\User\Ports\UserGroupRepository; +use SP\Domain\Common\Models\Simple; +use SP\Domain\Common\Dtos\QueryResult; +use SP\Domain\ItemPreset\Models\AccountPermission; use SP\Tests\Support\UnitaryTestCase; /** @@ -62,6 +67,10 @@ class AccountPresetTest extends UnitaryTestCase private ValidatorInterface|MockObject $passwordValidator; private MockObject|AccountToUserGroupRepository $accountToUserGroupRepository; private AccountToUserRepository|MockObject $accountToUserRepository; + private UserRepository|MockObject $userRepository; + private UserGroupRepository|MockObject $userGroupRepository; + /** @var int[] Ids a test has decided are gone from the database. */ + private array $deletedIds = []; /** * @throws QueryException @@ -549,6 +558,53 @@ private function buildPasswordPresetWithExpireDays(int $expireDays): Password ); } + /** + * A preset naming a user who has since been deleted still saves the account. + * + * The permission preset keeps its user and group ids inside a serialized blob, and no foreign + * key reaches in there — the one on ItemPreset covers the preset's own scope columns, not its + * contents. So deleting a user named in a fixed preset is allowed, and the ids AccountToUser + * *does* have a foreign key on then fail on the next insert: error 1452, raised inside + * Account::create()'s transaction, rolling the whole save back. Every account create and edit + * by anybody in that preset's scope, until an administrator worked out which preset to edit. + * + * The ones that still exist are applied; the stale one is dropped. + * + * @throws ConstraintException + * @throws QueryException + * @throws SPException + */ + #[Test] + public function testAddPresetPermissionsSkipsAUserThatNoLongerExists() + { + $accountPermission = new AccountPermission( + usersView: [11, 12], + usersEdit: [], + userGroupsView: [], + userGroupsEdit: [] + ); + + $this->itemPresetService + ->expects(self::once()) + ->method('getForCurrentUser') + ->with('account.permission') + ->willReturn( + ItemPresetDataGenerator::factory() + ->buildItemPresetData($accountPermission) + ->mutate(['fixed' => 1]) + ); + + // 12 has been deleted since the preset named it. + $this->deletedIds = [12]; + + $this->accountToUserRepository + ->expects(self::once()) + ->method('addByType') + ->with(100, [11], false); + + $this->accountPreset->addPresetPermissions(100); + } + protected function setUp(): void { parent::setUp(); @@ -560,6 +616,16 @@ protected function setUp(): void $this->passwordValidator = $this->createMock(PasswordValidator::class); $this->accountToUserGroupRepository = $this->createMock(AccountToUserGroupRepository::class); $this->accountToUserRepository = $this->createMock(AccountToUserRepository::class); + $this->userRepository = $this->createMock(UserRepository::class); + $this->userGroupRepository = $this->createMock(UserGroupRepository::class); + + // Every id the preset names still exists unless a test puts one in $deletedIds, which is + // the ordinary case — the filter is there for the one where it does not. One stub rather + // than a per-test override, because the first stub registered is the one that answers. + $echo = fn(array $ids): array => array_values(array_diff($ids, $this->deletedIds)); + + $this->userRepository->method('getExistingIds')->willReturnCallback($echo); + $this->userGroupRepository->method('getExistingIds')->willReturnCallback($echo); $this->accountPreset = new AccountPreset( @@ -568,7 +634,10 @@ protected function setUp(): void $this->accountToUserGroupRepository, $this->accountToUserRepository, $configData, - $this->passwordValidator + $this->passwordValidator, + $this->userRepository, + $this->userGroupRepository ); } + } From 998052339df961e45aaee930bdc33f821f4c1861 Mon Sep 17 00:00:00 2001 From: blaipr Date: Wed, 26 Aug 2026 21:20:19 +0200 Subject: [PATCH 2/2] style: close the class on the line after its body PHPCS (PSR2) caught the blank line my helper insertion left before the closing brace. Worth noting for next time: `composer phpcs` is a separate CI gate from PHPStan, and I had only been running the latter. --- src/Application/Account/Services/AccountPreset.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Application/Account/Services/AccountPreset.php b/src/Application/Account/Services/AccountPreset.php index a098c9bfb..b2834ede2 100644 --- a/src/Application/Account/Services/AccountPreset.php +++ b/src/Application/Account/Services/AccountPreset.php @@ -241,5 +241,4 @@ private function existingUserGroups(array $ids): array return $this->userGroupRepository->getExistingIds($ids); } - }