From f534e271daa6e062d6a3f9fb75514d59c83eef40 Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 24 Aug 2026 09:05:52 +0200 Subject: [PATCH 1/2] fix: a history purge reports what it removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AccountHistory::delete()` throws when the row it was given did not exist. The batch beside it returned the affected count and left it there, and both callers — the history manager's delete and the bulk edit's "delete history" option — discard the return and answer success regardless. So selecting a set of history entries to destroy, where some of them no longer matched — a stale id, a row another session had already removed — was reported as a purge that had happened. History is where an account's previous passwords live, so the thing the operator believes they destroyed is precisely the thing that is still there. deleteByIdBatch() is given history row ids, so every one of them should have matched, and it now throws when the count differs — the check the single delete has always made and the one PublicLink::deleteByIdBatch() makes for the same reason. deleteByAccountIdBatch() is deliberately left alone, and now says so: those are *account* ids, and an account may have no history at all, so removing nothing from one is the right answer rather than a failure. There is no count there to compare against, and holding it to the same rule would fail a purge that had done exactly what it was asked. Checked by making the comparison always false: the new test fails and the one pinning the account-id case does not. --- .../Account/Services/AccountHistory.php | 19 ++++++- .../Account/Services/AccountHistoryTest.php | 51 +++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/src/Application/Account/Services/AccountHistory.php b/src/Application/Account/Services/AccountHistory.php index 4ac2bc85a..e8a52a9c4 100644 --- a/src/Application/Account/Services/AccountHistory.php +++ b/src/Application/Account/Services/AccountHistory.php @@ -141,14 +141,31 @@ public function delete(int $id): void */ public function deleteByIdBatch(array $ids): int { - return $this->accountHistoryRepository->transactionAware(function () use ($ids) { + $count = $this->accountHistoryRepository->transactionAware(function () use ($ids) { return $this->accountHistoryRepository->deleteByIdBatch($ids); }, $this); + + // These are history row ids, so every one of them should have matched. Returning the count + // and leaving it at that meant a purge which removed fewer rows than it was given — a + // stale id, a row another session had already deleted — was reported as done: the + // controller discards this value and answers success either way. An old password the + // operator believes they destroyed is exactly what history is kept in. + // + // The single delete beside this one has always thrown; this is that check, for the batch. + if ($count !== count($ids)) { + throw new ServiceException(__u('Error while deleting the accounts'), SPException::WARNING); + } + + return $count; } /** * Deletes all the items for given accounts id * + * Deliberately unchecked, unlike deleteByIdBatch() above: these are *account* ids, and an + * account may have no history at all — a purge that removes nothing from it has done exactly + * what it was asked. There is no count here to compare against. + * * @param int[] $ids * * @return int diff --git a/tests/Unit/Application/Account/Services/AccountHistoryTest.php b/tests/Unit/Application/Account/Services/AccountHistoryTest.php index 34710e020..22a955377 100644 --- a/tests/Unit/Application/Account/Services/AccountHistoryTest.php +++ b/tests/Unit/Application/Account/Services/AccountHistoryTest.php @@ -216,6 +216,57 @@ public function testDeleteByIdBatch() $this->accountHistory->deleteByIdBatch($ids); } + /** + * A purge that removed fewer rows than it was given says so. + * + * The ids here are history rows, so every one of them should have matched. The count was + * returned and thrown away — the controller discards it and answers "Accounts updated" either + * way — so a stale id, or a row another session had already deleted, was reported as a purge + * that had happened. An old password the operator believes they destroyed is precisely what + * history holds. + * + * @throws ServiceException + */ + public function testDeleteByIdBatchThatRemovedFewerRowsThanItWasGiven() + { + $ids = [1, 2, 3]; + + $this->accountHistoryRepository + ->expects($this->once()) + ->method('transactionAware') + ->with(self::withResolveCallableCallback()) + ->willReturn(2); + + $this->accountHistoryRepository + ->expects(self::once()) + ->method('deleteByIdBatch') + ->with($ids) + ->willReturn(2); + + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('Error while deleting the accounts'); + + $this->accountHistory->deleteByIdBatch($ids); + } + + /** + * Deleting by *account* id is deliberately not held to that: an account may have no history at + * all, so removing nothing from it is the right answer rather than a failure. There is no count + * to compare against. + */ + public function testDeleteByAccountIdBatchAcceptsAccountsWithNoHistory() + { + $ids = [1, 2, 3]; + + $this->accountHistoryRepository + ->expects(self::once()) + ->method('deleteByAccountIdBatch') + ->with($ids) + ->willReturn(0); + + self::assertSame(0, $this->accountHistory->deleteByAccountIdBatch($ids)); + } + public function testSearch() { $itemSearchData = From 8ca488d541396b28cb2630ca4764dfa3b717fe34 Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 24 Aug 2026 09:17:48 +0200 Subject: [PATCH 2/2] test: the database double reports the rows the purge removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The integration harness answers every statement with one affected row, so a delete given three history ids reported one — which the service now reads, correctly, as a purge that did not remove what it was asked to. The double answers three for the delete in that test. This is the harness modelling the server rather than the test being bent around the change: a real DELETE matching three rows reports three, and the whole point of the check is that anything less is not a purge that happened. --- .../AccountHistoryManagerTest.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/AccountHistoryManager/AccountHistoryManagerTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/AccountHistoryManager/AccountHistoryManagerTest.php index 04d56d2af..da8b020a1 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/AccountHistoryManager/AccountHistoryManagerTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/AccountHistoryManager/AccountHistoryManagerTest.php @@ -84,6 +84,19 @@ public function deleteMultiple() new QueryResult([AccountDataGenerator::factory()->buildAccountHistoryData()]) ); + // The delete has to report as many rows as it was given ids, because the service now + // checks: a purge that removed fewer rows than it was asked to is not a purge that + // happened, and history is where an account's previous passwords live. The harness's + // default answer is one affected row whatever the statement, which for three ids reads as + // exactly that failure. Not a static closure — the harness binds it with Closure::call(). + $this->databaseQueryResolver = function (QueryData $queryData): QueryResult { + if (str_contains($queryData->getQuery()->getStatement(), 'DELETE')) { + return new QueryResult([], 3); + } + + return new QueryResult([], 1, 100); + }; + $container = $this->buildContainer( IntegrationTestCase::buildRequest( 'post',