From f40e07b36100d849b3ee20df396d8357191879e5 Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 17 Sep 2026 23:52:15 +0200 Subject: [PATCH] fix: restoring history cannot change an account's privacy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AccountUseCases::restoreModified() copies isPrivate and isPrivateGroup straight from the history snapshot, and the repository's restoreModified() excludes userId/userGroupId from the UPDATE but not those two, so a restore wrote them back unconditionally. On the edit screen AccountForm::constrainPrivacyToPermission() allows them only for an application administrator, or the owner holding isAccPrivate() (the group holding isAccPrivateGroup()). A history DTO never passes through AccountForm, so nothing re-applied that rule coming back in — and SaveEditRestoreController checks only ACCOUNT_EDIT_RESTORE, which AccountPermission buckets with plain edit access. So anybody the account was shared with for editing could restore an old version to mark it 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. This method already refuses to take the owner and the group from the snapshot, for the same reason. Privacy is the same question about a different pair of columns, and the decision now sits beside those two. --- src/Application/Account/Services/Account.php | 45 +++++++++++++++ .../Account/Services/AccountTest.php | 56 ++++++++++++++++++- 2 files changed, 99 insertions(+), 2 deletions(-) diff --git a/src/Application/Account/Services/Account.php b/src/Application/Account/Services/Account.php index bb8b0ea86..9defbbb25 100644 --- a/src/Application/Account/Services/Account.php +++ b/src/Application/Account/Services/Account.php @@ -294,6 +294,39 @@ public function getById(int $id): AccountModel * * @return bool */ + /** + * The privacy flags a restore may write, decided the way the edit screen decides them. + * + * 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. + * + * @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); + + return $dto->mutate( + [ + 'isPrivate' => $mayBePrivate ? $dto->isPrivate : 0, + 'isPrivateGroup' => $mayBePrivateGroup ? $dto->isPrivateGroup : 0, + ] + ); + } + protected function userCanChangeOwner( UserDto $userData, ProfileData $userProfile, @@ -595,6 +628,18 @@ function () use ($dto) { $changeUserGroup = $this->userCanChangeGroup($userData, $userProfile, $account); } + // 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 + // 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); + $this->addHistory($dto->accountId); $result = $this->accountRepository->restoreModified( diff --git a/tests/Unit/Application/Account/Services/AccountTest.php b/tests/Unit/Application/Account/Services/AccountTest.php index dc50ebb84..22773a81b 100644 --- a/tests/Unit/Application/Account/Services/AccountTest.php +++ b/tests/Unit/Application/Account/Services/AccountTest.php @@ -1226,11 +1226,15 @@ public function testRestoreModified() $this->accountHistoryService->expects(self::once())->method('create') ->with($accountHistoryCreateDto); + // The privacy flags a restore may write are decided the way the edit screen decides them, + // not taken from the snapshot — the harness user is an ordinary one, so both are refused. + $restored = $accountHistoryDto->mutate(['isPrivate' => 0, 'isPrivateGroup' => 0]); + $this->accountRepository->expects(self::once())->method('restoreModified') ->with( $accountHistoryDto->accountId, AccountModel::restoreModified( - $accountHistoryDto, + $restored, $this->context->getUserData()->id ) ) @@ -1239,6 +1243,51 @@ public function testRestoreModified() $this->account->restoreModified($accountHistoryDto); } + /** + * A restore cannot mark an account private for somebody who could not mark it private. + * + * `AccountForm::constrainPrivacyToPermission()` gates both flags on the edit screen — only an + * application administrator, or the owner holding `isAccPrivate()` — and nothing re-applied it + * on the way back from a history row. `SaveEditRestoreController` checks only + * `ACCOUNT_EDIT_RESTORE`, which `AccountPermission` buckets with plain edit access, so anybody + * the account was shared with for editing could restore an old version to hide it. `AccountAcl` + * tests privacy *before* the administrator branch, so a private account disappears for account + * administrators too. + * + * The same shape as the owner and group flags one field over, which this method already + * refuses to take from the snapshot. + * + * @throws Exception + */ + public function testRestoreModifiedCannotMakeAnAccountPrivate() + { + $this->configService->method('getByParam')->willReturn(self::$faker->password()); + + $accountDataGenerator = AccountDataGenerator::factory(); + + $this->accountRepository->method('getById') + ->willReturn(new QueryResult([$accountDataGenerator->buildAccount()])); + + $accountHistoryDto = $accountDataGenerator->buildAccountHistoryDto() + ->mutate(['isPrivate' => 1, 'isPrivateGroup' => 1]); + + $this->accountRepository + ->expects(self::once()) + ->method('restoreModified') + ->with( + self::anything(), + self::callback( + static fn(AccountModel $account): bool => (int)$account->getIsPrivate() === 0 + && (int)$account->getIsPrivateGroup() === 0 + ), + self::anything(), + self::anything() + ) + ->willReturn(new QueryResult(null, 1)); + + $this->account->restoreModified($accountHistoryDto); + } + /** * @throws ServiceException */ @@ -1263,11 +1312,14 @@ public function testRestoreModifiedError() ->with($accountHistoryCreateDto); + // As above: the privacy flags are decided rather than taken from the snapshot. + $restored = $accountHistoryDto->mutate(['isPrivate' => 0, 'isPrivateGroup' => 0]); + $this->accountRepository->expects(self::once())->method('restoreModified') ->with( $accountHistoryDto->accountId, AccountModel::restoreModified( - $accountHistoryDto, + $restored, $this->context->getUserData()->id ) )