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. *