From 569cd00aa503306644a3c527836c4fdb246393c4 Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 24 Sep 2026 16:59:50 +0200 Subject: [PATCH] fix: who may mark an account private is decided for every door --- CLAUDE.md | 3 + src/Application/Account/Services/Account.php | 94 ++++++++++---- .../Adapter/In/Web/Forms/AccountForm.php | 41 ------ .../Account/Services/AccountTest.php | 119 ++++++++++++++++-- .../Adapter/In/Web/Forms/AccountFormTest.php | 48 +------ 5 files changed, 190 insertions(+), 115 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 380fd7e27..e06fccf37 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -471,6 +471,9 @@ user and account endpoints did neither. Custom-field values were masked for a ca enforced in five web config actions and mentioned nowhere on the API at all — the sharpest case, because a demo publishes its administrator's credentials, so the ACL stops nobody and that guard is the whole boundary. Search paging clamped a negative offset in one of the two DTOs that carry one. +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. 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 9defbbb25..1e0a3bf24 100644 --- a/src/Application/Account/Services/Account.php +++ b/src/Application/Account/Services/Account.php @@ -295,29 +295,42 @@ public function getById(int $id): AccountModel * @return bool */ /** - * The privacy flags a restore may write, decided the way the edit screen decides them. + * Whether the signed-in user may mark an account owned by `$ownerId` / `$groupId` private to its + * owner and to its group — the one definition every write asks. * - * Mirrors `AccountForm::constrainPrivacyToPermission()`. Both flags only ever *withhold* an - * account, so this is not a way to reach anything — it is a way to hide one, or to stop hiding - * one, without the permission that governs it. + * Only an application administrator may, or the owner whose profile holds `isAccPrivate()` (the + * main group, `isAccPrivateGroup()`). Both flags only ever *withhold* an account, so this is not + * a way to reach anything; it is a way to hide one, or to stop hiding one, without the + * permission that governs it — and `AccountAcl` tests privacy before the administrator branch, + * so a private account disappears for account administrators as well. + * + * It used to be decided in `AccountForm`, which only the web reaches: `POST account/create` and + * `account/edit` on the API put the caller's `private` / `privateGroup` straight into the row. + * + * @return array{0: bool, 1: bool} May be private, may be private to the group + */ + private function privacyAllowedFor(?int $ownerId, ?int $groupId): array + { + $userData = $this->context->getUserData(); + $userProfile = $this->context->getUserProfile() ?? new ProfileData(); + + return [ + $userData->isAdminApp || ($userProfile->isAccPrivate() && $ownerId === $userData->id), + $userData->isAdminApp + || ($userProfile->isAccPrivateGroup() && $groupId === $userData->userGroupId), + ]; + } + + /** + * The privacy flags a restore may write, decided the way every other write decides them. * * @param AccountHistoryDto $dto - * @param UserDto $userData - * @param ProfileData $userProfile * * @return AccountHistoryDto */ - private function constrainPrivacyToPermission( - AccountHistoryDto $dto, - UserDto $userData, - ProfileData $userProfile - ): AccountHistoryDto { - $mayBePrivate = $userData->isAdminApp - || ($userProfile->isAccPrivate() && $dto->userId === $userData->id); - - $mayBePrivateGroup = $userData->isAdminApp - || ($userProfile->isAccPrivateGroup() - && $dto->userGroupId === $userData->userGroupId); + private function constrainPrivacyToPermission(AccountHistoryDto $dto): AccountHistoryDto + { + [$mayBePrivate, $mayBePrivateGroup] = $this->privacyAllowedFor($dto->userId, $dto->userGroupId); return $dto->mutate( [ @@ -327,6 +340,32 @@ private function constrainPrivacyToPermission( ); } + /** + * The privacy flags a create or an edit may write, for the account's effective owner and group. + * + * @template T of AccountCreateDto|AccountUpdateDto + * @param T $dto + * + * @return T + */ + private function constrainAccountPrivacy( + AccountCreateDto|AccountUpdateDto $dto, + ?int $ownerId, + ?int $groupId + ): AccountCreateDto|AccountUpdateDto { + [$mayBePrivate, $mayBePrivateGroup] = $this->privacyAllowedFor($ownerId, $groupId); + + if (!$mayBePrivate) { + $dto = $dto->withPrivate(false); + } + + if (!$mayBePrivateGroup) { + $dto = $dto->withPrivateGroup(false); + } + + return $dto; + } + protected function userCanChangeOwner( UserDto $userData, ProfileData $userProfile, @@ -402,6 +441,11 @@ function () use ($accountCreateDto) { $this->accountCryptService->getPasswordEncrypted($accountCreateDto->pass) ); + $accountCreateDto = $this->constrainAccountPrivacy( + $accountCreateDto, + $accountCreateDto->userId, + $accountCreateDto->userGroupId + ); $accountCreateDto = $this->setPresetPrivate($accountCreateDto); $accountId = $this->accountRepository->create(AccountModel::create($accountCreateDto))->getLastId(); @@ -501,12 +545,20 @@ function () use ($id, $accountUpdateDto) { ); } } else { + $account = $this->getById($id); $changeOwner = false; $changeUserGroup = false; } $this->addHistory($id); + // Decided for whoever will own the account once this is saved: an owner or group + // the caller may not change is not changed, whatever the request said. + $accountUpdateDto = $this->constrainAccountPrivacy( + $accountUpdateDto, + $changeOwner ? $accountUpdateDto->userId : $account->getUserId(), + $changeUserGroup ? $accountUpdateDto->userGroupId : $account->getUserGroupId() + ); $accountUpdateDto = $this->setPresetPrivate($accountUpdateDto, $id); if ($accountUpdateDto->userEditId === null) { @@ -631,14 +683,14 @@ function () use ($dto) { // And the two privacy flags, which are the same question asked about a different // pair of columns and were still being taken from the snapshot. // - // `AccountForm::constrainPrivacyToPermission()` decides this on the edit screen: - // only an application administrator, or the owner holding `isAccPrivate()` (the - // group holding `isAccPrivateGroup()`), may set them. Nothing re-applied it on the + // `privacyAllowedFor()` decides this for a create or an edit: only an application + // administrator, or the owner holding `isAccPrivate()` (the group holding + // `isAccPrivateGroup()`), may set them. Nothing re-applied it on the // way back from a history row, so a restore let anybody with edit rights mark an // account private — and `AccountAcl` tests privacy *before* the administrator // branch, so a private account disappears for account administrators too — or strip // the privacy from one that had it. - $dto = $this->constrainPrivacyToPermission($dto, $userData, $userProfile); + $dto = $this->constrainPrivacyToPermission($dto); $this->addHistory($dto->accountId); diff --git a/src/Infrastructure/Adapter/In/Web/Forms/AccountForm.php b/src/Infrastructure/Adapter/In/Web/Forms/AccountForm.php index afeabc2e4..f1a9a4853 100644 --- a/src/Infrastructure/Adapter/In/Web/Forms/AccountForm.php +++ b/src/Infrastructure/Adapter/In/Web/Forms/AccountForm.php @@ -139,52 +139,11 @@ private function analyzeRequestData(): AccountCreateDto|AccountUpdateDto 'userGroupId' => $this->request->analyzeInt('main_usergroup_id'), ]; - $properties = $this->constrainPrivacyToPermission($properties); - return $this->itemId === null ? AccountCreateDto::fromArray($properties) : AccountUpdateDto::fromArray( $properties ); } - /** - * Hold the two privacy flags to the same rule the interface applies when it decides whether to - * offer them. - * - * `AccountHelper` computes `allowPrivate` / `allowPrivateGroup` from the profile permission and - * ownership, and the template omits the checkbox when they are false — but nothing looked at - * either on the way back in, so a request that carried `private_enabled` anyway was honoured. - * That is not a way to reach anything: both flags only ever *withhold* an account. It is a way - * to hide one, and `AccountAcl` tests privacy before the administrator branch, so an account - * marked private disappears for account administrators too. - * - * @param array $properties - * - * @return array - */ - private function constrainPrivacyToPermission(array $properties): array - { - $userData = $this->context->getUserData(); - $userProfile = $this->context->getUserProfile(); - - $mayBePrivate = $userData->isAdminApp - || (($userProfile?->isAccPrivate() ?? false) - && $properties['userId'] === $userData->id); - - $mayBePrivateGroup = $userData->isAdminApp - || (($userProfile?->isAccPrivateGroup() ?? false) - && $properties['userGroupId'] === $userData->userGroupId); - - if (!$mayBePrivate) { - $properties['isPrivate'] = 0; - } - - if (!$mayBePrivateGroup) { - $properties['isPrivateGroup'] = 0; - } - - return $properties; - } - /** * @throws ValidationException */ diff --git a/tests/Unit/Application/Account/Services/AccountTest.php b/tests/Unit/Application/Account/Services/AccountTest.php index 9d385a8c9..156adb8a8 100644 --- a/tests/Unit/Application/Account/Services/AccountTest.php +++ b/tests/Unit/Application/Account/Services/AccountTest.php @@ -130,6 +130,10 @@ public function testUpdateUserCannotChangePermissionsWithoutPermission() $accountDataGenerator = AccountDataGenerator::factory(); $accountUpdateDto = $accountDataGenerator->buildAccountUpdateDto(); + // Nobody signed in here may mark an account private, so whatever the request carried, the + // account is written with both flags off. + $expectedDto = $accountUpdateDto->withPrivate(false)->withPrivateGroup(false); + $this->context->setUserData( UserDto::fromModel( UserDataGenerator::factory() @@ -146,19 +150,107 @@ public function testUpdateUserCannotChangePermissionsWithoutPermission() $this->itemPresetService->expects(self::once())->method('getForCurrentUser') ->with(ItemPresetInterface::ITEM_TYPE_ACCOUNT_PRIVATE) ->willReturn(null); - $this->accountRepository->expects(self::once())->method('getById') + // Once for the history, once to know who owns the account the flags are decided for. + $this->accountRepository->expects(self::exactly(2))->method('getById') ->with($id) ->willReturn(new QueryResult([$accountDataGenerator->buildAccount()])); $this->accountRepository->expects(self::once())->method('update') - ->with($id, AccountModel::update($accountUpdateDto), false, false) + ->with($id, AccountModel::update($expectedDto), false, false) ->willReturn(new QueryResult(null, 1)); $this->accountItemsService->expects(self::once())->method('updateItems') - ->with(false, $id, $accountUpdateDto); + ->with(false, $id, $expectedDto); $this->accountPresetService->expects(self::once())->method('addPresetPermissions')->with($id); $this->account->update($id, $accountUpdateDto); } + /** + * An owner whose profile may mark accounts private keeps the flags they asked for. + * + * @throws ServiceException + * @throws SPException + */ + public function testUpdateKeepsPrivacyAskedForByAnOwnerWithThePermission() + { + $id = self::$faker->randomNumber(); + $accountDataGenerator = AccountDataGenerator::factory(); + $userData = UserDto::fromModel( + UserDataGenerator::factory()->buildUserData()->mutate(['isAdminApp' => false, 'isAdminAcc' => false]) + ); + + $this->context->setUserData($userData); + $this->context->setUserProfile(new ProfileData(['accPrivate' => true, 'accPrivateGroup' => true])); + + $accountUpdateDto = $accountDataGenerator->buildAccountUpdateDto()->withPrivate(true)->withPrivateGroup(true); + $stored = $accountDataGenerator->buildAccount()->mutate( + ['userId' => $userData->id, 'userGroupId' => $userData->userGroupId] + ); + + $this->configService->method('getByParam')->willReturn(self::$faker->password()); + $this->itemPresetService->method('getForCurrentUser')->willReturn(null); + $this->accountRepository->method('getById')->willReturn(new QueryResult([$stored])); + $this->accountRepository->expects(self::once())->method('update') + ->with( + $id, + self::callback( + static fn(AccountModel $account) => $account->getIsPrivate() === 1 + && $account->getIsPrivateGroup() === 1 + ), + false, + false + ) + ->willReturn(new QueryResult(null, 1)); + + $this->account->update($id, $accountUpdateDto); + } + + /** + * The flags are decided for whoever owns the account once it is saved. A caller who may not + * change the owner does not become it by naming themselves in the request, so the permission to + * make one's *own* accounts private does not reach somebody else's — which the API's + * `account/edit` allowed, since it carried `private` / `privateGroup` straight into the row. + * + * @throws ServiceException + * @throws SPException + */ + public function testUpdatePrivacyIsDecidedForTheStoredOwnerNotTheOneRequested() + { + $id = self::$faker->randomNumber(); + $accountDataGenerator = AccountDataGenerator::factory(); + $userData = UserDto::fromModel( + UserDataGenerator::factory()->buildUserData()->mutate(['isAdminApp' => false, 'isAdminAcc' => false]) + ); + + $this->context->setUserData($userData); + $this->context->setUserProfile(new ProfileData(['accPrivate' => true, 'accPrivateGroup' => true])); + + $accountUpdateDto = $accountDataGenerator->buildAccountUpdateDto() + ->withUserId($userData->id) + ->withUserGroupId($userData->userGroupId) + ->withPrivate(true) + ->withPrivateGroup(true); + $stored = $accountDataGenerator->buildAccount()->mutate( + ['userId' => $userData->id + 1, 'userGroupId' => $userData->userGroupId + 1] + ); + + $this->configService->method('getByParam')->willReturn(self::$faker->password()); + $this->itemPresetService->method('getForCurrentUser')->willReturn(null); + $this->accountRepository->method('getById')->willReturn(new QueryResult([$stored])); + $this->accountRepository->expects(self::once())->method('update') + ->with( + $id, + self::callback( + static fn(AccountModel $account) => $account->getIsPrivate() === 0 + && $account->getIsPrivateGroup() === 0 + ), + false, + false + ) + ->willReturn(new QueryResult(null, 1)); + + $this->account->update($id, $accountUpdateDto); + } + /** * @throws ServiceException * @throws SPException @@ -169,6 +261,10 @@ public function testUpdateUserCanChangePermissionsWithAdminAcc() $accountDataGenerator = AccountDataGenerator::factory(); $accountUpdateDto = $accountDataGenerator->buildAccountUpdateDto(); + // Nobody signed in here may mark an account private, so whatever the request carried, the + // account is written with both flags off. + $expectedDto = $accountUpdateDto->withPrivate(false)->withPrivateGroup(false); + $this->context->setUserData( UserDto::fromModel( UserDataGenerator::factory() @@ -189,10 +285,10 @@ public function testUpdateUserCanChangePermissionsWithAdminAcc() ->with($id) ->willReturn(new QueryResult([$accountDataGenerator->buildAccount()])); $this->accountRepository->expects(self::once())->method('update') - ->with($id, AccountModel::update($accountUpdateDto), true, true) + ->with($id, AccountModel::update($expectedDto), true, true) ->willReturn(new QueryResult(null, 1)); $this->accountItemsService->expects(self::once())->method('updateItems') - ->with(true, $id, $accountUpdateDto); + ->with(true, $id, $expectedDto); $this->accountPresetService->expects(self::once())->method('addPresetPermissions')->with($id); $this->account->update($id, $accountUpdateDto); @@ -208,6 +304,10 @@ public function testUpdateUserCanChangePermissionsWithProfilePermission() $accountDataGenerator = AccountDataGenerator::factory(); $accountUpdateDto = $accountDataGenerator->buildAccountUpdateDto(); + // Nobody signed in here may mark an account private, so whatever the request carried, the + // account is written with both flags off. + $expectedDto = $accountUpdateDto->withPrivate(false)->withPrivateGroup(false); + $this->context->setUserData( UserDto::fromModel( UserDataGenerator::factory() @@ -228,10 +328,10 @@ public function testUpdateUserCanChangePermissionsWithProfilePermission() ->with($id) ->willReturn(new QueryResult([$accountDataGenerator->buildAccount()])); $this->accountRepository->expects(self::once())->method('update') - ->with($id, AccountModel::update($accountUpdateDto), false, false) + ->with($id, AccountModel::update($expectedDto), false, false) ->willReturn(new QueryResult(null, 1)); $this->accountItemsService->expects(self::once())->method('updateItems') - ->with(true, $id, $accountUpdateDto); + ->with(true, $id, $expectedDto); $this->accountPresetService->expects(self::once())->method('addPresetPermissions')->with($id); $this->account->update($id, $accountUpdateDto); @@ -1445,7 +1545,10 @@ public function testCreateCannotChangePermissions() ->with(ItemPresetInterface::ITEM_TYPE_ACCOUNT_PRIVATE) ->willReturn(null); - $encryptedDto = $accountCreateDto->withEncryptedPassword($encryptedPassword); + // Not allowed to mark it private, so both flags are written off whatever was asked. + $encryptedDto = $accountCreateDto->withEncryptedPassword($encryptedPassword) + ->withPrivate(false) + ->withPrivateGroup(false); $this->accountRepository->expects(self::once())->method('create') ->with(self::anAccountStampedNow(AccountModel::create($encryptedDto))) diff --git a/tests/Unit/Infrastructure/Adapter/In/Web/Forms/AccountFormTest.php b/tests/Unit/Infrastructure/Adapter/In/Web/Forms/AccountFormTest.php index 14a443756..9ea948bcb 100644 --- a/tests/Unit/Infrastructure/Adapter/In/Web/Forms/AccountFormTest.php +++ b/tests/Unit/Infrastructure/Adapter/In/Web/Forms/AccountFormTest.php @@ -8,7 +8,6 @@ use PHPUnit\Framework\MockObject\MockObject; use SP\Application\Account\Ports\AccountPresetService; use SP\Domain\Core\Acl\AclActionsInterface; -use SP\Domain\User\Models\ProfileData; use SP\Domain\Core\Exceptions\ValidationException; use SP\Domain\Http\Ports\RequestService; use SP\Infrastructure\Adapter\In\Web\Forms\AccountForm; @@ -47,10 +46,8 @@ public function testValidateForUnhandledActionThrowsValidationException(): void */ public function testPrivateFlagsReachTheDto(): void { - // The form now holds both flags to the profile permission and to ownership, the way the - // interface does when it decides whether to offer the checkbox — so the user this runs as - // has to be one the flags are actually available to. - $this->context->setUserProfile(new ProfileData(['accPrivate' => true, 'accPrivateGroup' => true])); + // Whether the flags may be set is the account service's to decide — it is where the API + // arrives too — so the form only has to carry what was submitted. $userData = $this->context->getUserData(); @@ -60,7 +57,7 @@ public function testPrivateFlagsReachTheDto(): void static fn(string $param) => $param === 'name' ? 'an_account' : null ); // owner_id and main_usergroup_id carry the signed-in user: the real analyzeInt() falls back - // to that default, and the privacy flags are only available to somebody marking their own. + // to that default. $request->method('analyzeInt')->willReturnCallback( static fn(string $param) => match ($param) { 'client_id', 'category_id' => 1, @@ -87,45 +84,6 @@ public function testPrivateFlagsReachTheDto(): void self::assertTrue($accountDto->isPrivateGroup); } - /** - * The interface only offers the privacy checkboxes to a user whose profile carries the - * permission — `AccountHelper` computes `allowPrivate` / `allowPrivateGroup` for exactly that — - * but nothing looked at either flag on the way back in, so a request carrying `private_enabled` - * anyway was honoured. - * - * Neither flag reaches anything: both only ever withhold. What they do is hide, and `AccountAcl` - * tests privacy before the administrator branch, so an account marked private disappears for - * account administrators as well. - */ - public function testPrivateFlagsAreRefusedWithoutThePermission(): void - { - $this->context->setUserProfile(new ProfileData()); - - $request = $this->createMock(RequestService::class); - - $request->method('analyzeString')->willReturnCallback( - static fn(string $param) => $param === 'name' ? 'an_account' : null - ); - $request->method('analyzeInt')->willReturnCallback( - static fn(string $param) => in_array($param, ['client_id', 'category_id'], true) ? 1 : null - ); - $request->method('analyzeEncrypted')->willReturn('a_password'); - $request->method('analyzeBool')->willReturnCallback( - static fn(string $param) => in_array($param, ['private_enabled', 'private_group_enabled'], true) - ); - - $accountPresetService = $this->createMock(AccountPresetService::class); - $accountPresetService->method('checkPasswordPreset')->willReturnArgument(0); - $accountPresetService->method('checkPasswordExpiry')->willReturnArgument(0); - - $form = new AccountForm($this->application, $request, $accountPresetService); - - $accountDto = $form->validateFor(AclActionsInterface::ACCOUNT_CREATE)->getItemData(); - - self::assertFalse($accountDto->isPrivate); - self::assertFalse($accountDto->isPrivateGroup); - } - /** * An account with no name would be unidentifiable in the listing/search; checkCommon() * must refuse it rather than let a nameless row reach the database.