From 0efc2a74af0939cc4116c7dd2bdc0e06aec0d4d2 Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 24 Sep 2026 17:27:20 +0200 Subject: [PATCH] fix: the account manager cannot act on an account private to somebody else --- CLAUDE.md | 2 + src/Application/Account/Services/Account.php | 56 +++++++- .../Application/Account/AccountAccessTest.php | 33 +++++ .../Web/Controllers/Account/AccountTest.php | 2 +- .../AccountManager/AccountManagerTest.php | 6 +- .../Account/Services/AccountTest.php | 133 ++++++++++++++++-- 6 files changed, 208 insertions(+), 24 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e06fccf37..140341523 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -474,6 +474,8 @@ the whole boundary. Search paging clamped a negative offset in one of the two DT Who may mark an account private was decided in `AccountForm`, so the API's account create and edit wrote `private` / `privateGroup` as sent; it is now `Account::privacyAllowedFor()`, for the owner the account will actually have. +The account manager's grid left out an account private to somebody else, while its delete and +bulk edit acted on whatever ids were posted; `Account::assertNotWithheldAsPrivate()` guards those. It runs the other way too, and that is the more useful half: the API re-reads the user on every request and refuses a disabled one, while the web trusted what login had put in the session — so disabling an account stopped its token at once and left its browser session working, and since the diff --git a/src/Application/Account/Services/Account.php b/src/Application/Account/Services/Account.php index 1e0a3bf24..98097bc15 100644 --- a/src/Application/Account/Services/Account.php +++ b/src/Application/Account/Services/Account.php @@ -239,17 +239,19 @@ function () use ($accountUpdateBulkDto) { $userCanChangePermissions = AccountAcl::getShowPermission($userData, $userProfile); foreach ($accountUpdateBulkDto->getAccountUpdateDto() as $accountId => $accountUpdateDto) { + $account = $this->getById($accountId); + + $this->assertNotWithheldAsPrivate($account); + $changeOwner = false; $changeUserGroup = false; if ($userCanChangePermissions) { - $account = $this->getById($accountId); - $changeOwner = $this->userCanChangeOwner($userData, $userProfile, $account); $changeUserGroup = $this->userCanChangeGroup($userData, $userProfile, $account); } - $this->addHistory($accountId); + $this->addHistoryFor($account); if ($accountUpdateDto->userEditId === null) { $accountUpdateDto = $accountUpdateDto->mutate(['userEditId' => $userData->id]); @@ -399,10 +401,21 @@ protected function userCanChangeGroup( * @throws SPException */ private function addHistory(int $accountId, bool $isDelete = false): void + { + $this->addHistoryFor($this->getById($accountId), $isDelete); + } + + /** + * Records the account as it stands, from a copy the caller has already read + * + * @throws SPException + * @throws ServiceException + */ + private function addHistoryFor(AccountModel $account, bool $isDelete = false): void { $this->accountHistoryService->create( new AccountHistoryCreateDto( - $this->getById($accountId), + $account, !$isDelete, $isDelete, $this->configService->getByParam('masterPwd') @@ -728,6 +741,31 @@ public function restoreRemoved(AccountHistoryDto $accountHistoryDto): void } } + /** + * Refuse an account that is private to somebody else, answering as if it did not exist. + * + * The same rule `AccountAcl::isWithheldAsPrivate()` applies and `AccountFilter::buildFilterPrivate()` + * writes as SQL: `isPrivate` withholds an account from everybody but its owner, `isPrivateGroup` + * from everybody outside its group — application administrators included. The account + * manager's grid applies it, so a private account is not listed there, but its delete and bulk + * edit took whatever ids were posted and acted on them: a holder of `mgmAccounts` could delete, + * or overwrite the client, category, tags and expiry of, an account nobody but its owner can + * reach anywhere else. The answer is "not found" for the same reason the grid leaves it out. + * + * @throws NoSuchItemException + * @throws SPException + */ + private function assertNotWithheldAsPrivate(AccountModel $account): void + { + $userData = $this->context->getUserData(); + + if (($account->getIsPrivate() && $account->getUserId() !== $userData->id) + || ($account->getIsPrivateGroup() && $account->getUserGroupId() !== $userData->userGroupId) + ) { + throw new NoSuchItemException(__u('The account doesn\'t exist')); + } + } + /** * @throws ServiceException */ @@ -735,7 +773,10 @@ public function delete(int $id): AccountService { $this->accountRepository->transactionAware( function () use ($id) { - $this->addHistory($id, true); + $account = $this->getById($id); + + $this->assertNotWithheldAsPrivate($account); + $this->addHistoryFor($account, true); if ($this->accountRepository->delete($id)->getAffectedNumRows() === 0) { throw new NoSuchItemException(__u('Account not found')); @@ -761,7 +802,10 @@ public function deleteByIdBatch(array $ids): void // while deleting the same accounts as a selection destroyed them outright — the same // action, recoverable or not depending only on how many were ticked. foreach ($ids as $id) { - $this->addHistory((int)$id, true); + $account = $this->getById((int)$id); + + $this->assertNotWithheldAsPrivate($account); + $this->addHistoryFor($account, true); } $affectedNumRows = $this->accountRepository->deleteByIdBatch($ids)->getAffectedNumRows(); diff --git a/tests/Integration/Application/Account/AccountAccessTest.php b/tests/Integration/Application/Account/AccountAccessTest.php index 7d45346c5..6097e1aa6 100644 --- a/tests/Integration/Application/Account/AccountAccessTest.php +++ b/tests/Integration/Application/Account/AccountAccessTest.php @@ -56,6 +56,7 @@ use SP\Domain\Database\Ports\DbStorageHandler; use SP\Domain\File\FileSystem; use SP\Domain\User\Dtos\UserDto; +use SP\Domain\Common\Services\ServiceException; use SP\Domain\Core\Acl\AccountPermissionException; use SP\Domain\User\Models\ProfileData; use SP\Domain\User\Models\User as UserModel; @@ -872,6 +873,38 @@ public function testAPrivateAccountIsNotInTheManagerGridEither(): void self::assertContains($sharedId, $listed, 'the manager grid still lists accounts it always did'); } + /** + * Nor can the manager act on it by id. The grid leaves a private account out, but its delete + * and bulk edit took whatever ids were posted — so a holder of mgmAccounts could delete an + * account that nobody but its owner reaches anywhere else, by guessing a sequential id. + * + * @throws Exception + */ + public function testTheManagerCannotDeleteAPrivateAccountByItsId(): void + { + $ownerGroupId = $this->createGroup('mgm-del-owner'); + $ownerId = $this->createUser('mgm-del-owner', $ownerGroupId); + + $strangerGroupId = $this->createGroup('mgm-del-stranger'); + $strangerId = $this->createUser('mgm-del-stranger', $strangerGroupId); + + $privateId = $this->createAccount('mgm-del-private', 'MgmDel!1', $ownerId, $ownerGroupId, isPrivate: true); + + $this->setContextUser($strangerId, $strangerGroupId, 'mgm-del-stranger'); + + // transactionAware() rethrows the refusal as a ServiceException carrying its message. + try { + $this->dic->get(AccountService::class)->deleteByIdBatch([$privateId]); + self::fail('a private account was deleted by somebody it is withheld from'); + } catch (ServiceException $e) { + self::assertSame('The account doesn\'t exist', $e->getMessage()); + } + + $this->setContextUser($ownerId, $ownerGroupId, 'mgm-del-owner'); + + self::assertSame('MgmDel!1', $this->readPassword($privateId), 'the account is gone'); + } + /** * Its owner still sees it there, or the rule would have become "nobody manages a private * account" rather than "only its owner does". diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Account/AccountTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Account/AccountTest.php index 6d432ce61..1b536503b 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Account/AccountTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Account/AccountTest.php @@ -650,7 +650,7 @@ public function saveDelete() $this->addDatabaseMapperResolver( Account::class, - new QueryResult([$accountDataGenerator->buildAccount()]) + new QueryResult([$accountDataGenerator->buildAccount()->mutate(['isPrivate' => 0, 'isPrivateGroup' => 0])]) ); $container = $this->buildContainer( diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/AccountManager/AccountManagerTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/AccountManager/AccountManagerTest.php index 2a0872c2b..cffd84f53 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/AccountManager/AccountManagerTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/AccountManager/AccountManagerTest.php @@ -147,7 +147,7 @@ public function deleteSingle() $this->addDatabaseMapperResolver( Account::class, - new QueryResult([$accountDataGenerator->buildAccount()]) + new QueryResult([$accountDataGenerator->buildAccount()->mutate(['isPrivate' => 0, 'isPrivateGroup' => 0])]) ); $container = $this->buildContainer( @@ -180,7 +180,7 @@ public function deleteMultiple() return new QueryResult([$accountView]); } elseif ($queryData->getMapClassName() === Account::class) { $account = AccountDataGenerator::factory() - ->buildAccount(); + ->buildAccount()->mutate(['isPrivate' => 0, 'isPrivateGroup' => 0]); return new QueryResult([$account]); } elseif ($queryData->getMapClassName() === Config::class) { @@ -217,7 +217,7 @@ public function saveBulkEdit() $this->addDatabaseMapperResolver( Account::class, - new QueryResult([$accountDataGenerator->buildAccount()]) + new QueryResult([$accountDataGenerator->buildAccount()->mutate(['isPrivate' => 0, 'isPrivateGroup' => 0])]) ); $this->addDatabaseMapperResolver( diff --git a/tests/Unit/Application/Account/Services/AccountTest.php b/tests/Unit/Application/Account/Services/AccountTest.php index 156adb8a8..d3756187f 100644 --- a/tests/Unit/Application/Account/Services/AccountTest.php +++ b/tests/Unit/Application/Account/Services/AccountTest.php @@ -832,12 +832,12 @@ public function testUpdateBulk() ) ); - $consecutive = array_merge($accountsId, $accountsId); - sort($consecutive); + // Read once each: the same copy decides privacy, ownership and what goes into history. + $consecutive = $accountsId; $this->accountRepository->expects(self::exactly(count($consecutive)))->method('getById') ->with(...self::withConsecutive(...array_map(fn($v) => [$v], $consecutive))) - ->willReturn(new QueryResult([$accountDataGenerator->buildAccount()])); + ->willReturn(new QueryResult([self::anAccountNobodyWithholds()])); $this->configService->expects(self::exactly(count($accountsId)))->method('getByParam') ->with('masterPwd')->willReturn(self::$faker->password()); $this->accountItemsService->expects(self::exactly(count($accountsId))) @@ -879,7 +879,7 @@ public function testUpdateBulkCannotChangePermissionsWithoutAdminApp() $this->accountRepository->expects(self::exactly(count($accountsId)))->method('getById') ->with(...self::withConsecutive(...array_map(fn($v) => [$v], $accountsId))) - ->willReturn(new QueryResult([$accountDataGenerator->buildAccount()])); + ->willReturn(new QueryResult([self::anAccountNobodyWithholds()])); $this->configService->expects(self::exactly(count($accountsId)))->method('getByParam') ->with('masterPwd')->willReturn(self::$faker->password()); $this->accountItemsService->expects(self::exactly(count($accountsId)))->method('updateItems') @@ -920,12 +920,12 @@ public function testUpdateBulkCanChangePermissionsWithAdminAcc() $this->context->setUserProfile(new ProfileData(['accPermission' => false])); - $consecutive = array_merge($accountsId, $accountsId); - sort($consecutive); + // Read once each: the same copy decides privacy, ownership and what goes into history. + $consecutive = $accountsId; $this->accountRepository->expects(self::exactly(count($consecutive)))->method('getById') ->with(...self::withConsecutive(...array_map(fn($v) => [$v], $consecutive))) - ->willReturn(new QueryResult([$accountDataGenerator->buildAccount()])); + ->willReturn(new QueryResult([self::anAccountNobodyWithholds()])); $this->configService->expects(self::exactly(count($accountsId)))->method('getByParam') ->with('masterPwd')->willReturn(self::$faker->password()); $this->accountItemsService->expects(self::exactly(count($accountsId)))->method('updateItems') @@ -966,12 +966,12 @@ public function testUpdateBulkCanChangePermissionsWithProfilePermission() $this->context->setUserProfile(new ProfileData(['accPermission' => true])); - $consecutive = array_merge($accountsId, $accountsId); - sort($consecutive); + // Read once each: the same copy decides privacy, ownership and what goes into history. + $consecutive = $accountsId; $this->accountRepository->expects(self::exactly(count($consecutive)))->method('getById') ->with(...self::withConsecutive(...array_map(fn($v) => [$v], $consecutive))) - ->willReturn(new QueryResult([$accountDataGenerator->buildAccount()])); + ->willReturn(new QueryResult([self::anAccountNobodyWithholds()])); $this->configService->expects(self::exactly(count($accountsId)))->method('getByParam') ->with('masterPwd')->willReturn(self::$faker->password()); $this->accountItemsService->expects(self::exactly(count($accountsId)))->method('updateItems') @@ -1008,7 +1008,7 @@ public function testUpdateBulkDefaultsOmittedUserEditIdToTheCurrentSessionUser() $this->accountRepository->expects(self::once())->method('getById') ->with($id) - ->willReturn(new QueryResult([$accountDataGenerator->buildAccount()])); + ->willReturn(new QueryResult([self::anAccountNobodyWithholds()])); $this->configService->expects(self::once())->method('getByParam') ->with('masterPwd')->willReturn(self::$faker->password()); @@ -1049,7 +1049,7 @@ public function testDelete() { $id = self::$faker->randomNumber(); $password = self::$faker->password(); - $account = AccountDataGenerator::factory()->buildAccount(); + $account = self::anAccountNobodyWithholds(); $accountHistoryCreateDto = new AccountHistoryCreateDto($account, false, true, $password); $this->configService->expects(self::once()) @@ -1072,6 +1072,102 @@ public function testDelete() $this->account->delete($id); } + /** + * The account manager deletes by the ids it is posted, and its grid leaves out an account + * private to somebody else — so posting that id anyway must not delete it. Answered as not + * found, as the grid answers by not listing it. + * + * @throws ServiceException + * @throws SPException + */ + public function testDeleteRefusesAnAccountPrivateToSomebodyElse() + { + $id = self::$faker->randomNumber(); + $userData = $this->context->getUserData(); + $account = self::anAccountNobodyWithholds()->mutate(['isPrivate' => 1, 'userId' => $userData->id + 1]); + + $this->accountRepository->method('getById')->willReturn(new QueryResult([$account])); + $this->accountHistoryService->expects(self::never())->method('create'); + $this->accountRepository->expects(self::never())->method('delete'); + + $this->expectException(NoSuchItemException::class); + $this->expectExceptionMessage('The account doesn\'t exist'); + + $this->account->delete($id); + } + + /** + * Its owner still deletes it: privacy withholds an account from everybody else. + * + * @throws ServiceException + * @throws SPException + */ + public function testTheOwnerMayDeleteTheirOwnPrivateAccount() + { + $id = self::$faker->randomNumber(); + $userData = $this->context->getUserData(); + $account = self::anAccountNobodyWithholds()->mutate( + [ + 'isPrivate' => 1, + 'isPrivateGroup' => 1, + 'userId' => $userData->id, + 'userGroupId' => $userData->userGroupId, + ] + ); + + $this->configService->method('getByParam')->willReturn(self::$faker->password()); + $this->accountRepository->method('getById')->willReturn(new QueryResult([$account])); + $this->accountHistoryService->expects(self::once())->method('create'); + $this->accountRepository->expects(self::once())->method('delete')->willReturn(new QueryResult(null, 1)); + + $this->account->delete($id); + } + + /** + * A selection is refused whole when any account in it is private to somebody else's group — + * nothing in it is deleted, and nothing goes into history. + * + * @throws ServiceException + * @throws SPException + */ + public function testDeleteByIdBatchRefusesASelectionHoldingAnAccountPrivateToAnotherGroup() + { + $userData = $this->context->getUserData(); + $withheld = self::anAccountNobodyWithholds()->mutate( + ['isPrivateGroup' => 1, 'userGroupId' => $userData->userGroupId + 1] + ); + + $this->accountRepository->method('getById')->willReturn(new QueryResult([$withheld])); + $this->accountHistoryService->expects(self::never())->method('create'); + $this->accountRepository->expects(self::never())->method('deleteByIdBatch'); + + $this->expectException(NoSuchItemException::class); + + $this->account->deleteByIdBatch([1, 2]); + } + + /** + * And a bulk edit does not overwrite one either. + * + * @throws ServiceException + * @throws SPException + */ + public function testUpdateBulkRefusesAnAccountPrivateToSomebodyElse() + { + $userData = $this->context->getUserData(); + $withheld = self::anAccountNobodyWithholds()->mutate(['isPrivate' => 1, 'userId' => $userData->id + 1]); + + $this->accountRepository->method('getById')->willReturn(new QueryResult([$withheld])); + $this->accountHistoryService->expects(self::never())->method('create'); + $this->accountRepository->expects(self::never())->method('updateBulk'); + + $this->expectException(NoSuchItemException::class); + + $this->account->updateBulk( + new AccountUpdateBulkDto([1], [1 => AccountDataGenerator::factory()->buildAccountUpdateDto()]) + ); + } + /** * @throws ServiceException */ @@ -1079,7 +1175,7 @@ public function testDeleteNotFound() { $id = self::$faker->randomNumber(); $password = self::$faker->password(); - $account = AccountDataGenerator::factory()->buildAccount(); + $account = self::anAccountNobodyWithholds(); $accountHistoryCreateDto = new AccountHistoryCreateDto($account, false, true, $password); $this->configService->expects(self::once())->method('getByParam') @@ -1838,10 +1934,19 @@ private function givenEachAccountIsPushedIntoHistory(int $count): void $this->accountRepository ->method('getById') - ->willReturn(new QueryResult([AccountDataGenerator::factory()->buildAccount()])); + ->willReturn(new QueryResult([self::anAccountNobodyWithholds()])); $this->accountHistoryService->expects(self::exactly($count))->method('create'); } + /** + * An account private to nobody, so the manager's paths — which refuse one private to somebody + * else — act on it. The generator draws both flags at random for a random owner. + */ + private static function anAccountNobodyWithholds(): AccountModel + { + return AccountDataGenerator::factory()->buildAccount()->mutate(['isPrivate' => 0, 'isPrivateGroup' => 0]); + } + /** * @throws ServiceException