From c0294b7d75ae8e27f590848e9f5dd9c9f4cf6a53 Mon Sep 17 00:00:00 2001 From: blaipr Date: Wed, 26 Aug 2026 22:38:10 +0200 Subject: [PATCH] fix: a restore cannot hand an account to somebody else MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `update()` excludes `userId` and `userGroupId` from the columns it writes and puts them back only when `userCanChangeOwner()` and `userCanChangeGroup()` say the caller may change them — an application admin, an accounts admin, or the owner holding `accPermission`. `restoreModified()` excluded neither, so it wrote every column of the historical snapshot back, those two among them. Restoring an old version therefore reverted who owns the account and which group it belongs to. Those are exactly the columns `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 `ACCOUNT_EDIT`. So anybody the account was merely shared with for editing could hand it back to a previous owner and group, which is the thing `userCanChangeOwner()` exists to refuse them two screens away. The restore asks the same two questions the edit asks, and writes those columns only if the answer is yes. Everything else in the snapshot is restored exactly as before. Checked by putting the two columns back in the exclusion list: the new test fails, asserting on the bound values of the emitted statement rather than on a row, because what matters is that the UPDATE does not carry them at all. --- src/Application/Account/Services/Account.php | 27 ++++++++++++++- .../Account/Ports/AccountRepository.php | 7 +++- .../Out/Account/Repositories/Account.php | 21 ++++++++++-- .../Out/Account/Repositories/AccountTest.php | 34 ++++++++++++++++++- 4 files changed, 84 insertions(+), 5 deletions(-) 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); } /**