0

[password manager] Wire editing passkeys

Update SavedPasswordsPresenter::EditSavedCredentials to support editing
passkeys. Surface this through a new `changeCredential` method on the
passwordsPrivate extension API. This generic method can update all of
password, federated, blocked, and passkey credentials.

A follow-up will remove the old `changeSavedPassword` passwordsPrivate
method which is now redundant.

Bug: 1432717
Change-Id: I6bbd4be42fbe9d944ae403c67145c079ca429d91
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4585682
Commit-Queue: Nina Satragno <nsatragno@chromium.org>
Auto-Submit: Nina Satragno <nsatragno@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1156555}
This commit is contained in:
Nina Satragno
2023-06-12 23:39:12 +00:00
committed by Chromium LUCI CQ
parent 069ffb1c6f
commit dd52c941d5
15 changed files with 387 additions and 3 deletions

@ -72,6 +72,26 @@ ResponseAction PasswordsPrivateChangeSavedPasswordFunction::Run() {
"id."));
}
// PasswordsPrivateChangeCredentialFunction
ResponseAction PasswordsPrivateChangeCredentialFunction::Run() {
if (!GetDelegate(browser_context())) {
return RespondNow(Error(kNoDelegateError));
}
auto parameters =
api::passwords_private::ChangeCredential::Params::Create(args());
EXTENSION_FUNCTION_VALIDATE(parameters);
bool success =
GetDelegate(browser_context())->ChangeCredential(parameters->credential);
if (success) {
return RespondNow(NoArguments());
}
return RespondNow(Error(
"Could not change the credential. Either the arguments are not valid or "
"the credential does not exist"));
}
// PasswordsPrivateRemoveCredentialFunction
ResponseAction PasswordsPrivateRemoveCredentialFunction::Run() {
if (!GetDelegate(browser_context())) {

@ -41,6 +41,18 @@ class PasswordsPrivateChangeSavedPasswordFunction : public ExtensionFunction {
ResponseAction Run() override;
};
class PasswordsPrivateChangeCredentialFunction : public ExtensionFunction {
public:
DECLARE_EXTENSION_FUNCTION("passwordsPrivate.changeCredential",
PASSWORDSPRIVATE_CHANGECREDENTIAL)
protected:
~PasswordsPrivateChangeCredentialFunction() override = default;
// ExtensionFunction overrides.
ResponseAction Run() override;
};
class PasswordsPrivateRemoveCredentialFunction : public ExtensionFunction {
public:
DECLARE_EXTENSION_FUNCTION("passwordsPrivate.removeCredential",

@ -209,6 +209,20 @@ IN_PROC_BROWSER_TEST_F(PasswordsPrivateApiTest,
<< message_;
}
IN_PROC_BROWSER_TEST_F(PasswordsPrivateApiTest,
ChangeCredentialChangePassword) {
EXPECT_TRUE(RunPasswordsSubtest("changeCredentialChangePassword"))
<< message_;
}
IN_PROC_BROWSER_TEST_F(PasswordsPrivateApiTest, ChangeCredentialChangePasskey) {
EXPECT_TRUE(RunPasswordsSubtest("changeCredentialChangePasskey")) << message_;
}
IN_PROC_BROWSER_TEST_F(PasswordsPrivateApiTest, ChangeCredentialNotFound) {
EXPECT_TRUE(RunPasswordsSubtest("changeCredentialNotFound")) << message_;
}
IN_PROC_BROWSER_TEST_F(PasswordsPrivateApiTest,
RemoveAndUndoRemoveSavedPassword) {
EXPECT_TRUE(RunPasswordsSubtest("removeAndUndoRemoveSavedPassword"))

@ -96,6 +96,15 @@ class PasswordsPrivateDelegate
int id,
const api::passwords_private::ChangeSavedPasswordParams& params) = 0;
// Updates a credential. Not all attributes can be updated.
// |credential|: The credential to be updated. Matched to an existing
// credential by id.
// Returns absl::nullopt if the credential could not be found or updated.
// Otherwise, returns the newly updated credential. Note that the new
// credential may have a different ID, so it should replace the old one.
virtual bool ChangeCredential(
const api::passwords_private::PasswordUiEntry& credential) = 0;
// Removes the credential entry corresponding to the |id| in the specified
// |from_stores|. Any invalid id will be ignored.
virtual void RemoveCredential(

@ -446,6 +446,38 @@ absl::optional<int> PasswordsPrivateDelegateImpl::ChangeSavedPassword(
return credential_id_generator_.GenerateId(std::move(updated_credential));
}
bool PasswordsPrivateDelegateImpl::ChangeCredential(
const api::passwords_private::PasswordUiEntry& credential) {
const CredentialUIEntry* original_credential =
credential_id_generator_.TryGetKey(credential.id);
if (!original_credential) {
return false;
}
CredentialUIEntry updated_credential = *original_credential;
updated_credential.username = base::UTF8ToUTF16(credential.username);
if (credential.password) {
updated_credential.password = base::UTF8ToUTF16(*credential.password);
}
if (credential.note) {
updated_credential.note = base::UTF8ToUTF16(*credential.note);
}
if (credential.display_name) {
CHECK(!updated_credential.passkey_credential_id.empty());
updated_credential.user_display_name =
base::UTF8ToUTF16(*credential.display_name);
}
switch (saved_passwords_presenter_.EditSavedCredentials(*original_credential,
updated_credential)) {
case password_manager::SavedPasswordsPresenter::EditResult::kSuccess:
case password_manager::SavedPasswordsPresenter::EditResult::kNothingChanged:
return true;
case password_manager::SavedPasswordsPresenter::EditResult::kNotFound:
case password_manager::SavedPasswordsPresenter::EditResult::kAlreadyExisits:
case password_manager::SavedPasswordsPresenter::EditResult::kEmptyPassword:
return false;
}
}
void PasswordsPrivateDelegateImpl::RemoveCredential(
int id,
api::passwords_private::PasswordStoreSet from_stores) {

@ -74,6 +74,8 @@ class PasswordsPrivateDelegateImpl
absl::optional<int> ChangeSavedPassword(
int id,
const api::passwords_private::ChangeSavedPasswordParams& params) override;
bool ChangeCredential(
const api::passwords_private::PasswordUiEntry& credential) override;
void RemoveCredential(
int id,
api::passwords_private::PasswordStoreSet from_stores) override;

@ -351,6 +351,10 @@ class PasswordsPrivateDelegateImplTest : public WebAppTest {
return new PasswordsPrivateDelegateImpl(profile());
}
// Queries and returns the list of saved credentials, blocking until finished.
PasswordsPrivateDelegate::UiEntries GetCredentials(
PasswordsPrivateDelegate& delegate);
protected:
raw_ptr<extensions::TestEventRouter, DanglingUntriaged> event_router_ =
nullptr;
@ -412,6 +416,22 @@ void PasswordsPrivateDelegateImplTest::SetUpRouters() {
profile(), base::BindRepeating(&BuildPasswordsPrivateEventRouter));
}
PasswordsPrivateDelegate::UiEntries
PasswordsPrivateDelegateImplTest::GetCredentials(
PasswordsPrivateDelegate& delegate) {
PasswordsPrivateDelegate::UiEntries result;
base::RunLoop run_loop;
delegate.GetSavedPasswordsList(base::BindLambdaForTesting(
[&](const PasswordsPrivateDelegate::UiEntries& entries) {
for (const auto& entry : entries) {
result.emplace_back(entry.Clone());
}
run_loop.Quit();
}));
run_loop.Run();
return result;
}
TEST_F(PasswordsPrivateDelegateImplTest, GetSavedPasswordsList) {
auto delegate = CreateDelegate();
@ -852,6 +872,190 @@ TEST_F(PasswordsPrivateDelegateImplTest, ChangeSavedPasswordInAccountStore) {
EXPECT_THAT(result, new_account_form_id);
}
TEST_F(PasswordsPrivateDelegateImplTest, ChangeCredential_Password) {
password_manager::PasswordForm sample_form = CreateSampleForm();
SetUpPasswordStores({sample_form});
auto delegate = CreateDelegate();
// Spin the loop to allow PasswordStore tasks posted on the creation of
// |delegate| to be completed.
base::RunLoop().RunUntilIdle();
api::passwords_private::PasswordUiEntry updated_credential =
GetCredentials(*delegate).at(0).Clone();
updated_credential.password = "new_pass";
updated_credential.username = "new_user";
EXPECT_TRUE(delegate->ChangeCredential(updated_credential));
// Spin the loop to allow PasswordStore tasks posted when changing the
// password to be completed.
base::RunLoop().RunUntilIdle();
// Check that the changing the password got reflected in the passwords list.
// `note` field should not be filled when `GetSavedPasswordsList` is called.
const PasswordsPrivateDelegate::UiEntries& credentials =
GetCredentials(*delegate);
EXPECT_EQ(credentials.size(), 1u);
const api::passwords_private::PasswordUiEntry& refreshed_credential =
credentials.at(0);
EXPECT_EQ(refreshed_credential.username, "new_user");
EXPECT_EQ(refreshed_credential.note, absl::nullopt);
}
TEST_F(PasswordsPrivateDelegateImplTest,
ChangeCredential_PasswordInBothStores) {
password_manager::PasswordForm profile_form = CreateSampleForm();
password_manager::PasswordForm account_form = profile_form;
account_form.in_store = password_manager::PasswordForm::Store::kAccountStore;
SetUpPasswordStores({profile_form, account_form});
auto delegate = CreateDelegate();
// Spin the loop to allow PasswordStore tasks posted on the creation of
// |delegate| to be completed.
base::RunLoop().RunUntilIdle();
api::passwords_private::PasswordUiEntry updated_credential =
GetCredentials(*delegate).at(0).Clone();
updated_credential.password = "new_pass";
updated_credential.username = "new_user";
EXPECT_TRUE(delegate->ChangeCredential(updated_credential));
// Spin the loop to allow PasswordStore tasks posted when changing the
// password to be completed.
base::RunLoop().RunUntilIdle();
const PasswordsPrivateDelegate::UiEntries& credentials =
GetCredentials(*delegate);
EXPECT_EQ(credentials.size(), 1u);
const api::passwords_private::PasswordUiEntry& refreshed_credential =
credentials.at(0);
EXPECT_EQ(refreshed_credential.username, "new_user");
EXPECT_EQ(refreshed_credential.stored_in,
api::passwords_private::PasswordStoreSet::
PASSWORD_STORE_SET_DEVICE_AND_ACCOUNT);
}
TEST_F(PasswordsPrivateDelegateImplTest,
ChangeCredential_PasswordInAccountStore) {
password_manager::PasswordForm profile_form = CreateSampleForm();
profile_form.password_value = u"different_pass";
password_manager::PasswordForm account_form = CreateSampleForm();
account_form.in_store = password_manager::PasswordForm::Store::kAccountStore;
SetUpPasswordStores({profile_form, account_form});
auto delegate = CreateDelegate();
// Spin the loop to allow PasswordStore tasks posted on the creation of
// |delegate| to be completed.
base::RunLoop().RunUntilIdle();
// Get the account credential.
const PasswordsPrivateDelegate::UiEntries& credentials =
GetCredentials(*delegate);
EXPECT_EQ(credentials.size(), 2u);
const auto account_credential_it =
std::ranges::find_if(credentials, [](const auto& credential) {
return credential.stored_in ==
api::passwords_private::PasswordStoreSet::
PASSWORD_STORE_SET_ACCOUNT;
});
ASSERT_NE(account_credential_it, credentials.end());
api::passwords_private::PasswordUiEntry updated_credential =
account_credential_it->Clone();
updated_credential.password = "new_pass";
updated_credential.username = "new_user";
EXPECT_TRUE(delegate->ChangeCredential(updated_credential));
// Spin the loop to allow PasswordStore tasks posted when changing the
// password to be completed.
base::RunLoop().RunUntilIdle();
const PasswordsPrivateDelegate::UiEntries& updated_credentials =
GetCredentials(*delegate);
EXPECT_EQ(updated_credentials.size(), 2u);
const auto refreshed_credential_it =
std::ranges::find_if(updated_credentials, [](const auto& credential) {
return credential.stored_in ==
api::passwords_private::PasswordStoreSet::
PASSWORD_STORE_SET_ACCOUNT;
});
ASSERT_NE(account_credential_it, updated_credentials.end());
EXPECT_EQ(refreshed_credential_it->username, "new_user");
}
TEST_F(PasswordsPrivateDelegateImplTest, ChangeCredential_Passkey) {
base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitWithFeatures(
{password_manager::features::kPasswordsGrouping,
password_manager::features::kPasswordManagerPasskeys,
syncer::kSyncWebauthnCredentials},
/*disabled_features=*/{});
PasskeyModel* passkey_model = PasskeyModelFactory::GetForProfile(profile());
ASSERT_EQ(passkey_model, PasskeyModelFactory::GetForProfile(profile()));
ASSERT_TRUE(passkey_model);
sync_pb::WebauthnCredentialSpecifics passkey = CreatePasskey();
passkey_model->AddNewPasskeyForTesting(passkey);
auto delegate = CreateDelegate();
// Spin the loop to allow PasskeyModel tasks posted on the creation of
// |delegate| to be completed.
base::RunLoop().RunUntilIdle();
// Get the passkey credential.
const PasswordsPrivateDelegate::UiEntries& credentials =
GetCredentials(*delegate);
EXPECT_EQ(credentials.size(), 1u);
const api::passwords_private::PasswordUiEntry& existing_credential =
credentials.at(0);
EXPECT_TRUE(existing_credential.is_passkey);
api::passwords_private::PasswordUiEntry updated_credential =
existing_credential.Clone();
updated_credential.username = "new_user";
updated_credential.display_name = "new_display_name";
EXPECT_TRUE(delegate->ChangeCredential(updated_credential));
// Spin the loop to allow PasskeyModel tasks posted when changing the
// password to be completed.
base::RunLoop().RunUntilIdle();
const PasswordsPrivateDelegate::UiEntries& updated_credentials =
GetCredentials(*delegate);
EXPECT_EQ(updated_credentials.size(), 1u);
EXPECT_EQ(updated_credentials.at(0).username, "new_user");
EXPECT_EQ(updated_credentials.at(0).display_name, "new_display_name");
}
TEST_F(PasswordsPrivateDelegateImplTest, ChangeCredential_NotFound) {
SetUpPasswordStores({});
auto delegate = CreateDelegate();
// Spin the loop to allow PasswordStore tasks posted on the creation of
// |delegate| to be completed.
base::RunLoop().RunUntilIdle();
EXPECT_FALSE(
delegate->ChangeCredential(api::passwords_private::PasswordUiEntry()));
}
TEST_F(PasswordsPrivateDelegateImplTest, ChangeCredential_EmptyPassword) {
password_manager::PasswordForm sample_form = CreateSampleForm();
SetUpPasswordStores({sample_form});
auto delegate = CreateDelegate();
// Spin the loop to allow PasswordStore tasks posted on the creation of
// |delegate| to be completed.
base::RunLoop().RunUntilIdle();
api::passwords_private::PasswordUiEntry updated_credential =
GetCredentials(*delegate).at(0).Clone();
updated_credential.password = "";
updated_credential.username = "new_user";
EXPECT_FALSE(delegate->ChangeCredential(updated_credential));
}
// Checking callback result of RequestPlaintextPassword with reason Copy.
// By implementation for Copy, callback will receive empty string.
TEST_F(PasswordsPrivateDelegateImplTest, TestCopyPasswordCallbackResult) {

@ -118,6 +118,27 @@ absl::optional<int> TestPasswordsPrivateDelegate::ChangeSavedPassword(
return id;
}
bool TestPasswordsPrivateDelegate::ChangeCredential(
const api::passwords_private::PasswordUiEntry& credential) {
const auto existing = std::ranges::find_if(
current_entries_,
[&credential](const auto& entry) { return entry.id == credential.id; });
if (existing == current_entries_.end()) {
return false;
}
existing->username = credential.username;
if (credential.password) {
existing->password = credential.password;
}
if (credential.display_name) {
existing->display_name = credential.display_name;
}
if (credential.note) {
existing->note = credential.note;
}
return true;
}
void TestPasswordsPrivateDelegate::RemoveCredential(
int id,
api::passwords_private::PasswordStoreSet from_stores) {

@ -9,6 +9,7 @@
#include "base/strings/utf_string_conversions.h"
#include "chrome/browser/extensions/api/passwords_private/passwords_private_delegate.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/common/extensions/api/passwords_private.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
namespace extensions {
@ -44,6 +45,8 @@ class TestPasswordsPrivateDelegate : public PasswordsPrivateDelegate {
absl::optional<int> ChangeSavedPassword(
const int id,
const api::passwords_private::ChangeSavedPasswordParams& params) override;
bool ChangeCredential(
const api::passwords_private::PasswordUiEntry& credential) override;
void RemoveCredential(
int id,
api::passwords_private::PasswordStoreSet from_store) override;

@ -347,11 +347,21 @@ namespace passwordsPrivate {
// |id|: The id for the password entry being updated.
// |params|: The dictionary which holds the changed parameters.
// |callback|: The callback that gets invoked in the end.
// TODO(crbug.com/1420597): clean up after old password manager is removed.
[supportsPromises] static void changeSavedPassword(
long id,
ChangeSavedPasswordParams params,
optional ChangeSavedPasswordCallback callback);
// Changes the credential. Not all attributes can be updated.
// Optional attributes that are not set will be unchanged.
// Returns a promise that resolves if successful, and rejects otherwise.
// |credential|: The credential to update. This will be matched to the
// existing credential by id.
[supportsPromises] static void changeCredential(
PasswordUiEntry credential,
optional VoidCallback callback);
// Removes the credential corresponding to |id| in |fromStores|. If no
// credential for this pair exists, this function is a no-op.
// |id|: The id for the credential being removed.

@ -2,9 +2,9 @@
"key": "MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQC74Vbx3EbhPc/FOvn6+HxCjMSml0HdPMiuRjj5a3b+MnRML1iJ9OAgbKUYJ/u3s25/cGq8pNB0NbyupHGEqvqAE7TcNr1mdgs0PWxh2IOI1GKrxlzxpqzQuFmxq5WHKr5RrwZ4/Xq0t/+e8JkvhZdW0jarz/28Jom0gkM5lorsewIDAQAB",
"name": "passwordsPrivate API interface test",
"version": "0.1",
"manifest_version": 2,
"manifest_version": 3,
"description": "Test of chrome.passwordsPrivate interface",
"permissions": [
"passwordsPrivate"
]
}
}

@ -121,6 +121,59 @@ var availableTests = [
});
},
async function changeCredentialChangePassword() {
let groups = await chrome.passwordsPrivate.getCredentialGroups();
let credential = groups[0].entries[0];
chrome.test.assertFalse(credential.isPasskey);
credential.username = 'anya';
credential.password = 'secret';
credential.note = 'note';
await chrome.passwordsPrivate.changeCredential(credential);
groups = await chrome.passwordsPrivate.getCredentialGroups();
credential = groups[0].entries.find(entry => entry.username == 'anya');
chrome.test.assertTrue(!!credential);
chrome.test.assertEq(credential.note, 'note');
chrome.test.succeed();
},
async function changeCredentialChangePasskey() {
let groups = await chrome.passwordsPrivate.getCredentialGroups();
let credential = groups[0].entries.find(credential => credential.isPasskey);
credential.username = 'anya';
credential.displayName = 'Anya Forger';
await chrome.passwordsPrivate.changeCredential(credential);
groups = await chrome.passwordsPrivate.getCredentialGroups();
credential = groups[0].entries.find(entry => entry.username == 'anya');
chrome.test.assertTrue(!!credential);
chrome.test.assertEq(credential.displayName, 'Anya Forger');
chrome.test.succeed();
},
async function changeCredentialNotFound() {
const expected =
'Error: Could not change the credential. Either the arguments are ' +
'not valid or the credential does not exist';
await chrome.test.assertPromiseRejects(
chrome.passwordsPrivate.changeCredential({
id: 42,
urls: {
shown: 'example.com',
link: 'https://example.com',
signonRealm: 'https://example.com',
},
isAndroidCredential: false,
isPasskey: false,
username: 'alice',
storedIn: chrome.passwordsPrivate.PasswordStoreSet.DEVICE,
note: '',
}), expected);
chrome.test.succeed();
},
function removeAndUndoRemoveSavedPassword() {
var numCalls = 0;
var numSavedPasswords;
@ -677,7 +730,7 @@ var availableTests = [
// The last entry should be a passkey.
var passkey = group.entries[group.entries.length - 1];
chrome.test.assertTrue(passkey.isPasskey);
chrome.test.assertEq(passkey.displayName, "displayName");
chrome.test.assertEq(passkey.displayName, 'displayName');
// Ensure that all entry ids are unique.
chrome.test.assertEq(group.entries.length, idSet.size);

@ -1853,6 +1853,7 @@ enum HistogramValue {
AUTOTESTPRIVATE_REMOVEBRUSCHETTA = 1791,
AUTOFILLPRIVATE_AUTHENTICATEUSERTOEDITLOCALCARD = 1792,
AUTOTESTPRIVATE_ISFEATUREENABLED = 1793,
PASSWORDSPRIVATE_CHANGECREDENTIAL = 1794,
// Last entry: Add new entries above, then run:
// tools/metrics/histograms/update_extension_histograms.py
ENUM_BOUNDARY

@ -36418,6 +36418,7 @@ Called by update_extension_histograms.py.-->
<int value="1791" label="AUTOTESTPRIVATE_REMOVEBRUSCHETTA"/>
<int value="1792" label="AUTOFILLPRIVATE_AUTHENTICATEUSERTOEDITLOCALCARD"/>
<int value="1793" label="AUTOTESTPRIVATE_ISFEATUREENABLED"/>
<int value="1794" label="PASSWORDSPRIVATE_CHANGECREDENTIAL"/>
</enum>
<enum name="ExtensionIconState">

@ -172,6 +172,8 @@ declare global {
export function recordPasswordsPageAccessInSettings(): void;
export function changeSavedPassword(
id: number, params: ChangeSavedPasswordParams): Promise<number>;
export function changeCredential(credential: PasswordUiEntry):
Promise<void>;
export function removeCredential(
id: number, fromStores: PasswordStoreSet): void;
export function removePasswordException(id: number): void;