fix: requesting access does not name an account you could not list

CLAUDE.md

@@ -536,6 +536,23 @@ and whether the thing being guarded varies with it.** Here it did not — which
 `CustomField::valueFor()` decides on `isValueEncrypted` and never looks at the type, was right all
 along. The fix computes the decision once per field, above the switch.
 
+**An unfiltered read whose caller was the one without a check.** `AccountRepository::getByIdEnriched()`
+applies no filter on purpose — most callers pair it with an explicit per-account ACL check, and the
+"swept and clean" note above records that every path to a secret does. `account/requestAccess` did
+neither: `ACCOUNT_REQUEST` sits in the unconditional arm of `Acl::checkUserAccess()` beside the
+notification actions, and `AccountRequestHelper` is the one helper in its directory that does not
+call `checkAccess()`, where `AccountHelper` and `AccountHistoryHelper` both do. Any signed-in user
+could walk the ids and read back each account's name and client — private accounts included, which
+the search filter withholds from everybody, administrators among them.
+
+The fix is the interesting part, and the obvious one is wrong: **the usual per-account check would
+have broken the feature.** Requesting a modification exists precisely for an account you can see
+listed and cannot open, which under global search (`isFilterWithoutGlobalSearch()`) is an account
+you have no relationship with at all — `AccountSearchItem::isShowRequest()` is literally
+`!isShow()`. The bound is therefore the *search filter*, which is what decides listability, not the
+ACL. **Ask what a feature is for before deciding which check it was missing**; a guard copied from
+a sibling can be the wrong guard.
+
 **A guard on the read but not on the write.** `Notification` has the rule written down and named —
 `checkUserAccess()`, admins may reach any notification and regular users only their own, answering
 "not found" so ids cannot be enumerated by the difference. It was called from `getById()` and

src/Application/Account/Ports/AccountService.php

@@ -138,6 +138,18 @@ public function create(AccountCreateDto $accountCreateDto): int;
      */
     public function getByIdEnriched(int $id): AccountView;
 
+    /**
+     * The same account, but only when the signed-in user could have found it by searching.
+     *
+     * @param int $id
+     *
+     * @return AccountView
+     * @throws QueryException
+     * @throws NoSuchItemException
+     * @throws ConstraintException
+     */
+    public function getByIdEnrichedForUser(int $id): AccountView;
+
     /**
      * @param int $id The account ID
      *

src/Application/Account/Services/Account.php

@@ -191,6 +191,37 @@ public function getByIdEnriched(int $id): AccountView
         return $result->getData(AccountView::class);
     }
 
+    /**
+     * The same account, but only when the signed-in user could have found it by searching.
+     *
+     * `getByIdEnriched()` applies no filter, on purpose: most of its callers pair it with an
+     * explicit per-account ACL check. The "request modification" flow has neither, and cannot use
+     * the usual check either — asking about an account you can see listed and cannot open is what
+     * the feature is for, and under global search that is an account you have no relationship
+     * with. The search filter is what decides listability, so it is the right bound, and it
+     * withholds a private account from everybody including administrators.
+     *
+     * An account the user could not have listed is refused with the same "doesn't exist" as one
+     * that is really absent, so the two cannot be told apart by the answer.
+     *
+     * @param int $id
+     *
+     * @return AccountView
+     * @throws ConstraintException
+     * @throws NoSuchItemException
+     * @throws QueryException
+     */
+    public function getByIdEnrichedForUser(int $id): AccountView
+    {
+        $result = $this->accountRepository->getByIdEnrichedForUser($id);
+
+        if ($result->getNumRows() === 0) {
+            throw new NoSuchItemException(__u('The account doesn\'t exist'));
+        }
+
+        return $result->getData(AccountView::class);
+    }
+
     /**
      * Update accounts in bulk mode
      *

src/Domain/Account/Ports/AccountRepository.php

@@ -159,6 +159,17 @@ public function incrementViewCounter(int $accountId): QueryResult;
      */
     public function getDataForLink(int $accountId): QueryResult;
 
+    /**
+     * The enriched account, but only when the signed-in user could have found it by searching.
+     *
+     * @param int $accountId
+     *
+     * @return QueryResult<AccountViewModel>
+     * @throws ConstraintException
+     * @throws QueryException
+     */
+    public function getByIdEnrichedForUser(int $accountId): QueryResult;
+
     /**
      * @param int|null $accountId
      *

src/Infrastructure/Adapter/In/Web/Controllers/Account/RequestAccessController.php

@@ -75,7 +75,7 @@ public function requestAccessAction(int $id): ActionResponse
         $this->accountRequestHelper->initializeFor(AclActionsInterface::ACCOUNT_REQUEST);
         $this->accountRequestHelper->setIsView(true);
         $this->accountRequestHelper->setViewForRequest(
-            new AccountEnrichedDto($this->accountService->getByIdEnriched($id))
+            new AccountEnrichedDto($this->accountService->getByIdEnrichedForUser($id))
         );
 
         $this->view->addTemplate('account-request');

src/Infrastructure/Adapter/In/Web/Controllers/Account/SaveRequestController.php

@@ -74,7 +74,7 @@ public function saveRequestAction(int $id): ActionResponse
             throw new ValidationException(__u('A description is needed'));
         }
 
-        $accountView = $this->accountService->getByIdEnriched($id);
+        $accountView = $this->accountService->getByIdEnrichedForUser($id);
 
         $baseUrl = ($this->configData->getApplicationUrl() ?: $this->uriContext->getWebUri()) .
                    $this->uriContext->getSubUri();

src/Infrastructure/Adapter/Out/Account/Repositories/Account.php

@@ -639,6 +639,42 @@ public function getDataForLink(int $accountId): QueryResult
         return $this->db->runQuery($queryData);
     }
 
+    /**
+     * The enriched account, but only when the signed-in user could have found it by searching.
+     *
+     * `getByIdEnriched()` above applies no filter, on purpose: most of its callers pair it with an
+     * explicit per-account ACL check. The "request modification" flow has neither, and cannot use
+     * the usual check either — asking about an account you can see listed and cannot open is what
+     * that feature is *for*, and under global search that is an account you have no relationship
+     * with. So the bound is the search filter itself, which is what decides listability, and which
+     * withholds a private account from everybody, administrators included.
+     *
+     * The view is aliased to `Account` because that is the name `AccountFilter` qualifies its
+     * conditions with, and `account_data_v` exposes every column they read — `id`, `userId`,
+     * `userGroupId`, `isPrivate`, `isPrivateGroup`.
+     *
+     * @param int $accountId
+     *
+     * @return QueryResult<AccountViewModel>
+     * @throws ConstraintException
+     * @throws QueryException
+     */
+    public function getByIdEnrichedForUser(int $accountId): QueryResult
+    {
+        $query = $this->queryFactory
+            ->newSelect()
+            ->from(sprintf('%s AS Account', AccountViewModel::TABLE))
+            ->cols(AccountViewModel::getCols())
+            ->where('Account.id = :id')
+            ->bindValues(['id' => $accountId])
+            ->limit(1);
+
+        $queryData = QueryData::buildWithMapper($this->accountFilterUser->buildFilter(false, $query), AccountViewModel::class)
+                              ->setOnErrorMessage(__u('Error while retrieving account\'s data'));
+
+        return $this->db->runQuery($queryData);
+    }
+
     /**
      * @param int|null $accountId
      *

tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Account/AccountTest.php

@@ -49,6 +49,7 @@
 use SP\Domain\User\Models\ProfileData;
 use SP\Domain\User\Models\User as UserModel;
 use SP\Domain\Common\Dtos\QueryResult;
+use SP\Infrastructure\Database\QueryData;
 use SP\Tests\Support\BodyChecker;
 use SP\Tests\Support\Generators\AccountDataGenerator;
 use SP\Tests\Support\Generators\PublicLinkDataGenerator;
@@ -66,6 +67,8 @@
 #[InjectVault]
 class AccountTest extends IntegrationTestCase
 {
+    private const UNLISTABLE_ACCOUNT_NAME = 'an-account-this-user-cannot-list';
+
     private const OWNER_NAME = 'Fixture Person';
     private const GROUP_NAME = 'Fixture Team';
 
@@ -882,6 +885,60 @@ public function requestAccess()
         IntegrationTestCase::runApp($container);
     }
 
+    /**
+     * Requesting a modification does not name an account the caller could not have found.
+     *
+     * `ACCOUNT_REQUEST` answers `true` for every signed-in user — it is in the same unconditional
+     * arm as the notification actions — and the controller read the account with
+     * `getByIdEnriched()`, whose query is a bare `WHERE id = :id`. `AccountRequestHelper` is the
+     * one helper in its directory that does not call `checkAccess()`, where `AccountHelper` and
+     * `AccountHistoryHelper` both do. So any authenticated user could walk the ids and read back
+     * each account's name and client — including accounts marked private, which the search filter
+     * withholds from everybody, administrators included.
+     *
+     * The fix cannot be the usual per-account ACL check: this feature exists to ask about an
+     * account you can see listed and cannot open, which under global search is an account you have
+     * no relationship with. The bound is the search filter, which is what decides listability.
+     *
+     * The two queries are told apart by their statement rather than by their mapper, because both
+     * map to `AccountView` — only the filtered one joins `AccountToUser`, and on the old code no
+     * statement did, so the account was found and rendered.
+     *
+     * @throws ContainerExceptionInterface
+     * @throws Exception
+     * @throws NotFoundExceptionInterface
+     */
+    #[Test]
+    #[BodyChecker('outputCheckerRequestAccessRefused')]
+    public function requestAccessDoesNotNameAnAccountTheUserCannotList()
+    {
+        $account = AccountDataGenerator::factory()->buildAccountDataView()->mutate(
+            ['name' => self::UNLISTABLE_ACCOUNT_NAME]
+        );
+
+        // Not a static closure: the harness binds it with Closure::call(). Everything other than
+        // the account read has to keep answering the way the harness's default does, or the
+        // request fails before it renders and the assertion below passes for the wrong reason.
+        $this->databaseQueryResolver = function (QueryData $queryData) use ($account): QueryResult {
+            if ($queryData->getMapClassName() !== AccountView::class) {
+                return new QueryResult([], 1, 100);
+            }
+
+            if (str_contains($queryData->getQuery()->getStatement(), 'AccountToUser')) {
+                // The filtered read: this user could not have listed it.
+                return new QueryResult([]);
+            }
+
+            return new QueryResult([$account]);
+        };
+
+        $container = $this->buildContainer(
+            IntegrationTestCase::buildRequest('get', 'index.php', ['r' => 'account/requestAccess/100'])
+        );
+
+        IntegrationTestCase::runApp($container);
+    }
+
     /**
      * @throws ContainerExceptionInterface
      * @throws Exception
@@ -1121,6 +1178,14 @@ private function outputCheckerSearch(string $output): void
         self::assertEquals('OK', $json->status);
     }
 
+    /**
+     * The account's name is nowhere in what was sent back.
+     */
+    private function outputCheckerRequestAccessRefused(string $output): void
+    {
+        self::assertStringNotContainsString(self::UNLISTABLE_ACCOUNT_NAME, $output);
+    }
+
     /**
      * @param string $output
      * @return void