From 8197b3019ab577d189d0157731b2d9094de2b90d Mon Sep 17 00:00:00 2001 From: blaipr Date: Fri, 18 Sep 2026 00:20:51 +0200 Subject: [PATCH] fix: an account is not saved with only part of its sharing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AccountItems::addItems() wrapped all five sharing writes — view/edit groups, view/edit users, tags — in a catch (SPException) that logged and returned. So a failure part-way left the account saved with whatever subset had been inserted first, or with nothing shared at all if the first call threw, and the create answered with plain success. AccountToUserGroup, AccountToUser and AccountToTag each carry a foreign key to the row they name, so a group, user or tag deleted between the form being drawn and submitted throws a ConstraintException from inside one of them — an ordinary thing for two administrators to do to each other, not a contrived race. The edit path already does it right: replaceUserGroups() and replaceUsers() run inside transactionAware() and let failures propagate. Account::create() already wraps the whole create in transactionAware() too; what was missing was letting the failure reach it. testAddItemsWithException called addItems() with a throwing repository and no expectException, so it passed only because nothing propagated — it was pinning the swallow as intended behaviour. It now asserts the failure is reported. --- .../Account/Services/AccountItems.php | 76 +++++++++++-------- .../Account/Services/AccountItemsTest.php | 21 ++++- .../Account/Services/AccountTest.php | 44 +++++++++++ 3 files changed, 108 insertions(+), 33 deletions(-) diff --git a/src/Application/Account/Services/AccountItems.php b/src/Application/Account/Services/AccountItems.php index 2fa27e4d4..d69024614 100644 --- a/src/Application/Account/Services/AccountItems.php +++ b/src/Application/Account/Services/AccountItems.php @@ -39,8 +39,6 @@ use SP\Domain\Core\Exceptions\QueryException; use SP\Domain\Core\Exceptions\SPException; -use function SP\processException; - /** * Class AccountItems */ @@ -161,43 +159,57 @@ function () use ($accountId, $userIds, $isEdit) { ); } + /** + * The sharing an account is created with — all of it, or none of the account. + * + * This used to wrap the five writes below in a `catch (SPException)` that called + * `processException()` and returned, so a failure part-way left the account saved with whatever + * subset had been inserted first — or with nothing shared at all, if the first call was the one + * that threw — and the create still reported plain success. `AccountToUserGroup`, + * `AccountToUser` and `AccountToTag` all carry a foreign key to the row they name, so a group, + * user or tag deleted between the form being drawn and submitted throws a `ConstraintException` + * from inside one of them. + * + * `Account::create()` calls this inside its own `transactionAware()`, so letting the exception + * out rolls the account back with it — which is the whole point of that transaction, and what + * the edit path has always done: `replaceUserGroups()` and `replaceUsers()` run their + * delete-and-insert inside a transaction and let failures propagate. + * + * @throws SPException + */ public function addItems(bool $userCanChangePermissions, int $accountId, AccountCreateDto $accountCreateDto): void { - try { - if ($userCanChangePermissions) { - if (null !== $accountCreateDto->userGroupsView - && !empty($accountCreateDto->userGroupsView) - ) { - $this->accountToUserGroupRepository->addByType( - $accountId, - $accountCreateDto->userGroupsView - ); - } - - if (null !== $accountCreateDto->userGroupsEdit - && !empty($accountCreateDto->userGroupsEdit) - ) { - $this->accountToUserGroupRepository->addByType( - $accountId, - $accountCreateDto->userGroupsEdit, - true - ); - } + if ($userCanChangePermissions) { + if (null !== $accountCreateDto->userGroupsView + && !empty($accountCreateDto->userGroupsView) + ) { + $this->accountToUserGroupRepository->addByType( + $accountId, + $accountCreateDto->userGroupsView + ); + } - if (null !== $accountCreateDto->usersView && !empty($accountCreateDto->usersView)) { - $this->accountToUserRepository->addByType($accountId, $accountCreateDto->usersView); - } + if (null !== $accountCreateDto->userGroupsEdit + && !empty($accountCreateDto->userGroupsEdit) + ) { + $this->accountToUserGroupRepository->addByType( + $accountId, + $accountCreateDto->userGroupsEdit, + true + ); + } - if (null !== $accountCreateDto->usersEdit && !empty($accountCreateDto->usersEdit)) { - $this->accountToUserRepository->addByType($accountId, $accountCreateDto->usersEdit, true); - } + if (null !== $accountCreateDto->usersView && !empty($accountCreateDto->usersView)) { + $this->accountToUserRepository->addByType($accountId, $accountCreateDto->usersView); } - if (null !== $accountCreateDto->tags && !empty($accountCreateDto->tags)) { - $this->accountToTagRepository->add($accountId, $accountCreateDto->tags); + if (null !== $accountCreateDto->usersEdit && !empty($accountCreateDto->usersEdit)) { + $this->accountToUserRepository->addByType($accountId, $accountCreateDto->usersEdit, true); } - } catch (SPException $e) { - processException($e); + } + + if (null !== $accountCreateDto->tags && !empty($accountCreateDto->tags)) { + $this->accountToTagRepository->add($accountId, $accountCreateDto->tags); } } } diff --git a/tests/Unit/Application/Account/Services/AccountItemsTest.php b/tests/Unit/Application/Account/Services/AccountItemsTest.php index e71327696..6e64fa6e1 100644 --- a/tests/Unit/Application/Account/Services/AccountItemsTest.php +++ b/tests/Unit/Application/Account/Services/AccountItemsTest.php @@ -302,7 +302,23 @@ public function testAddItemsWithNoPermission() $this->accountItems->addItems(false, 100, $accountCreateDto); } - public function testAddItemsWithException() + /** + * A failure adding an account's sharing is reported, not swallowed. + * + * This used to assert the opposite — it called `addItems()` with a throwing repository and no + * `expectException`, so it passed only because nothing propagated. What that cost: the five + * writes were wrapped in a `catch (SPException)` that logged and returned, so an account was + * saved with whatever subset had been inserted before the failure (or with nothing shared at + * all, if the first call threw) and the create reported plain success. + * + * `AccountToUserGroup`, `AccountToUser` and `AccountToTag` each carry a foreign key to the row + * they name, so a group, user or tag deleted between the form being drawn and submitted throws + * a `ConstraintException` from inside one of them. + * + * `Account::create()` calls this inside its own `transactionAware()`, so the exception now + * takes the account with it — which is what the edit path has always done. + */ + public function testAddItemsReportsAFailureRatherThanSavingPartOfTheSharing() { $accountCreateDto = AccountDataGenerator::factory()->buildAccountCreateDto(); @@ -319,6 +335,9 @@ public function testAddItemsWithException() ->method('add') ->willThrowException(SPException::error('test')); + $this->expectException(SPException::class); + $this->expectExceptionMessage('test'); + $this->accountItems->addItems(false, 100, $accountCreateDto); } diff --git a/tests/Unit/Application/Account/Services/AccountTest.php b/tests/Unit/Application/Account/Services/AccountTest.php index 22773a81b..9d385a8c9 100644 --- a/tests/Unit/Application/Account/Services/AccountTest.php +++ b/tests/Unit/Application/Account/Services/AccountTest.php @@ -1879,6 +1879,50 @@ public function testEditPasswordThatMatchedNoRowIsNotReportedAsSaved(): void $this->account->editPassword($id, $accountUpdateDto); } + /** + * An account whose sharing could not be applied is not saved at all. + * + * `addItems()` used to catch and log, so a create whose group/user/tag insert failed part-way + * left the account stored with whatever subset had landed first — or with nothing shared, if + * the first call threw — and answered with plain success. Those three tables each carry a + * foreign key to the row they name, so a group, user or tag deleted between the form being + * drawn and submitted reaches exactly that. + * + * The account create already runs inside `transactionAware()`; what was missing was letting the + * failure reach it. Asserted through the service rather than on `addItems()` directly, because + * the thing that matters is that the *account* goes with it. + * + * @throws Exception + */ + public function testCreateIsAbandonedWhenItsSharingCannotBeApplied() + { + $accountCreateDto = AccountDataGenerator::factory()->buildAccountCreateDto(); + + $this->accountCryptService + ->method('getPasswordEncrypted') + ->willReturn(new EncryptedPassword(self::$faker->password(), self::$faker->password())); + + $this->itemPresetService->method('getForCurrentUser')->willReturn(null); + + $this->accountRepository + ->method('create') + ->willReturn(new QueryResult(null, 0, self::$faker->numberBetween(1, 1000))); + + $this->accountItemsService + ->expects(self::once()) + ->method('addItems') + ->willThrowException(SPException::error('a shared group no longer exists')); + + // Nothing after the sharing runs, and the transaction the create sits in takes the account + // with it. + $this->accountPresetService->expects(self::never())->method('addPresetPermissions'); + + $this->expectException(SPException::class); + $this->expectExceptionMessage('a shared group no longer exists'); + + $this->account->create($accountCreateDto); + } + /** * The account the service is expected to write, give or take the second it was stamped in. *