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 ) )