From 71d0122a367e3fccb6a0671624c108ec269a3474 Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 3 Sep 2026 00:50:07 +0200 Subject: [PATCH] fix: an account edit that matched no row is not saved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Account::update() and Account::editPassword() called the repository and threw the result away. The other four writes in the same file — restoreModified(), updatePasswordMasterPass(), delete() and deleteByIdBatch() — all read getAffectedNumRows() and refuse when it is zero. So an edit that matched no row came back as saved, and for a password change the user is told their new password is stored when nothing was written. How narrow it is: addHistory() runs first and calls getById(), which throws for an account that is already gone, so the ordinary case was caught. What was not is the window between that read and the write — both inside the same transaction, and the UPDATE takes the latest committed row, so a delete landing in between matches nothing here. It is defence in depth more than a live hole, and it is the check four siblings in the same file already make. The connection sets ATTR_FOUND_ROWS, so affected means matched, not changed: a save that alters no field still returns 1 and is not mistaken for a missing row. The eleven existing tests over these paths now stub a matched row, where they relied on PHPUnit's default QueryResult, which reports zero affected. --- src/Application/Account/Services/Account.php | 25 ++++- .../Account/Services/AccountTest.php | 103 ++++++++++++++++-- 2 files changed, 115 insertions(+), 13 deletions(-) diff --git a/src/Application/Account/Services/Account.php b/src/Application/Account/Services/Account.php index b2cdd7247..f19b0768b 100644 --- a/src/Application/Account/Services/Account.php +++ b/src/Application/Account/Services/Account.php @@ -449,13 +449,28 @@ function () use ($id, $accountUpdateDto) { $accountUpdateDto = $accountUpdateDto->mutate(['userEditId' => $userData->id]); } - $this->accountRepository->update( + // An edit whose WHERE matched nothing has saved nothing, and answering the caller + // with success reports the edit of an account another session has since deleted as + // done. `restoreModified()`, `updatePasswordMasterPass()`, `delete()` and + // `deleteByIdBatch()` all make this check; `update()` and `editPassword()` were the + // two writes in this file that threw the count away. + // + // `addHistory()` above reads the account and throws for one that is already gone, + // so this is the window between that read and this write — both inside the same + // transaction, and the update takes the latest committed row, so a delete that + // landed in between matches nothing here. The connection sets FOUND_ROWS, so a save + // that alters no field still matches its row and is not mistaken for a missing one. + $result = $this->accountRepository->update( $id, AccountModel::update($accountUpdateDto), $changeOwner, $changeUserGroup ); + if ($result->getAffectedNumRows() === 0) { + throw new NoSuchItemException(__u('Account not found')); + } + $this->accountItemsService->updateItems($userCanChangePermissions, $id, $accountUpdateDto); $this->accountPresetService->addPresetPermissions($id); @@ -482,10 +497,16 @@ function () use ($id, $accountUpdateDto) { $encryptedPassword = $this->accountCryptService->getPasswordEncrypted($accountUpdateDto->pass); - $this->accountRepository->editPassword( + // As in update() above: without this, changing the password of an account that has + // just been deleted comes back as saved. + $result = $this->accountRepository->editPassword( $id, AccountModel::updatePassword($accountUpdateDto->withEncryptedPassword($encryptedPassword)) ); + + if ($result->getAffectedNumRows() === 0) { + throw new NoSuchItemException(__u('Account not found')); + } }, $this ); diff --git a/tests/Unit/Application/Account/Services/AccountTest.php b/tests/Unit/Application/Account/Services/AccountTest.php index ee69114eb..dc50ebb84 100644 --- a/tests/Unit/Application/Account/Services/AccountTest.php +++ b/tests/Unit/Application/Account/Services/AccountTest.php @@ -111,7 +111,8 @@ public function testUpdate() ->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($accountUpdateDto), true, true) + ->willReturn(new QueryResult(null, 1)); $this->accountItemsService->expects(self::once())->method('updateItems') ->with(true, $id, $accountUpdateDto); $this->accountPresetService->expects(self::once())->method('addPresetPermissions')->with($id); @@ -149,7 +150,8 @@ public function testUpdateUserCannotChangePermissionsWithoutPermission() ->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($accountUpdateDto), false, false) + ->willReturn(new QueryResult(null, 1)); $this->accountItemsService->expects(self::once())->method('updateItems') ->with(false, $id, $accountUpdateDto); $this->accountPresetService->expects(self::once())->method('addPresetPermissions')->with($id); @@ -187,7 +189,8 @@ 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($accountUpdateDto), true, true) + ->willReturn(new QueryResult(null, 1)); $this->accountItemsService->expects(self::once())->method('updateItems') ->with(true, $id, $accountUpdateDto); $this->accountPresetService->expects(self::once())->method('addPresetPermissions')->with($id); @@ -225,7 +228,8 @@ 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($accountUpdateDto), false, false) + ->willReturn(new QueryResult(null, 1)); $this->accountItemsService->expects(self::once())->method('updateItems') ->with(true, $id, $accountUpdateDto); $this->accountPresetService->expects(self::once())->method('addPresetPermissions')->with($id); @@ -296,7 +300,8 @@ public function testUpdateWithPresetPrivateForUser() }), true, true - ); + ) + ->willReturn(new QueryResult(null, 1)); $this->accountItemsService->expects(self::once())->method('updateItems') ->with( true, @@ -374,7 +379,8 @@ public function testUpdateWithPresetPrivateForGroup() }), true, true - ); + ) + ->willReturn(new QueryResult(null, 1)); $this->accountItemsService->expects(self::once())->method('updateItems') ->with( true, @@ -430,7 +436,8 @@ public function testUpdateWithPresetPrivateWithNullData() ->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($accountUpdateDto), true, true) + ->willReturn(new QueryResult(null, 1)); $this->accountItemsService->expects(self::once())->method('updateItems') ->with(true, $id, $accountUpdateDto); $this->accountPresetService->expects(self::once())->method('addPresetPermissions')->with($id); @@ -476,7 +483,8 @@ public function testUpdateDefaultsOmittedOwnerAndGroupToTheAccountsCurrentOnes() ); $this->accountRepository->expects(self::once())->method('update') - ->with($id, AccountModel::update($expectedDto), true, true); + ->with($id, AccountModel::update($expectedDto), true, true) + ->willReturn(new QueryResult(null, 1)); $this->accountItemsService->expects(self::once())->method('updateItems') ->with(true, $id, $expectedDto); $this->accountPresetService->expects(self::once())->method('addPresetPermissions')->with($id); @@ -517,7 +525,8 @@ public function testUpdateDefaultsOmittedUserEditIdToTheCurrentSessionUser() $expectedDto = $accountUpdateDto->mutate(['userEditId' => $userData->id]); $this->accountRepository->expects(self::once())->method('update') - ->with($id, AccountModel::update($expectedDto), true, true); + ->with($id, AccountModel::update($expectedDto), true, true) + ->willReturn(new QueryResult(null, 1)); $this->accountItemsService->expects(self::once())->method('updateItems') ->with(true, $id, $expectedDto); $this->accountPresetService->expects(self::once())->method('addPresetPermissions')->with($id); @@ -1144,7 +1153,8 @@ public function testEditPassword() ); $this->accountRepository->expects(self::once())->method('editPassword') - ->with($id, self::anAccountStampedNow(AccountModel::updatePassword($accountUpdateDto))); + ->with($id, self::anAccountStampedNow(AccountModel::updatePassword($accountUpdateDto))) + ->willReturn(new QueryResult(null, 1)); $this->account->editPassword($id, $accountUpdateDto); } @@ -1186,7 +1196,8 @@ public function testEditPasswordDefaultsOmittedUserEditIdToTheCurrentSessionUser ); $this->accountRepository->expects(self::once())->method('editPassword') - ->with($id, self::anAccountStampedNow(AccountModel::updatePassword($expectedDto))); + ->with($id, self::anAccountStampedNow(AccountModel::updatePassword($expectedDto))) + ->willReturn(new QueryResult(null, 1)); $this->account->editPassword($id, $accountUpdateDto); } @@ -1746,6 +1757,76 @@ public function testIncrementDecryptCounterNoRows() $this->assertFalse($this->account->incrementDecryptCounter($id)); } + /** + * An edit whose WHERE matched no row has saved nothing, and must not come back as saved. + * + * `restoreModified()`, `updatePasswordMasterPass()`, `delete()` and `deleteByIdBatch()` all + * check what they affected; `update()` and `editPassword()` were the two writes in this service + * that threw the count away, so editing an account another session had just deleted answered + * with success. `addHistory()` reads the account first and catches the ordinary case, which is + * why this needs the repository to report the miss directly. + * + * @throws Exception + */ + public function testUpdateThatMatchedNoRowIsNotReportedAsSaved(): void + { + $id = self::$faker->numberBetween(1, 1000); + $accountUpdateDto = AccountDataGenerator::factory()->buildAccountUpdateDto(); + + $this->accountRepository + ->method('getById') + ->willReturn(new QueryResult([AccountDataGenerator::factory()->buildAccount()])); + + // addHistory() writes the row it is about to replace, and needs the master-pass hash. + $this->configService->method('getByParam')->willReturn(self::$faker->sha1()); + + $this->accountRepository + ->expects(self::once()) + ->method('update') + ->willReturn(new QueryResult(null, 0)); + + // Nothing downstream of the write may run for an account that is not there. + $this->accountItemsService->expects(self::never())->method('updateItems'); + $this->accountPresetService->expects(self::never())->method('addPresetPermissions'); + + $this->expectException(NoSuchItemException::class); + $this->expectExceptionMessage('Account not found'); + + $this->account->update($id, $accountUpdateDto); + } + + /** + * The same for a password change, which is the other write that discarded the count. + * + * @throws Exception + */ + public function testEditPasswordThatMatchedNoRowIsNotReportedAsSaved(): void + { + $id = self::$faker->numberBetween(1, 1000); + $accountUpdateDto = AccountDataGenerator::factory()->buildAccountUpdateDto(); + + $this->accountRepository + ->method('getById') + ->willReturn(new QueryResult([AccountDataGenerator::factory()->buildAccount()])); + + // addHistory() writes the row it is about to replace, and needs the master-pass hash. + $this->configService->method('getByParam')->willReturn(self::$faker->sha1()); + + $this->accountCryptService + ->method('getPasswordEncrypted') + ->willReturn(new EncryptedPassword(self::$faker->password(), self::$faker->password())); + + $this->accountRepository + ->expects(self::once()) + ->method('editPassword') + ->willReturn(new QueryResult(null, 0)); + + $this->expectException(NoSuchItemException::class); + $this->expectExceptionMessage('Account not found'); + + $this->account->editPassword($id, $accountUpdateDto); + } + /** * The account the service is expected to write, give or take the second it was stamped in. *