From 52fe8e849bcd6ca1f00a054f10fc2e0785ad2d79 Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 24 Aug 2026 02:21:03 +0200 Subject: [PATCH] fix: an edit that says nothing about sharing leaves it alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AccountItems::updateItems()` read `null` as "delete every row of this type" and an empty array as nothing at all. Both are the wrong way round. `null` is what a caller sends when they said nothing about a list, and an empty array is what they send to empty it. The consequence on the API is the serious one. `AccountUpdateDto`'s four sharing lists are never set by `Account/EditController` — there is no REST parameter for them and no endpoint for account sharing anywhere — so they were always null. Every `PUT` to an account, by a caller whose profile can manage permissions, silently deleted all four kinds of sharing on the account it was editing. A script correcting a URL removed everyone's access to the account, reported success, and left nothing in the response to say so. The web reached the same place by a different route: `AccountForm::analyzeItems()` only sets a list when the matching `_update` flag is posted, and the theme posts that only for a select the user actually changed — so an edit that touched the name and nothing else sent null for all four. And the third case is the one that was supposed to work: the bulk edit's "Delete" checkboxes post exactly the empty array, which matched neither branch. The one deliberate way to clear an account's sharing was the one way that did nothing, while answering "Accounts updated". So: null leaves the list alone, an empty array clears it, a non-empty array replaces it. The delete and the add are one transaction, as before, so a failure adding leaves the existing rows rather than an account shared with nobody. The tags carried the identical two branches and are aligned with the rest — a rule that holds for four of five lists is not a rule. That needed the API's own default changing too: `getParamArray('tagsId', false, [])` handed an empty array when the parameter was absent, which under the corrected reading would mean "clear the tags". It is null now, and an empty array from a caller who did supply one still clears them. The existing test pinned the old contract — it asserted the two deletes that this change removes — so it is rewritten rather than kept, alongside a new one for the empty-array case that never had one. --- .../Account/Services/AccountItems.php | 138 ++++++++++-------- .../Controllers/Account/EditController.php | 6 +- .../Account/Services/AccountItemsTest.php | 85 +++++++---- 3 files changed, 137 insertions(+), 92 deletions(-) diff --git a/src/Application/Account/Services/AccountItems.php b/src/Application/Account/Services/AccountItems.php index f892156a4..2fa27e4d4 100644 --- a/src/Application/Account/Services/AccountItems.php +++ b/src/Application/Account/Services/AccountItems.php @@ -68,70 +68,33 @@ public function updateItems( AccountUpdateDto $accountUpdateDto ): void { if ($userCanChangePermissions) { - if (null === $accountUpdateDto->userGroupsView) { - $this->accountToUserGroupRepository->deleteTypeByAccountId($accountId, false); - } elseif (!empty($accountUpdateDto->userGroupsView)) { - $this->accountToUserGroupRepository->transactionAware( - function () use ($accountUpdateDto, $accountId) { - $this->accountToUserGroupRepository - ->deleteTypeByAccountId($accountId, false); - $this->accountToUserGroupRepository - ->addByType($accountId, $accountUpdateDto->userGroupsView); - }, - $this - ); - } - - if (null === $accountUpdateDto->userGroupsEdit) { - $this->accountToUserGroupRepository->deleteTypeByAccountId($accountId, true); - } elseif (!empty($accountUpdateDto->userGroupsEdit)) { - $this->accountToUserGroupRepository->transactionAware( - function () use ($accountUpdateDto, $accountId) { - $this->accountToUserGroupRepository - ->deleteTypeByAccountId($accountId, true); - $this->accountToUserGroupRepository - ->addByType($accountId, $accountUpdateDto->userGroupsEdit, true); - }, - $this - ); - } - - if (null === $accountUpdateDto->usersView) { - $this->accountToUserRepository->deleteTypeByAccountId($accountId, false); - } elseif (!empty($accountUpdateDto->usersView)) { - $this->accountToUserRepository->transactionAware( - function () use ($accountUpdateDto, $accountId) { - $this->accountToUserRepository - ->deleteTypeByAccountId($accountId, false); - $this->accountToUserRepository - ->addByType($accountId, $accountUpdateDto->usersView); - }, - $this - ); - } - - if (null === $accountUpdateDto->usersEdit) { - $this->accountToUserRepository->deleteTypeByAccountId($accountId, true); - } elseif (!empty($accountUpdateDto->usersEdit)) { - $this->accountToUserRepository->transactionAware( - function () use ($accountUpdateDto, $accountId) { - $this->accountToUserRepository - ->deleteTypeByAccountId($accountId, true); - $this->accountToUserRepository - ->addByType($accountId, $accountUpdateDto->usersEdit, true); - }, - $this - ); - } + // null means the caller said nothing about this list, and an empty array means they + // said to empty it. It used to be the other way round: null deleted every row of that + // type and an empty array matched neither branch and did nothing at all. + // + // Nothing supplies these on the API — there is no parameter for them — so every REST + // account edit silently removed all four kinds of sharing from the account it was + // editing. On the web the form only sets a list when the corresponding `_update` flag + // is posted, which the theme sends only for a select the user actually changed, so an + // edit that touched the name or the URL did the same thing. And the bulk edit's + // "Delete" checkboxes, which post exactly the empty array, were the case that did + // nothing — so the one way to deliberately clear sharing was the one way that never + // worked. + $this->replaceUserGroups($accountId, $accountUpdateDto->userGroupsView, false); + $this->replaceUserGroups($accountId, $accountUpdateDto->userGroupsEdit, true); + $this->replaceUsers($accountId, $accountUpdateDto->usersView, false); + $this->replaceUsers($accountId, $accountUpdateDto->usersEdit, true); } - if (null === $accountUpdateDto->tags) { - $this->accountToTagRepository->deleteByAccountId($accountId); - } elseif (!empty($accountUpdateDto->tags)) { + // Same rule for the tags, which had the same two branches. + if ($accountUpdateDto->tags !== null) { $this->accountToTagRepository->transactionAware( function () use ($accountUpdateDto, $accountId) { $this->accountToTagRepository->deleteByAccountId($accountId); - $this->accountToTagRepository->add($accountId, $accountUpdateDto->tags); + + if (!empty($accountUpdateDto->tags)) { + $this->accountToTagRepository->add($accountId, $accountUpdateDto->tags); + } }, $this ); @@ -141,6 +104,63 @@ function () use ($accountUpdateDto, $accountId) { /** * Adds external items to the account */ + /** + * Replace an account's shared groups of one kind, or leave them alone when none were supplied. + * + * The delete and the add are one transaction, so a failure adding leaves the existing rows + * rather than an account shared with nobody. + * + * @param int[]|null $userGroupIds + * + * @throws ConstraintException + * @throws QueryException + * @throws ServiceException + */ + private function replaceUserGroups(int $accountId, ?array $userGroupIds, bool $isEdit): void + { + if ($userGroupIds === null) { + return; + } + + $this->accountToUserGroupRepository->transactionAware( + function () use ($accountId, $userGroupIds, $isEdit) { + $this->accountToUserGroupRepository->deleteTypeByAccountId($accountId, $isEdit); + + if (!empty($userGroupIds)) { + $this->accountToUserGroupRepository->addByType($accountId, $userGroupIds, $isEdit); + } + }, + $this + ); + } + + /** + * The same for an account's shared users. + * + * @param int[]|null $userIds + * + * @throws ConstraintException + * @throws QueryException + * @throws ServiceException + */ + private function replaceUsers(int $accountId, ?array $userIds, bool $isEdit): void + { + if ($userIds === null) { + return; + } + + $this->accountToUserRepository->transactionAware( + function () use ($accountId, $userIds, $isEdit) { + $this->accountToUserRepository->deleteTypeByAccountId($accountId, $isEdit); + + if (!empty($userIds)) { + $this->accountToUserRepository->addByType($accountId, $userIds, $isEdit); + } + }, + $this + ); + } + public function addItems(bool $userCanChangePermissions, int $accountId, AccountCreateDto $accountCreateDto): void { try { diff --git a/src/Infrastructure/Adapter/In/Api/Controllers/Account/EditController.php b/src/Infrastructure/Adapter/In/Api/Controllers/Account/EditController.php index 988351347..6f4498486 100644 --- a/src/Infrastructure/Adapter/In/Api/Controllers/Account/EditController.php +++ b/src/Infrastructure/Adapter/In/Api/Controllers/Account/EditController.php @@ -101,7 +101,11 @@ private function buildAccountUpdateDto(): AccountUpdateDto notes: $this->apiService->getParamString('notes'), isPrivate: (bool)$this->apiService->getParamInt('private'), isPrivateGroup: (bool)$this->apiService->getParamInt('privateGroup'), - tags: array_map(intval(...), $this->apiService->getParamArray('tagsId', false, [])), + // null rather than [], so that an edit which says nothing about tags leaves them + // alone. An empty array now means "clear them", which is what it reads as. + tags: ($tags = $this->apiService->getParamArray('tagsId', false)) === null + ? null + : array_map(intval(...), $tags), ); } } diff --git a/tests/Unit/Application/Account/Services/AccountItemsTest.php b/tests/Unit/Application/Account/Services/AccountItemsTest.php index b3c2018a1..e71327696 100644 --- a/tests/Unit/Application/Account/Services/AccountItemsTest.php +++ b/tests/Unit/Application/Account/Services/AccountItemsTest.php @@ -136,44 +136,65 @@ public function testUpdateItemsWithNoItems() ] ); - $this->accountToUserGroupRepository - ->expects($this->never()) - ->method('transactionAware'); - - $this->accountToUserGroupRepository - ->expects($this->exactly(2)) - ->method('deleteTypeByAccountId') - ->with(...self::withConsecutive([100, false], [100, true])); - - $this->accountToUserGroupRepository - ->expects($this->never()) - ->method('addByType'); + // Nothing at all. null is "the caller said nothing about this list", and an edit that says + // nothing about sharing must leave it alone — it used to delete every row of every type, + // which is what an account edit through the REST API did on every call, there being no + // parameter that could have said otherwise. + foreach ([$this->accountToUserGroupRepository, $this->accountToUserRepository] as $repository) { + $repository->expects($this->never())->method('transactionAware'); + $repository->expects($this->never())->method('deleteTypeByAccountId'); + $repository->expects($this->never())->method('addByType'); + } + + $this->accountToTagRepository->expects($this->never())->method('transactionAware'); + $this->accountToTagRepository->expects($this->never())->method('deleteByAccountId'); + $this->accountToTagRepository->expects($this->never())->method('add'); - $this->accountToUserRepository - ->expects($this->never()) - ->method('transactionAware'); - - $this->accountToUserRepository - ->expects($this->exactly(2)) - ->method('deleteTypeByAccountId') - ->with(...self::withConsecutive([100, false], [100, true])); + $this->accountItems->updateItems(true, 100, $accountUpdateDto); + } - $this->accountToUserRepository - ->expects($this->never()) - ->method('addByType'); + /** + * An empty list clears that kind of sharing. + * + * This is the case the bulk edit's "Delete" checkboxes post, and it used to match neither + * branch and do nothing at all — so the one deliberate way to clear an account's sharing was + * the one way that never worked, while reporting success. + * + * @throws ConstraintException + * @throws QueryException + * @throws ServiceException + */ + public function testUpdateItemsWithEmptyListsClearsThem() + { + $accountUpdateDto = AccountDataGenerator::factory() + ->buildAccountUpdateDto() + ->mutate( + [ + 'usersView' => [], + 'usersEdit' => [], + 'userGroupsView' => [], + 'userGroupsEdit' => [], + 'tags' => [] + ] + ); - $this->accountToTagRepository - ->expects($this->never()) - ->method('transactionAware'); + foreach ([$this->accountToUserGroupRepository, $this->accountToUserRepository] as $repository) { + $repository->expects($this->exactly(2)) + ->method('transactionAware') + ->with(self::withResolveCallableCallback()); + $repository->expects($this->exactly(2)) + ->method('deleteTypeByAccountId') + ->with(...self::withConsecutive([100, false], [100, true])); + // Cleared, not replaced. + $repository->expects($this->never())->method('addByType'); + } $this->accountToTagRepository ->expects($this->once()) - ->method('deleteByAccountId') - ->with(100); - - $this->accountToTagRepository - ->expects($this->never()) - ->method('add'); + ->method('transactionAware') + ->with(self::withResolveCallableCallback()); + $this->accountToTagRepository->expects($this->once())->method('deleteByAccountId')->with(100); + $this->accountToTagRepository->expects($this->never())->method('add'); $this->accountItems->updateItems(true, 100, $accountUpdateDto); }