From 16a9dddf8fd89bfe2767c9892bc7541e4b533b4a Mon Sep 17 00:00:00 2001 From: Nina Satragno Date: Mon, 23 Aug 2021 19:20:31 +0000 Subject: [PATCH] [autofill] Wait for username on webauthn forms Have the password autofill wait for username if a form field is annotated as `autofill="webauthn"` and the WebAuthenticationConditionalUI flag is enabled. This is the first part of an integration between WebAuthn Conditional UI and password autofill. PR spec: https://github.com/w3c/webauthn/pull/1576 Design doc: https://docs.google.com/document/d/1KzEWP0aoLMZ0asfw6d3-7UHJ6csTtxLA478EgptCvkk Bug: 1171985 Change-Id: I4c9879e36fc3923555731da8f7bf4a957e080e71 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3107526 Commit-Queue: Nina Satragno Reviewed-by: Vasilii Sukhanov Cr-Commit-Position: refs/heads/main@{#914458} --- .../chrome_password_manager_client.cc | 6 +++++ .../chrome_password_manager_client.h | 1 + .../core/browser/form_parsing/form_parser.cc | 25 ++++++++++++++--- .../core/browser/form_parsing/form_parser.h | 1 + .../form_parsing/form_parser_unittest.cc | 27 +++++++++++++++++++ .../core/browser/password_form.h | 4 +++ .../core/browser/password_form_filling.cc | 4 +++ .../browser/password_form_filling_unittest.cc | 22 +++++++++++++++ .../browser/password_form_metrics_recorder.h | 4 ++- .../core/browser/password_manager_client.cc | 4 +++ .../core/browser/password_manager_client.h | 3 +++ tools/metrics/histograms/enums.xml | 3 +++ 12 files changed, 100 insertions(+), 4 deletions(-) diff --git a/chrome/browser/password_manager/chrome_password_manager_client.cc b/chrome/browser/password_manager/chrome_password_manager_client.cc index b6fb2087446198..e097225a2ee585 100644 --- a/chrome/browser/password_manager/chrome_password_manager_client.cc +++ b/chrome/browser/password_manager/chrome_password_manager_client.cc @@ -12,6 +12,7 @@ #include "base/bind.h" #include "base/callback_helpers.h" #include "base/command_line.h" +#include "base/feature_list.h" #include "base/memory/ptr_util.h" #include "base/memory/ref_counted.h" #include "base/memory/singleton.h" @@ -98,6 +99,7 @@ #include "content/public/browser/ssl_status.h" #include "content/public/browser/storage_partition.h" #include "content/public/browser/web_contents.h" +#include "content/public/common/content_features.h" #include "content/public/common/content_switches.h" #include "extensions/buildflags/buildflags.h" #include "google_apis/gaia/gaia_urls.h" @@ -948,6 +950,10 @@ FieldInfoManager* ChromePasswordManagerClient::GetFieldInfoManager() const { return FieldInfoManagerFactory::GetForBrowserContext(profile_); } +bool ChromePasswordManagerClient::IsWebAuthnAutofillEnabled() const { + return base::FeatureList::IsEnabled(features::kWebAuthConditionalUI); +} + void ChromePasswordManagerClient::AutomaticGenerationAvailable( const autofill::password_generation::PasswordGenerationUIData& ui_data) { if (!password_manager::bad_message::CheckChildProcessSecurityPolicyForURL( diff --git a/chrome/browser/password_manager/chrome_password_manager_client.h b/chrome/browser/password_manager/chrome_password_manager_client.h index 8357571a44c432..18f2054635a0d9 100644 --- a/chrome/browser/password_manager/chrome_password_manager_client.h +++ b/chrome/browser/password_manager/chrome_password_manager_client.h @@ -233,6 +233,7 @@ class ChromePasswordManagerClient bool IsIsolationForPasswordSitesEnabled() const override; bool IsNewTabPage() const override; password_manager::FieldInfoManager* GetFieldInfoManager() const override; + bool IsWebAuthnAutofillEnabled() const override; // autofill::mojom::PasswordGenerationDriver overrides. void AutomaticGenerationAvailable( diff --git a/components/password_manager/core/browser/form_parsing/form_parser.cc b/components/password_manager/core/browser/form_parsing/form_parser.cc index ac4cc7ade6cbd7..b40a4609a8d3de 100644 --- a/components/password_manager/core/browser/form_parsing/form_parser.cc +++ b/components/password_manager/core/browser/form_parsing/form_parser.cc @@ -42,6 +42,7 @@ constexpr char kAutocompleteCurrentPassword[] = "current-password"; constexpr char kAutocompleteNewPassword[] = "new-password"; constexpr char kAutocompleteCreditCardPrefix[] = "cc-"; constexpr char kAutocompleteOneTimePassword[] = "one-time-code"; +constexpr char kAutocompleteWebAuthn[] = "webauthn"; // The autocomplete attribute has one of the following structures: // [section-*] [shipping|billing] [type_hint] field_type @@ -50,9 +51,9 @@ constexpr char kAutocompleteOneTimePassword[] = "one-time-code"; // https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#autofilling-form-controls%3A-the-autocomplete-attribute). // For password forms, only the field_type is relevant. So parsing the attribute // amounts to just taking the last token. If that token is one of "username", -// "current-password" or "new-password", this returns an appropriate enum value. -// If the token starts with a "cc-" prefix or is "one-time-code" token, this -// returns kNonPassword. +// "current-password", "new-password" or "webauthn", this returns an appropriate +// enum value. If the token starts with a "cc-" prefix or is "one-time-code" +// token, this returns kNonPassword. // Otherwise, returns kNone. AutocompleteFlag ExtractAutocompleteFlag(const std::string& attribute) { std::vector tokens = @@ -68,6 +69,8 @@ AutocompleteFlag ExtractAutocompleteFlag(const std::string& attribute) { return AutocompleteFlag::kCurrentPassword; if (base::LowerCaseEqualsASCII(field_type, kAutocompleteNewPassword)) return AutocompleteFlag::kNewPassword; + if (base::LowerCaseEqualsASCII(field_type, kAutocompleteWebAuthn)) + return AutocompleteFlag::kWebAuthn; if (base::LowerCaseEqualsASCII(field_type, kAutocompleteOneTimePassword) || base::StartsWith(field_type, kAutocompleteCreditCardPrefix, @@ -202,6 +205,10 @@ struct SignificantFields { // True if the current form has only username, but no passwords. bool is_single_username = false; + // True if the current form accepts webauthn crendentials from an active + // webauthn request. + bool accepts_webauthn_credentials = false; + // Returns true if some password field is present. This is the minimal // requirement for a successful creation of a PasswordForm is present. bool HasPasswords() const { @@ -480,6 +487,7 @@ void ParseUsingAutocomplete(const std::vector& processed_fields, result->confirmation_password = processed_field.field; } break; + case AutocompleteFlag::kWebAuthn: case AutocompleteFlag::kNonPassword: case AutocompleteFlag::kNone: break; @@ -976,6 +984,8 @@ std::unique_ptr AssemblePasswordForm( significant_fields.is_new_password_reliable; result->only_for_fallback = significant_fields.is_fallback; result->submission_event = form_data.submission_event; + result->accepts_webauthn_credentials = + significant_fields.accepts_webauthn_credentials; for (const FormFieldData& field : form_data.fields) { if (field.form_control_type == "password" && @@ -1093,6 +1103,15 @@ std::unique_ptr FormDataParser::Parse(const FormData& form_data, base::FeatureList::IsEnabled( features::kTreatNewPasswordHeuristicsAsReliable)); + if (mode == Mode::kFilling) { + for (const auto& field : processed_fields) { + if (field.autocomplete_flag == AutocompleteFlag::kWebAuthn) { + significant_fields.accepts_webauthn_credentials = true; + break; + } + } + } + base::UmaHistogramEnumeration("PasswordManager.UsernameDetectionMethod", method); diff --git a/components/password_manager/core/browser/form_parsing/form_parser.h b/components/password_manager/core/browser/form_parsing/form_parser.h index e14d8c2f0464b7..9c4eccf81fadeb 100644 --- a/components/password_manager/core/browser/form_parsing/form_parser.h +++ b/components/password_manager/core/browser/form_parsing/form_parser.h @@ -28,6 +28,7 @@ enum class AutocompleteFlag { kUsername, kCurrentPassword, kNewPassword, + kWebAuthn, // Represents the whole family of cc-* flags + OTP flag. kNonPassword }; diff --git a/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc b/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc index 09dfdc3996c6cd..549d2a088ca023 100644 --- a/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc +++ b/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc @@ -101,6 +101,7 @@ struct FormParsingTestCase { SubmissionIndicatorEvent submission_event = SubmissionIndicatorEvent::NONE; absl::optional is_new_password_reliable; bool form_has_autofilled_value = false; + bool accepts_webauthn_credentials = false; }; // Returns numbers which are distinct from each other within the scope of one @@ -368,6 +369,9 @@ void CheckTestData(const std::vector& test_cases) { EXPECT_EQ(*test_case.is_new_password_reliable, parsed_form->is_new_password_reliable); } + EXPECT_EQ(test_case.accepts_webauthn_credentials && + mode == FormDataParser::Mode::kFilling, + parsed_form->accepts_webauthn_credentials); EXPECT_EQ(test_case.form_has_autofilled_value, parsed_form->form_has_autofilled_value); @@ -2852,6 +2856,29 @@ TEST(FormParserTest, UsernameWithTypePasswordAndServerPredictions) { }); } +// Tests that if a field is marked as autofill="webauthn" then the +// `accepts_webauthn_credentials` flag is set. +TEST(FormParserTest, AcceptsWebAuthnCredentials) { + CheckTestData({ + { + .description_for_logging = "Field tagged with autofill=\"webauthn\"", + .fields = + { + {.role = ElementRole::USERNAME, + .autocomplete_attribute = "webauthn", + .value = u"rosalina", + .name = u"username", + .form_control_type = "text"}, + {.role = ElementRole::CURRENT_PASSWORD, + .value = u"luma", + .name = u"password", + .form_control_type = "password"}, + }, + .accepts_webauthn_credentials = true, + }, + }); +} + } // namespace } // namespace password_manager diff --git a/components/password_manager/core/browser/password_form.h b/components/password_manager/core/browser/password_form.h index 7a424b34fa0de5..c49af8c9287870 100644 --- a/components/password_manager/core/browser/password_form.h +++ b/components/password_manager/core/browser/password_form.h @@ -343,6 +343,10 @@ struct PasswordForm { // password generation eligibility. bool is_new_password_reliable = false; + // True iff the form may be filled with webauthn credentials from an active + // webauthn request. + bool accepts_webauthn_credentials = false; + // Serialized to prefs, so don't change numeric values! // These values are persisted to logs. Entries should not be renumbered and // numeric values should never be reused. diff --git a/components/password_manager/core/browser/password_form_filling.cc b/components/password_manager/core/browser/password_form_filling.cc index da8f3ccd71b260..5f931e873c582f 100644 --- a/components/password_manager/core/browser/password_form_filling.cc +++ b/components/password_manager/core/browser/password_form_filling.cc @@ -240,6 +240,10 @@ LikelyFormFilling SendFillInformationToRenderer( wait_for_username_reason = WaitForUsernameReason::kInsecureOrigin; } else if (IsFillOnAccountSelectFeatureEnabled()) { wait_for_username_reason = WaitForUsernameReason::kFoasFeature; + } else if (observed_form.accepts_webauthn_credentials && + client->IsWebAuthnAutofillEnabled()) { + wait_for_username_reason = + WaitForUsernameReason::kAcceptsWebAuthnCredentials; } // Record no "FirstWaitForUsernameReason" metrics for a form that is not meant diff --git a/components/password_manager/core/browser/password_form_filling_unittest.cc b/components/password_manager/core/browser/password_form_filling_unittest.cc index 8383bf86c73e3c..a0609442bd8a56 100644 --- a/components/password_manager/core/browser/password_form_filling_unittest.cc +++ b/components/password_manager/core/browser/password_form_filling_unittest.cc @@ -71,6 +71,7 @@ class MockPasswordManagerClient : public StubPasswordManagerClient { (const GURL&), (const, override)); MOCK_METHOD(bool, IsCommittedMainFrameSecure, (), (const, override)); + MOCK_METHOD(bool, IsWebAuthnAutofillEnabled, (), (const, override)); }; PasswordForm CreateForm(std::u16string username, @@ -263,6 +264,27 @@ TEST_F(PasswordFormFillingTest, TestFillOnLoadSuggestion) { } } +#if !defined(ANDROID) && !defined(OS_IOS) +TEST_F(PasswordFormFillingTest, DontFillOnLoadWebAuthnCredentials) { + observed_form_.accepts_webauthn_credentials = true; + for (bool webauthn_autofill_enabled : {false, true}) { + PasswordFormFillData fill_data; + EXPECT_CALL(client_, IsWebAuthnAutofillEnabled()) + .WillOnce(Return(webauthn_autofill_enabled)); + EXPECT_CALL(driver_, FillPasswordForm(_)).WillOnce(SaveArg<0>(&fill_data)); + EXPECT_CALL(client_, PasswordWasAutofilled); + LikelyFormFilling likely_form_filling = SendFillInformationToRenderer( + &client_, &driver_, observed_form_, {&saved_match_}, federated_matches_, + &saved_match_, /*blocked_by_user=*/false, metrics_recorder_.get()); + if (webauthn_autofill_enabled) { + EXPECT_EQ(LikelyFormFilling::kFillOnAccountSelect, likely_form_filling); + } else { + EXPECT_EQ(LikelyFormFilling::kFillOnPageLoad, likely_form_filling); + } + } +} +#endif + // Test autofill when username and password are prefilled. Overwrite password // if server side classification thought the username was a placeholder or the // classification failed. Do not overwrite if username doesn't look like a diff --git a/components/password_manager/core/browser/password_form_metrics_recorder.h b/components/password_manager/core/browser/password_form_metrics_recorder.h index 0c6630cd91b3bc..bb74bfe8caf33f 100644 --- a/components/password_manager/core/browser/password_form_metrics_recorder.h +++ b/components/password_manager/core/browser/password_form_metrics_recorder.h @@ -230,7 +230,9 @@ class PasswordFormMetricsRecorder kPasswordPrefilled = 8, // A credential exists for affiliated website. kAffiliatedWebsite = 9, - kMaxValue = kAffiliatedWebsite, + // The form may accept WebAuthn credentials. + kAcceptsWebAuthnCredentials = 10, + kMaxValue = kAcceptsWebAuthnCredentials, }; // Used in UMA histogram, please do NOT reorder. diff --git a/components/password_manager/core/browser/password_manager_client.cc b/components/password_manager/core/browser/password_manager_client.cc index 9797115d912f61..cab8e558314b52 100644 --- a/components/password_manager/core/browser/password_manager_client.cc +++ b/components/password_manager/core/browser/password_manager_client.cc @@ -167,4 +167,8 @@ network::mojom::NetworkContext* PasswordManagerClient::GetNetworkContext() bool PasswordManagerClient::IsUnderAdvancedProtection() const { return false; } + +bool PasswordManagerClient::IsWebAuthnAutofillEnabled() const { + return false; +} } // namespace password_manager diff --git a/components/password_manager/core/browser/password_manager_client.h b/components/password_manager/core/browser/password_manager_client.h index 0f59af27c648aa..76f88a6f9815c0 100644 --- a/components/password_manager/core/browser/password_manager_client.h +++ b/components/password_manager/core/browser/password_manager_client.h @@ -421,6 +421,9 @@ class PasswordManagerClient { // Returns a FieldInfoManager associated with the current profile. virtual FieldInfoManager* GetFieldInfoManager() const = 0; + // Returns true if integration between WebAuthn and Autofill is enabled. + virtual bool IsWebAuthnAutofillEnabled() const; + // Returns if the Autofill Assistant UI is shown. virtual bool IsAutofillAssistantUIVisible() const = 0; diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml index 85aa4d42b0e95b..8321f34780558c 100644 --- a/tools/metrics/histograms/enums.xml +++ b/tools/metrics/histograms/enums.xml @@ -63935,6 +63935,9 @@ Called by update_net_trust_anchors.py.--> A credential exists for an affiliated matched site but not for the current security origin. + + The form acccepts WebAuthn credentials from an active WebAuthn request. +