Skip to content

fix: an account edit that matched no row is not saved #900

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
blaipr merged 1 commit into from
Sep 2, 2026
Merged

fix: an account edit that matched no row is not saved #900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions src/Application/Account/Services/Account.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
);
Expand Down
103 changes: 92 additions & 11 deletions tests/Unit/Application/Account/Services/AccountTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -296,7 +300,8 @@ public function testUpdateWithPresetPrivateForUser()
}),
true,
true
);
)
->willReturn(new QueryResult(null, 1));
$this->accountItemsService->expects(self::once())->method('updateItems')
->with(
true,
Expand Down Expand Up @@ -374,7 +379,8 @@ public function testUpdateWithPresetPrivateForGroup()
}),
true,
true
);
)
->willReturn(new QueryResult(null, 1));
$this->accountItemsService->expects(self::once())->method('updateItems')
->with(
true,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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.
*
Expand Down