diff --git a/src/Application/Account/Services/Account.php b/src/Application/Account/Services/Account.php index af9c43bee..b2cdd7247 100644 --- a/src/Application/Account/Services/Account.php +++ b/src/Application/Account/Services/Account.php @@ -520,11 +520,36 @@ public function restoreModified(AccountHistoryDto $dto): void { $this->accountRepository->transactionAware( function () use ($dto) { + $userData = $this->context->getUserData(); + $userProfile = $this->context->getUserProfile() ?? new ProfileData(); + + // Who owns the account, and which group it belongs to, are decided by the same two + // checks an ordinary edit makes — not by what the historical row happened to say. + // + // A restore writes every column of the snapshot back, and these two were among + // them, so restoring an old version silently reverted the owner and the group. + // Those are what AccountAcl compares the signed-in user against, so it changes who + // can see the account; and the door needs only ACCOUNT_EDIT_RESTORE, which + // AccountPermission buckets with plain edit — so anybody the account is shared + // with for editing could hand it back to a previous owner, which is exactly what + // userCanChangeOwner() exists to refuse them on the edit screen. + $changeOwner = false; + $changeUserGroup = false; + + if (AccountAcl::getShowPermission($userData, $userProfile)) { + $account = $this->getById($dto->accountId); + + $changeOwner = $this->userCanChangeOwner($userData, $userProfile, $account); + $changeUserGroup = $this->userCanChangeGroup($userData, $userProfile, $account); + } + $this->addHistory($dto->accountId); $result = $this->accountRepository->restoreModified( $dto->accountId, - AccountModel::restoreModified($dto, $this->context->getUserData()->id ?? 0) + AccountModel::restoreModified($dto, $userData->id ?? 0), + $changeOwner, + $changeUserGroup ); if ($result->getAffectedNumRows() === 0) { diff --git a/src/Domain/Account/Ports/AccountRepository.php b/src/Domain/Account/Ports/AccountRepository.php index 453dc1486..ea25d8b3c 100644 --- a/src/Domain/Account/Ports/AccountRepository.php +++ b/src/Domain/Account/Ports/AccountRepository.php @@ -117,7 +117,12 @@ public function updatePassword(int $accountId, EncryptedPassword $encryptedPassw * @throws ConstraintException * @throws QueryException */ - public function restoreModified(int $accountId, Account $account): QueryResult; + public function restoreModified( + int $accountId, + Account $account, + bool $changeOwner, + bool $changeUserGroup + ): QueryResult; /** * Updates an item for bulk action diff --git a/src/Infrastructure/Adapter/Out/Account/Repositories/Account.php b/src/Infrastructure/Adapter/Out/Account/Repositories/Account.php index 6492d0ad1..ac4ce852e 100644 --- a/src/Infrastructure/Adapter/Out/Account/Repositories/Account.php +++ b/src/Infrastructure/Adapter/Out/Account/Repositories/Account.php @@ -263,8 +263,12 @@ public function updatePassword(int $accountId, EncryptedPassword $encryptedPassw * @throws ConstraintException * @throws QueryException */ - public function restoreModified(int $accountId, AccountModel $account): QueryResult - { + public function restoreModified( + int $accountId, + AccountModel $account, + bool $changeOwner, + bool $changeUserGroup + ): QueryResult { $query = $this->queryFactory ->newUpdate() ->table(AccountModel::TABLE) @@ -277,6 +281,11 @@ public function restoreModified(int $accountId, AccountModel $account): QueryRes 'countDecrypt', 'countView', 'dateEdit', + // Excluded here for the same reason update() excludes them: who owns an + // account and which group it belongs to is not an ordinary field. They are + // written back only when the caller is allowed to change them. + 'userGroupId', + 'userId', 'id', ] ) @@ -285,6 +294,14 @@ public function restoreModified(int $accountId, AccountModel $account): QueryRes ->where('id = :id') ->bindValues(['id' => $accountId]); + if ($changeUserGroup) { + $query->col('userGroupId', $account->getUserGroupId()); + } + + if ($changeOwner) { + $query->col('userId', $account->getUserId()); + } + $queryData = QueryData::build($query)->setOnErrorMessage(__u('Error on restoring the account')); return $this->db->runQuery($queryData); diff --git a/tests/Unit/Infrastructure/Adapter/Out/Account/Repositories/AccountTest.php b/tests/Unit/Infrastructure/Adapter/Out/Account/Repositories/AccountTest.php index 2ff3e2502..25a6e5998 100644 --- a/tests/Unit/Infrastructure/Adapter/Out/Account/Repositories/AccountTest.php +++ b/tests/Unit/Infrastructure/Adapter/Out/Account/Repositories/AccountTest.php @@ -257,6 +257,38 @@ static function (QueryData $arg) use ($id, $encryptedPassword) { $this->account->updatePassword($id, $encryptedPassword); } + /** + * A restore by somebody who may not change the owner does not change the owner. + * + * The snapshot carries every column, `userId` and `userGroupId` among them, and the restore + * wrote all of it back — so restoring an old version silently reverted who owns the account + * and which group it belongs to. Those are exactly what AccountAcl compares the signed-in user + * against, so it changes who can see it. And the door needs only ACCOUNT_EDIT_RESTORE, which + * AccountPermission buckets with plain edit, so anybody the account is shared with for editing + * could do it — the thing userCanChangeOwner() refuses them on the edit screen. + * + * @throws QueryException + * @throws ConstraintException + */ + public function testEditRestoreLeavesTheOwnerAloneWithoutThePermission(): void + { + $account = AccountDataGenerator::factory()->buildAccount(); + + $callback = new Callback( + static function (QueryData $arg) { + $params = $arg->getQuery()->getBindValues(); + + return !array_key_exists('userId', $params) + && !array_key_exists('userGroupId', $params) + && !empty($arg->getQuery()->getStatement()); + } + ); + + $this->database->expects(self::once())->method('runQuery')->with($callback); + + $this->account->restoreModified($account->getId(), $account, false, false); + } + /** * @throws QueryException * @throws ConstraintException @@ -294,7 +326,7 @@ static function (QueryData $arg) use ($account) { $this->database->expects(self::once())->method('runQuery')->with($callback); - $this->account->restoreModified($account->getId(), $account); + $this->account->restoreModified($account->getId(), $account, true, true); } /**