diff --git a/CLAUDE.md b/CLAUDE.md index dfa9cdd4d..d71fffb0e 100644 --- a/CLAUDE.md +++ b/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 diff --git a/src/Application/Account/Ports/AccountService.php b/src/Application/Account/Ports/AccountService.php index dc6be6a9b..49976f765 100644 --- a/src/Application/Account/Ports/AccountService.php +++ b/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 * diff --git a/src/Application/Account/Services/Account.php b/src/Application/Account/Services/Account.php index f19b0768b..bb8b0ea86 100644 --- a/src/Application/Account/Services/Account.php +++ b/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 * diff --git a/src/Domain/Account/Ports/AccountRepository.php b/src/Domain/Account/Ports/AccountRepository.php index ea25d8b3c..be7a406d9 100644 --- a/src/Domain/Account/Ports/AccountRepository.php +++ b/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 + * @throws ConstraintException + * @throws QueryException + */ + public function getByIdEnrichedForUser(int $accountId): QueryResult; + /** * @param int|null $accountId * diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/Account/RequestAccessController.php b/src/Infrastructure/Adapter/In/Web/Controllers/Account/RequestAccessController.php index 0f19e0fae..5c041d162 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/Account/RequestAccessController.php +++ b/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'); diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/Account/SaveRequestController.php b/src/Infrastructure/Adapter/In/Web/Controllers/Account/SaveRequestController.php index f9eb574aa..5921ec04c 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/Account/SaveRequestController.php +++ b/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(); diff --git a/src/Infrastructure/Adapter/Out/Account/Repositories/Account.php b/src/Infrastructure/Adapter/Out/Account/Repositories/Account.php index ac4ce852e..b0352fd0c 100644 --- a/src/Infrastructure/Adapter/Out/Account/Repositories/Account.php +++ b/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 + * @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 * 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 01a998f8c..6d432ce61 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Account/AccountTest.php +++ b/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