diff --git a/src/Application/Account/Services/AccountPreset.php b/src/Application/Account/Services/AccountPreset.php index 6c7ae594a..b2834ede2 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,36 @@ 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 ); } + }