fix: the account manager cannot act on an account private to somebody else

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

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,14 +741,42 @@ 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
      */
     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();

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".

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(

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(