diff options
Diffstat (limited to 'chromium/components/password_manager')
82 files changed, 2665 insertions, 359 deletions
diff --git a/chromium/components/password_manager/content/browser/PRESUBMIT.py b/chromium/components/password_manager/content/browser/PRESUBMIT.py index 1507afc4db2..c17399159d8 100644 --- a/chromium/components/password_manager/content/browser/PRESUBMIT.py +++ b/chromium/components/password_manager/content/browser/PRESUBMIT.py @@ -8,7 +8,7 @@ match the corresponding bad_message.h file. def _RunHistogramChecks(input_api, output_api, histogram_name): try: - # Setup sys.path so that we can call histrogram code + # Setup sys.path so that we can call histograms code. import sys original_sys_path = sys.path sys.path = sys.path + [input_api.os_path.join( diff --git a/chromium/components/password_manager/content/browser/content_password_manager_driver.cc b/chromium/components/password_manager/content/browser/content_password_manager_driver.cc index 90b20e9f4e3..3a847908681 100644 --- a/chromium/components/password_manager/content/browser/content_password_manager_driver.cc +++ b/chromium/components/password_manager/content/browser/content_password_manager_driver.cc @@ -38,6 +38,7 @@ ContentPasswordManagerDriver::ContentPasswordManagerDriver( password_generation_manager_(client, this), password_autofill_manager_(this, autofill_client), next_free_key_(0), + is_main_frame_(render_frame_host->GetParent() == nullptr), password_manager_binding_(this), weak_factory_(this) { // Does nothing if a VisiblePasswordObserver has already been created @@ -161,6 +162,10 @@ autofill::AutofillDriver* ContentPasswordManagerDriver::GetAutofillDriver() { render_frame_host_); } +bool ContentPasswordManagerDriver::IsMainFrame() const { + return is_main_frame_; +} + PasswordGenerationManager* ContentPasswordManagerDriver::GetPasswordGenerationManager() { return &password_generation_manager_; @@ -283,6 +288,14 @@ void ContentPasswordManagerDriver::SaveGenerationFieldDetectedByClassifier( password_form, generation_field); } +void ContentPasswordManagerDriver::CheckSafeBrowsingReputation( + const GURL& form_action, + const GURL& frame_url) { +#if defined(SAFE_BROWSING_DB_LOCAL) + client_->CheckSafeBrowsingReputation(form_action, frame_url); +#endif +} + void ContentPasswordManagerDriver::ShowPasswordSuggestions( int key, base::i18n::TextDirection text_direction, @@ -332,9 +345,8 @@ ContentPasswordManagerDriver::GetAutofillAgent() { const autofill::mojom::PasswordAutofillAgentPtr& ContentPasswordManagerDriver::GetPasswordAutofillAgent() { if (!password_autofill_agent_) { - autofill::mojom::PasswordAutofillAgentRequest request( - &password_autofill_agent_); - // Some test codes may have no initialized remote interfaces. + auto request = mojo::MakeRequest(&password_autofill_agent_); + // Some test environments may have no remote interface support. if (render_frame_host_->GetRemoteInterfaces()) { render_frame_host_->GetRemoteInterfaces()->GetInterface( std::move(request)); diff --git a/chromium/components/password_manager/content/browser/content_password_manager_driver.h b/chromium/components/password_manager/content/browser/content_password_manager_driver.h index a1c2168995b..c7a867a23ec 100644 --- a/chromium/components/password_manager/content/browser/content_password_manager_driver.h +++ b/chromium/components/password_manager/content/browser/content_password_manager_driver.h @@ -78,6 +78,7 @@ class ContentPasswordManagerDriver void SendLoggingAvailability() override; void AllowToRunFormClassifier() override; autofill::AutofillDriver* GetAutofillDriver() override; + bool IsMainFrame() const override; PasswordGenerationManager* GetPasswordGenerationManager() override; PasswordManager* GetPasswordManager() override; @@ -109,6 +110,8 @@ class ContentPasswordManagerDriver void SaveGenerationFieldDetectedByClassifier( const autofill::PasswordForm& password_form, const base::string16& generation_field) override; + void CheckSafeBrowsingReputation(const GURL& form_action, + const GURL& frame_url) override; void OnPasswordFormsParsedNoRenderCheck( const std::vector<autofill::PasswordForm>& forms); @@ -143,6 +146,11 @@ class ContentPasswordManagerDriver // it to each other over IPC. The counter below is used to generate new IDs. int next_free_key_; + // It should be filled in the constructor, since later the frame might be + // detached and it would be impossible to check whether the frame is a main + // frame. + const bool is_main_frame_; + autofill::mojom::PasswordAutofillAgentPtr password_autofill_agent_; autofill::mojom::PasswordGenerationAgentPtr password_gen_agent_; diff --git a/chromium/components/password_manager/content/browser/content_password_manager_driver_factory.cc b/chromium/components/password_manager/content/browser/content_password_manager_driver_factory.cc index eb9c1b119b9..c1dddc0ee88 100644 --- a/chromium/components/password_manager/content/browser/content_password_manager_driver_factory.cc +++ b/chromium/components/password_manager/content/browser/content_password_manager_driver_factory.cc @@ -51,7 +51,7 @@ void ContentPasswordManagerDriverFactory::CreateForWebContents( web_contents->SetUserData( kContentPasswordManagerDriverFactoryWebContentsUserDataKey, - new_factory.release()); + std::move(new_factory)); } ContentPasswordManagerDriverFactory::ContentPasswordManagerDriverFactory( @@ -76,6 +76,7 @@ ContentPasswordManagerDriverFactory::FromWebContents( // static void ContentPasswordManagerDriverFactory::BindPasswordManagerDriver( content::RenderFrameHost* render_frame_host, + const service_manager::BindSourceInfo& source_info, autofill::mojom::PasswordManagerDriverRequest request) { content::WebContents* web_contents = content::WebContents::FromRenderFrameHost(render_frame_host); @@ -100,6 +101,7 @@ void ContentPasswordManagerDriverFactory::BindPasswordManagerDriver( // static void ContentPasswordManagerDriverFactory::BindSensitiveInputVisibilityService( content::RenderFrameHost* render_frame_host, + const service_manager::BindSourceInfo& source_info, blink::mojom::SensitiveInputVisibilityServiceRequest request) { content::WebContents* web_contents = content::WebContents::FromRenderFrameHost(render_frame_host); diff --git a/chromium/components/password_manager/content/browser/content_password_manager_driver_factory.h b/chromium/components/password_manager/content/browser/content_password_manager_driver_factory.h index 6c5de068d5d..12c3aef641e 100644 --- a/chromium/components/password_manager/content/browser/content_password_manager_driver_factory.h +++ b/chromium/components/password_manager/content/browser/content_password_manager_driver_factory.h @@ -17,6 +17,7 @@ #include "components/password_manager/core/browser/password_manager.h" #include "components/password_manager/core/browser/password_manager_driver.h" #include "content/public/browser/web_contents_observer.h" +#include "services/service_manager/public/cpp/bind_source_info.h" #include "third_party/WebKit/public/platform/modules/sensitive_input_visibility/sensitive_input_visibility_service.mojom.h" namespace content { @@ -43,10 +44,12 @@ class ContentPasswordManagerDriverFactory static void BindPasswordManagerDriver( content::RenderFrameHost* render_frame_host, + const service_manager::BindSourceInfo& source_info, autofill::mojom::PasswordManagerDriverRequest request); static void BindSensitiveInputVisibilityService( content::RenderFrameHost* render_frame_host, + const service_manager::BindSourceInfo& source_info, blink::mojom::SensitiveInputVisibilityServiceRequest request); ContentPasswordManagerDriver* GetDriverForFrame( diff --git a/chromium/components/password_manager/content/browser/content_password_manager_driver_unittest.cc b/chromium/components/password_manager/content/browser/content_password_manager_driver_unittest.cc index e91435b3185..69741b25a5a 100644 --- a/chromium/components/password_manager/content/browser/content_password_manager_driver_unittest.cc +++ b/chromium/components/password_manager/content/browser/content_password_manager_driver_unittest.cc @@ -43,6 +43,9 @@ class MockPasswordManagerClient : public StubPasswordManagerClient { ~MockPasswordManagerClient() override = default; MOCK_CONST_METHOD0(GetLogManager, const LogManager*()); +#if defined(SAFE_BROWSING_DB_LOCAL) + MOCK_METHOD2(CheckSafeBrowsingReputation, void(const GURL&, const GURL&)); +#endif private: DISALLOW_COPY_AND_ASSIGN(MockPasswordManagerClient); @@ -59,8 +62,8 @@ class FakePasswordAutofillAgent ~FakePasswordAutofillAgent() override {} void BindRequest(mojo::ScopedMessagePipeHandle handle) { - binding_.Bind(mojo::MakeRequest<autofill::mojom::PasswordAutofillAgent>( - std::move(handle))); + binding_.Bind( + autofill::mojom::PasswordAutofillAgentRequest(std::move(handle))); } bool called_set_logging_state() { return called_set_logging_state_; } @@ -86,7 +89,7 @@ class FakePasswordAutofillAgent const autofill::FormsPredictionsMap& predictions) override {} void FindFocusedPasswordForm( - const FindFocusedPasswordFormCallback& callback) override {} + FindFocusedPasswordFormCallback callback) override {} // Records whether SetLoggingState() gets called. bool called_set_logging_state_; @@ -425,6 +428,16 @@ TEST_F(ContentPasswordManagerDriverTest, ClearPasswordsOnAutofill) { } } +#if defined(SAFE_BROWSING_DB_LOCAL) +TEST_F(ContentPasswordManagerDriverTest, CheckSafeBrowsingReputationCalled) { + std::unique_ptr<ContentPasswordManagerDriver> driver( + new ContentPasswordManagerDriver(main_rfh(), &password_manager_client_, + &autofill_client_)); + EXPECT_CALL(password_manager_client_, CheckSafeBrowsingReputation(_, _)); + driver->CheckSafeBrowsingReputation(GURL(), GURL()); +} +#endif + INSTANTIATE_TEST_CASE_P(, ContentPasswordManagerDriverTest, testing::Values(true, false)); diff --git a/chromium/components/password_manager/content/browser/credential_manager_impl.cc b/chromium/components/password_manager/content/browser/credential_manager_impl.cc index 14261ccac92..d423a3a64f6 100644 --- a/chromium/components/password_manager/content/browser/credential_manager_impl.cc +++ b/chromium/components/password_manager/content/browser/credential_manager_impl.cc @@ -22,7 +22,6 @@ #include "components/password_manager/core/browser/password_manager_metrics_util.h" #include "components/password_manager/core/browser/password_manager_util.h" #include "components/password_manager/core/browser/password_store.h" -#include "components/password_manager/core/common/credential_manager_types.h" #include "components/password_manager/core/common/password_manager_pref_names.h" #include "content/public/browser/web_contents.h" @@ -30,9 +29,9 @@ namespace password_manager { namespace { -void RunMojoGetCallback(const mojom::CredentialManager::GetCallback& callback, +void RunMojoGetCallback(mojom::CredentialManager::GetCallback callback, const CredentialInfo& info) { - callback.Run(mojom::CredentialManagerError::SUCCESS, info); + std::move(callback).Run(mojom::CredentialManagerError::SUCCESS, info); } } // namespace @@ -41,7 +40,10 @@ void RunMojoGetCallback(const mojom::CredentialManager::GetCallback& callback, CredentialManagerImpl::CredentialManagerImpl(content::WebContents* web_contents, PasswordManagerClient* client) - : WebContentsObserver(web_contents), client_(client), weak_factory_(this) { + : WebContentsObserver(web_contents), + client_(client), + binding_(this), + weak_factory_(this) { DCHECK(web_contents); auto_signin_enabled_.Init(prefs::kCredentialsEnableAutosignin, client_->GetPrefs()); @@ -50,12 +52,30 @@ CredentialManagerImpl::CredentialManagerImpl(content::WebContents* web_contents, CredentialManagerImpl::~CredentialManagerImpl() {} void CredentialManagerImpl::BindRequest( - mojom::CredentialManagerRequest request) { - bindings_.AddBinding(this, std::move(request)); + mojom::CredentialManagerAssociatedRequest request) { + DCHECK(!binding_.is_bound()); + binding_.Bind(std::move(request)); + + // The browser side will close the message pipe on DidFinishNavigation before + // the renderer side would be destroyed, and the renderer never explicitly + // closes the pipe. So a connection error really means an error here, in which + // case the renderer will try to reconnect when the next call to the API is + // made. Make sure this implementation will no longer be bound to a broken + // pipe once that happens, so the DCHECK above will succeed. + binding_.set_connection_error_handler(base::Bind( + &CredentialManagerImpl::DisconnectBinding, base::Unretained(this))); +} + +bool CredentialManagerImpl::HasBinding() const { + return binding_.is_bound(); +} + +void CredentialManagerImpl::DisconnectBinding() { + binding_.Close(); } void CredentialManagerImpl::Store(const CredentialInfo& credential, - const StoreCallback& callback) { + StoreCallback callback) { DCHECK_NE(CredentialType::CREDENTIAL_TYPE_EMPTY, credential.type); if (password_manager_util::IsLoggingActive(client_)) { @@ -65,7 +85,7 @@ void CredentialManagerImpl::Store(const CredentialInfo& credential, } // Send acknowledge response back. - callback.Run(); + std::move(callback).Run(); if (!client_->IsSavingAndFillingEnabledForCurrentPage() || !client_->OnCredentialManagerUsed()) @@ -79,9 +99,11 @@ void CredentialManagerImpl::Store(const CredentialInfo& credential, std::unique_ptr<autofill::PasswordForm> observed_form = CreateObservedPasswordFormFromOrigin(origin); - // Create a custom form fetcher with suppressed HTTP->HTTPS migration. + // Create a custom form fetcher without HTTP->HTTPS migration, as well as + // without fetching of suppressed HTTPS credentials on HTTP origins as the API + // is only available on HTTPS origins. auto form_fetcher = base::MakeUnique<FormFetcherImpl>( - PasswordStore::FormDigest(*observed_form), client_, false); + PasswordStore::FormDigest(*observed_form), client_, false, false); form_manager_ = base::MakeUnique<CredentialManagerPasswordFormManager>( client_, GetDriver(), *observed_form, std::move(form), this, nullptr, std::move(form_fetcher)); @@ -125,14 +147,14 @@ void CredentialManagerImpl::OnProvisionalSaveComplete() { client_->PromptUserToSaveOrUpdatePassword(std::move(form_manager_), false); } -void CredentialManagerImpl::RequireUserMediation( - const RequireUserMediationCallback& callback) { +void CredentialManagerImpl::PreventSilentAccess( + PreventSilentAccessCallback callback) { if (password_manager_util::IsLoggingActive(client_)) { CredentialManagerLogger(client_->GetLogManager()) - .LogRequireUserMediation(web_contents()->GetLastCommittedURL()); + .LogPreventSilentAccess(web_contents()->GetLastCommittedURL()); } // Send acknowledge response back. - callback.Run(); + std::move(callback).Run(); PasswordStore* store = GetPasswordStore(); if (!store || !client_->IsSavingAndFillingEnabledForCurrentPage() || @@ -141,33 +163,32 @@ void CredentialManagerImpl::RequireUserMediation( if (!pending_require_user_mediation_) { pending_require_user_mediation_.reset( - new CredentialManagerPendingRequireUserMediationTask(this)); + new CredentialManagerPendingPreventSilentAccessTask(this)); } pending_require_user_mediation_->AddOrigin(GetSynthesizedFormForOrigin()); } -void CredentialManagerImpl::Get(bool zero_click_only, +void CredentialManagerImpl::Get(CredentialMediationRequirement mediation, bool include_passwords, const std::vector<GURL>& federations, - const GetCallback& callback) { + GetCallback callback) { using metrics_util::LogCredentialManagerGetResult; - metrics_util::CredentialManagerGetMediation mediation_status = - zero_click_only ? metrics_util::CREDENTIAL_MANAGER_GET_UNMEDIATED - : metrics_util::CREDENTIAL_MANAGER_GET_MEDIATED; + PasswordStore* store = GetPasswordStore(); if (password_manager_util::IsLoggingActive(client_)) { CredentialManagerLogger(client_->GetLogManager()) - .LogRequestCredential(web_contents()->GetLastCommittedURL(), - zero_click_only, federations); + .LogRequestCredential(web_contents()->GetLastCommittedURL(), mediation, + federations); } if (pending_request_ || !store) { // Callback error. - callback.Run(pending_request_ - ? mojom::CredentialManagerError::PENDINGREQUEST - : mojom::CredentialManagerError::PASSWORDSTOREUNAVAILABLE, - base::nullopt); + std::move(callback).Run( + pending_request_ + ? mojom::CredentialManagerError::PENDINGREQUEST + : mojom::CredentialManagerError::PASSWORDSTOREUNAVAILABLE, + base::nullopt); LogCredentialManagerGetResult(metrics_util::CREDENTIAL_MANAGER_GET_REJECTED, - mediation_status); + mediation); return; } @@ -175,23 +196,25 @@ void CredentialManagerImpl::Get(bool zero_click_only, // page is being prerendered. if (!client_->IsFillingEnabledForCurrentPage() || !client_->OnCredentialManagerUsed()) { - callback.Run(mojom::CredentialManagerError::SUCCESS, CredentialInfo()); + std::move(callback).Run(mojom::CredentialManagerError::SUCCESS, + CredentialInfo()); LogCredentialManagerGetResult(metrics_util::CREDENTIAL_MANAGER_GET_NONE, - mediation_status); + mediation); return; } // Return an empty credential if zero-click is required but disabled. - if (zero_click_only && !IsZeroClickAllowed()) { + if (mediation == CredentialMediationRequirement::kSilent && + !IsZeroClickAllowed()) { // Callback with empty credential info. - callback.Run(mojom::CredentialManagerError::SUCCESS, CredentialInfo()); + std::move(callback).Run(mojom::CredentialManagerError::SUCCESS, + CredentialInfo()); LogCredentialManagerGetResult( - metrics_util::CREDENTIAL_MANAGER_GET_NONE_ZERO_CLICK_OFF, - mediation_status); + metrics_util::CREDENTIAL_MANAGER_GET_NONE_ZERO_CLICK_OFF, mediation); return; } pending_request_.reset(new CredentialManagerPendingRequestTask( - this, base::Bind(&RunMojoGetCallback, callback), zero_click_only, + this, base::Bind(&RunMojoGetCallback, base::Passed(&callback)), mediation, include_passwords, federations)); // This will result in a callback to // PendingRequestTask::OnGetPasswordStoreResults(). @@ -232,6 +255,7 @@ void CredentialManagerImpl::SendCredential( void CredentialManagerImpl::SendPasswordForm( const SendCredentialCallback& send_callback, + CredentialMediationRequirement mediation, const autofill::PasswordForm* form) { CredentialInfo info; if (form) { @@ -250,14 +274,12 @@ void CredentialManagerImpl::SendPasswordForm( base::RecordAction( base::UserMetricsAction("CredentialManager_AccountChooser_Accepted")); metrics_util::LogCredentialManagerGetResult( - metrics_util::CREDENTIAL_MANAGER_GET_ACCOUNT_CHOOSER, - metrics_util::CREDENTIAL_MANAGER_GET_MEDIATED); + metrics_util::CREDENTIAL_MANAGER_GET_ACCOUNT_CHOOSER, mediation); } else { base::RecordAction( base::UserMetricsAction("CredentialManager_AccountChooser_Dismissed")); metrics_util::LogCredentialManagerGetResult( - metrics_util::CREDENTIAL_MANAGER_GET_NONE, - metrics_util::CREDENTIAL_MANAGER_GET_MEDIATED); + metrics_util::CREDENTIAL_MANAGER_GET_NONE, mediation); } SendCredential(send_callback, info); } diff --git a/chromium/components/password_manager/content/browser/credential_manager_impl.h b/chromium/components/password_manager/content/browser/credential_manager_impl.h index 3b3f5cd97ba..4e6369dfe30 100644 --- a/chromium/components/password_manager/content/browser/credential_manager_impl.h +++ b/chromium/components/password_manager/content/browser/credential_manager_impl.h @@ -12,12 +12,13 @@ #include "base/memory/weak_ptr.h" #include "components/password_manager/content/common/credential_manager.mojom.h" #include "components/password_manager/core/browser/credential_manager_password_form_manager.h" +#include "components/password_manager/core/browser/credential_manager_pending_prevent_silent_access_task.h" #include "components/password_manager/core/browser/credential_manager_pending_request_task.h" -#include "components/password_manager/core/browser/credential_manager_pending_require_user_mediation_task.h" #include "components/password_manager/core/browser/password_store_consumer.h" +#include "components/password_manager/core/common/credential_manager_types.h" #include "components/prefs/pref_member.h" #include "content/public/browser/web_contents_observer.h" -#include "mojo/public/cpp/bindings/binding_set.h" +#include "mojo/public/cpp/bindings/associated_binding.h" class GURL; @@ -41,23 +42,23 @@ class CredentialManagerImpl public content::WebContentsObserver, public CredentialManagerPasswordFormManagerDelegate, public CredentialManagerPendingRequestTaskDelegate, - public CredentialManagerPendingRequireUserMediationTaskDelegate { + public CredentialManagerPendingPreventSilentAccessTaskDelegate { public: CredentialManagerImpl(content::WebContents* web_contents, PasswordManagerClient* client); ~CredentialManagerImpl() override; - void BindRequest(mojom::CredentialManagerRequest request); + void BindRequest(mojom::CredentialManagerAssociatedRequest request); + bool HasBinding() const; + void DisconnectBinding(); // mojom::CredentialManager methods: - void Store(const CredentialInfo& credential, - const StoreCallback& callback) override; - void RequireUserMediation( - const RequireUserMediationCallback& callback) override; - void Get(bool zero_click_only, + void Store(const CredentialInfo& credential, StoreCallback callback) override; + void PreventSilentAccess(PreventSilentAccessCallback callback) override; + void Get(CredentialMediationRequirement mediation, bool include_passwords, const std::vector<GURL>& federations, - const GetCallback& callback) override; + GetCallback callback) override; // CredentialManagerPendingRequestTaskDelegate: bool IsZeroClickAllowed() const override; @@ -65,6 +66,7 @@ class CredentialManagerImpl void SendCredential(const SendCredentialCallback& send_callback, const CredentialInfo& info) override; void SendPasswordForm(const SendCredentialCallback& send_callback, + CredentialMediationRequirement mediation, const autofill::PasswordForm* form) override; PasswordManagerClient* client() const override; @@ -94,10 +96,10 @@ class CredentialManagerImpl // they can properly respond to the request once the PasswordStore gives // us data. std::unique_ptr<CredentialManagerPendingRequestTask> pending_request_; - std::unique_ptr<CredentialManagerPendingRequireUserMediationTask> + std::unique_ptr<CredentialManagerPendingPreventSilentAccessTask> pending_require_user_mediation_; - mojo::BindingSet<mojom::CredentialManager> bindings_; + mojo::AssociatedBinding<mojom::CredentialManager> binding_; base::WeakPtrFactory<CredentialManagerImpl> weak_factory_; diff --git a/chromium/components/password_manager/content/browser/credential_manager_impl_unittest.cc b/chromium/components/password_manager/content/browser/credential_manager_impl_unittest.cc index 9563fc28c69..0e7a132ff74 100644 --- a/chromium/components/password_manager/content/browser/credential_manager_impl_unittest.cc +++ b/chromium/components/password_manager/content/browser/credential_manager_impl_unittest.cc @@ -8,6 +8,7 @@ #include <string> #include <tuple> +#include <utility> #include <vector> #include "base/bind.h" @@ -15,6 +16,7 @@ #include "base/location.h" #include "base/macros.h" #include "base/memory/ptr_util.h" +#include "base/message_loop/message_loop.h" #include "base/run_loop.h" #include "base/single_thread_task_runner.h" #include "base/strings/string16.h" @@ -290,17 +292,17 @@ class CredentialManagerImplTest : public content::RenderViewHostTestHarness { content::RenderViewHostTestHarness::TearDown(); } - void ExpectZeroClickSignInFailure(bool zero_click_only, + void ExpectZeroClickSignInFailure(CredentialMediationRequirement mediation, bool include_passwords, const std::vector<GURL>& federations) { bool called = false; mojom::CredentialManagerError error; base::Optional<CredentialInfo> credential; - CallGet(zero_click_only, include_passwords, federations, - base::Bind(&GetCredentialCallback, &called, &error, &credential)); EXPECT_CALL(*client_, PromptUserToChooseCredentialsPtr(_, _, _)) .Times(testing::Exactly(0)); EXPECT_CALL(*client_, NotifyUserAutoSigninPtr()).Times(testing::Exactly(0)); + CallGet(mediation, include_passwords, federations, + base::Bind(&GetCredentialCallback, &called, &error, &credential)); RunAllPendingTasks(); @@ -309,18 +311,18 @@ class CredentialManagerImplTest : public content::RenderViewHostTestHarness { EXPECT_EQ(CredentialType::CREDENTIAL_TYPE_EMPTY, credential->type); } - void ExpectZeroClickSignInSuccess(bool zero_click_only, + void ExpectZeroClickSignInSuccess(CredentialMediationRequirement mediation, bool include_passwords, const std::vector<GURL>& federations, CredentialType type) { bool called = false; mojom::CredentialManagerError error; base::Optional<CredentialInfo> credential; - CallGet(zero_click_only, include_passwords, federations, - base::Bind(&GetCredentialCallback, &called, &error, &credential)); EXPECT_CALL(*client_, PromptUserToChooseCredentialsPtr(_, _, _)) .Times(testing::Exactly(0)); EXPECT_CALL(*client_, NotifyUserAutoSigninPtr()).Times(testing::Exactly(1)); + CallGet(mediation, include_passwords, federations, + base::Bind(&GetCredentialCallback, &called, &error, &credential)); RunAllPendingTasks(); @@ -329,14 +331,14 @@ class CredentialManagerImplTest : public content::RenderViewHostTestHarness { EXPECT_EQ(type, credential->type); } - void ExpectCredentialType(bool zero_click_only, + void ExpectCredentialType(CredentialMediationRequirement mediation, bool include_passwords, const std::vector<GURL>& federations, CredentialType type) { bool called = false; mojom::CredentialManagerError error; base::Optional<CredentialInfo> credential; - CallGet(zero_click_only, include_passwords, federations, + CallGet(mediation, include_passwords, federations, base::Bind(&GetCredentialCallback, &called, &error, &credential)); RunAllPendingTasks(); @@ -350,21 +352,21 @@ class CredentialManagerImplTest : public content::RenderViewHostTestHarness { // Helpers for testing CredentialManagerImpl methods. void CallStore(const CredentialInfo& info, - const CredentialManagerImpl::StoreCallback& callback) { - cm_service_impl_->Store(info, callback); + CredentialManagerImpl::StoreCallback callback) { + cm_service_impl_->Store(info, std::move(callback)); } - void CallRequireUserMediation( - const CredentialManagerImpl::RequireUserMediationCallback& callback) { - cm_service_impl_->RequireUserMediation(callback); + void CallPreventSilentAccess( + CredentialManagerImpl::PreventSilentAccessCallback callback) { + cm_service_impl_->PreventSilentAccess(std::move(callback)); } - void CallGet(bool zero_click_only, + void CallGet(CredentialMediationRequirement mediation, bool include_passwords, const std::vector<GURL>& federations, - const CredentialManagerImpl::GetCallback& callback) { - cm_service_impl_->Get(zero_click_only, include_passwords, federations, - callback); + CredentialManagerImpl::GetCallback callback) { + cm_service_impl_->Get(mediation, include_passwords, federations, + std::move(callback)); } protected: @@ -678,7 +680,7 @@ TEST_F(CredentialManagerImplTest, CredentialManagerGetOverwriteZeroClick) { bool called = false; mojom::CredentialManagerError error; base::Optional<CredentialInfo> credential; - CallGet(false, true, federations, + CallGet(CredentialMediationRequirement::kOptional, true, federations, base::Bind(&GetCredentialCallback, &called, &error, &credential)); RunAllPendingTasks(); @@ -709,7 +711,7 @@ TEST_F(CredentialManagerImplTest, EXPECT_FALSE(client_->pending_manager()); } -TEST_F(CredentialManagerImplTest, CredentialManagerOnRequireUserMediation) { +TEST_F(CredentialManagerImplTest, CredentialManagerOnPreventSilentAccess) { store_->AddLogin(form_); store_->AddLogin(subdomain_form_); store_->AddLogin(cross_origin_form_); @@ -725,7 +727,7 @@ TEST_F(CredentialManagerImplTest, CredentialManagerOnRequireUserMediation) { EXPECT_FALSE(passwords[cross_origin_form_.signon_realm][0].skip_zero_click); bool called = false; - CallRequireUserMediation(base::Bind(&RespondCallback, &called)); + CallPreventSilentAccess(base::Bind(&RespondCallback, &called)); RunAllPendingTasks(); @@ -742,7 +744,7 @@ TEST_F(CredentialManagerImplTest, CredentialManagerOnRequireUserMediation) { } TEST_F(CredentialManagerImplTest, - CredentialManagerOnRequireUserMediationIncognito) { + CredentialManagerOnPreventSilentAccessIncognito) { EXPECT_CALL(*client_, IsSavingAndFillingEnabledForCurrentPage()) .WillRepeatedly(testing::Return(false)); store_->AddLogin(form_); @@ -754,7 +756,7 @@ TEST_F(CredentialManagerImplTest, EXPECT_FALSE(passwords[form_.signon_realm][0].skip_zero_click); bool called = false; - CallRequireUserMediation(base::Bind(&RespondCallback, &called)); + CallPreventSilentAccess(base::Bind(&RespondCallback, &called)); RunAllPendingTasks(); EXPECT_TRUE(called); @@ -766,7 +768,7 @@ TEST_F(CredentialManagerImplTest, } TEST_F(CredentialManagerImplTest, - CredentialManagerOnRequireUserMediationWithAffiliation) { + CredentialManagerOnPreventSilentAccessWithAffiliation) { store_->AddLogin(form_); store_->AddLogin(cross_origin_form_); store_->AddLogin(affiliated_form1_); @@ -791,7 +793,7 @@ TEST_F(CredentialManagerImplTest, EXPECT_FALSE(passwords[affiliated_form2_.signon_realm][0].skip_zero_click); bool called = false; - CallRequireUserMediation(base::Bind(&RespondCallback, &called)); + CallPreventSilentAccess(base::Bind(&RespondCallback, &called)); RunAllPendingTasks(); passwords = store_->stored_passwords(); @@ -811,8 +813,8 @@ TEST_F(CredentialManagerImplTest, .Times(testing::Exactly(0)); EXPECT_CALL(*client_, NotifyUserAutoSigninPtr()).Times(testing::Exactly(0)); - ExpectCredentialType(false, true, federations, - CredentialType::CREDENTIAL_TYPE_EMPTY); + ExpectCredentialType(CredentialMediationRequirement::kOptional, true, + federations, CredentialType::CREDENTIAL_TYPE_EMPTY); } TEST_F(CredentialManagerImplTest, @@ -824,8 +826,8 @@ TEST_F(CredentialManagerImplTest, EXPECT_CALL(*client_, NotifyUserAutoSigninPtr()).Times(testing::Exactly(0)); std::vector<GURL> federations; - ExpectCredentialType(false, true, federations, - CredentialType::CREDENTIAL_TYPE_EMPTY); + ExpectCredentialType(CredentialMediationRequirement::kOptional, true, + federations, CredentialType::CREDENTIAL_TYPE_EMPTY); } TEST_F(CredentialManagerImplTest, @@ -837,7 +839,8 @@ TEST_F(CredentialManagerImplTest, UnorderedElementsAre(Pointee(subdomain_form_)), _, _)); EXPECT_CALL(*client_, NotifyUserAutoSigninPtr()).Times(0); - ExpectCredentialType(false, true, std::vector<GURL>(), + ExpectCredentialType(CredentialMediationRequirement::kOptional, true, + std::vector<GURL>(), CredentialType::CREDENTIAL_TYPE_PASSWORD); } @@ -852,7 +855,8 @@ TEST_F(CredentialManagerImplTest, Pointee(form_)), _, _)); - ExpectCredentialType(false, true, std::vector<GURL>(), + ExpectCredentialType(CredentialMediationRequirement::kOptional, true, + std::vector<GURL>(), CredentialType::CREDENTIAL_TYPE_PASSWORD); } @@ -867,7 +871,8 @@ TEST_F(CredentialManagerImplTest, store_->AddLogin(duplicate); std::vector<GURL> federations; - ExpectZeroClickSignInSuccess(false, true, federations, + ExpectZeroClickSignInSuccess(CredentialMediationRequirement::kOptional, true, + federations, CredentialType::CREDENTIAL_TYPE_PASSWORD); } @@ -910,7 +915,7 @@ TEST_F(CredentialManagerImplTest, base::Optional<CredentialInfo> credential; std::vector<GURL> federations; federations.push_back(GURL("https://google.com/")); - CallGet(false, true, federations, + CallGet(CredentialMediationRequirement::kOptional, true, federations, base::Bind(&GetCredentialCallback, &called, &error, &credential)); RunAllPendingTasks(); @@ -927,8 +932,8 @@ TEST_F(CredentialManagerImplTest, .Times(testing::Exactly(0)); EXPECT_CALL(*client_, NotifyUserAutoSigninPtr()).Times(testing::Exactly(0)); - ExpectCredentialType(false, true, federations, - CredentialType::CREDENTIAL_TYPE_EMPTY); + ExpectCredentialType(CredentialMediationRequirement::kOptional, true, + federations, CredentialType::CREDENTIAL_TYPE_EMPTY); } TEST_F(CredentialManagerImplTest, @@ -944,7 +949,7 @@ TEST_F(CredentialManagerImplTest, bool called = false; mojom::CredentialManagerError error; base::Optional<CredentialInfo> credential; - CallGet(false, true, federations, + CallGet(CredentialMediationRequirement::kOptional, true, federations, base::Bind(&GetCredentialCallback, &called, &error, &credential)); RunAllPendingTasks(); @@ -961,7 +966,8 @@ TEST_F( .Times(testing::Exactly(0)); EXPECT_CALL(*client_, NotifyUserAutoSigninPtr()).Times(testing::Exactly(0)); - ExpectZeroClickSignInFailure(true, true, federations); + ExpectZeroClickSignInFailure(CredentialMediationRequirement::kSilent, true, + federations); } TEST_F(CredentialManagerImplTest, @@ -973,7 +979,8 @@ TEST_F(CredentialManagerImplTest, EXPECT_CALL(*client_, NotifyUserCouldBeAutoSignedInPtr(_)).Times(0); - ExpectZeroClickSignInSuccess(true, true, federations, + ExpectZeroClickSignInSuccess(CredentialMediationRequirement::kSilent, true, + federations, CredentialType::CREDENTIAL_TYPE_PASSWORD); } @@ -986,7 +993,8 @@ TEST_F(CredentialManagerImplTest, EXPECT_CALL(*client_, NotifyUserCouldBeAutoSignedInPtr(_)).Times(0); - ExpectZeroClickSignInFailure(true, false, federations); + ExpectZeroClickSignInFailure(CredentialMediationRequirement::kSilent, false, + federations); } TEST_F(CredentialManagerImplTest, @@ -1001,7 +1009,8 @@ TEST_F(CredentialManagerImplTest, EXPECT_CALL(*client_, NotifyUserCouldBeAutoSignedInPtr(_)).Times(0); - ExpectZeroClickSignInSuccess(true, true, federations, + ExpectZeroClickSignInSuccess(CredentialMediationRequirement::kSilent, true, + federations, CredentialType::CREDENTIAL_TYPE_FEDERATED); } @@ -1017,7 +1026,8 @@ TEST_F(CredentialManagerImplTest, EXPECT_CALL(*client_, NotifyUserCouldBeAutoSignedInPtr(_)).Times(0); - ExpectZeroClickSignInFailure(true, true, federations); + ExpectZeroClickSignInFailure(CredentialMediationRequirement::kSilent, true, + federations); } TEST_F(CredentialManagerImplTest, @@ -1036,7 +1046,8 @@ TEST_F(CredentialManagerImplTest, // We pass in 'true' for the 'include_passwords' argument to ensure that // password-type credentials are included as potential matches. - ExpectZeroClickSignInSuccess(true, true, federations, + ExpectZeroClickSignInSuccess(CredentialMediationRequirement::kSilent, true, + federations, CredentialType::CREDENTIAL_TYPE_PASSWORD); } @@ -1056,7 +1067,8 @@ TEST_F(CredentialManagerImplTest, // We pass in 'false' for the 'include_passwords' argument to ensure that // password-type credentials are excluded as potential matches. - ExpectZeroClickSignInFailure(true, false, federations); + ExpectZeroClickSignInFailure(CredentialMediationRequirement::kSilent, false, + federations); } TEST_F(CredentialManagerImplTest, @@ -1078,7 +1090,8 @@ TEST_F(CredentialManagerImplTest, ->ExpectCallToGetAffiliatedAndroidRealms( cm_service_impl_->GetSynthesizedFormForOrigin(), affiliated_realms); - ExpectZeroClickSignInSuccess(true, true, federations, + ExpectZeroClickSignInSuccess(CredentialMediationRequirement::kSilent, true, + federations, CredentialType::CREDENTIAL_TYPE_FEDERATED); } @@ -1101,7 +1114,8 @@ TEST_F(CredentialManagerImplTest, ->ExpectCallToGetAffiliatedAndroidRealms( cm_service_impl_->GetSynthesizedFormForOrigin(), affiliated_realms); - ExpectZeroClickSignInFailure(true, true, federations); + ExpectZeroClickSignInFailure(CredentialMediationRequirement::kSilent, true, + federations); } TEST_F(CredentialManagerImplTest, RequestCredentialWithoutFirstRun) { @@ -1114,7 +1128,8 @@ TEST_F(CredentialManagerImplTest, RequestCredentialWithoutFirstRun) { NotifyUserCouldBeAutoSignedInPtr(testing::Pointee(form_))) .Times(1); - ExpectZeroClickSignInFailure(true, true, federations); + ExpectZeroClickSignInFailure(CredentialMediationRequirement::kSilent, true, + federations); } TEST_F(CredentialManagerImplTest, RequestCredentialWithFirstRunAndSkip) { @@ -1128,7 +1143,8 @@ TEST_F(CredentialManagerImplTest, RequestCredentialWithFirstRunAndSkip) { NotifyUserCouldBeAutoSignedInPtr(testing::Pointee(form_))) .Times(1); - ExpectZeroClickSignInFailure(true, true, federations); + ExpectZeroClickSignInFailure(CredentialMediationRequirement::kSilent, true, + federations); } TEST_F(CredentialManagerImplTest, RequestCredentialWithTLSErrors) { @@ -1140,7 +1156,8 @@ TEST_F(CredentialManagerImplTest, RequestCredentialWithTLSErrors) { std::vector<GURL> federations; - ExpectZeroClickSignInFailure(true, true, federations); + ExpectZeroClickSignInFailure(CredentialMediationRequirement::kSilent, true, + federations); } TEST_F(CredentialManagerImplTest, RequestCredentialWhilePrerendering) { @@ -1152,7 +1169,8 @@ TEST_F(CredentialManagerImplTest, RequestCredentialWhilePrerendering) { std::vector<GURL> federations; - ExpectZeroClickSignInFailure(true, true, federations); + ExpectZeroClickSignInFailure(CredentialMediationRequirement::kSilent, true, + federations); } TEST_F(CredentialManagerImplTest, @@ -1166,8 +1184,8 @@ TEST_F(CredentialManagerImplTest, EXPECT_CALL(*client_, NotifyUserAutoSigninPtr()).Times(testing::Exactly(0)); // With two items in the password store, we shouldn't get credentials back. - ExpectCredentialType(true, true, federations, - CredentialType::CREDENTIAL_TYPE_EMPTY); + ExpectCredentialType(CredentialMediationRequirement::kSilent, true, + federations, CredentialType::CREDENTIAL_TYPE_EMPTY); } TEST_F(CredentialManagerImplTest, @@ -1183,8 +1201,8 @@ TEST_F(CredentialManagerImplTest, // With two items in the password store, we shouldn't get credentials back, // even though only one item has |skip_zero_click| set |false|. - ExpectCredentialType(true, true, federations, - CredentialType::CREDENTIAL_TYPE_EMPTY); + ExpectCredentialType(CredentialMediationRequirement::kSilent, true, + federations, CredentialType::CREDENTIAL_TYPE_EMPTY); } TEST_F(CredentialManagerImplTest, @@ -1201,8 +1219,8 @@ TEST_F(CredentialManagerImplTest, // We only have cross-origin zero-click credentials; they should not be // returned. - ExpectCredentialType(true, true, federations, - CredentialType::CREDENTIAL_TYPE_EMPTY); + ExpectCredentialType(CredentialMediationRequirement::kSilent, true, + federations, CredentialType::CREDENTIAL_TYPE_EMPTY); } TEST_F(CredentialManagerImplTest, @@ -1220,14 +1238,14 @@ TEST_F(CredentialManagerImplTest, mojom::CredentialManagerError error_1; base::Optional<CredentialInfo> credential_1; CallGet( - false, true, federations, + CredentialMediationRequirement::kOptional, true, federations, base::Bind(&GetCredentialCallback, &called_1, &error_1, &credential_1)); // 2nd request. bool called_2 = false; mojom::CredentialManagerError error_2; base::Optional<CredentialInfo> credential_2; CallGet( - false, true, federations, + CredentialMediationRequirement::kOptional, true, federations, base::Bind(&GetCredentialCallback, &called_2, &error_2, &credential_2)); EXPECT_CALL(*client_, PromptUserToChooseCredentialsPtr(_, _, _)) @@ -1284,7 +1302,7 @@ TEST_F(CredentialManagerImplTest, ResetSkipZeroClickAfterPrompt) { bool called = false; mojom::CredentialManagerError error; base::Optional<CredentialInfo> credential; - CallGet(false, true, federations, + CallGet(CredentialMediationRequirement::kOptional, true, federations, base::Bind(&GetCredentialCallback, &called, &error, &credential)); RunAllPendingTasks(); @@ -1320,7 +1338,7 @@ TEST_F(CredentialManagerImplTest, NoResetSkipZeroClickAfterPromptInIncognito) { bool called = false; mojom::CredentialManagerError error; base::Optional<CredentialInfo> credential; - CallGet(false, true, std::vector<GURL>(), + CallGet(CredentialMediationRequirement::kOptional, true, std::vector<GURL>(), base::Bind(&GetCredentialCallback, &called, &error, &credential)); RunAllPendingTasks(); @@ -1341,8 +1359,8 @@ TEST_F(CredentialManagerImplTest, IncognitoZeroClickRequestCredential) { .Times(testing::Exactly(0)); EXPECT_CALL(*client_, NotifyUserAutoSigninPtr()).Times(testing::Exactly(0)); - ExpectCredentialType(true, true, federations, - CredentialType::CREDENTIAL_TYPE_EMPTY); + ExpectCredentialType(CredentialMediationRequirement::kSilent, true, + federations, CredentialType::CREDENTIAL_TYPE_EMPTY); } TEST_F(CredentialManagerImplTest, ZeroClickWithAffiliatedFormInPasswordStore) { @@ -1361,7 +1379,8 @@ TEST_F(CredentialManagerImplTest, ZeroClickWithAffiliatedFormInPasswordStore) { ->ExpectCallToGetAffiliatedAndroidRealms( cm_service_impl_->GetSynthesizedFormForOrigin(), affiliated_realms); - ExpectZeroClickSignInSuccess(true, true, federations, + ExpectZeroClickSignInSuccess(CredentialMediationRequirement::kSilent, true, + federations, CredentialType::CREDENTIAL_TYPE_PASSWORD); } @@ -1383,7 +1402,8 @@ TEST_F(CredentialManagerImplTest, ->ExpectCallToGetAffiliatedAndroidRealms( cm_service_impl_->GetSynthesizedFormForOrigin(), affiliated_realms); - ExpectZeroClickSignInFailure(true, true, federations); + ExpectZeroClickSignInFailure(CredentialMediationRequirement::kSilent, true, + federations); } TEST_F(CredentialManagerImplTest, @@ -1410,7 +1430,8 @@ TEST_F(CredentialManagerImplTest, ->ExpectCallToGetAffiliatedAndroidRealms(digest, affiliated_realms); std::vector<GURL> federations; - ExpectZeroClickSignInFailure(true, true, federations); + ExpectZeroClickSignInFailure(CredentialMediationRequirement::kSilent, true, + federations); } TEST_F(CredentialManagerImplTest, @@ -1430,7 +1451,8 @@ TEST_F(CredentialManagerImplTest, ->ExpectCallToGetAffiliatedAndroidRealms( cm_service_impl_->GetSynthesizedFormForOrigin(), affiliated_realms); - ExpectZeroClickSignInSuccess(true, true, federations, + ExpectZeroClickSignInSuccess(CredentialMediationRequirement::kSilent, true, + federations, CredentialType::CREDENTIAL_TYPE_PASSWORD); } @@ -1438,7 +1460,8 @@ TEST_F(CredentialManagerImplTest, ZeroClickWithPSLCredential) { subdomain_form_.skip_zero_click = false; store_->AddLogin(subdomain_form_); - ExpectZeroClickSignInFailure(true, true, std::vector<GURL>()); + ExpectZeroClickSignInFailure(CredentialMediationRequirement::kSilent, true, + std::vector<GURL>()); } TEST_F(CredentialManagerImplTest, ZeroClickWithPSLAndNormalCredentials) { @@ -1450,7 +1473,8 @@ TEST_F(CredentialManagerImplTest, ZeroClickWithPSLAndNormalCredentials) { store_->AddLogin(subdomain_form_); std::vector<GURL> federations = {GURL("https://google.com/")}; - ExpectZeroClickSignInSuccess(true, true, federations, + ExpectZeroClickSignInSuccess(CredentialMediationRequirement::kSilent, true, + federations, CredentialType::CREDENTIAL_TYPE_FEDERATED); } @@ -1464,7 +1488,8 @@ TEST_F(CredentialManagerImplTest, ZeroClickAfterMigratingHttpCredential) { store_->AddLogin(form_); std::vector<GURL> federations; - ExpectZeroClickSignInSuccess(true, true, federations, + ExpectZeroClickSignInSuccess(CredentialMediationRequirement::kSilent, true, + federations, CredentialType::CREDENTIAL_TYPE_PASSWORD); } @@ -1473,7 +1498,30 @@ TEST_F(CredentialManagerImplTest, MigrateWithEmptyStore) { NavigateAndCommit(GURL("http://127.0.0.1:8000/")); std::vector<GURL> federations; - ExpectZeroClickSignInFailure(false, true, federations); + ExpectZeroClickSignInFailure(CredentialMediationRequirement::kOptional, true, + federations); +} + +TEST_F(CredentialManagerImplTest, MediationRequiredPreventsAutoSignIn) { + form_.skip_zero_click = false; + store_->AddLogin(form_); + + std::vector<GURL> federations; + bool called = false; + mojom::CredentialManagerError error; + base::Optional<CredentialInfo> credential; + + EXPECT_CALL(*client_, PromptUserToChooseCredentialsPtr(_, _, _)) + .Times(testing::Exactly(1)); + EXPECT_CALL(*client_, NotifyUserAutoSigninPtr()).Times(testing::Exactly(0)); + CallGet(CredentialMediationRequirement::kRequired, true, federations, + base::Bind(&GetCredentialCallback, &called, &error, &credential)); + + RunAllPendingTasks(); + + EXPECT_TRUE(called); + EXPECT_EQ(mojom::CredentialManagerError::SUCCESS, error); + EXPECT_EQ(CredentialType::CREDENTIAL_TYPE_PASSWORD, credential->type); } TEST_F(CredentialManagerImplTest, GetSynthesizedFormForOrigin) { diff --git a/chromium/components/password_manager/content/common/OWNERS b/chromium/components/password_manager/content/common/OWNERS index 154435234ea..2c44a463856 100644 --- a/chromium/components/password_manager/content/common/OWNERS +++ b/chromium/components/password_manager/content/common/OWNERS @@ -2,3 +2,5 @@ per-file *.mojom=set noparent per-file *.mojom=file://ipc/SECURITY_OWNERS per-file *_struct_traits*.*=set noparent per-file *_struct_traits*.*=file://ipc/SECURITY_OWNERS +per-file *.typemap=set noparent +per-file *.typemap=file://ipc/SECURITY_OWNERS diff --git a/chromium/components/password_manager/content/common/credential_manager.mojom b/chromium/components/password_manager/content/common/credential_manager.mojom index cc4981d8bc8..ea7cf47559a 100644 --- a/chromium/components/password_manager/content/common/credential_manager.mojom +++ b/chromium/components/password_manager/content/common/credential_manager.mojom @@ -13,6 +13,12 @@ enum CredentialType { FEDERATED }; +enum CredentialMediationRequirement { + kSilent, + kOptional, + kRequired +}; + enum CredentialManagerError { SUCCESS, DISABLED, @@ -34,13 +40,13 @@ interface CredentialManager { // Store credential. For navigator.credentials.store(). Store(CredentialInfo credential) => (); - // Require user mediation. For navigator.credentials.requireUserMediation(). - RequireUserMediation() => (); + // Require user mediation. For navigator.credentials.preventSilentAccess(). + PreventSilentAccess() => (); // Get Credential. For navigator.credentials.get(). // The result callback will return a non-null and valid CredentialInfo // if succeeded, or null with a CredentialManagerError if failed. - Get(bool zero_click_only, + Get(CredentialMediationRequirement mediation, bool include_passwords, array<url.mojom.Url> federations) => (CredentialManagerError error, CredentialInfo? credential); diff --git a/chromium/components/password_manager/content/common/credential_manager.typemap b/chromium/components/password_manager/content/common/credential_manager.typemap index 180b08170aa..571ba267ca3 100644 --- a/chromium/components/password_manager/content/common/credential_manager.typemap +++ b/chromium/components/password_manager/content/common/credential_manager.typemap @@ -16,5 +16,6 @@ deps = [ type_mappings = [ "password_manager.mojom.CredentialType=password_manager::CredentialType", + "password_manager.mojom.CredentialMediationRequirement=password_manager::CredentialMediationRequirement", "password_manager.mojom.CredentialInfo=password_manager::CredentialInfo", ] diff --git a/chromium/components/password_manager/content/common/credential_manager_struct_traits.cc b/chromium/components/password_manager/content/common/credential_manager_struct_traits.cc index 315ebaf39aa..4ca8787b46d 100644 --- a/chromium/components/password_manager/content/common/credential_manager_struct_traits.cc +++ b/chromium/components/password_manager/content/common/credential_manager_struct_traits.cc @@ -49,6 +49,45 @@ bool EnumTraits<mojom::CredentialType, CredentialType>::FromMojom( } // static +mojom::CredentialMediationRequirement EnumTraits< + mojom::CredentialMediationRequirement, + CredentialMediationRequirement>::ToMojom(CredentialMediationRequirement + input) { + switch (input) { + case CredentialMediationRequirement::kSilent: + return mojom::CredentialMediationRequirement::kSilent; + case CredentialMediationRequirement::kOptional: + return mojom::CredentialMediationRequirement::kOptional; + case CredentialMediationRequirement::kRequired: + return mojom::CredentialMediationRequirement::kRequired; + } + + NOTREACHED(); + return mojom::CredentialMediationRequirement::kOptional; +} + +// static +bool EnumTraits<mojom::CredentialMediationRequirement, + CredentialMediationRequirement>:: + FromMojom(mojom::CredentialMediationRequirement input, + CredentialMediationRequirement* output) { + switch (input) { + case mojom::CredentialMediationRequirement::kSilent: + *output = CredentialMediationRequirement::kSilent; + return true; + case mojom::CredentialMediationRequirement::kOptional: + *output = CredentialMediationRequirement::kOptional; + return true; + case mojom::CredentialMediationRequirement::kRequired: + *output = CredentialMediationRequirement::kRequired; + return true; + } + + NOTREACHED(); + return false; +} + +// static bool StructTraits<mojom::CredentialInfoDataView, CredentialInfo>::Read( mojom::CredentialInfoDataView data, CredentialInfo* out) { diff --git a/chromium/components/password_manager/content/common/credential_manager_struct_traits.h b/chromium/components/password_manager/content/common/credential_manager_struct_traits.h index cf8c5a7c33e..aa544b782ce 100644 --- a/chromium/components/password_manager/content/common/credential_manager_struct_traits.h +++ b/chromium/components/password_manager/content/common/credential_manager_struct_traits.h @@ -22,6 +22,16 @@ struct EnumTraits<password_manager::mojom::CredentialType, }; template <> +struct EnumTraits<password_manager::mojom::CredentialMediationRequirement, + password_manager::CredentialMediationRequirement> { + static password_manager::mojom::CredentialMediationRequirement ToMojom( + password_manager::CredentialMediationRequirement input); + static bool FromMojom( + password_manager::mojom::CredentialMediationRequirement input, + password_manager::CredentialMediationRequirement* output); +}; + +template <> struct StructTraits<password_manager::mojom::CredentialInfoDataView, password_manager::CredentialInfo> { static password_manager::CredentialType type( diff --git a/chromium/components/password_manager/content/renderer/credential_manager_client.cc b/chromium/components/password_manager/content/renderer/credential_manager_client.cc index b859abb1185..6a6d0b2eba4 100644 --- a/chromium/components/password_manager/content/renderer/credential_manager_client.cc +++ b/chromium/components/password_manager/content/renderer/credential_manager_client.cc @@ -13,9 +13,9 @@ #include "base/logging.h" #include "base/memory/ptr_util.h" #include "components/password_manager/core/common/credential_manager_types.h" +#include "content/public/common/associated_interface_provider.h" #include "content/public/renderer/render_frame.h" #include "content/public/renderer/render_view.h" -#include "services/service_manager/public/cpp/interface_provider.h" #include "third_party/WebKit/public/platform/WebCredential.h" #include "third_party/WebKit/public/platform/WebCredentialManagerError.h" #include "third_party/WebKit/public/platform/WebFederatedCredential.h" @@ -65,6 +65,21 @@ std::unique_ptr<blink::WebCredential> CredentialInfoToWebCredential( return nullptr; } +CredentialMediationRequirement GetCredentialMediationRequirementFromBlink( + blink::WebCredentialMediationRequirement mediation) { + switch (mediation) { + case blink::WebCredentialMediationRequirement::kSilent: + return CredentialMediationRequirement::kSilent; + case blink::WebCredentialMediationRequirement::kOptional: + return CredentialMediationRequirement::kOptional; + case blink::WebCredentialMediationRequirement::kRequired: + return CredentialMediationRequirement::kRequired; + } + + NOTREACHED(); + return CredentialMediationRequirement::kOptional; +} + blink::WebCredentialManagerError GetWebCredentialManagerErrorFromMojo( mojom::CredentialManagerError error) { switch (error) { @@ -216,18 +231,18 @@ void CredentialManagerClient::DispatchStore( base::Owned(new NotificationCallbacksWrapper(callbacks)))); } -void CredentialManagerClient::DispatchRequireUserMediation( +void CredentialManagerClient::DispatchPreventSilentAccess( blink::WebCredentialManagerClient::NotificationCallbacks* callbacks) { DCHECK(callbacks); ConnectToMojoCMIfNeeded(); - mojo_cm_service_->RequireUserMediation( + mojo_cm_service_->PreventSilentAccess( base::Bind(&RespondToNotificationCallback, base::Owned(new NotificationCallbacksWrapper(callbacks)))); } void CredentialManagerClient::DispatchGet( - bool zero_click_only, + blink::WebCredentialMediationRequirement mediation, bool include_passwords, const blink::WebVector<blink::WebURL>& federations, RequestCallbacks* callbacks) { @@ -239,7 +254,8 @@ void CredentialManagerClient::DispatchGet( federation_vector.push_back(federations[i]); mojo_cm_service_->Get( - zero_click_only, include_passwords, federation_vector, + GetCredentialMediationRequirementFromBlink(mediation), include_passwords, + federation_vector, base::Bind(&RespondToRequestCallback, base::Owned(new RequestCallbacksWrapper(callbacks)))); } @@ -249,7 +265,18 @@ void CredentialManagerClient::ConnectToMojoCMIfNeeded() { return; content::RenderFrame* main_frame = render_view()->GetMainRenderFrame(); - main_frame->GetRemoteInterfaces()->GetInterface(&mojo_cm_service_); + main_frame->GetRemoteAssociatedInterfaces()->GetInterface(&mojo_cm_service_); + + // The remote end of the pipe will be forcibly closed by the browser side + // after each main frame navigation. Set up an error handler to reset the + // local end so that there will be an attempt at reestablishing the connection + // at the next call to the API. + mojo_cm_service_.set_connection_error_handler(base::Bind( + &CredentialManagerClient::OnMojoConnectionError, base::Unretained(this))); +} + +void CredentialManagerClient::OnMojoConnectionError() { + mojo_cm_service_.reset(); } void CredentialManagerClient::OnDestruct() { diff --git a/chromium/components/password_manager/content/renderer/credential_manager_client.h b/chromium/components/password_manager/content/renderer/credential_manager_client.h index 388ed941ec8..513b389ecd2 100644 --- a/chromium/components/password_manager/content/renderer/credential_manager_client.h +++ b/chromium/components/password_manager/content/renderer/credential_manager_client.h @@ -11,6 +11,7 @@ #include "content/public/renderer/render_view_observer.h" #include "third_party/WebKit/public/platform/WebCredentialManagerClient.h" #include "third_party/WebKit/public/platform/WebCredentialManagerError.h" +#include "third_party/WebKit/public/platform/WebCredentialMediationRequirement.h" #include "third_party/WebKit/public/platform/WebVector.h" namespace blink { @@ -46,8 +47,8 @@ class CredentialManagerClient : public blink::WebCredentialManagerClient, void DispatchStore( const blink::WebCredential& credential, WebCredentialManagerClient::NotificationCallbacks* callbacks) override; - void DispatchRequireUserMediation(NotificationCallbacks* callbacks) override; - void DispatchGet(bool zero_click_only, + void DispatchPreventSilentAccess(NotificationCallbacks* callbacks) override; + void DispatchGet(blink::WebCredentialMediationRequirement mediation, bool include_passwords, const blink::WebVector<blink::WebURL>& federations, RequestCallbacks* callbacks) override; @@ -57,8 +58,9 @@ class CredentialManagerClient : public blink::WebCredentialManagerClient, void OnDestruct() override; void ConnectToMojoCMIfNeeded(); + void OnMojoConnectionError(); - mojom::CredentialManagerPtr mojo_cm_service_; + mojom::CredentialManagerAssociatedPtr mojo_cm_service_; DISALLOW_COPY_AND_ASSIGN(CredentialManagerClient); }; diff --git a/chromium/components/password_manager/content/renderer/credential_manager_client_browsertest.cc b/chromium/components/password_manager/content/renderer/credential_manager_client_browsertest.cc index 8b25db6779c..e384b24c885 100644 --- a/chromium/components/password_manager/content/renderer/credential_manager_client_browsertest.cc +++ b/chromium/components/password_manager/content/renderer/credential_manager_client_browsertest.cc @@ -8,20 +8,23 @@ #include <memory> #include <tuple> +#include <utility> #include "base/location.h" +#include "base/message_loop/message_loop.h" #include "base/run_loop.h" #include "base/single_thread_task_runner.h" #include "base/threading/thread_task_runner_handle.h" +#include "content/public/common/associated_interface_provider.h" #include "content/public/renderer/render_frame.h" #include "content/public/renderer/render_view.h" #include "content/public/test/render_view_test.h" -#include "mojo/public/cpp/bindings/binding_set.h" -#include "services/service_manager/public/cpp/interface_provider.h" +#include "mojo/public/cpp/bindings/associated_binding_set.h" #include "testing/gtest/include/gtest/gtest.h" #include "third_party/WebKit/public/platform/WebCredential.h" #include "third_party/WebKit/public/platform/WebCredentialManagerClient.h" #include "third_party/WebKit/public/platform/WebCredentialManagerError.h" +#include "third_party/WebKit/public/platform/WebCredentialMediationRequirement.h" #include "third_party/WebKit/public/platform/WebPasswordCredential.h" namespace password_manager { @@ -37,41 +40,42 @@ class FakeCredentialManager : public mojom::CredentialManager { FakeCredentialManager() {} ~FakeCredentialManager() override {} - void BindRequest(mojom::CredentialManagerRequest request) { + void BindRequest(mojom::CredentialManagerAssociatedRequest request) { bindings_.AddBinding(this, std::move(request)); } private: // mojom::CredentialManager methods: void Store(const CredentialInfo& credential, - const StoreCallback& callback) override { - callback.Run(); + StoreCallback callback) override { + std::move(callback).Run(); } - void RequireUserMediation( - const RequireUserMediationCallback& callback) override { - callback.Run(); + void PreventSilentAccess(PreventSilentAccessCallback callback) override { + std::move(callback).Run(); } - void Get(bool zero_click_only, + void Get(CredentialMediationRequirement mediation, bool include_passwords, const std::vector<GURL>& federations, - const GetCallback& callback) override { + GetCallback callback) override { const std::string& url = federations[0].spec(); if (url == kTestCredentialPassword) { CredentialInfo info; info.type = CredentialType::CREDENTIAL_TYPE_PASSWORD; - callback.Run(mojom::CredentialManagerError::SUCCESS, info); + std::move(callback).Run(mojom::CredentialManagerError::SUCCESS, info); } else if (url == kTestCredentialEmpty) { - callback.Run(mojom::CredentialManagerError::SUCCESS, CredentialInfo()); + std::move(callback).Run(mojom::CredentialManagerError::SUCCESS, + CredentialInfo()); } else if (url == kTestCredentialReject) { - callback.Run(mojom::CredentialManagerError::PASSWORDSTOREUNAVAILABLE, - base::nullopt); + std::move(callback).Run( + mojom::CredentialManagerError::PASSWORDSTOREUNAVAILABLE, + base::nullopt); } } - mojo::BindingSet<mojom::CredentialManager> bindings_; + mojo::AssociatedBindingSet<mojom::CredentialManager> bindings_; }; class CredentialManagerClientTest : public content::RenderViewTest { @@ -84,10 +88,9 @@ class CredentialManagerClientTest : public content::RenderViewTest { content::RenderViewTest::SetUp(); client_.reset(new CredentialManagerClient(view_)); - service_manager::InterfaceProvider* remote_interfaces = - view_->GetMainRenderFrame()->GetRemoteInterfaces(); - service_manager::InterfaceProvider::TestApi test_api(remote_interfaces); - test_api.SetBinderForName( + content::AssociatedInterfaceProvider* remote_interfaces = + view_->GetMainRenderFrame()->GetRemoteAssociatedInterfaces(); + remote_interfaces->OverrideBinderForTesting( mojom::CredentialManager::Name_, base::Bind(&CredentialManagerClientTest::BindCredentialManager, base::Unretained(this))); @@ -104,9 +107,9 @@ class CredentialManagerClientTest : public content::RenderViewTest { bool callback_succeeded() const { return callback_succeeded_; } void set_callback_succeeded(bool state) { callback_succeeded_ = state; } - void BindCredentialManager(mojo::ScopedMessagePipeHandle handle) { + void BindCredentialManager(mojo::ScopedInterfaceEndpointHandle handle) { fake_cm_.BindRequest( - mojo::MakeRequest<mojom::CredentialManager>(std::move(handle))); + mojom::CredentialManagerAssociatedRequest(std::move(handle))); } std::unique_ptr<blink::WebPasswordCredential> credential_; @@ -193,7 +196,7 @@ TEST_F(CredentialManagerClientTest, SendStore) { TEST_F(CredentialManagerClientTest, SendRequestUserMediation) { std::unique_ptr<TestNotificationCallbacks> callbacks( new TestNotificationCallbacks(this)); - client_->DispatchRequireUserMediation(callbacks.release()); + client_->DispatchPreventSilentAccess(callbacks.release()); RunAllPendingTasks(); @@ -206,7 +209,8 @@ TEST_F(CredentialManagerClientTest, SendRequestCredential) { new TestRequestCallbacks(this)); std::vector<GURL> federations; federations.push_back(GURL(kTestCredentialPassword)); - client_->DispatchGet(false, true, federations, callbacks.release()); + client_->DispatchGet(blink::WebCredentialMediationRequirement::kOptional, + true, federations, callbacks.release()); RunAllPendingTasks(); @@ -221,7 +225,8 @@ TEST_F(CredentialManagerClientTest, SendRequestCredentialEmpty) { new TestRequestCallbacks(this)); std::vector<GURL> federations; federations.push_back(GURL(kTestCredentialEmpty)); - client_->DispatchGet(false, true, federations, callbacks.release()); + client_->DispatchGet(blink::WebCredentialMediationRequirement::kOptional, + true, federations, callbacks.release()); RunAllPendingTasks(); @@ -235,7 +240,8 @@ TEST_F(CredentialManagerClientTest, SendRequestCredentialReject) { new TestRequestCallbacks(this)); std::vector<GURL> federations; federations.push_back(GURL(kTestCredentialReject)); - client_->DispatchGet(false, true, federations, callbacks.release()); + client_->DispatchGet(blink::WebCredentialMediationRequirement::kOptional, + true, federations, callbacks.release()); RunAllPendingTasks(); diff --git a/chromium/components/password_manager/core/browser/BUILD.gn b/chromium/components/password_manager/core/browser/BUILD.gn index d7be596e6c7..056d7d6164c 100644 --- a/chromium/components/password_manager/core/browser/BUILD.gn +++ b/chromium/components/password_manager/core/browser/BUILD.gn @@ -35,10 +35,10 @@ static_library("browser") { "credential_manager_logger.h", "credential_manager_password_form_manager.cc", "credential_manager_password_form_manager.h", + "credential_manager_pending_prevent_silent_access_task.cc", + "credential_manager_pending_prevent_silent_access_task.h", "credential_manager_pending_request_task.cc", "credential_manager_pending_request_task.h", - "credential_manager_pending_require_user_mediation_task.cc", - "credential_manager_pending_require_user_mediation_task.h", "credentials_filter.h", "export/csv_writer.cc", "export/csv_writer.h", @@ -122,6 +122,8 @@ static_library("browser") { "sql_table_builder.h", "statistics_table.cc", "statistics_table.h", + "suppressed_form_fetcher.cc", + "suppressed_form_fetcher.h", "test_affiliation_fetcher_factory.h", "webdata/logins_table.cc", "webdata/logins_table.h", @@ -163,6 +165,7 @@ static_library("browser") { "//components/url_formatter", "//components/variations", "//components/webdata/common", + "//crypto", "//google_apis", "//net", "//sql", @@ -319,6 +322,7 @@ source_set("unit_tests") { "psl_matching_helper_unittest.cc", "sql_table_builder_unittest.cc", "statistics_table_unittest.cc", + "suppressed_form_fetcher_unittest.cc", ] if (is_mac) { sources -= [ "password_store_default_unittest.cc" ] diff --git a/chromium/components/password_manager/core/browser/DEPS b/chromium/components/password_manager/core/browser/DEPS index 2b537df53bd..d4cbab297ac 100644 --- a/chromium/components/password_manager/core/browser/DEPS +++ b/chromium/components/password_manager/core/browser/DEPS @@ -9,6 +9,7 @@ include_rules = [ "+components/url_formatter", "+components/variations", "+components/webdata/common", + "+crypto", "+google_apis", "+grit", ] diff --git a/chromium/components/password_manager/core/browser/affiliation_fetcher.cc b/chromium/components/password_manager/core/browser/affiliation_fetcher.cc index 15eae8c99d6..fb486e7bb93 100644 --- a/chromium/components/password_manager/core/browser/affiliation_fetcher.cc +++ b/chromium/components/password_manager/core/browser/affiliation_fetcher.cc @@ -119,7 +119,7 @@ void AffiliationFetcher::StartRequest() { chrome_policy { SyncDisabled { policy_options {mode: MANDATORY} - SyncDisabled: True + SyncDisabled: true } } })"); diff --git a/chromium/components/password_manager/core/browser/browser_save_password_progress_logger.cc b/chromium/components/password_manager/core/browser/browser_save_password_progress_logger.cc index e2523ec7a17..c4d79d28522 100644 --- a/chromium/components/password_manager/core/browser/browser_save_password_progress_logger.cc +++ b/chromium/components/password_manager/core/browser/browser_save_password_progress_logger.cc @@ -164,6 +164,16 @@ void BrowserSavePasswordProgressLogger::LogString(StringID label, LogValue(label, base::Value(s)); } +void BrowserSavePasswordProgressLogger::LogSuccessfulSubmissionIndicatorEvent( + autofill::PasswordForm::SubmissionIndicatorEvent event) { + std::ostringstream submission_event_string_stream; + submission_event_string_stream << event; + std::string message = + GetStringFromID(STRING_SUCCESSFUL_SUBMISSION_INDICATOR_EVENT) + ": " + + submission_event_string_stream.str(); + SendLog(message); +} + void BrowserSavePasswordProgressLogger::SendLog(const std::string& log) { log_manager_->LogSavePasswordProgress(log); } diff --git a/chromium/components/password_manager/core/browser/browser_save_password_progress_logger.h b/chromium/components/password_manager/core/browser/browser_save_password_progress_logger.h index d5c94232dec..de2f893b0b5 100644 --- a/chromium/components/password_manager/core/browser/browser_save_password_progress_logger.h +++ b/chromium/components/password_manager/core/browser/browser_save_password_progress_logger.h @@ -8,6 +8,7 @@ #include <string> #include "base/macros.h" +#include "components/autofill/core/common/password_form.h" #include "components/autofill/core/common/save_password_progress_logger.h" #include "url/gurl.h" @@ -45,6 +46,10 @@ class BrowserSavePasswordProgressLogger // passed to SendLog for display. void LogString(StringID label, const std::string& s); + // Log a password successful submission event. + void LogSuccessfulSubmissionIndicatorEvent( + autofill::PasswordForm::SubmissionIndicatorEvent event); + protected: // autofill::SavePasswordProgressLogger: void SendLog(const std::string& log) override; diff --git a/chromium/components/password_manager/core/browser/credential_manager_logger.cc b/chromium/components/password_manager/core/browser/credential_manager_logger.cc index f5173c1868a..ac083ae6b32 100644 --- a/chromium/components/password_manager/core/browser/credential_manager_logger.cc +++ b/chromium/components/password_manager/core/browser/credential_manager_logger.cc @@ -21,11 +21,22 @@ CredentialManagerLogger::~CredentialManagerLogger() = default; void CredentialManagerLogger::LogRequestCredential( const GURL& url, - bool zero_click_only, + CredentialMediationRequirement mediation, const std::vector<GURL>& federations) { std::string s("CM API get credentials: origin=" + SavePasswordProgressLogger::ScrubURL(url)); - s += ", zero_click_only=" + base::IntToString(zero_click_only); + s += ", mediation="; + switch (mediation) { + case CredentialMediationRequirement::kSilent: + s += "silent"; + break; + case CredentialMediationRequirement::kOptional: + s += "optional"; + break; + case CredentialMediationRequirement::kRequired: + s += "required"; + break; + } s += ", federations="; for (const GURL& federation_provider : federations) s += SavePasswordProgressLogger::ScrubURL(federation_provider) + ", "; @@ -49,7 +60,7 @@ void CredentialManagerLogger::LogStoreCredential(const GURL& url, log_manager_->LogSavePasswordProgress(s); } -void CredentialManagerLogger::LogRequireUserMediation(const GURL& url) { +void CredentialManagerLogger::LogPreventSilentAccess(const GURL& url) { std::string s("CM API sign out: origin=" + SavePasswordProgressLogger::ScrubURL(url)); log_manager_->LogSavePasswordProgress(s); diff --git a/chromium/components/password_manager/core/browser/credential_manager_logger.h b/chromium/components/password_manager/core/browser/credential_manager_logger.h index c99f7f692a3..ff342e572e9 100644 --- a/chromium/components/password_manager/core/browser/credential_manager_logger.h +++ b/chromium/components/password_manager/core/browser/credential_manager_logger.h @@ -23,11 +23,11 @@ class CredentialManagerLogger { ~CredentialManagerLogger(); void LogRequestCredential(const GURL& url, - bool zero_click_only, + CredentialMediationRequirement mediation, const std::vector<GURL>& federations); void LogSendCredential(const GURL& url, CredentialType type); void LogStoreCredential(const GURL& url, CredentialType type); - void LogRequireUserMediation(const GURL& url); + void LogPreventSilentAccess(const GURL& url); private: // The LogManager to which logs can be sent for display. diff --git a/chromium/components/password_manager/core/browser/credential_manager_logger_unittest.cc b/chromium/components/password_manager/core/browser/credential_manager_logger_unittest.cc index af6a98faba5..74e31a547f5 100644 --- a/chromium/components/password_manager/core/browser/credential_manager_logger_unittest.cc +++ b/chromium/components/password_manager/core/browser/credential_manager_logger_unittest.cc @@ -53,7 +53,8 @@ TEST_F(CredentialManagerLoggerTest, LogRequestCredential) { EXPECT_CALL(log_manager(), LogSavePasswordProgress( AllOf(HasSubstr(kSiteOrigin), HasSubstr(kFederationOrigin)))); - logger().LogRequestCredential(GURL(kSiteOrigin), true, + logger().LogRequestCredential(GURL(kSiteOrigin), + CredentialMediationRequirement::kSilent, {GURL(kFederationOrigin)}); } @@ -69,9 +70,9 @@ TEST_F(CredentialManagerLoggerTest, LogStoreCredential) { CredentialType::CREDENTIAL_TYPE_PASSWORD); } -TEST_F(CredentialManagerLoggerTest, LogRequireUserMediation) { +TEST_F(CredentialManagerLoggerTest, LogPreventSilentAccess) { EXPECT_CALL(log_manager(), LogSavePasswordProgress(HasSubstr(kSiteOrigin))); - logger().LogRequireUserMediation(GURL(kSiteOrigin)); + logger().LogPreventSilentAccess(GURL(kSiteOrigin)); } } // namespace diff --git a/chromium/components/password_manager/core/browser/credential_manager_pending_require_user_mediation_task.cc b/chromium/components/password_manager/core/browser/credential_manager_pending_prevent_silent_access_task.cc index 79cfe6b1816..b0e1ab14608 100644 --- a/chromium/components/password_manager/core/browser/credential_manager_pending_require_user_mediation_task.cc +++ b/chromium/components/password_manager/core/browser/credential_manager_pending_prevent_silent_access_task.cc @@ -2,29 +2,28 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "components/password_manager/core/browser/credential_manager_pending_require_user_mediation_task.h" +#include "components/password_manager/core/browser/credential_manager_pending_prevent_silent_access_task.h" #include "components/autofill/core/common/password_form.h" namespace password_manager { -CredentialManagerPendingRequireUserMediationTask:: - CredentialManagerPendingRequireUserMediationTask( - CredentialManagerPendingRequireUserMediationTaskDelegate* delegate) +CredentialManagerPendingPreventSilentAccessTask:: + CredentialManagerPendingPreventSilentAccessTask( + CredentialManagerPendingPreventSilentAccessTaskDelegate* delegate) : delegate_(delegate), pending_requests_(0) {} -CredentialManagerPendingRequireUserMediationTask:: - ~CredentialManagerPendingRequireUserMediationTask() = default; +CredentialManagerPendingPreventSilentAccessTask:: + ~CredentialManagerPendingPreventSilentAccessTask() = default; -void CredentialManagerPendingRequireUserMediationTask::AddOrigin( +void CredentialManagerPendingPreventSilentAccessTask::AddOrigin( const PasswordStore::FormDigest& form_digest) { delegate_->GetPasswordStore()->GetLogins(form_digest, this); pending_requests_++; } -void CredentialManagerPendingRequireUserMediationTask:: - OnGetPasswordStoreResults( - std::vector<std::unique_ptr<autofill::PasswordForm>> results) { +void CredentialManagerPendingPreventSilentAccessTask::OnGetPasswordStoreResults( + std::vector<std::unique_ptr<autofill::PasswordForm>> results) { PasswordStore* store = delegate_->GetPasswordStore(); for (const auto& form : results) { if (!form->skip_zero_click) { diff --git a/chromium/components/password_manager/core/browser/credential_manager_pending_require_user_mediation_task.h b/chromium/components/password_manager/core/browser/credential_manager_pending_prevent_silent_access_task.h index a703bc5c0a7..38fcb4ee3dd 100644 --- a/chromium/components/password_manager/core/browser/credential_manager_pending_require_user_mediation_task.h +++ b/chromium/components/password_manager/core/browser/credential_manager_pending_prevent_silent_access_task.h @@ -2,8 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_CREDENTIAL_MANAGER_PENDING_REQUIRE_USER_MEDIATION_TASK_H_ -#define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_CREDENTIAL_MANAGER_PENDING_REQUIRE_USER_MEDIATION_TASK_H_ +#ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_CREDENTIAL_MANAGER_PENDING_PREVENT_SILENT_ACCESS_TASK_H_ +#define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_CREDENTIAL_MANAGER_PENDING_PREVENT_SILENT_ACCESS_TASK_H_ #include "base/macros.h" #include "components/password_manager/core/browser/password_store.h" @@ -12,9 +12,9 @@ namespace password_manager { // Handles mediation completion and retrieves embedder-dependent services. -class CredentialManagerPendingRequireUserMediationTaskDelegate { +class CredentialManagerPendingPreventSilentAccessTaskDelegate { public: - virtual ~CredentialManagerPendingRequireUserMediationTaskDelegate() {} + virtual ~CredentialManagerPendingPreventSilentAccessTaskDelegate() {} // Retrieves the PasswordStore. virtual PasswordStore* GetPasswordStore() = 0; @@ -24,12 +24,12 @@ class CredentialManagerPendingRequireUserMediationTaskDelegate { }; // Notifies the password store that a list of origins require user mediation. -class CredentialManagerPendingRequireUserMediationTask +class CredentialManagerPendingPreventSilentAccessTask : public PasswordStoreConsumer { public: - explicit CredentialManagerPendingRequireUserMediationTask( - CredentialManagerPendingRequireUserMediationTaskDelegate* delegate); - ~CredentialManagerPendingRequireUserMediationTask() override; + explicit CredentialManagerPendingPreventSilentAccessTask( + CredentialManagerPendingPreventSilentAccessTaskDelegate* delegate); + ~CredentialManagerPendingPreventSilentAccessTask() override; // Adds an origin to require user mediation. void AddOrigin(const PasswordStore::FormDigest& form_digest); @@ -39,15 +39,15 @@ class CredentialManagerPendingRequireUserMediationTask void OnGetPasswordStoreResults( std::vector<std::unique_ptr<autofill::PasswordForm>> results) override; - CredentialManagerPendingRequireUserMediationTaskDelegate* const + CredentialManagerPendingPreventSilentAccessTaskDelegate* const delegate_; // Weak. // Number of password store requests to be resolved. int pending_requests_; - DISALLOW_COPY_AND_ASSIGN(CredentialManagerPendingRequireUserMediationTask); + DISALLOW_COPY_AND_ASSIGN(CredentialManagerPendingPreventSilentAccessTask); }; } // namespace password_manager -#endif // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_CREDENTIAL_MANAGER_PENDING_REQUIRE_USER_MEDIATION_TASK_H_ +#endif // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_CREDENTIAL_MANAGER_PENDING_PREVENT_SILENT_ACCESS_TASK_H_ diff --git a/chromium/components/password_manager/core/browser/credential_manager_pending_request_task.cc b/chromium/components/password_manager/core/browser/credential_manager_pending_request_task.cc index cd89fb0e894..6901dc02dfe 100644 --- a/chromium/components/password_manager/core/browser/credential_manager_pending_request_task.cc +++ b/chromium/components/password_manager/core/browser/credential_manager_pending_request_task.cc @@ -113,12 +113,12 @@ void FilterDuplicatesAndEmptyUsername( CredentialManagerPendingRequestTask::CredentialManagerPendingRequestTask( CredentialManagerPendingRequestTaskDelegate* delegate, const SendCredentialCallback& callback, - bool request_zero_click_only, + CredentialMediationRequirement mediation, bool include_passwords, const std::vector<GURL>& request_federations) : delegate_(delegate), send_callback_(callback), - zero_click_only_(request_zero_click_only), + mediation_(mediation), origin_(delegate_->GetOrigin()), include_passwords_(include_passwords) { CHECK(!delegate_->client()->DidLastPageLoadEncounterSSLErrors()); @@ -149,12 +149,9 @@ void CredentialManagerPendingRequestTask::ProcessMigratedForms( void CredentialManagerPendingRequestTask::ProcessForms( std::vector<std::unique_ptr<autofill::PasswordForm>> results) { using metrics_util::LogCredentialManagerGetResult; - metrics_util::CredentialManagerGetMediation mediation_status = - zero_click_only_ ? metrics_util::CREDENTIAL_MANAGER_GET_UNMEDIATED - : metrics_util::CREDENTIAL_MANAGER_GET_MEDIATED; if (delegate_->GetOrigin() != origin_) { LogCredentialManagerGetResult(metrics_util::CREDENTIAL_MANAGER_GET_NONE, - mediation_status); + mediation_); delegate_->SendCredential(send_callback_, CredentialInfo()); return; } @@ -187,13 +184,15 @@ void CredentialManagerPendingRequestTask::ProcessForms( FilterDuplicatesAndEmptyUsername(&local_results, &has_empty_username, &has_duplicates); - // We only perform zero-click sign-in when the result is completely - // unambigious. If there is one and only one entry, and zero-click is + // We only perform zero-click sign-in when it is not forbidden via the + // mediation requirement and the result is completely unambigious. + // If there is one and only one entry, and zero-click is // enabled for that entry, return it. // // Moreover, we only return such a credential if the user has opted-in via the // first-run experience. const bool can_use_autosignin = + mediation_ != CredentialMediationRequirement::kRequired && local_results.size() == 1u && delegate_->IsZeroClickAllowed(); if (can_use_autosignin && !local_results[0]->skip_zero_click && !password_bubble_experiment::ShouldShowAutoSignInPromptFirstRunExperience( @@ -206,12 +205,12 @@ void CredentialManagerPendingRequestTask::ProcessForms( origin_); base::RecordAction(base::UserMetricsAction("CredentialManager_Autosignin")); LogCredentialManagerGetResult( - metrics_util::CREDENTIAL_MANAGER_GET_AUTOSIGNIN, mediation_status); + metrics_util::CREDENTIAL_MANAGER_GET_AUTOSIGNIN, mediation_); delegate_->SendCredential(send_callback_, info); return; } - if (zero_click_only_) { + if (mediation_ == CredentialMediationRequirement::kSilent) { metrics_util::CredentialManagerGetResult get_result; if (local_results.empty()) get_result = metrics_util::CREDENTIAL_MANAGER_GET_NONE_EMPTY_STORE; @@ -231,7 +230,7 @@ void CredentialManagerPendingRequestTask::ProcessForms( std::move(local_results[0])); } - LogCredentialManagerGetResult(get_result, mediation_status); + LogCredentialManagerGetResult(get_result, mediation_); delegate_->SendCredential(send_callback_, CredentialInfo()); return; } @@ -246,8 +245,7 @@ void CredentialManagerPendingRequestTask::ProcessForms( if (local_results.empty()) { LogCredentialManagerGetResult( - metrics_util::CREDENTIAL_MANAGER_GET_NONE_EMPTY_STORE, - mediation_status); + metrics_util::CREDENTIAL_MANAGER_GET_NONE_EMPTY_STORE, mediation_); delegate_->SendCredential(send_callback_, CredentialInfo()); return; } @@ -258,9 +256,9 @@ void CredentialManagerPendingRequestTask::ProcessForms( std::move(local_results), origin_, base::Bind( &CredentialManagerPendingRequestTaskDelegate::SendPasswordForm, - base::Unretained(delegate_), send_callback_))) { + base::Unretained(delegate_), send_callback_, mediation_))) { LogCredentialManagerGetResult(metrics_util::CREDENTIAL_MANAGER_GET_NONE, - mediation_status); + mediation_); delegate_->SendCredential(send_callback_, CredentialInfo()); } } diff --git a/chromium/components/password_manager/core/browser/credential_manager_pending_request_task.h b/chromium/components/password_manager/core/browser/credential_manager_pending_request_task.h index a444a723130..6c31ab0cbac 100644 --- a/chromium/components/password_manager/core/browser/credential_manager_pending_request_task.h +++ b/chromium/components/password_manager/core/browser/credential_manager_pending_request_task.h @@ -15,6 +15,7 @@ #include "components/password_manager/core/browser/http_password_store_migrator.h" #include "components/password_manager/core/browser/password_store.h" #include "components/password_manager/core/browser/password_store_consumer.h" +#include "components/password_manager/core/common/credential_manager_types.h" #include "url/gurl.h" namespace autofill { @@ -49,6 +50,7 @@ class CredentialManagerPendingRequestTaskDelegate { // Updates |skip_zero_click| for |form| in the PasswordStore if required. // Sends a credential to JavaScript. virtual void SendPasswordForm(const SendCredentialCallback& send_callback, + CredentialMediationRequirement mediation, const autofill::PasswordForm* form) = 0; }; @@ -60,7 +62,7 @@ class CredentialManagerPendingRequestTask CredentialManagerPendingRequestTask( CredentialManagerPendingRequestTaskDelegate* delegate, const SendCredentialCallback& callback, - bool request_zero_click_only, + CredentialMediationRequirement mediation, bool include_passwords, const std::vector<GURL>& request_federations); ~CredentialManagerPendingRequestTask() override; @@ -82,7 +84,7 @@ class CredentialManagerPendingRequestTask CredentialManagerPendingRequestTaskDelegate* delegate_; // Weak; SendCredentialCallback send_callback_; - const bool zero_click_only_; + const CredentialMediationRequirement mediation_; const GURL origin_; const bool include_passwords_; std::set<std::string> federations_; diff --git a/chromium/components/password_manager/core/browser/fake_form_fetcher.cc b/chromium/components/password_manager/core/browser/fake_form_fetcher.cc index e2f13a896c3..7e7911c3eee 100644 --- a/chromium/components/password_manager/core/browser/fake_form_fetcher.cc +++ b/chromium/components/password_manager/core/browser/fake_form_fetcher.cc @@ -37,6 +37,25 @@ FakeFormFetcher::GetFederatedMatches() const { return federated_; } +const std::vector<const PasswordForm*>& +FakeFormFetcher::GetSuppressedHTTPSForms() const { + return suppressed_https_forms_; +} + +const std::vector<const autofill::PasswordForm*>& +FakeFormFetcher::GetSuppressedPSLMatchingForms() const { + return suppressed_psl_matching_forms_; +} + +const std::vector<const autofill::PasswordForm*>& +FakeFormFetcher::GetSuppressedSameOrganizationNameForms() const { + return suppressed_same_organization_name_forms_; +} + +bool FakeFormFetcher::DidCompleteQueryingSuppressedForms() const { + return did_complete_querying_suppressed_forms_; +} + void FakeFormFetcher::SetNonFederated( const std::vector<const autofill::PasswordForm*>& non_federated, size_t filtered_count) { diff --git a/chromium/components/password_manager/core/browser/fake_form_fetcher.h b/chromium/components/password_manager/core/browser/fake_form_fetcher.h index 891291d1938..d7c64a327a2 100644 --- a/chromium/components/password_manager/core/browser/fake_form_fetcher.h +++ b/chromium/components/password_manager/core/browser/fake_form_fetcher.h @@ -56,6 +56,39 @@ class FakeFormFetcher : public FormFetcher { federated_ = federated; } + const std::vector<const autofill::PasswordForm*>& GetSuppressedHTTPSForms() + const override; + + // The pointees in |suppressed_forms| must outlive the fetcher. + void set_suppressed_https_forms( + const std::vector<const autofill::PasswordForm*>& suppressed_forms) { + suppressed_https_forms_ = suppressed_forms; + } + + const std::vector<const autofill::PasswordForm*>& + GetSuppressedPSLMatchingForms() const override; + + // The pointees in |suppressed_forms| must outlive the fetcher. + void set_suppressed_psl_matching_forms( + const std::vector<const autofill::PasswordForm*>& suppressed_forms) { + suppressed_psl_matching_forms_ = suppressed_forms; + } + + const std::vector<const autofill::PasswordForm*>& + GetSuppressedSameOrganizationNameForms() const override; + + // The pointees in |suppressed_forms| must outlive the fetcher. + void set_suppressed_same_organization_name_forms( + const std::vector<const autofill::PasswordForm*>& suppressed_forms) { + suppressed_same_organization_name_forms_ = suppressed_forms; + } + + bool DidCompleteQueryingSuppressedForms() const override; + + void set_did_complete_querying_suppressed_forms(bool value) { + did_complete_querying_suppressed_forms_ = value; + } + void SetNonFederated( const std::vector<const autofill::PasswordForm*>& non_federated, size_t filtered_count); @@ -71,6 +104,11 @@ class FakeFormFetcher : public FormFetcher { State state_ = State::NOT_WAITING; std::vector<InteractionsStats> stats_; std::vector<const autofill::PasswordForm*> federated_; + std::vector<const autofill::PasswordForm*> suppressed_https_forms_; + std::vector<const autofill::PasswordForm*> suppressed_psl_matching_forms_; + std::vector<const autofill::PasswordForm*> + suppressed_same_organization_name_forms_; + bool did_complete_querying_suppressed_forms_ = false; DISALLOW_COPY_AND_ASSIGN(FakeFormFetcher); }; diff --git a/chromium/components/password_manager/core/browser/form_fetcher.h b/chromium/components/password_manager/core/browser/form_fetcher.h index bfbd22f7709..22bcd52aa55 100644 --- a/chromium/components/password_manager/core/browser/form_fetcher.h +++ b/chromium/components/password_manager/core/browser/form_fetcher.h @@ -71,6 +71,42 @@ class FormFetcher { virtual const std::vector<const autofill::PasswordForm*>& GetFederatedMatches() const = 0; + // The following accessors return various kinds of `suppressed` credentials. + // These are stored credentials that are not (auto-)filled, because they are + // for an origin that is similar to, but not exactly matching the origin that + // this FormFetcher was created for. They are used for recording metrics on + // how often such -- potentially, but not necessarily related -- credentials + // are not offered to the user, unduly increasing log-in friction. + // + // There are currently three kinds of suppressed credentials: + // 1.) HTTPS credentials not filled on the HTTP version of the origin. + // 2.) PSL-matches that are not auto-filled (but filled on account select). + // 3.) Same-organization name credentials, not filled. + // + // Results below are queried on a best-effort basis, might be somewhat stale, + // and are available shortly after the Consumer::ProcessMatches callback. + + // When this instance fetches forms for an HTTP origin: Returns saved + // credentials, if any, found for the HTTPS version of that origin. Empty + // otherwise. + virtual const std::vector<const autofill::PasswordForm*>& + GetSuppressedHTTPSForms() const = 0; + + // Returns saved credentials, if any, for PSL-matching origins. Autofilling + // these is suppressed, however, they *can be* filled on account select. + virtual const std::vector<const autofill::PasswordForm*>& + GetSuppressedPSLMatchingForms() const = 0; + + // Returns saved credentials, if any, found for HTTP/HTTPS origins with the + // same organization name as the origin this FormFetcher was created for. + virtual const std::vector<const autofill::PasswordForm*>& + GetSuppressedSameOrganizationNameForms() const = 0; + + // Whether querying suppressed forms (of all flavors) was attempted and did + // complete at least once during the lifetime of this instance, regardless of + // whether there have been any results. + virtual bool DidCompleteQueryingSuppressedForms() const = 0; + // Fetches stored matching logins. In addition the statistics is fetched on // platforms with the password bubble. This is called automatically during // construction and can be called manually later as well to cause an update diff --git a/chromium/components/password_manager/core/browser/form_fetcher_impl.cc b/chromium/components/password_manager/core/browser/form_fetcher_impl.cc index b73501a2c74..c8127b0ebd5 100644 --- a/chromium/components/password_manager/core/browser/form_fetcher_impl.cc +++ b/chromium/components/password_manager/core/browser/form_fetcher_impl.cc @@ -15,6 +15,7 @@ #include "components/password_manager/core/browser/password_manager_client.h" #include "components/password_manager/core/browser/password_manager_util.h" #include "components/password_manager/core/browser/password_store.h" +#include "components/password_manager/core/browser/psl_matching_helper.h" #include "components/password_manager/core/browser/statistics_table.h" using autofill::PasswordForm; @@ -46,6 +47,43 @@ std::vector<std::unique_ptr<PasswordForm>> SplitFederatedMatches( return federated_matches; } +void SplitSuppressedFormsAndAssignTo( + const PasswordStore::FormDigest& observed_form_digest, + std::vector<std::unique_ptr<PasswordForm>> suppressed_forms, + std::vector<std::unique_ptr<PasswordForm>>* same_origin_https_forms, + std::vector<std::unique_ptr<PasswordForm>>* psl_matching_forms, + std::vector<std::unique_ptr<PasswordForm>>* same_organization_name_forms) { + DCHECK(same_origin_https_forms); + DCHECK(psl_matching_forms); + DCHECK(same_organization_name_forms); + same_origin_https_forms->clear(); + psl_matching_forms->clear(); + same_organization_name_forms->clear(); + for (auto& form : suppressed_forms) { + switch (GetMatchResult(*form, observed_form_digest)) { + case MatchResult::PSL_MATCH: + psl_matching_forms->push_back(std::move(form)); + break; + case MatchResult::NO_MATCH: + if (form->origin.host() != observed_form_digest.origin.host()) { + same_organization_name_forms->push_back(std::move(form)); + } else if (form->origin.SchemeIs(url::kHttpsScheme) && + observed_form_digest.origin.SchemeIs(url::kHttpScheme)) { + same_origin_https_forms->push_back(std::move(form)); + } else { + // HTTP form suppressed on HTTPS observed page: The HTTP->HTTPS + // migration can leave tons of such HTTP forms behind, ignore these. + } + break; + case MatchResult::EXACT_MATCH: + case MatchResult::FEDERATED_MATCH: + case MatchResult::FEDERATED_PSL_MATCH: + NOTREACHED() << "Suppressed match cannot be exact or federated."; + break; + } + } +} + // Create a vector of const PasswordForm from a vector of // unique_ptr<PasswordForm> by applying get() item-wise. std::vector<const PasswordForm*> MakeWeakCopies( @@ -73,10 +111,12 @@ std::vector<std::unique_ptr<PasswordForm>> MakeCopies( FormFetcherImpl::FormFetcherImpl(PasswordStore::FormDigest form_digest, const PasswordManagerClient* client, - bool should_migrate_http_passwords) + bool should_migrate_http_passwords, + bool should_query_suppressed_forms) : form_digest_(std::move(form_digest)), client_(client), - should_migrate_http_passwords_(should_migrate_http_passwords) {} + should_migrate_http_passwords_(should_migrate_http_passwords), + should_query_suppressed_forms_(should_query_suppressed_forms) {} FormFetcherImpl::~FormFetcherImpl() = default; @@ -106,6 +146,25 @@ const std::vector<const PasswordForm*>& FormFetcherImpl::GetFederatedMatches() return weak_federated_; } +const std::vector<const PasswordForm*>& +FormFetcherImpl::GetSuppressedHTTPSForms() const { + return weak_suppressed_same_origin_https_forms_; +} + +const std::vector<const PasswordForm*>& +FormFetcherImpl::GetSuppressedPSLMatchingForms() const { + return weak_suppressed_psl_matching_forms_; +} + +const std::vector<const PasswordForm*>& +FormFetcherImpl::GetSuppressedSameOrganizationNameForms() const { + return weak_suppressed_same_organization_name_forms_; +} + +bool FormFetcherImpl::DidCompleteQueryingSuppressedForms() const { + return did_complete_querying_suppressed_forms_; +} + void FormFetcherImpl::OnGetPasswordStoreResults( std::vector<std::unique_ptr<PasswordForm>> results) { DCHECK_EQ(State::WAITING, state_); @@ -126,6 +185,17 @@ void FormFetcherImpl::OnGetPasswordStoreResults( logger->LogNumber(Logger::STRING_NUMBER_RESULTS, results.size()); } + // Kick off the discovery of suppressed credentials, regardless of whether + // there are some precisely matching |results|. These results are used only + // for recording metrics at PasswordFormManager desctruction time, this is why + // they are requested this late. + if (should_query_suppressed_forms_ && + form_digest_.scheme == PasswordForm::SCHEME_HTML && + GURL(form_digest_.signon_realm).SchemeIsHTTPOrHTTPS()) { + suppressed_form_fetcher_ = base::MakeUnique<SuppressedFormFetcher>( + form_digest_.signon_realm, client_, this); + } + if (should_migrate_http_passwords_ && results.empty() && form_digest_.origin.SchemeIs(url::kHttpsScheme)) { http_migrator_ = base::MakeUnique<HttpPasswordStoreMigrator>( @@ -148,6 +218,21 @@ void FormFetcherImpl::ProcessMigratedForms( ProcessPasswordStoreResults(std::move(forms)); } +void FormFetcherImpl::ProcessSuppressedForms( + std::vector<std::unique_ptr<autofill::PasswordForm>> forms) { + did_complete_querying_suppressed_forms_ = true; + SplitSuppressedFormsAndAssignTo(form_digest_, std::move(forms), + &suppressed_same_origin_https_forms_, + &suppressed_psl_matching_forms_, + &suppressed_same_organization_name_forms_); + weak_suppressed_same_origin_https_forms_ = + MakeWeakCopies(suppressed_same_origin_https_forms_); + weak_suppressed_psl_matching_forms_ = + MakeWeakCopies(suppressed_psl_matching_forms_); + weak_suppressed_same_organization_name_forms_ = + MakeWeakCopies(suppressed_same_organization_name_forms_); +} + void FormFetcherImpl::Fetch() { std::unique_ptr<BrowserSavePasswordProgressLogger> logger; if (password_manager_util::IsLoggingActive(client_)) { @@ -188,14 +273,27 @@ std::unique_ptr<FormFetcher> FormFetcherImpl::Clone() { // Create the copy without the "HTTPS migration" activated. If it was needed, // then it was done by |this| already. - auto result = base::MakeUnique<FormFetcherImpl>(form_digest_, client_, false); + auto result = base::MakeUnique<FormFetcherImpl>( + form_digest_, client_, false, should_query_suppressed_forms_); result->non_federated_ = MakeCopies(this->non_federated_); result->federated_ = MakeCopies(this->federated_); result->interactions_stats_ = this->interactions_stats_; + result->suppressed_same_origin_https_forms_ = + MakeCopies(this->suppressed_same_origin_https_forms_); + result->suppressed_psl_matching_forms_ = + MakeCopies(this->suppressed_psl_matching_forms_); + result->suppressed_same_organization_name_forms_ = + MakeCopies(this->suppressed_same_organization_name_forms_); result->weak_non_federated_ = MakeWeakCopies(result->non_federated_); result->weak_federated_ = MakeWeakCopies(result->federated_); + result->weak_suppressed_same_origin_https_forms_ = + MakeWeakCopies(result->suppressed_same_origin_https_forms_); + result->weak_suppressed_psl_matching_forms_ = + MakeWeakCopies(result->suppressed_psl_matching_forms_); + result->weak_suppressed_same_organization_name_forms_ = + MakeWeakCopies(result->suppressed_same_organization_name_forms_); result->filtered_count_ = this->filtered_count_; result->state_ = this->state_; diff --git a/chromium/components/password_manager/core/browser/form_fetcher_impl.h b/chromium/components/password_manager/core/browser/form_fetcher_impl.h index af6ba69f034..29fe6a7b340 100644 --- a/chromium/components/password_manager/core/browser/form_fetcher_impl.h +++ b/chromium/components/password_manager/core/browser/form_fetcher_impl.h @@ -14,6 +14,7 @@ #include "components/password_manager/core/browser/http_password_store_migrator.h" #include "components/password_manager/core/browser/password_store.h" #include "components/password_manager/core/browser/password_store_consumer.h" +#include "components/password_manager/core/browser/suppressed_form_fetcher.h" namespace password_manager { @@ -23,13 +24,15 @@ class PasswordManagerClient; // with a particular origin. class FormFetcherImpl : public FormFetcher, public PasswordStoreConsumer, - public HttpPasswordStoreMigrator::Consumer { + public HttpPasswordStoreMigrator::Consumer, + public SuppressedFormFetcher::Consumer { public: // |form_digest| describes what credentials need to be retrieved and // |client| serves the PasswordStore, the logging information etc. FormFetcherImpl(PasswordStore::FormDigest form_digest, const PasswordManagerClient* client, - bool should_migrate_http_passwords); + bool should_migrate_http_passwords, + bool should_query_suppressed_forms); ~FormFetcherImpl() override; @@ -40,6 +43,13 @@ class FormFetcherImpl : public FormFetcher, const std::vector<InteractionsStats>& GetInteractionsStats() const override; const std::vector<const autofill::PasswordForm*>& GetFederatedMatches() const override; + const std::vector<const autofill::PasswordForm*>& GetSuppressedHTTPSForms() + const override; + const std::vector<const autofill::PasswordForm*>& + GetSuppressedPSLMatchingForms() const override; + const std::vector<const autofill::PasswordForm*>& + GetSuppressedSameOrganizationNameForms() const override; + bool DidCompleteQueryingSuppressedForms() const override; void Fetch() override; std::unique_ptr<FormFetcher> Clone() override; @@ -52,6 +62,10 @@ class FormFetcherImpl : public FormFetcher, void ProcessMigratedForms( std::vector<std::unique_ptr<autofill::PasswordForm>> forms) override; + // SuppressedFormFetcher::Consumer: + void ProcessSuppressedForms( + std::vector<std::unique_ptr<autofill::PasswordForm>> forms) override; + private: // Processes password form results and forwards them to the |consumers_|. void ProcessPasswordStoreResults( @@ -71,9 +85,27 @@ class FormFetcherImpl : public FormFetcher, // Statistics for the current domain. std::vector<InteractionsStats> interactions_stats_; + std::vector<std::unique_ptr<autofill::PasswordForm>> + suppressed_same_origin_https_forms_; + std::vector<std::unique_ptr<autofill::PasswordForm>> + suppressed_psl_matching_forms_; + std::vector<std::unique_ptr<autofill::PasswordForm>> + suppressed_same_organization_name_forms_; + + // Whether querying |suppressed_https_forms_| was attempted and did complete + // at least once during the lifetime of this instance, regardless of whether + // there have been any results. + bool did_complete_querying_suppressed_forms_ = false; + // Non-owning copies of the vectors above. std::vector<const autofill::PasswordForm*> weak_non_federated_; std::vector<const autofill::PasswordForm*> weak_federated_; + std::vector<const autofill::PasswordForm*> + weak_suppressed_same_origin_https_forms_; + std::vector<const autofill::PasswordForm*> + weak_suppressed_psl_matching_forms_; + std::vector<const autofill::PasswordForm*> + weak_suppressed_same_organization_name_forms_; // Consumers of the fetcher, all are assumed to outlive |this|. std::set<FormFetcher::Consumer*> consumers_; @@ -95,9 +127,18 @@ class FormFetcherImpl : public FormFetcher, // Indicates whether HTTP passwords should be migrated to HTTPS. const bool should_migrate_http_passwords_; + // Indicates whether to query suppressed forms. + const bool should_query_suppressed_forms_; + // Does the actual migration. std::unique_ptr<HttpPasswordStoreMigrator> http_migrator_; + // Responsible for looking up `suppressed` credentials. These are stored + // credentials that were not filled, even though they might be related to the + // origin that this instance was created for. Look-up happens asynchronously, + // without blocking Consumer::ProcessMatches. + std::unique_ptr<SuppressedFormFetcher> suppressed_form_fetcher_; + DISALLOW_COPY_AND_ASSIGN(FormFetcherImpl); }; diff --git a/chromium/components/password_manager/core/browser/form_fetcher_impl_unittest.cc b/chromium/components/password_manager/core/browser/form_fetcher_impl_unittest.cc index 90c36a90dbf..07cf4325d65 100644 --- a/chromium/components/password_manager/core/browser/form_fetcher_impl_unittest.cc +++ b/chromium/components/password_manager/core/browser/form_fetcher_impl_unittest.cc @@ -20,6 +20,7 @@ #include "build/build_config.h" #include "components/autofill/core/common/password_form.h" #include "components/password_manager/core/browser/mock_password_store.h" +#include "components/password_manager/core/browser/password_manager_test_utils.h" #include "components/password_manager/core/browser/password_store.h" #include "components/password_manager/core/browser/statistics_table.h" #include "components/password_manager/core/browser/stub_credentials_filter.h" @@ -38,12 +39,29 @@ using testing::IsEmpty; using testing::Pointee; using testing::Return; using testing::UnorderedElementsAre; +using testing::UnorderedElementsAreArray; using testing::WithArg; namespace password_manager { namespace { +constexpr const char kTestHttpURL[] = "http://example.in/"; +constexpr const char kTestHttpActionURL[] = "http://login.example.org/"; + +constexpr const char kTestHttpsURL[] = "https://example.in/"; +constexpr const char kTestHttpsActionURL[] = "https://login.example.org/"; + +constexpr const char kTestPSLMatchingHttpURL[] = "http://psl.example.in/"; +constexpr const char kTestPSLMatchingHttpsURL[] = "https://psl.example.in/"; + +constexpr const char kTestHttpSameOrgNameURL[] = "http://sub.example.com/"; +constexpr const char kTestHttpsSameOrgNameURL[] = "https://sub.example.com/"; + +constexpr const char kTestFederatedRealm[] = + "federation://example.in/accounts.google.com"; +constexpr const char kTestFederationURL[] = "https://accounts.google.com/"; + class MockConsumer : public FormFetcher::Consumer { public: MOCK_METHOD2(ProcessMatches, @@ -102,42 +120,46 @@ class FakePasswordManagerClient : public StubPasswordManagerClient { DISALLOW_COPY_AND_ASSIGN(FakePasswordManagerClient); }; +PasswordForm CreateHTMLForm(const char* origin_url, + const char* username_value, + const char* password_value) { + PasswordForm form; + form.scheme = PasswordForm::SCHEME_HTML; + form.origin = GURL(origin_url); + form.signon_realm = origin_url; + form.username_value = ASCIIToUTF16(username_value); + form.password_value = ASCIIToUTF16(password_value); + return form; +} + // Creates a dummy non-federated form with some basic arbitrary values. PasswordForm CreateNonFederated() { - PasswordForm form; - form.origin = GURL("https://example.in"); - form.signon_realm = form.origin.spec(); - form.action = GURL("https://login.example.org"); - form.username_value = ASCIIToUTF16("user"); - form.password_value = ASCIIToUTF16("password"); + PasswordForm form = CreateHTMLForm(kTestHttpsURL, "user", "password"); + form.action = GURL(kTestHttpsActionURL); return form; } // Creates a dummy non-federated HTTP form with some basic arbitrary values. PasswordForm CreateHTTPNonFederated() { - PasswordForm form; - form.origin = GURL("http://example.in"); - form.signon_realm = form.origin.spec(); - form.action = GURL("http://login.example.org"); - form.username_value = ASCIIToUTF16("user"); - form.password_value = ASCIIToUTF16("password"); + PasswordForm form = CreateHTMLForm(kTestHttpURL, "user", "password"); + form.action = GURL(kTestHttpActionURL); return form; } // Creates a dummy federated form with some basic arbitrary values. PasswordForm CreateFederated() { PasswordForm form = CreateNonFederated(); + form.signon_realm = kTestFederatedRealm; form.password_value.clear(); - form.federation_origin = url::Origin(GURL("https://accounts.google.com/")); + form.federation_origin = url::Origin(GURL(kTestFederationURL)); return form; } // Creates an Android federated credential. PasswordForm CreateAndroidFederated() { - PasswordForm form = CreateFederated(); - form.signon_realm = "android://hash@com.example.android/"; - form.origin = GURL(form.signon_realm); - form.action = GURL(); + PasswordForm form = + CreateHTMLForm("android://hash@com.example.android/", "user", ""); + form.federation_origin = url::Origin(GURL(kTestFederationURL)); form.is_affiliation_based_match = true; return form; } @@ -152,6 +174,15 @@ std::vector<std::unique_ptr<PasswordForm>> MakeResults( return results; } +std::vector<PasswordForm> PointeeValues( + const std::vector<const PasswordForm*> forms) { + std::vector<PasswordForm> result; + result.reserve(forms.size()); + for (const PasswordForm* form : forms) + result.push_back(*form); + return result; +} + ACTION_P(GetAndAssignWeakPtr, ptr) { *ptr = arg0->GetWeakPtr(); } @@ -162,13 +193,14 @@ class FormFetcherImplTest : public testing::Test { public: FormFetcherImplTest() : form_digest_(PasswordForm::SCHEME_HTML, - "http://accounts.google.com", - GURL("http://accounts.google.com/a/LoginAuth")) { + kTestHttpURL, + GURL(kTestHttpURL)) { mock_store_ = new MockPasswordStore(); client_.set_store(mock_store_.get()); form_fetcher_ = base::MakeUnique<FormFetcherImpl>( - form_digest_, &client_, /* should_migrate_http_passwords */ false); + form_digest_, &client_, false /* should_migrate_http_passwords */, + false /* should_query_suppressed_https_forms */); } ~FormFetcherImplTest() override { mock_store_->ShutdownOnUIThread(); } @@ -186,6 +218,55 @@ class FormFetcherImplTest : public testing::Test { testing::Mock::VerifyAndClearExpectations(mock_store_.get()); } + void RecreateFormFetcherWithQueryingSuppressedForms() { + form_fetcher_ = base::MakeUnique<FormFetcherImpl>( + form_digest_, &client_, false /* should_migrate_http_passwords */, + true /* should_query_suppressed_https_forms */); + EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u)); + form_fetcher_->AddConsumer(&consumer_); + testing::Mock::VerifyAndClearExpectations(&consumer_); + } + + // Simulates a call to Fetch(), and supplies |simulated_matches| as the + // PasswordStore results. Expects that this will trigger the querying of + // suppressed forms by means of a GetLoginsForSameOrganizationName call + // being issued against the |expected_signon_realm|. + // + // Call CompleteQueryingSuppressedForms with the emitted |consumer_ptr| + // to complete the query. + void SimulateFetchAndExpectQueryingSuppressedForms( + const std::vector<PasswordForm>& simulated_get_logins_matches, + const std::string& expected_signon_realm, + base::WeakPtr<PasswordStoreConsumer>* consumer_ptr /* out */) { + ASSERT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState()); + + Fetch(); + + EXPECT_CALL(*mock_store_, + GetLoginsForSameOrganizationName(expected_signon_realm, _)) + .WillOnce(::testing::WithArg<1>(GetAndAssignWeakPtr(consumer_ptr))); + const size_t num_matches = simulated_get_logins_matches.size(); + EXPECT_CALL(consumer_, ProcessMatches(::testing::SizeIs(num_matches), 0u)); + + form_fetcher_->OnGetPasswordStoreResults( + MakeResults(simulated_get_logins_matches)); + + ASSERT_TRUE(testing::Mock::VerifyAndClearExpectations(&consumer_)); + ASSERT_TRUE(testing::Mock::VerifyAndClearExpectations(mock_store_.get())); + ASSERT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState()); + ASSERT_TRUE(*consumer_ptr); + } + + void CompleteQueryingSuppressedForms( + const std::vector<PasswordForm>& simulated_suppressed_forms, + base::WeakPtr<PasswordStoreConsumer> consumer_ptr) { + ASSERT_TRUE(consumer_ptr); + ASSERT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState()); + consumer_ptr->OnGetPasswordStoreResults( + MakeResults(simulated_suppressed_forms)); + ASSERT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState()); + } + base::MessageLoop message_loop_; // Used by mock_store_. PasswordStore::FormDigest form_digest_; std::unique_ptr<FormFetcherImpl> form_fetcher_; @@ -399,7 +480,8 @@ TEST_F(FormFetcherImplTest, DoNotTryToMigrateHTTPPasswordsOnHTTPSites) { // A new form fetcher is created to be able to set the form digest and // migration flag. form_fetcher_ = base::MakeUnique<FormFetcherImpl>( - form_digest_, &client_, /* should_migrate_http_passwords */ true); + form_digest_, &client_, true /* should_migrate_http_passwords */, + false /* should_query_suppressed_https_forms */); EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u)); form_fetcher_->AddConsumer(&consumer_); @@ -441,7 +523,8 @@ TEST_F(FormFetcherImplTest, TryToMigrateHTTPPasswordsOnHTTPSSites) { // A new form fetcher is created to be able to set the form digest and // migration flag. form_fetcher_ = base::MakeUnique<FormFetcherImpl>( - form_digest_, &client_, /* should_migrate_http_passwords */ true); + form_digest_, &client_, true /* should_migrate_http_passwords */, + false /* should_query_suppressed_https_forms */); EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u)); form_fetcher_->AddConsumer(&consumer_); @@ -510,7 +593,8 @@ TEST_F(FormFetcherImplTest, StateIsWaitingDuringMigration) { // A new form fetcher is created to be able to set the form digest and // migration flag. form_fetcher_ = base::MakeUnique<FormFetcherImpl>( - form_digest_, &client_, /* should_migrate_http_passwords */ true); + form_digest_, &client_, true /* should_migrate_http_passwords */, + false /* should_query_suppressed_https_forms */); PasswordForm https_form = CreateNonFederated(); @@ -551,19 +635,229 @@ TEST_F(FormFetcherImplTest, StateIsWaitingDuringMigration) { EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState()); } +TEST_F(FormFetcherImplTest, SuppressedForms_QueriedForHTTPAndHTTPSOrigins) { + static const PasswordStore::FormDigest kObservedHTTPSFormDigest( + PasswordForm::SCHEME_HTML, kTestHttpsURL, GURL(kTestHttpsURL)); + + static const PasswordForm kFormHttpSameHost = + CreateHTMLForm(kTestHttpURL, "user_1", "pass_1"); + static const PasswordForm kFormHttpsSameHost = + CreateHTMLForm(kTestHttpsURL, "user_2", "pass_2"); + static const PasswordForm kFormHttpPSLMatchingHost = + CreateHTMLForm(kTestPSLMatchingHttpURL, "user_3", "pass_3"); + static const PasswordForm kFormHttpsPSLMatchingHost = + CreateHTMLForm(kTestPSLMatchingHttpsURL, "user_4", "pass_4"); + static const PasswordForm kFormHttpSameOrgNameHost = + CreateHTMLForm(kTestHttpSameOrgNameURL, "user_5", "pass_5"); + static const PasswordForm kFormHttpsSameOrgNameHost = + CreateHTMLForm(kTestHttpsSameOrgNameURL, "user_6", "pass_6"); + + static const struct { + const char* observed_form_origin; + const char* observed_form_realm; + std::vector<PasswordForm> matching_forms; + std::vector<PasswordForm> all_suppressed_forms; + std::vector<PasswordForm> expected_suppressed_https_forms; + std::vector<PasswordForm> expected_suppressed_psl_forms; + std::vector<PasswordForm> expected_suppressed_same_org_name_forms; + } kTestCases[] = { + {kTestHttpURL, + kTestHttpURL, + {kFormHttpSameHost}, + {kFormHttpsSameHost, kFormHttpPSLMatchingHost, kFormHttpsPSLMatchingHost, + kFormHttpSameOrgNameHost, kFormHttpsSameOrgNameHost}, + {kFormHttpsSameHost}, + {kFormHttpPSLMatchingHost}, + {kFormHttpsPSLMatchingHost, kFormHttpSameOrgNameHost, + kFormHttpsSameOrgNameHost}}, + + {kTestHttpsURL, + kTestHttpsURL, + {kFormHttpsSameHost}, + {kFormHttpSameHost, kFormHttpPSLMatchingHost, kFormHttpsPSLMatchingHost, + kFormHttpSameOrgNameHost, kFormHttpsSameOrgNameHost}, + std::vector<PasswordForm>(), + {kFormHttpsPSLMatchingHost}, + {kFormHttpPSLMatchingHost, kFormHttpSameOrgNameHost, + kFormHttpsSameOrgNameHost}}, + }; + + for (const auto& test_case : kTestCases) { + SCOPED_TRACE(test_case.observed_form_origin); + + form_digest_ = PasswordStore::FormDigest( + PasswordForm::SCHEME_HTML, test_case.observed_form_origin, + GURL(test_case.observed_form_origin)); + RecreateFormFetcherWithQueryingSuppressedForms(); + + // The matching PasswordStore results coming in should trigger another + // GetLogins request to fetcht the suppressed forms. + base::WeakPtr<PasswordStoreConsumer> suppressed_form_fetcher_ptr = nullptr; + ASSERT_NO_FATAL_FAILURE(SimulateFetchAndExpectQueryingSuppressedForms( + test_case.matching_forms, test_case.observed_form_realm, + &suppressed_form_fetcher_ptr)); + + EXPECT_FALSE(form_fetcher_->DidCompleteQueryingSuppressedForms()); + EXPECT_THAT(form_fetcher_->GetSuppressedHTTPSForms(), IsEmpty()); + + ASSERT_NO_FATAL_FAILURE(CompleteQueryingSuppressedForms( + test_case.all_suppressed_forms, suppressed_form_fetcher_ptr)); + + EXPECT_TRUE(form_fetcher_->DidCompleteQueryingSuppressedForms()); + EXPECT_THAT( + PointeeValues(form_fetcher_->GetSuppressedHTTPSForms()), + UnorderedElementsAreArray(test_case.expected_suppressed_https_forms)); + EXPECT_THAT( + PointeeValues(form_fetcher_->GetSuppressedPSLMatchingForms()), + UnorderedElementsAreArray(test_case.expected_suppressed_psl_forms)); + EXPECT_THAT( + PointeeValues(form_fetcher_->GetSuppressedSameOrganizationNameForms()), + UnorderedElementsAreArray( + test_case.expected_suppressed_same_org_name_forms)); + } +} + +TEST_F(FormFetcherImplTest, SuppressedForms_RequeriedOnRefetch) { + RecreateFormFetcherWithQueryingSuppressedForms(); + + base::WeakPtr<PasswordStoreConsumer> https_form_fetcher_ptr = nullptr; + ASSERT_NO_FATAL_FAILURE(SimulateFetchAndExpectQueryingSuppressedForms( + std::vector<PasswordForm>(), kTestHttpURL, &https_form_fetcher_ptr)); + ASSERT_NO_FATAL_FAILURE(CompleteQueryingSuppressedForms( + std::vector<PasswordForm>(), https_form_fetcher_ptr)); + + // Another call to Fetch() should refetch the list of suppressed credentials. + const PasswordForm suppressed_https_form = CreateNonFederated(); + ASSERT_NO_FATAL_FAILURE(SimulateFetchAndExpectQueryingSuppressedForms( + std::vector<PasswordForm>(), kTestHttpURL, &https_form_fetcher_ptr)); + ASSERT_NO_FATAL_FAILURE(CompleteQueryingSuppressedForms( + {suppressed_https_form}, https_form_fetcher_ptr)); + + EXPECT_THAT(form_fetcher_->GetSuppressedHTTPSForms(), + UnorderedElementsAre(Pointee(suppressed_https_form))); +} + +TEST_F(FormFetcherImplTest, SuppressedForms_NeverWiped) { + RecreateFormFetcherWithQueryingSuppressedForms(); + + static const PasswordForm kFormHttpsSameHost = + CreateHTMLForm(kTestHttpsURL, "user_1", "pass_1"); + static const PasswordForm kFormHttpPSLMatchingHost = + CreateHTMLForm(kTestPSLMatchingHttpURL, "user_2", "pass_2"); + static const PasswordForm kFormHttpSameOrgNameHost = + CreateHTMLForm(kTestHttpSameOrgNameURL, "user_3", "pass_3"); + + base::WeakPtr<PasswordStoreConsumer> https_form_fetcher_ptr = nullptr; + ASSERT_NO_FATAL_FAILURE(SimulateFetchAndExpectQueryingSuppressedForms( + std::vector<PasswordForm>(), kTestHttpURL, &https_form_fetcher_ptr)); + ASSERT_NO_FATAL_FAILURE(CompleteQueryingSuppressedForms( + {kFormHttpsSameHost, kFormHttpPSLMatchingHost, kFormHttpSameOrgNameHost}, + https_form_fetcher_ptr)); + + // Ensure that calling Fetch() does not wipe (even temporarily) the previously + // fetched list of suppressed HTTPS credentials. Stale is better than nothing. + ASSERT_NO_FATAL_FAILURE(SimulateFetchAndExpectQueryingSuppressedForms( + std::vector<PasswordForm>(), kTestHttpURL, &https_form_fetcher_ptr)); + + EXPECT_TRUE(form_fetcher_->DidCompleteQueryingSuppressedForms()); + EXPECT_THAT(form_fetcher_->GetSuppressedHTTPSForms(), + UnorderedElementsAre(Pointee(kFormHttpsSameHost))); + EXPECT_THAT(form_fetcher_->GetSuppressedPSLMatchingForms(), + UnorderedElementsAre(Pointee(kFormHttpPSLMatchingHost))); + EXPECT_THAT(form_fetcher_->GetSuppressedSameOrganizationNameForms(), + UnorderedElementsAre(Pointee(kFormHttpSameOrgNameHost))); +} + +TEST_F(FormFetcherImplTest, SuppressedForms_FormFetcherDestroyedWhileQuerying) { + RecreateFormFetcherWithQueryingSuppressedForms(); + + base::WeakPtr<PasswordStoreConsumer> https_form_fetcher_ptr = nullptr; + ASSERT_NO_FATAL_FAILURE(SimulateFetchAndExpectQueryingSuppressedForms( + std::vector<PasswordForm>(), kTestHttpURL, &https_form_fetcher_ptr)); + + EXPECT_FALSE(form_fetcher_->DidCompleteQueryingSuppressedForms()); + + // Destroy FormFetcher while SuppressedHTTPSFormFetcher is busy. + form_fetcher_.reset(); +} + +// Exercise the scenario where querying the suppressed HTTPS logins takes so +// long that in the meantime there is another call to Fetch(), which completes, +// and triggers fetching HTTPS suppressed forms yet again. In this case, the +// first SuppressedHTTPSFormFetcher is destroyed and its query cancelled. +TEST_F(FormFetcherImplTest, SuppressedForms_SimultaneousQueries) { + RecreateFormFetcherWithQueryingSuppressedForms(); + + base::WeakPtr<PasswordStoreConsumer> https_form_fetcher_ptr1; + ASSERT_NO_FATAL_FAILURE(SimulateFetchAndExpectQueryingSuppressedForms( + std::vector<PasswordForm>(), kTestHttpURL, &https_form_fetcher_ptr1)); + + base::WeakPtr<PasswordStoreConsumer> https_form_fetcher_ptr2; + ASSERT_NO_FATAL_FAILURE(SimulateFetchAndExpectQueryingSuppressedForms( + std::vector<PasswordForm>(), kTestHttpURL, &https_form_fetcher_ptr2)); + + EXPECT_FALSE(form_fetcher_->DidCompleteQueryingSuppressedForms()); + EXPECT_THAT(form_fetcher_->GetSuppressedHTTPSForms(), IsEmpty()); + EXPECT_FALSE(https_form_fetcher_ptr1); + ASSERT_TRUE(https_form_fetcher_ptr2); + + static const PasswordForm kFormHttpsSameHost = + CreateHTMLForm(kTestHttpsURL, "user_1", "pass_1"); + static const PasswordForm kFormHttpPSLMatchingHost = + CreateHTMLForm(kTestPSLMatchingHttpURL, "user_2", "pass_2"); + static const PasswordForm kFormHttpSameOrgNameHost = + CreateHTMLForm(kTestHttpSameOrgNameURL, "user_3", "pass_3"); + + ASSERT_NO_FATAL_FAILURE(CompleteQueryingSuppressedForms( + {kFormHttpsSameHost, kFormHttpPSLMatchingHost, kFormHttpSameOrgNameHost}, + https_form_fetcher_ptr2)); + + EXPECT_TRUE(form_fetcher_->DidCompleteQueryingSuppressedForms()); + + EXPECT_TRUE(form_fetcher_->DidCompleteQueryingSuppressedForms()); + EXPECT_THAT(form_fetcher_->GetSuppressedHTTPSForms(), + UnorderedElementsAre(Pointee(kFormHttpsSameHost))); + EXPECT_THAT(form_fetcher_->GetSuppressedPSLMatchingForms(), + UnorderedElementsAre(Pointee(kFormHttpPSLMatchingHost))); + EXPECT_THAT(form_fetcher_->GetSuppressedSameOrganizationNameForms(), + UnorderedElementsAre(Pointee(kFormHttpSameOrgNameHost))); +} + +TEST_F(FormFetcherImplTest, SuppressedForms_NotQueriedForFederatedRealms) { + form_digest_ = PasswordStore::FormDigest( + PasswordForm::SCHEME_HTML, kTestFederatedRealm, GURL(kTestFederationURL)); + RecreateFormFetcherWithQueryingSuppressedForms(); + Fetch(); + + EXPECT_CALL(*mock_store_, GetLogins(_, _)).Times(0); + EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u)); + + form_fetcher_->OnGetPasswordStoreResults( + MakeResults(std::vector<PasswordForm>())); + + EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState()); + EXPECT_FALSE(form_fetcher_->DidCompleteQueryingSuppressedForms()); +} + // Cloning a FormFetcherImpl with empty results should result in an // instance with empty results. TEST_F(FormFetcherImplTest, Clone_EmptyResults) { + RecreateFormFetcherWithQueryingSuppressedForms(); Fetch(); + EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u)); + EXPECT_CALL(*mock_store_, GetLoginsForSameOrganizationName(_, _)); form_fetcher_->OnGetPasswordStoreResults( std::vector<std::unique_ptr<PasswordForm>>()); + ASSERT_TRUE(::testing::Mock::VerifyAndClearExpectations(mock_store_.get())); // Clone() should not cause re-fetching from PasswordStore. EXPECT_CALL(*mock_store_, GetLogins(_, _)).Times(0); + EXPECT_CALL(*mock_store_, GetLoginsForSameOrganizationName(_, _)).Times(0); auto clone = form_fetcher_->Clone(); EXPECT_EQ(FormFetcher::State::NOT_WAITING, clone->GetState()); EXPECT_THAT(clone->GetInteractionsStats(), IsEmpty()); EXPECT_THAT(clone->GetFederatedMatches(), IsEmpty()); + EXPECT_THAT(clone->GetSuppressedHTTPSForms(), IsEmpty()); MockConsumer consumer; EXPECT_CALL(consumer, ProcessMatches(IsEmpty(), 0u)); clone->AddConsumer(&consumer); @@ -572,6 +866,7 @@ TEST_F(FormFetcherImplTest, Clone_EmptyResults) { // Cloning a FormFetcherImpl with non-empty results should result in an // instance with the same results. TEST_F(FormFetcherImplTest, Clone_NonEmptyResults) { + RecreateFormFetcherWithQueryingSuppressedForms(); Fetch(); PasswordForm non_federated = CreateNonFederated(); PasswordForm federated = CreateFederated(); @@ -580,10 +875,15 @@ TEST_F(FormFetcherImplTest, Clone_NonEmptyResults) { results.push_back(base::MakeUnique<PasswordForm>(non_federated)); results.push_back(base::MakeUnique<PasswordForm>(federated)); results.push_back(base::MakeUnique<PasswordForm>(android_federated)); + + EXPECT_CALL(consumer_, ProcessMatches(::testing::SizeIs(1), 0u)); + EXPECT_CALL(*mock_store_, GetLoginsForSameOrganizationName(_, _)); form_fetcher_->OnGetPasswordStoreResults(std::move(results)); + ASSERT_TRUE(::testing::Mock::VerifyAndClearExpectations(mock_store_.get())); // Clone() should not cause re-fetching from PasswordStore. EXPECT_CALL(*mock_store_, GetLogins(_, _)).Times(0); + EXPECT_CALL(*mock_store_, GetLoginsForSameOrganizationName(_, _)).Times(0); auto clone = form_fetcher_->Clone(); // Additionally, destroy the original FormFetcher. This should not invalidate @@ -615,6 +915,33 @@ TEST_F(FormFetcherImplTest, Clone_Stats) { EXPECT_EQ(1u, clone->GetInteractionsStats().size()); } +// Cloning a FormFetcherImpl with some suppressed credentials should +// result in an instance with the same suppressed credentials. +TEST_F(FormFetcherImplTest, Clone_SuppressedCredentials) { + Fetch(); + form_fetcher_->OnGetPasswordStoreResults( + std::vector<std::unique_ptr<PasswordForm>>()); + + static const PasswordForm kFormHttpsSameHost = + CreateHTMLForm(kTestHttpsURL, "user_1", "pass_1"); + static const PasswordForm kFormHttpPSLMatchingHost = + CreateHTMLForm(kTestPSLMatchingHttpURL, "user_2", "pass_2"); + static const PasswordForm kFormHttpSameOrgNameHost = + CreateHTMLForm(kTestHttpSameOrgNameURL, "user_3", "pass_3"); + + form_fetcher_->ProcessSuppressedForms( + MakeResults({kFormHttpsSameHost, kFormHttpPSLMatchingHost, + kFormHttpSameOrgNameHost})); + + auto clone = form_fetcher_->Clone(); + EXPECT_THAT(PointeeValues(clone->GetSuppressedHTTPSForms()), + UnorderedElementsAre(kFormHttpsSameHost)); + EXPECT_THAT(PointeeValues(clone->GetSuppressedPSLMatchingForms()), + UnorderedElementsAre(kFormHttpPSLMatchingHost)); + EXPECT_THAT(PointeeValues(clone->GetSuppressedSameOrganizationNameForms()), + UnorderedElementsAre(kFormHttpSameOrgNameHost)); +} + // Check that removing consumers stops them from receiving store updates. TEST_F(FormFetcherImplTest, RemoveConsumer) { Fetch(); diff --git a/chromium/components/password_manager/core/browser/login_database.cc b/chromium/components/password_manager/core/browser/login_database.cc index 5de021e0ea7..e2067b61a2f 100644 --- a/chromium/components/password_manager/core/browser/login_database.cc +++ b/chromium/components/password_manager/core/browser/login_database.cc @@ -30,12 +30,14 @@ #include "components/password_manager/core/browser/password_manager_client.h" #include "components/password_manager/core/browser/password_manager_metrics_util.h" #include "components/password_manager/core/browser/password_manager_util.h" +#include "components/password_manager/core/browser/psl_matching_helper.h" #include "components/password_manager/core/browser/sql_table_builder.h" #include "google_apis/gaia/gaia_auth_util.h" #include "google_apis/gaia/gaia_urls.h" #include "sql/connection.h" #include "sql/statement.h" #include "sql/transaction.h" +#include "third_party/re2/src/re2/re2.h" #include "url/origin.h" #include "url/url_constants.h" @@ -1133,6 +1135,48 @@ bool LoginDatabase::GetLogins( return false; } +bool LoginDatabase::GetLoginsForSameOrganizationName( + const std::string& signon_realm, + std::vector<std::unique_ptr<autofill::PasswordForm>>* forms) const { + DCHECK(forms); + forms->clear(); + + GURL signon_realm_as_url(signon_realm); + if (!signon_realm_as_url.SchemeIsHTTPOrHTTPS()) + return true; + + std::string organization_name = + GetOrganizationIdentifyingName(signon_realm_as_url); + if (organization_name.empty()) + return true; + + // SQLite does not provide a function to escape special characters, but + // seemingly uses POSIX Extended Regular Expressions (ERE), and so does RE2. + // In the worst case the bogus results will be filtered out below. + static constexpr char kRESchemeAndSubdomains[] = "^https?://([\\w+%-]+\\.)*"; + static constexpr char kREDotAndEffectiveTLD[] = "(\\.[\\w+%-]+)+/$"; + const std::string signon_realms_with_same_organization_name_regexp = + kRESchemeAndSubdomains + RE2::QuoteMeta(organization_name) + + kREDotAndEffectiveTLD; + sql::Statement s(db_.GetCachedStatement( + SQL_FROM_HERE, get_same_organization_name_logins_statement_.c_str())); + s.BindString(0, signon_realms_with_same_organization_name_regexp); + + bool success = StatementToForms(&s, nullptr, forms); + + using PasswordFormPtr = std::unique_ptr<autofill::PasswordForm>; + base::EraseIf(*forms, [&organization_name](const PasswordFormPtr& form) { + GURL candidate_signon_realm_as_url(form->signon_realm); + DCHECK_EQ(form->scheme, PasswordForm::SCHEME_HTML); + DCHECK(candidate_signon_realm_as_url.SchemeIsHTTPOrHTTPS()); + std::string candidate_form_organization_name = + GetOrganizationIdentifyingName(candidate_signon_realm_as_url); + return candidate_form_organization_name != organization_name; + }); + + return success; +} + bool LoginDatabase::GetLoginsCreatedBetween( const base::Time begin, const base::Time end, @@ -1315,6 +1359,11 @@ void LoginDatabase::InitializeStatementStrings(const SQLTableBuilder& builder) { DCHECK(get_statement_psl_federated_.empty()); get_statement_psl_federated_ = get_statement_ + psl_statement + psl_federated_statement; + DCHECK(get_same_organization_name_logins_statement_.empty()); + get_same_organization_name_logins_statement_ = + "SELECT " + all_column_names + + " FROM LOGINS" + " WHERE scheme == 0 AND signon_realm REGEXP ?"; DCHECK(created_statement_.empty()); created_statement_ = "SELECT " + all_column_names + diff --git a/chromium/components/password_manager/core/browser/login_database.h b/chromium/components/password_manager/core/browser/login_database.h index c6cf73dc834..ed53b3a8e08 100644 --- a/chromium/components/password_manager/core/browser/login_database.h +++ b/chromium/components/password_manager/core/browser/login_database.h @@ -91,6 +91,18 @@ class LoginDatabase { std::vector<std::unique_ptr<autofill::PasswordForm>>* forms) const WARN_UNUSED_RESULT; + // Retrieves all stored credentials with SCHEME_HTTP that have a realm whose + // organization-identifying name -- that is, the first domain name label below + // the effective TLD -- matches that of |signon_realm|. Return value indicates + // a successful query (but potentially no results). + // + // For example, the organization-identifying name of "https://foo.example.org" + // is `example`, and logins will be returned for "http://bar.example.co.uk", + // but not for "http://notexample.com" or "https://example.foo.com". + bool GetLoginsForSameOrganizationName( + const std::string& signon_realm, + std::vector<std::unique_ptr<autofill::PasswordForm>>* forms) const; + // Gets all logins created from |begin| onwards (inclusive) and before |end|. // You may use a null Time value to do an unbounded search in either // direction. @@ -224,6 +236,7 @@ class LoginDatabase { std::string get_statement_psl_; std::string get_statement_federated_; std::string get_statement_psl_federated_; + std::string get_same_organization_name_logins_statement_; std::string created_statement_; std::string synced_statement_; std::string blacklisted_statement_; diff --git a/chromium/components/password_manager/core/browser/login_database_unittest.cc b/chromium/components/password_manager/core/browser/login_database_unittest.cc index e04d1c25c32..f219fd8be9e 100644 --- a/chromium/components/password_manager/core/browser/login_database_unittest.cc +++ b/chromium/components/password_manager/core/browser/login_database_unittest.cc @@ -21,6 +21,7 @@ #include "build/build_config.h" #include "components/autofill/core/common/password_form.h" #include "components/os_crypt/os_crypt_mocker.h" +#include "components/password_manager/core/browser/password_manager_test_utils.h" #include "components/password_manager/core/browser/psl_matching_helper.h" #include "sql/connection.h" #include "sql/statement.h" @@ -447,7 +448,7 @@ TEST_F(LoginDatabaseTest, TestFederatedMatchingLocalhost) { EXPECT_EQ(AddChangeForForm(form), db().AddLogin(form)); EXPECT_EQ(AddChangeForForm(form_with_port), db().AddLogin(form_with_port)); - // Match twice. + // Match localhost with and without port. PasswordStore::FormDigest form_request(PasswordForm::SCHEME_HTML, "http://localhost/", GURL("http://localhost/")); @@ -455,7 +456,6 @@ TEST_F(LoginDatabaseTest, TestFederatedMatchingLocalhost) { EXPECT_TRUE(db().GetLogins(form_request, &result)); EXPECT_THAT(result, UnorderedElementsAre(Pointee(form))); - // Match against the mobile site. form_request.origin = GURL("http://localhost:8080/"); form_request.signon_realm = "http://localhost:8080/"; EXPECT_TRUE(db().GetLogins(form_request, &result)); @@ -790,6 +790,146 @@ TEST_F(LoginDatabaseTest, TestPublicSuffixDomainMatchingRegexp) { EXPECT_EQ(0U, result.size()); } +TEST_F(LoginDatabaseTest, + GetLoginsForSameOrganizationName_OnlyWebHTTPFormsAreConsidered) { + static constexpr const struct { + const PasswordFormData form_data; + const char* other_queried_signon_realm; + bool expected_matches_itself; + bool expected_matches_other_realm; + } kTestCases[] = { + {{PasswordForm::SCHEME_HTML, "https://example.com/", + "https://example.com/origin", "", L"", L"", L"", L"u", L"p", false, 1}, + nullptr, + true, + true}, + {{PasswordForm::SCHEME_BASIC, "http://example.com/realm", + "http://example.com/", "", L"", L"", L"", L"u", L"p", false, 1}, + nullptr, + false, + false}, + {{PasswordForm::SCHEME_OTHER, "ftp://example.com/realm", + "ftp://example.com/", "", L"", L"", L"", L"u", L"p", false, 1}, + "http://example.com/realm", + false, + false}, + {{PasswordForm::SCHEME_HTML, + "federation://example.com/accounts.google.com", + "https://example.com/orgin", "", L"", L"", L"", L"u", + kTestingFederatedLoginMarker, false, 1}, + "http://example.com/", + false, + false}, + {{PasswordForm::SCHEME_HTML, "android://hash@example.com/", + "android://hash@example.com/", "", L"", L"", L"", L"u", L"p", false, 1}, + "http://example.com/", + false, + false}, + }; + + for (const auto& test_case : kTestCases) { + SCOPED_TRACE(test_case.form_data.signon_realm); + + std::unique_ptr<PasswordForm> form = + CreatePasswordFormFromDataForTesting(test_case.form_data); + ASSERT_EQ(AddChangeForForm(*form), db().AddLogin(*form)); + + std::vector<std::unique_ptr<PasswordForm>> same_organization_forms; + EXPECT_TRUE(db().GetLoginsForSameOrganizationName( + form->signon_realm, &same_organization_forms)); + EXPECT_EQ(test_case.expected_matches_itself ? 1u : 0u, + same_organization_forms.size()); + + if (test_case.other_queried_signon_realm) { + same_organization_forms.clear(); + EXPECT_TRUE(db().GetLoginsForSameOrganizationName( + test_case.other_queried_signon_realm, &same_organization_forms)); + EXPECT_EQ(test_case.expected_matches_other_realm ? 1u : 0u, + same_organization_forms.size()); + } + + ASSERT_TRUE(db().RemoveLogin(*form)); + } +} + +TEST_F(LoginDatabaseTest, GetLoginsForSameOrganizationName_DetailsOfMatching) { + const struct { + const char* saved_signon_realm; + const char* queried_signon_realm; + bool expected_matches; + } kTestCases[] = { + // PSL matches are also same-organization-name matches. + {"http://psl.example.com/", "http://example.com/", true}, + {"http://example.com/", "http://sub.example.com/", true}, + {"https://a.b.example.co.uk/", "https://c.d.e.example.co.uk/", true}, + + // Non-PSL but same-organization-name matches. Also an illustration why it + // would be unsafe to offer these credentials for filling. + {"https://example.com/", "https://example.co.uk/", true}, + {"https://example.co.uk/", "https://example.com/", true}, + {"https://a.example.appspot.com/", "https://b.example.co.uk/", true}, + + // Same-organization-name matches are HTTP/HTTPS-agnostic. + {"https://example.com/", "http://example.com/", true}, + {"http://example.com/", "https://example.com/", true}, + + {"http://www.foo-bar.com/", "http://sub.foo-bar.com", true}, + {"http://www.foo_bar.com/", "http://sub.foo_bar.com", true}, + {"http://www.foo-bar.com/", "http://sub.foo%2Dbar.com", true}, + {"http://www.foo%21bar.com/", "http://sub.foo!bar.com", true}, + {"http://a.xn--sztr-7na0i.co.uk/", "http://xn--sztr-7na0i.com/", true}, + {"http://a.xn--sztr-7na0i.co.uk/", "http://www.sz\xc3\xb3t\xc3\xa1r.com/", + true}, + + {"http://www.foo+bar.com/", "http://sub.foo+bar.com", true}, + {"http://www.foooobar.com/", "http://sub.foo+bar.com", false}, + {"http://www.fobar.com/", "http://sub.foo?bar.com", false}, + {"http://www.foozbar.com/", "http://sub.foo.bar.com", false}, + {"http://www.foozbar.com/", "http://sub.foo[a-z]bar.com", false}, + + {"https://notexample.com/", "https://example.com/", false}, + {"https://a.notexample.com/", "https://example.com/", false}, + {"https://example.com/", "https://notexample.com/", false}, + {"https://example.com/", "https://example.bar.com/", false}, + {"https://example.foo.com/", "https://example.com/", false}, + {"https://example.foo.com/", "https://example.bar.com/", false}, + + // URLs without host portions, hosts without registry controlled domains + // or hosts consisting of a registry. + {"http://localhost/", "http://localhost/", false}, + {"https://example/", "https://example/", false}, + {"https://co.uk/", "https://co.uk/", false}, + {"https://example/", "https://example.com/", false}, + {"https://a.example/", "https://example.com/", false}, + {"https://example.com/", "https://example/", false}, + {"https://127.0.0.1/", "https://127.0.0.1/", false}, + {"https:/[3ffe:2a00:100:7031::1]/", "https:/[3ffe:2a00:100:7031::1]/", + false}, + + // Queried |signon-realms| are invalid URIs. + {"https://example.com/", "", false}, + {"https://example.com/", "bad url", false}, + {"https://example.com/", "https://", false}, + {"https://example.com/", "http://www.foo;bar.com", false}, + {"https://example.com/", "example", false}, + }; + + for (const auto& test_case : kTestCases) { + SCOPED_TRACE(test_case.saved_signon_realm); + SCOPED_TRACE(test_case.queried_signon_realm); + + std::unique_ptr<PasswordForm> form = CreatePasswordFormFromDataForTesting( + {PasswordForm::SCHEME_HTML, test_case.saved_signon_realm, + test_case.saved_signon_realm, "", L"", L"", L"", L"u", L"p", true, 1}); + std::vector<std::unique_ptr<PasswordForm>> result; + ASSERT_EQ(AddChangeForForm(*form), db().AddLogin(*form)); + EXPECT_TRUE(db().GetLoginsForSameOrganizationName( + test_case.queried_signon_realm, &result)); + EXPECT_EQ(test_case.expected_matches ? 1u : 0u, result.size()); + ASSERT_TRUE(db().RemoveLogin(*form)); + } +} + static bool AddTimestampedLogin(LoginDatabase* db, std::string url, const std::string& unique_string, diff --git a/chromium/components/password_manager/core/browser/mock_password_store.h b/chromium/components/password_manager/core/browser/mock_password_store.h index a8dbbf4d36b..fe210f3da18 100644 --- a/chromium/components/password_manager/core/browser/mock_password_store.h +++ b/chromium/components/password_manager/core/browser/mock_password_store.h @@ -13,18 +13,23 @@ #include "components/password_manager/core/browser/statistics_table.h" #include "testing/gmock/include/gmock/gmock.h" +class PrefService; + namespace password_manager { class MockPasswordStore : public PasswordStore { public: MockPasswordStore(); - bool Init(const syncer::SyncableService::StartSyncFlare& flare) override { + bool Init(const syncer::SyncableService::StartSyncFlare& flare, + PrefService* prefs) override { return true; }; MOCK_METHOD1(RemoveLogin, void(const autofill::PasswordForm&)); MOCK_METHOD2(GetLogins, void(const PasswordStore::FormDigest&, PasswordStoreConsumer*)); + MOCK_METHOD2(GetLoginsForSameOrganizationName, + void(const std::string&, PasswordStoreConsumer*)); MOCK_METHOD1(AddLogin, void(const autofill::PasswordForm&)); MOCK_METHOD1(UpdateLogin, void(const autofill::PasswordForm&)); MOCK_METHOD2(UpdateLoginWithPrimaryKey, @@ -57,6 +62,10 @@ class MockPasswordStore : public PasswordStore { const PasswordStore::FormDigest& form) override { return std::vector<std::unique_ptr<autofill::PasswordForm>>(); } + std::vector<std::unique_ptr<autofill::PasswordForm>> + FillLoginsForSameOrganizationName(const std::string& signon_realm) override { + return std::vector<std::unique_ptr<autofill::PasswordForm>>(); + } MOCK_METHOD1(FillAutofillableLogins, bool(std::vector<std::unique_ptr<autofill::PasswordForm>>*)); MOCK_METHOD1(FillBlacklistLogins, diff --git a/chromium/components/password_manager/core/browser/password_form_manager.cc b/chromium/components/password_manager/core/browser/password_form_manager.cc index eb88be67ab7..98100d29dd9 100644 --- a/chromium/components/password_manager/core/browser/password_form_manager.cc +++ b/chromium/components/password_manager/core/browser/password_form_manager.cc @@ -11,6 +11,7 @@ #include <utility> #include "base/feature_list.h" +#include "base/logging.h" #include "base/memory/ptr_util.h" #include "base/metrics/histogram_macros.h" #include "base/metrics/user_metrics.h" @@ -197,14 +198,6 @@ void LabelFields(const FieldTypeMap& field_types, } } -// Check whether |form_data| corresponds to a 2 field form with 1 text field and -// 1 password field. Such form is likely sign-in form. -bool IsSignInSubmission(const FormData& form_data) { - return form_data.fields.size() == 2 && - form_data.fields[0].form_control_type == "text" && - form_data.fields[1].form_control_type == "password"; -} - } // namespace PasswordFormManager::PasswordFormManager( @@ -239,12 +232,13 @@ PasswordFormManager::PasswordFormManager( submit_result_(kSubmitResultNotSubmitted), form_type_(kFormTypeUnspecified), form_saver_(std::move(form_saver)), - owned_form_fetcher_(form_fetcher - ? nullptr - : base::MakeUnique<FormFetcherImpl>( - PasswordStore::FormDigest(observed_form), - client, - /* should_migrate_http_passwords */ true)), + owned_form_fetcher_( + form_fetcher ? nullptr + : base::MakeUnique<FormFetcherImpl>( + PasswordStore::FormDigest(observed_form), + client, + true /* should_migrate_http_passwords */, + true /* should_query_suppressed_https_forms */)), form_fetcher_(form_fetcher ? form_fetcher : owned_form_fetcher_.get()), is_main_frame_secure_(client->IsMainFrameSecure()) { if (owned_form_fetcher_) @@ -261,6 +255,7 @@ PasswordFormManager::~PasswordFormManager() { UMA_HISTOGRAM_ENUMERATION("PasswordManager.ActionsTakenV3", GetActionsTaken(), kMaxNumActionsTaken); + // Use the visible main frame URL at the time the PasswordFormManager // is created, in case a navigation has already started and the // visible URL has changed. @@ -268,6 +263,9 @@ PasswordFormManager::~PasswordFormManager() { UMA_HISTOGRAM_ENUMERATION("PasswordManager.ActionsTakenOnNonSecureForm", GetActionsTaken(), kMaxNumActionsTaken); } + + RecordHistogramsOnSuppressedAccounts(); + if (submit_result_ == kSubmitResultNotSubmitted) { if (has_generated_password_) metrics_util::LogPasswordGenerationSubmissionEvent( @@ -276,6 +274,7 @@ PasswordFormManager::~PasswordFormManager() { metrics_util::LogPasswordGenerationAvailableSubmissionEvent( metrics_util::PASSWORD_NOT_SUBMITTED); } + if (form_type_ != kFormTypeUnspecified) { UMA_HISTOGRAM_ENUMERATION("PasswordManager.SubmittedFormType", form_type_, kFormTypeMax); @@ -292,6 +291,95 @@ int PasswordFormManager::GetActionsTaken() const { (manager_action_ + kManagerActionMax * submit_result_); } +int PasswordFormManager::GetHistogramSampleForSuppressedAccounts( + const std::vector<const autofill::PasswordForm*> suppressed_forms, + PasswordForm::Type manual_or_generated) const { + DCHECK(form_fetcher_->DidCompleteQueryingSuppressedForms()); + + SuppressedAccountExistence best_matching_account = kSuppressedAccountNone; + for (const autofill::PasswordForm* form : suppressed_forms) { + if (form->type != manual_or_generated) + continue; + + SuppressedAccountExistence current_account; + if (pending_credentials_.password_value.empty()) + current_account = kSuppressedAccountExists; + else if (form->username_value != pending_credentials_.username_value) + current_account = kSuppressedAccountExistsDifferentUsername; + else if (form->password_value != pending_credentials_.password_value) + current_account = kSuppressedAccountExistsSameUsername; + else + current_account = kSuppressedAccountExistsSameUsernameAndPassword; + + best_matching_account = std::max(best_matching_account, current_account); + } + + // Merge kManagerActionNone and kManagerActionBlacklisted_Obsolete. This + // lowers the number of histogram buckets used by 33%. + ManagerActionNew manager_action_new = + (manager_action_ == kManagerActionAutofilled) + ? kManagerActionNewAutofilled + : kManagerActionNewNone; + + // Encoding: most significant digit is the |best_matching_account|. + int mixed_base_encoding = 0; + mixed_base_encoding += best_matching_account; + (mixed_base_encoding *= kSubmitResultMax) += submit_result_; + (mixed_base_encoding *= kManagerActionNewMax) += manager_action_new; + (mixed_base_encoding *= kUserActionMax) += user_action_; + DCHECK_LT(mixed_base_encoding, kMaxSuppressedAccountStats); + return mixed_base_encoding; +} + +void PasswordFormManager::RecordHistogramsOnSuppressedAccounts() const { + UMA_HISTOGRAM_BOOLEAN("PasswordManager.QueryingSuppressedAccountsFinished", + form_fetcher_->DidCompleteQueryingSuppressedForms()); + + if (!form_fetcher_->DidCompleteQueryingSuppressedForms()) + return; + + if (!observed_form_.origin.SchemeIsCryptographic()) { + UMA_HISTOGRAM_ENUMERATION( + "PasswordManager.SuppressedAccount.Generated.HTTPSNotHTTP", + GetHistogramSampleForSuppressedAccounts( + form_fetcher_->GetSuppressedHTTPSForms(), + PasswordForm::TYPE_GENERATED), + kMaxSuppressedAccountStats); + UMA_HISTOGRAM_ENUMERATION( + "PasswordManager.SuppressedAccount.Manual.HTTPSNotHTTP", + GetHistogramSampleForSuppressedAccounts( + form_fetcher_->GetSuppressedHTTPSForms(), + PasswordForm::TYPE_MANUAL), + kMaxSuppressedAccountStats); + } + + UMA_HISTOGRAM_ENUMERATION( + "PasswordManager.SuppressedAccount.Generated.PSLMatching", + GetHistogramSampleForSuppressedAccounts( + form_fetcher_->GetSuppressedPSLMatchingForms(), + PasswordForm::TYPE_GENERATED), + kMaxSuppressedAccountStats); + UMA_HISTOGRAM_ENUMERATION( + "PasswordManager.SuppressedAccount.Manual.PSLMatching", + GetHistogramSampleForSuppressedAccounts( + form_fetcher_->GetSuppressedPSLMatchingForms(), + PasswordForm::TYPE_MANUAL), + kMaxSuppressedAccountStats); + + UMA_HISTOGRAM_ENUMERATION( + "PasswordManager.SuppressedAccount.Generated.SameOrganizationName", + GetHistogramSampleForSuppressedAccounts( + form_fetcher_->GetSuppressedSameOrganizationNameForms(), + PasswordForm::TYPE_GENERATED), + kMaxSuppressedAccountStats); + UMA_HISTOGRAM_ENUMERATION( + "PasswordManager.SuppressedAccount.Manual.SameOrganizationName", + GetHistogramSampleForSuppressedAccounts( + form_fetcher_->GetSuppressedSameOrganizationNameForms(), + PasswordForm::TYPE_MANUAL), + kMaxSuppressedAccountStats); +} + // static base::string16 PasswordFormManager::PasswordToSave(const PasswordForm& form) { if (form.new_password_element.empty() || form.new_password_value.empty()) @@ -419,6 +507,9 @@ void PasswordFormManager::Save() { DCHECK_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState()); DCHECK(!client_->IsIncognito()); + metrics_util::LogPasswordAcceptedSaveUpdateSubmissionIndicatorEvent( + submitted_form_->submission_event); + if ((user_action_ == kUserActionNone) && DidPreferenceChange(best_matches_, pending_credentials_.username_value)) { SetUserAction(kUserActionChoose); @@ -451,6 +542,8 @@ void PasswordFormManager::Save() { void PasswordFormManager::Update( const autofill::PasswordForm& credentials_to_update) { + metrics_util::LogPasswordAcceptedSaveUpdateSubmissionIndicatorEvent( + submitted_form_->submission_event); if (observed_form_.IsPossibleChangePasswordForm()) { FormStructure form_structure(credentials_to_update.form_data); UploadPasswordVote(observed_form_, autofill::NEW_PASSWORD, @@ -623,19 +716,12 @@ void PasswordFormManager::ProcessFrameInternal( // Proceed to autofill. // Note that we provide the choices but don't actually prefill a value if: - // (1) we are in Incognito mode, (2) the ACTION paths don't match, - // (3) if it matched using public suffix domain matching, or - // (4) the form is change password form. - // However, 2 and 3 should not apply to Android-based credentials found - // via affiliation-based matching (we want to autofill them). - // TODO(engedy): Clean this up. See: https://crbug.com/476519. - bool wait_for_username = - client_->IsIncognito() || - (!IsValidAndroidFacetURI(preferred_match_->signon_realm) && - (observed_form_.action.GetWithEmptyPath() != - preferred_match_->action.GetWithEmptyPath() || - preferred_match_->is_public_suffix_match || - observed_form_.IsPossibleChangePasswordForm())); + // (1) we are in Incognito mode, or + // (2) if it matched using public suffix domain matching, or + // (3) the form is change password form. + bool wait_for_username = client_->IsIncognito() || + preferred_match_->is_public_suffix_match || + observed_form_.IsPossibleChangePasswordForm(); if (wait_for_username) { manager_action_ = kManagerActionNone; } else { @@ -1298,8 +1384,16 @@ void PasswordFormManager::SendVotesOnSave() { if (observed_form_.IsPossibleChangePasswordFormWithoutUsername()) return; - if (IsSignInSubmission(pending_credentials_.form_data)) { - SendSignInVote(pending_credentials_.form_data); + // Send votes for sign-in form. + autofill::FormData& form_data = pending_credentials_.form_data; + if (form_data.fields.size() == 2 && + form_data.fields[0].form_control_type == "text" && + form_data.fields[1].form_control_type == "password") { + // |form_data| is received from the renderer and does not contain field + // values. Fill username field value with username to allow AutofillManager + // to detect username autofill type. + form_data.fields[0].value = pending_credentials_.username_value; + SendSignInVote(form_data); return; } diff --git a/chromium/components/password_manager/core/browser/password_form_manager.h b/chromium/components/password_manager/core/browser/password_form_manager.h index 0ad2261d268..2bc3bb26ab8 100644 --- a/chromium/components/password_manager/core/browser/password_form_manager.h +++ b/chromium/components/password_manager/core/browser/password_form_manager.h @@ -273,6 +273,13 @@ class PasswordFormManager : public FormFetcher::Consumer { kManagerActionMax }; + // Same as above, without the obsoleted 'Blacklisted' action. + enum ManagerActionNew { + kManagerActionNewNone, + kManagerActionNewAutofilled, + kManagerActionNewMax + }; + // UserAction - What does the user do with this form? If they do nothing // (either by accepting what the password manager did, or by simply (not // typing anything at all), you get None. If there were multiple choices and @@ -325,6 +332,30 @@ class PasswordFormManager : public FormFetcher::Consumer { static const int kMaxNumActionsTaken = kManagerActionMax * kUserActionMax * kSubmitResultMax; + // Enumerates whether there were `suppressed` credentials. These are stored + // credentials that were not filled, even though they might be related to the + // |observed_form_|. See FormFetcher::GetSuppressed* for details. + // + // If suppressed credentials exist, it is also recorded whether their username + // and/or password matched those submitted. + enum SuppressedAccountExistence { + kSuppressedAccountNone, + // Recorded when there exists a suppressed account, but there was no + // submitted form to compare its username and password to. + kSuppressedAccountExists, + // Recorded when there was a submitted form. + kSuppressedAccountExistsDifferentUsername, + kSuppressedAccountExistsSameUsername, + kSuppressedAccountExistsSameUsernameAndPassword, + kSuppressedAccountExistenceMax, + }; + + // The maximum number of combinations recorded into histograms in the + // PasswordManager.SuppressedAccount.* family. + static constexpr int kMaxSuppressedAccountStats = + kSuppressedAccountExistenceMax * kManagerActionNewMax * kUserActionMax * + kSubmitResultMax; + // Through |driver|, supply the associated frame with appropriate information // (fill data, whether to allow password generation, etc.). void ProcessFrameInternal(const base::WeakPtr<PasswordManagerDriver>& driver); @@ -399,6 +430,23 @@ class PasswordFormManager : public FormFetcher::Consumer { // UMA. int GetActionsTaken() const; + // When supplied with the list of all |suppressed_forms| that belong to + // certain suppressed credential type (see FormFetcher::GetSuppressed*), + // filters that list down to forms that are either |manual_or_generated|, and + // based on that, computes the histogram sample that is a mixed-based + // representation of a combination of four attributes: + // -- whether there were suppressed credentials (and if so, their relation to + // the submitted username/password). + // -- whether the |observed_form_| got ultimately submitted + // -- what action the password manager performed (|manager_action_|), + // -- and what action the user performed (|user_action_|_). + int GetHistogramSampleForSuppressedAccounts( + const std::vector<const autofill::PasswordForm*> suppressed_forms, + autofill::PasswordForm::Type manual_or_generated) const; + + // Records all histograms in the PasswordManager.SuppressedAccount.* family. + void RecordHistogramsOnSuppressedAccounts() const; + // Tries to set all votes (e.g. autofill field types, generation vote) to // a |FormStructure| and upload it to the server. Returns true on success. bool UploadPasswordVote(const autofill::PasswordForm& form_to_upload, @@ -469,7 +517,7 @@ class PasswordFormManager : public FormFetcher::Consumer { // Set of blacklisted forms from the PasswordStore that best match the current // form. They are owned by |form_fetcher_|, with the exception that if // |new_blacklisted_| is not null, the address of that form is also inside - // |blacklisted_matches_|.. + // |blacklisted_matches_|. std::vector<const autofill::PasswordForm*> blacklisted_matches_; // If the observed form gets blacklisted through |this|, the blacklist entry diff --git a/chromium/components/password_manager/core/browser/password_form_manager_unittest.cc b/chromium/components/password_manager/core/browser/password_form_manager_unittest.cc index fd752c0aa7a..e4ed9f70800 100644 --- a/chromium/components/password_manager/core/browser/password_form_manager_unittest.cc +++ b/chromium/components/password_manager/core/browser/password_form_manager_unittest.cc @@ -713,6 +713,92 @@ class PasswordFormManagerTest : public testing::Test { // To spare typing for PasswordFormManager instances which need no driver. const base::WeakPtr<PasswordManagerDriver> kNoDriver; + protected: + enum class SimulatedManagerAction { NONE, AUTOFILLED, OFFERED, OFFERED_PSL }; + enum class SimulatedSubmitResult { NONE, PASSED, FAILED }; + enum class SuppressedFormType { HTTPS, PSL_MATCH, SAME_ORGANIZATION_NAME }; + + PasswordForm CreateSuppressedForm(SuppressedFormType suppression_type, + const char* username, + const char* password, + PasswordForm::Type manual_or_generated) { + PasswordForm form = *saved_match(); + switch (suppression_type) { + case SuppressedFormType::HTTPS: + form.origin = GURL("https://accounts.google.com/a/LoginAuth"); + form.signon_realm = "https://accounts.google.com/"; + break; + case SuppressedFormType::PSL_MATCH: + form.origin = GURL("http://other.google.com/"); + form.signon_realm = "http://other.google.com/"; + break; + case SuppressedFormType::SAME_ORGANIZATION_NAME: + form.origin = GURL("https://may-or-may-not-be.google.appspot.com/"); + form.signon_realm = "https://may-or-may-not-be.google.appspot.com/"; + break; + } + form.type = manual_or_generated; + form.username_value = ASCIIToUTF16(username); + form.password_value = ASCIIToUTF16(password); + return form; + } + + void SimulateActionsOnHTTPObservedForm( + FakeFormFetcher* fetcher, + SimulatedManagerAction manager_action, + SimulatedSubmitResult submit_result, + const char* filled_username, + const char* filled_password, + const char* submitted_password = nullptr) { + PasswordFormManager form_manager( + password_manager(), client(), client()->driver(), *observed_form(), + base::MakeUnique<NiceMock<MockFormSaver>>(), fetcher); + + EXPECT_CALL(*client()->mock_driver()->mock_autofill_download_manager(), + StartUploadRequest(_, _, _, _, _)) + .Times(::testing::AnyNumber()); + + PasswordForm http_stored_form = *saved_match(); + http_stored_form.username_value = base::ASCIIToUTF16(filled_username); + http_stored_form.password_value = base::ASCIIToUTF16(filled_password); + if (manager_action == SimulatedManagerAction::OFFERED_PSL) + http_stored_form.is_public_suffix_match = true; + + std::vector<const PasswordForm*> matches; + if (manager_action != SimulatedManagerAction::NONE) + matches.push_back(&http_stored_form); + + // Extra mile: kUserActionChoose is only recorded if there were multiple + // logins available and the preferred one was changed. + PasswordForm http_stored_form2 = http_stored_form; + if (manager_action == SimulatedManagerAction::OFFERED) { + http_stored_form.preferred = false; + http_stored_form2.username_value = ASCIIToUTF16("user-other@gmail.com"); + matches.push_back(&http_stored_form2); + } + + fetcher->Fetch(); + fetcher->SetNonFederated(matches, 0u); + + if (submit_result != SimulatedSubmitResult::NONE) { + PasswordForm submitted_form(*observed_form()); + submitted_form.preferred = true; + submitted_form.username_value = base::ASCIIToUTF16(filled_username); + submitted_form.password_value = + submitted_password ? base::ASCIIToUTF16(submitted_password) + : base::ASCIIToUTF16(filled_password); + + form_manager.ProvisionallySave( + submitted_form, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES); + if (submit_result == SimulatedSubmitResult::PASSED) { + form_manager.LogSubmitPassed(); + form_manager.Save(); + } else { + form_manager.LogSubmitFailed(); + } + } + } + private: // Necessary for callbacks, and for TestAutofillDriver. base::MessageLoop message_loop_; @@ -3149,6 +3235,8 @@ TEST_F(PasswordFormManagerTest, UploadSignInForm_WithAutofillTypes) { ASSERT_EQ(2u, uploaded_form_structure->field_count()); autofill::ServerFieldTypeSet expected_types = {autofill::PASSWORD}; + EXPECT_EQ(form_to_save.username_value, + uploaded_form_structure->field(0)->value); EXPECT_EQ(expected_types, uploaded_form_structure->field(1)->possible_types()); } @@ -3180,4 +3268,231 @@ TEST_F(PasswordFormManagerTest, NoUploadsForSubmittedFormWithOnlyOneField) { form_manager.Save(); } +TEST_F(PasswordFormManagerTest, + SuppressedHTTPSFormsHistogram_NotRecordedIfStoreWasTooSlow) { + base::HistogramTester histogram_tester; + + fake_form_fetcher()->set_did_complete_querying_suppressed_forms(false); + fake_form_fetcher()->Fetch(); + std::unique_ptr<PasswordFormManager> form_manager = + base::MakeUnique<PasswordFormManager>( + password_manager(), client(), client()->driver(), *observed_form(), + base::MakeUnique<NiceMock<MockFormSaver>>(), fake_form_fetcher()); + fake_form_fetcher()->SetNonFederated(std::vector<const PasswordForm*>(), 0u); + form_manager.reset(); + + histogram_tester.ExpectUniqueSample( + "PasswordManager.QueryingSuppressedAccountsFinished", false, 1); + const auto sample_counts = histogram_tester.GetTotalCountsForPrefix( + "PasswordManager.SuppressedAccount."); + EXPECT_THAT(sample_counts, IsEmpty()); +} + +TEST_F(PasswordFormManagerTest, SuppressedFormsHistograms) { + static constexpr const struct { + SuppressedFormType type; + const char* expected_histogram_suffix; + void (FakeFormFetcher::*suppressed_forms_setter_func)( + const std::vector<const autofill::PasswordForm*>&); + } kSuppressedFormTypeParams[] = { + {SuppressedFormType::HTTPS, "HTTPSNotHTTP", + &FakeFormFetcher::set_suppressed_https_forms}, + {SuppressedFormType::PSL_MATCH, "PSLMatching", + &FakeFormFetcher::set_suppressed_psl_matching_forms}, + {SuppressedFormType::SAME_ORGANIZATION_NAME, "SameOrganizationName", + &FakeFormFetcher::set_suppressed_same_organization_name_forms}}; + + struct SuppressedFormData { + const char* username_value; + const char* password_value; + PasswordForm::Type manual_or_generated; + }; + + static constexpr const char kUsernameAlpha[] = "user-alpha@gmail.com"; + static constexpr const char kPasswordAlpha[] = "password-alpha"; + static constexpr const char kUsernameBeta[] = "user-beta@gmail.com"; + static constexpr const char kPasswordBeta[] = "password-beta"; + + static constexpr const SuppressedFormData kSuppressedGeneratedForm = { + kUsernameAlpha, kPasswordAlpha, PasswordForm::TYPE_GENERATED}; + static constexpr const SuppressedFormData kOtherSuppressedGeneratedForm = { + kUsernameBeta, kPasswordBeta, PasswordForm::TYPE_GENERATED}; + static constexpr const SuppressedFormData kSuppressedManualForm = { + kUsernameAlpha, kPasswordBeta, PasswordForm::TYPE_MANUAL}; + + const std::vector<const SuppressedFormData*> kSuppressedFormsNone; + const std::vector<const SuppressedFormData*> kSuppressedFormsOneGenerated = { + &kSuppressedGeneratedForm}; + const std::vector<const SuppressedFormData*> kSuppressedFormsTwoGenerated = { + &kSuppressedGeneratedForm, &kOtherSuppressedGeneratedForm}; + const std::vector<const SuppressedFormData*> kSuppressedFormsOneManual = { + &kSuppressedManualForm}; + const std::vector<const SuppressedFormData*> kSuppressedFormsTwoMixed = { + &kSuppressedGeneratedForm, &kSuppressedManualForm}; + + const struct { + std::vector<const SuppressedFormData*> simulated_suppressed_forms; + SimulatedManagerAction simulate_manager_action; + SimulatedSubmitResult simulate_submit_result; + const char* filled_username; + const char* filled_password; + int expected_histogram_sample_generated; + int expected_histogram_sample_manual; + const char* submitted_password; // nullptr if same as |filled_password|. + } kTestCases[] = { + // See PasswordManagerSuppressedAccountCrossActionsTaken in enums.xml. + // + // Legend: (SuppressAccountType, SubmitResult, SimulatedManagerAction, + // UserAction) + // 24 = (None, Passed, None, OverrideUsernameAndPassword) + {kSuppressedFormsNone, SimulatedManagerAction::NONE, + SimulatedSubmitResult::PASSED, kUsernameAlpha, kPasswordAlpha, 24, 24}, + // 5 = (None, NotSubmitted, Autofilled, None) + {kSuppressedFormsNone, SimulatedManagerAction::AUTOFILLED, + SimulatedSubmitResult::NONE, kUsernameAlpha, kPasswordAlpha, 5, 5}, + // 15 = (None, Failed, Autofilled, None) + {kSuppressedFormsNone, SimulatedManagerAction::AUTOFILLED, + SimulatedSubmitResult::FAILED, kUsernameAlpha, kPasswordAlpha, 15, 15}, + + // 35 = (Exists, NotSubmitted, Autofilled, None) + {kSuppressedFormsOneGenerated, SimulatedManagerAction::AUTOFILLED, + SimulatedSubmitResult::NONE, kUsernameAlpha, kPasswordAlpha, 35, 5}, + // 145 = (ExistsSameUsernameAndPassword, Passed, Autofilled, None) + // 25 = (None, Passed, Autofilled, None) + {kSuppressedFormsOneGenerated, SimulatedManagerAction::AUTOFILLED, + SimulatedSubmitResult::PASSED, kUsernameAlpha, kPasswordAlpha, 145, 25}, + // 104 = (ExistsSameUsername, Failed, None, OverrideUsernameAndPassword) + // 14 = (None, Failed, None, OverrideUsernameAndPassword) + {kSuppressedFormsOneGenerated, SimulatedManagerAction::NONE, + SimulatedSubmitResult::FAILED, kUsernameAlpha, kPasswordBeta, 104, 14}, + // 84 = (ExistsDifferentUsername, Passed, None, + // OverrideUsernameAndPassword) + {kSuppressedFormsOneGenerated, SimulatedManagerAction::NONE, + SimulatedSubmitResult::PASSED, kUsernameBeta, kPasswordAlpha, 84, 24}, + + // 144 = (ExistsSameUsernameAndPassword, Passed, None, + // OverrideUsernameAndPassword) + {kSuppressedFormsOneManual, SimulatedManagerAction::NONE, + SimulatedSubmitResult::PASSED, kUsernameAlpha, kPasswordBeta, 24, 144}, + {kSuppressedFormsTwoMixed, SimulatedManagerAction::NONE, + SimulatedSubmitResult::PASSED, kUsernameBeta, kPasswordAlpha, 84, 84}, + + // 115 = (ExistsSameUsername, Passed, Autofilled, None) + {kSuppressedFormsTwoGenerated, SimulatedManagerAction::AUTOFILLED, + SimulatedSubmitResult::PASSED, kUsernameAlpha, kPasswordAlpha, 145, 25}, + {kSuppressedFormsTwoGenerated, SimulatedManagerAction::AUTOFILLED, + SimulatedSubmitResult::PASSED, kUsernameAlpha, kPasswordBeta, 115, 25}, + {kSuppressedFormsTwoGenerated, SimulatedManagerAction::AUTOFILLED, + SimulatedSubmitResult::PASSED, kUsernameBeta, kPasswordAlpha, 115, 25}, + {kSuppressedFormsTwoGenerated, SimulatedManagerAction::AUTOFILLED, + SimulatedSubmitResult::PASSED, kUsernameBeta, kPasswordBeta, 145, 25}, + + // 86 = (ExistsDifferentUsername, Passed, Autofilled, Choose) + // 26 = (None, Passed, Autofilled, Choose) + {kSuppressedFormsOneGenerated, SimulatedManagerAction::OFFERED, + SimulatedSubmitResult::PASSED, kUsernameBeta, kPasswordAlpha, 86, 26}, + // 72 = (ExistsDifferentUsername, Failed, None, ChoosePSL) + // 12 = (None, Failed, None, ChoosePSL) + {kSuppressedFormsOneGenerated, SimulatedManagerAction::OFFERED_PSL, + SimulatedSubmitResult::FAILED, kUsernameBeta, kPasswordAlpha, 72, 12}, + // 148 = (ExistsSameUsernameAndPassword, Passed, Autofilled, + // OverridePassword) + // 28 = (None, Passed, Autofilled, OverridePassword) + {kSuppressedFormsTwoGenerated, SimulatedManagerAction::AUTOFILLED, + SimulatedSubmitResult::PASSED, kUsernameBeta, kPasswordAlpha, 148, 28, + kPasswordBeta}, + }; + + for (const auto& suppression_params : kSuppressedFormTypeParams) { + for (const auto& test_case : kTestCases) { + SCOPED_TRACE(suppression_params.expected_histogram_suffix); + SCOPED_TRACE(test_case.expected_histogram_sample_manual); + SCOPED_TRACE(test_case.expected_histogram_sample_generated); + + base::HistogramTester histogram_tester; + + std::vector<PasswordForm> suppressed_forms; + for (const auto* form_data : test_case.simulated_suppressed_forms) { + suppressed_forms.push_back(CreateSuppressedForm( + suppression_params.type, form_data->username_value, + form_data->password_value, form_data->manual_or_generated)); + } + + std::vector<const PasswordForm*> suppressed_forms_ptrs; + for (const auto& form : suppressed_forms) + suppressed_forms_ptrs.push_back(&form); + + FakeFormFetcher fetcher; + fetcher.set_did_complete_querying_suppressed_forms(true); + + (&fetcher->*suppression_params.suppressed_forms_setter_func)( + suppressed_forms_ptrs); + + SimulateActionsOnHTTPObservedForm( + &fetcher, test_case.simulate_manager_action, + test_case.simulate_submit_result, test_case.filled_username, + test_case.filled_password, test_case.submitted_password); + + histogram_tester.ExpectUniqueSample( + "PasswordManager.QueryingSuppressedAccountsFinished", true, 1); + + EXPECT_THAT( + histogram_tester.GetAllSamples( + "PasswordManager.SuppressedAccount.Generated." + + std::string(suppression_params.expected_histogram_suffix)), + ::testing::ElementsAre( + base::Bucket(test_case.expected_histogram_sample_generated, 1))); + EXPECT_THAT( + histogram_tester.GetAllSamples( + "PasswordManager.SuppressedAccount.Manual." + + std::string(suppression_params.expected_histogram_suffix)), + ::testing::ElementsAre( + base::Bucket(test_case.expected_histogram_sample_manual, 1))); + } + } +} + +// If the frame containing the login form is served over HTTPS, no histograms on +// supressed HTTPS forms should be recorded. Everything else should still be. +TEST_F(PasswordFormManagerTest, SuppressedHTTPSFormsHistogram_NotRecordedFor) { + base::HistogramTester histogram_tester; + + PasswordForm https_observed_form = *observed_form(); + https_observed_form.origin = GURL("https://accounts.google.com/a/LoginAuth"); + https_observed_form.signon_realm = "https://accounts.google.com/"; + + // Only the scheme of the frame containing the login form maters, not the + // scheme of the main frame. + ASSERT_FALSE(client()->IsMainFrameSecure()); + + // Even if there were any suppressed HTTPS forms, they should be are ignored + // (but there should be none in production in this case). + FakeFormFetcher fetcher; + fetcher.set_suppressed_https_forms({saved_match()}); + fetcher.set_did_complete_querying_suppressed_forms(true); + fetcher.Fetch(); + + std::unique_ptr<PasswordFormManager> form_manager = + base::MakeUnique<PasswordFormManager>( + password_manager(), client(), client()->driver(), https_observed_form, + base::MakeUnique<NiceMock<MockFormSaver>>(), &fetcher); + fetcher.SetNonFederated(std::vector<const PasswordForm*>(), 0u); + form_manager.reset(); + + histogram_tester.ExpectUniqueSample( + "PasswordManager.QueryingSuppressedAccountsFinished", true, 1); + histogram_tester.ExpectTotalCount( + "PasswordManager.SuppressedAccount.Generated.HTTPSNotHTTP", 0); + histogram_tester.ExpectTotalCount( + "PasswordManager.SuppressedAccount.Manual.HTTPSNotHTTP", 0); + histogram_tester.ExpectTotalCount( + "PasswordManager.SuppressedAccount.Generated.PSLMatching", 1); + histogram_tester.ExpectTotalCount( + "PasswordManager.SuppressedAccount.Manual.PSLMatching", 1); + histogram_tester.ExpectTotalCount( + "PasswordManager.SuppressedAccount.Generated.SameOrganizationName", 1); + histogram_tester.ExpectTotalCount( + "PasswordManager.SuppressedAccount.Manual.SameOrganizationName", 1); +} + } // namespace password_manager diff --git a/chromium/components/password_manager/core/browser/password_manager.cc b/chromium/components/password_manager/core/browser/password_manager.cc index dae64390d35..804aa5d4348 100644 --- a/chromium/components/password_manager/core/browser/password_manager.cc +++ b/chromium/components/password_manager/core/browser/password_manager.cc @@ -155,6 +155,8 @@ void PasswordManager::RegisterProfilePrefs( prefs::kCredentialsEnableAutosignin, true, user_prefs::PrefRegistrySyncable::SYNCABLE_PRIORITY_PREF); registry->RegisterBooleanPref(prefs::kWasObsoleteHttpDataCleaned, false); + registry->RegisterStringPref(prefs::kSyncPasswordHash, std::string(), + PrefRegistry::NO_REGISTRATION_FLAGS); #if defined(OS_MACOSX) registry->RegisterIntegerPref( prefs::kKeychainMigrationStatus, @@ -375,6 +377,29 @@ void PasswordManager::ProvisionallySavePassword( // post-submit navigation concludes, we compare the landing URL against the // cached and report the difference through UMA. main_frame_url_ = client_->GetMainFrameURL(); + + // Report SubmittedFormFrame metric. + if (driver) { + metrics_util::SubmittedFormFrame frame; + if (driver->IsMainFrame()) { + frame = metrics_util::SubmittedFormFrame::MAIN_FRAME; + } else if (form.origin == main_frame_url_) { + frame = + metrics_util::SubmittedFormFrame::IFRAME_WITH_SAME_URL_AS_MAIN_FRAME; + } else { + GURL::Replacements rep; + rep.SetPathStr(""); + std::string main_frame_signon_realm = + main_frame_url_.ReplaceComponents(rep).spec(); + frame = + (main_frame_signon_realm == form.signon_realm) + ? metrics_util::SubmittedFormFrame:: + IFRAME_WITH_DIFFERENT_URL_SAME_SIGNON_REALM_AS_MAIN_FRAME + : metrics_util::SubmittedFormFrame:: + IFRAME_WITH_DIFFERENT_SIGNON_REALM; + } + metrics_util::LogSubmittedFormFrame(frame); + } } void PasswordManager::UpdateFormManagers() { @@ -712,6 +737,14 @@ void PasswordManager::OnLoginSuccessful() { client_->GetStoreResultFilter()->ReportFormLoginSuccess( *provisional_save_manager_); + if (provisional_save_manager_->submitted_form()) { + metrics_util::LogPasswordSuccessfulSubmissionIndicatorEvent( + provisional_save_manager_->submitted_form()->submission_event); + if (logger) { + logger->LogSuccessfulSubmissionIndicatorEvent( + provisional_save_manager_->submitted_form()->submission_event); + } + } if (base::FeatureList::IsEnabled(features::kDropSyncCredential)) { DCHECK(provisional_save_manager_->submitted_form()); diff --git a/chromium/components/password_manager/core/browser/password_manager_client.h b/chromium/components/password_manager/core/browser/password_manager_client.h index 5e854e16db4..7004aee79ab 100644 --- a/chromium/components/password_manager/core/browser/password_manager_client.h +++ b/chromium/components/password_manager/core/browser/password_manager_client.h @@ -202,6 +202,16 @@ class PasswordManagerClient { // Return the PasswordProtectionService associated with this instance. virtual safe_browsing::PasswordProtectionService* GetPasswordProtectionService() const = 0; + + // Checks the safe browsing reputation of the webpage where the focused + // username/password field is on. + virtual void CheckSafeBrowsingReputation(const GURL& form_action, + const GURL& frame_url) = 0; + + // Checks the safe browsing reputation of the webpage where password reuse + // happens. + virtual void CheckProtectedPasswordEntry( + const std::string& password_saved_domain) = 0; #endif private: diff --git a/chromium/components/password_manager/core/browser/password_manager_driver.h b/chromium/components/password_manager/core/browser/password_manager_driver.h index 028bf7273ac..af6551c4256 100644 --- a/chromium/components/password_manager/core/browser/password_manager_driver.h +++ b/chromium/components/password_manager/core/browser/password_manager_driver.h @@ -101,6 +101,9 @@ class PasswordManagerDriver // Return the associated AutofillDriver. virtual autofill::AutofillDriver* GetAutofillDriver() = 0; + // Return true iff the driver corresponds to the main frame. + virtual bool IsMainFrame() const = 0; + private: DISALLOW_COPY_AND_ASSIGN(PasswordManagerDriver); }; diff --git a/chromium/components/password_manager/core/browser/password_manager_metrics_util.cc b/chromium/components/password_manager/core/browser/password_manager_metrics_util.cc index 642f0308eda..a7bc2fe54a5 100644 --- a/chromium/components/password_manager/core/browser/password_manager_metrics_util.cc +++ b/chromium/components/password_manager/core/browser/password_manager_metrics_util.cc @@ -132,14 +132,18 @@ void LogAccountChooserUsability(AccountChooserUsabilityMetric usability, } void LogCredentialManagerGetResult(CredentialManagerGetResult result, - CredentialManagerGetMediation status) { - switch (status) { - case CREDENTIAL_MANAGER_GET_UNMEDIATED: - UMA_HISTOGRAM_ENUMERATION("PasswordManager.GetUnmediated", result, + CredentialMediationRequirement mediation) { + switch (mediation) { + case CredentialMediationRequirement::kSilent: + UMA_HISTOGRAM_ENUMERATION("PasswordManager.MediationSilent", result, CREDENTIAL_MANAGER_GET_COUNT); break; - case CREDENTIAL_MANAGER_GET_MEDIATED: - UMA_HISTOGRAM_ENUMERATION("PasswordManager.GetMediated", result, + case CredentialMediationRequirement::kOptional: + UMA_HISTOGRAM_ENUMERATION("PasswordManager.MediationOptional", result, + CREDENTIAL_MANAGER_GET_COUNT); + break; + case CredentialMediationRequirement::kRequired: + UMA_HISTOGRAM_ENUMERATION("PasswordManager.MediationRequired", result, CREDENTIAL_MANAGER_GET_COUNT); break; } @@ -174,6 +178,27 @@ void LogShowedFormNotSecureWarningOnCurrentNavigation() { "PasswordManager.ShowedFormNotSecureWarningOnCurrentNavigation", true); } +void LogPasswordSuccessfulSubmissionIndicatorEvent( + autofill::PasswordForm::SubmissionIndicatorEvent event) { + UMA_HISTOGRAM_ENUMERATION( + "PasswordManager.SuccessfulSubmissionIndicatorEvent", event, + autofill::PasswordForm::SubmissionIndicatorEvent:: + SUBMISSION_INDICATOR_EVENT_COUNT); +} + +void LogPasswordAcceptedSaveUpdateSubmissionIndicatorEvent( + autofill::PasswordForm::SubmissionIndicatorEvent event) { + UMA_HISTOGRAM_ENUMERATION( + "PasswordManager.AcceptedSaveUpdateSubmissionIndicatorEvent", event, + autofill::PasswordForm::SubmissionIndicatorEvent:: + SUBMISSION_INDICATOR_EVENT_COUNT); +} + +void LogSubmittedFormFrame(SubmittedFormFrame frame) { + UMA_HISTOGRAM_ENUMERATION("PasswordManager.SubmittedFormFrame", frame, + SubmittedFormFrame::SUBMITTED_FORM_FRAME_COUNT); +} + } // namespace metrics_util } // namespace password_manager diff --git a/chromium/components/password_manager/core/browser/password_manager_metrics_util.h b/chromium/components/password_manager/core/browser/password_manager_metrics_util.h index 2fb1b731404..203be60f70d 100644 --- a/chromium/components/password_manager/core/browser/password_manager_metrics_util.h +++ b/chromium/components/password_manager/core/browser/password_manager_metrics_util.h @@ -9,6 +9,9 @@ #include <string> +#include "components/autofill/core/common/password_form.h" +#include "components/password_manager/core/common/credential_manager_types.h" + namespace password_manager { namespace metrics_util { @@ -168,11 +171,6 @@ enum CredentialManagerGetResult { CREDENTIAL_MANAGER_GET_COUNT }; -enum CredentialManagerGetMediation { - CREDENTIAL_MANAGER_GET_MEDIATED, - CREDENTIAL_MANAGER_GET_UNMEDIATED -}; - // Metrics: "PasswordManager.HttpPasswordMigrationMode" enum HttpPasswordMigrationMode { HTTP_PASSWORD_MIGRATION_MODE_MOVE, @@ -186,6 +184,15 @@ enum PasswordReusePasswordFieldDetected { PASSWORD_REUSE_PASSWORD_FIELD_DETECTED_COUNT }; +// Recorded into a UMA histogram, so order of enumerators should not be changed. +enum class SubmittedFormFrame { + MAIN_FRAME, + IFRAME_WITH_SAME_URL_AS_MAIN_FRAME, + IFRAME_WITH_DIFFERENT_URL_SAME_SIGNON_REALM_AS_MAIN_FRAME, + IFRAME_WITH_DIFFERENT_SIGNON_REALM, + SUBMITTED_FORM_FRAME_COUNT +}; + // A version of the UMA_HISTOGRAM_BOOLEAN macro that allows the |name| // to vary over the program's runtime. void LogUMAHistogramBoolean(const std::string& name, bool sample); @@ -245,10 +252,9 @@ void LogAccountChooserUsability(AccountChooserUsabilityMetric usability, int count_empty_icons, int count_accounts); -// Log the result of navigator.credentials.get. |status| specifies the -// "unmediated" parameter of the API method. +// Log the result of navigator.credentials.get. void LogCredentialManagerGetResult(CredentialManagerGetResult result, - CredentialManagerGetMediation status); + CredentialMediationRequirement mediation); // Log the password reuse. void LogPasswordReuse(int password_length, @@ -264,6 +270,18 @@ void LogShowedHttpNotSecureExplanation(); // per main-frame navigation. void LogShowedFormNotSecureWarningOnCurrentNavigation(); +// Log a password successful submission event. +void LogPasswordSuccessfulSubmissionIndicatorEvent( + autofill::PasswordForm::SubmissionIndicatorEvent event); + +// Log a password successful submission event for accepted by user password save +// or update. +void LogPasswordAcceptedSaveUpdateSubmissionIndicatorEvent( + autofill::PasswordForm::SubmissionIndicatorEvent event); + +// Log a frame of a submitted password form. +void LogSubmittedFormFrame(SubmittedFormFrame frame); + } // namespace metrics_util } // namespace password_manager diff --git a/chromium/components/password_manager/core/browser/password_manager_test_utils.h b/chromium/components/password_manager/core/browser/password_manager_test_utils.h index 1958745a632..893c75f7137 100644 --- a/chromium/components/password_manager/core/browser/password_manager_test_utils.h +++ b/chromium/components/password_manager/core/browser/password_manager_test_utils.h @@ -27,7 +27,7 @@ namespace password_manager { template <class Context, class Store> scoped_refptr<RefcountedKeyedService> BuildPasswordStore(Context* context) { scoped_refptr<password_manager::PasswordStore> store(new Store); - if (!store->Init(syncer::SyncableService::StartSyncFlare())) + if (!store->Init(syncer::SyncableService::StartSyncFlare(), nullptr)) return nullptr; return store; } diff --git a/chromium/components/password_manager/core/browser/password_manager_unittest.cc b/chromium/components/password_manager/core/browser/password_manager_unittest.cc index 668303a45f5..d0895e8c8b5 100644 --- a/chromium/components/password_manager/core/browser/password_manager_unittest.cc +++ b/chromium/components/password_manager/core/browser/password_manager_unittest.cc @@ -119,7 +119,9 @@ class PasswordManagerTest : public testing::Test { void SetUp() override { store_ = new testing::StrictMock<MockPasswordStore>; EXPECT_CALL(*store_, ReportMetrics(_, _)).Times(AnyNumber()); - CHECK(store_->Init(syncer::SyncableService::StartSyncFlare())); + EXPECT_CALL(*store_, GetLoginsForSameOrganizationName(_, _)) + .Times(AnyNumber()); + CHECK(store_->Init(syncer::SyncableService::StartSyncFlare(), nullptr)); EXPECT_CALL(client_, GetPasswordStore()) .WillRepeatedly(Return(store_.get())); @@ -1404,10 +1406,12 @@ TEST_F(PasswordManagerTest, PasswordGenerationPresavePasswordAndLogin) { if (found_matched_logins_in_store) { EXPECT_CALL(*store_, GetLogins(_, _)) .WillRepeatedly(WithArg<1>(InvokeConsumer(form))); + EXPECT_CALL(*store_, GetLoginsForSameOrganizationName(_, _)); EXPECT_CALL(driver_, FillPasswordForm(_)).Times(2); } else { EXPECT_CALL(*store_, GetLogins(_, _)) .WillRepeatedly(WithArg<1>(InvokeEmptyConsumerWithForms())); + EXPECT_CALL(*store_, GetLoginsForSameOrganizationName(_, _)); } std::unique_ptr<PasswordFormManager> form_manager; if (found_matched_logins_in_store) { diff --git a/chromium/components/password_manager/core/browser/password_manager_util.cc b/chromium/components/password_manager/core/browser/password_manager_util.cc index d8d4eb65b9c..d104a6b7cdc 100644 --- a/chromium/components/password_manager/core/browser/password_manager_util.cc +++ b/chromium/components/password_manager/core/browser/password_manager_util.cc @@ -11,6 +11,7 @@ #include "components/autofill/core/common/password_form.h" #include "components/password_manager/core/browser/log_manager.h" #include "components/sync/driver/sync_service.h" +#include "crypto/sha2.h" namespace password_manager_util { @@ -83,4 +84,18 @@ bool IsLoggingActive(const password_manager::PasswordManagerClient* client) { return log_manager && log_manager->IsLoggingActive(); } +uint64_t Calculate37BitsOfSHA256Hash(const base::StringPiece16& text) { + constexpr size_t kBytesFromSha256Hash = 5; + uint8_t hash[kBytesFromSha256Hash]; + base::StringPiece text_8bits(reinterpret_cast<const char*>(text.data()), + text.size() * 2); + crypto::SHA256HashString(text_8bits, hash, kBytesFromSha256Hash); + uint64_t hash37 = ((static_cast<uint64_t>(hash[0]))) | + ((static_cast<uint64_t>(hash[1])) << 8) | + ((static_cast<uint64_t>(hash[2])) << 16) | + ((static_cast<uint64_t>(hash[3])) << 24) | + (((static_cast<uint64_t>(hash[4])) & 0x1F) << 32); + return hash37; +} + } // namespace password_manager_util diff --git a/chromium/components/password_manager/core/browser/password_manager_util.h b/chromium/components/password_manager/core/browser/password_manager_util.h index 68fc65b47a1..65a0c09762c 100644 --- a/chromium/components/password_manager/core/browser/password_manager_util.h +++ b/chromium/components/password_manager/core/browser/password_manager_util.h @@ -49,6 +49,9 @@ void TrimUsernameOnlyCredentials( // and required to always return non-null. bool IsLoggingActive(const password_manager::PasswordManagerClient* client); +// Returns 37 bits from Sha256 hash. +uint64_t Calculate37BitsOfSHA256Hash(const base::StringPiece16& text); + } // namespace password_manager_util #endif // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_MANAGER_UTIL_H_ diff --git a/chromium/components/password_manager/core/browser/password_manager_util_unittest.cc b/chromium/components/password_manager/core/browser/password_manager_util_unittest.cc index de4cffb10bf..3cc9fe20d6d 100644 --- a/chromium/components/password_manager/core/browser/password_manager_util_unittest.cc +++ b/chromium/components/password_manager/core/browser/password_manager_util_unittest.cc @@ -4,6 +4,7 @@ #include "components/password_manager/core/browser/password_manager_util.h" +#include "base/macros.h" #include "base/memory/ptr_util.h" #include "base/strings/utf_string_conversions.h" #include "components/password_manager/core/browser/password_manager_test_utils.h" @@ -56,3 +57,19 @@ TEST(PasswordManagerUtil, TrimUsernameOnlyCredentials) { EXPECT_THAT(forms, UnorderedPasswordFormElementsAre(&expected_forms)); } + +TEST(PasswordManagerUtil, Calculate37BitsOfSHA256Hash) { + const char* kInputData[] = {"", "password", "secret"}; + + const uint64_t kExpectedResult[] = { + UINT64_C(0x1842c4b0e3), UINT64_C(0x55d0601e2), UINT64_C(0x8b9dea8b3)}; + + ASSERT_EQ(arraysize(kInputData), arraysize(kExpectedResult)); + + for (size_t i = 0; i < arraysize(kInputData); ++i) { + base::string16 input = base::UTF8ToUTF16(kInputData[i]); + + EXPECT_EQ(kExpectedResult[i], + password_manager_util::Calculate37BitsOfSHA256Hash(input)); + } +} diff --git a/chromium/components/password_manager/core/browser/password_reuse_detection_manager.cc b/chromium/components/password_manager/core/browser/password_reuse_detection_manager.cc index ab55375398f..f549345cfcf 100644 --- a/chromium/components/password_manager/core/browser/password_reuse_detection_manager.cc +++ b/chromium/components/password_manager/core/browser/password_reuse_detection_manager.cc @@ -49,7 +49,7 @@ void PasswordReuseDetectionManager::OnKeyPressed(const base::string16& text) { void PasswordReuseDetectionManager::OnReuseFound( const base::string16& password, - const std::string& saved_domain, + const std::string& legitimate_domain, int saved_passwords, int number_matches) { std::unique_ptr<BrowserSavePasswordProgressLogger> logger; @@ -57,7 +57,7 @@ void PasswordReuseDetectionManager::OnReuseFound( logger.reset( new BrowserSavePasswordProgressLogger(client_->GetLogManager())); logger->LogString(BrowserSavePasswordProgressLogger::STRING_REUSE_FOUND, - saved_domain); + legitimate_domain); } metrics_util::LogPasswordReuse( @@ -66,9 +66,7 @@ void PasswordReuseDetectionManager::OnReuseFound( #if defined(SAFE_BROWSING_DB_LOCAL) // TODO(jialiul): After CSD whitelist being added to Android, we should gate // this by either SAFE_BROWSING_DB_LOCAL or SAFE_BROWSING_DB_REMOTE. - safe_browsing::PasswordProtectionService* password_protection_service = - client_->GetPasswordProtectionService(); - password_protection_service->RecordPasswordReuse(main_frame_url_); + client_->CheckProtectedPasswordEntry(legitimate_domain); #endif } diff --git a/chromium/components/password_manager/core/browser/password_reuse_detection_manager.h b/chromium/components/password_manager/core/browser/password_reuse_detection_manager.h index 9235fa5ab0d..9aba12ba0e9 100644 --- a/chromium/components/password_manager/core/browser/password_reuse_detection_manager.h +++ b/chromium/components/password_manager/core/browser/password_reuse_detection_manager.h @@ -26,7 +26,7 @@ class PasswordReuseDetectionManager : public PasswordReuseDetectorConsumer { // PasswordReuseDetectorConsumer void OnReuseFound(const base::string16& password, - const std::string& saved_domain, + const std::string& legitimate_domain, int saved_passwords, int number_matches) override; diff --git a/chromium/components/password_manager/core/browser/password_reuse_detection_manager_unittest.cc b/chromium/components/password_manager/core/browser/password_reuse_detection_manager_unittest.cc index 3dadcc79450..d166a5b6a85 100644 --- a/chromium/components/password_manager/core/browser/password_reuse_detection_manager_unittest.cc +++ b/chromium/components/password_manager/core/browser/password_reuse_detection_manager_unittest.cc @@ -39,7 +39,7 @@ class PasswordReuseDetectionManagerTest : public ::testing::Test { PasswordReuseDetectionManagerTest() {} void SetUp() override { store_ = new testing::StrictMock<MockPasswordStore>; - CHECK(store_->Init(syncer::SyncableService::StartSyncFlare())); + CHECK(store_->Init(syncer::SyncableService::StartSyncFlare(), nullptr)); } void TearDown() override { store_->ShutdownOnUIThread(); diff --git a/chromium/components/password_manager/core/browser/password_reuse_detector.cc b/chromium/components/password_manager/core/browser/password_reuse_detector.cc index dd9b2a94a5a..a06e94eb183 100644 --- a/chromium/components/password_manager/core/browser/password_reuse_detector.cc +++ b/chromium/components/password_manager/core/browser/password_reuse_detector.cc @@ -7,8 +7,16 @@ #include <algorithm> #include "components/autofill/core/common/password_form.h" +#include "components/password_manager/core/browser/password_manager_util.h" #include "components/password_manager/core/browser/password_reuse_detector_consumer.h" #include "components/password_manager/core/browser/psl_matching_helper.h" +#include "components/password_manager/core/common/password_manager_pref_names.h" +#include "components/prefs/pref_service.h" +#include "google_apis/gaia/gaia_auth_util.h" +#include "google_apis/gaia/gaia_urls.h" +#include "url/origin.h" + +using url::Origin; namespace password_manager { @@ -35,7 +43,8 @@ bool ReverseStringLess::operator()(const base::string16& lhs, rhs.rend()); } -PasswordReuseDetector::PasswordReuseDetector() {} +PasswordReuseDetector::PasswordReuseDetector(PrefService* prefs) + : prefs_(prefs) {} PasswordReuseDetector::~PasswordReuseDetector() {} @@ -62,20 +71,69 @@ void PasswordReuseDetector::CheckReuse( if (input.size() < kMinPasswordLengthToCheck) return; + if (CheckSyncPasswordReuse(input, domain, consumer)) + return; + + if (CheckSavedPasswordReuse(input, domain, consumer)) + return; +} + +bool PasswordReuseDetector::CheckSyncPasswordReuse( + const base::string16& input, + const std::string& domain, + PasswordReuseDetectorConsumer* consumer) { + if (!sync_password_hash_.has_value()) + return false; + + const Origin gaia_origin(GaiaUrls::GetInstance()->gaia_url().GetOrigin()); + if (Origin(GURL(domain)).IsSameOriginWith(gaia_origin)) + return false; + + // Check that some suffix of |input| has the same hash as the sync password. + for (size_t i = 0; i + kMinPasswordLengthToCheck <= input.size(); ++i) { + base::StringPiece16 input_suffix(input.c_str() + i, input.size() - i); + if (password_manager_util::Calculate37BitsOfSHA256Hash(input_suffix) == + sync_password_hash_.value()) { + consumer->OnReuseFound(input_suffix.as_string(), gaia_origin.host(), 1, + 0); + return true; + } + } + + return false; +} + +bool PasswordReuseDetector::CheckSavedPasswordReuse( + const base::string16& input, + const std::string& domain, + PasswordReuseDetectorConsumer* consumer) { const std::string registry_controlled_domain = GetRegistryControlledDomain(GURL(domain)); auto passwords_iterator = FindSavedPassword(input); if (passwords_iterator == passwords_.end()) - return; + return false; const std::set<std::string>& domains = passwords_iterator->second; DCHECK(!domains.empty()); if (domains.find(registry_controlled_domain) == domains.end()) { // Return only one domain. - const std::string& saved_domain = *domains.begin(); - consumer->OnReuseFound(passwords_iterator->first, saved_domain, + const std::string& legitimate_domain = *domains.begin(); + consumer->OnReuseFound(passwords_iterator->first, legitimate_domain, saved_passwords_, domains.size()); - return; + return true; + } + + return false; +} + +void PasswordReuseDetector::SaveSyncPasswordHash( + const base::string16& password) { + sync_password_hash_ = + password_manager_util::Calculate37BitsOfSHA256Hash(password); + if (prefs_) { + // TODO(crbug.com/657041) Implement encrypting and saving of + // |sync_password_hash_| into preference kSyncPasswordHash. + prefs_->SetString(prefs::kSyncPasswordHash, std::string()); } } diff --git a/chromium/components/password_manager/core/browser/password_reuse_detector.h b/chromium/components/password_manager/core/browser/password_reuse_detector.h index 967c941a9cd..d42f3137c77 100644 --- a/chromium/components/password_manager/core/browser/password_reuse_detector.h +++ b/chromium/components/password_manager/core/browser/password_reuse_detector.h @@ -5,6 +5,7 @@ #ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_REUSE_DETECTOR_H_ #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_REUSE_DETECTOR_H_ +#include <stdint.h> #include <map> #include <memory> #include <set> @@ -12,10 +13,13 @@ #include <vector> #include "base/macros.h" +#include "base/optional.h" #include "base/strings/string16.h" #include "components/password_manager/core/browser/password_store_change.h" #include "components/password_manager/core/browser/password_store_consumer.h" +class PrefService; + namespace password_manager { class PasswordReuseDetectorConsumer; @@ -32,7 +36,7 @@ struct ReverseStringLess { // a password reuse. class PasswordReuseDetector : public PasswordStoreConsumer { public: - PasswordReuseDetector(); + explicit PasswordReuseDetector(PrefService* prefs); ~PasswordReuseDetector() override; // PasswordStoreConsumer @@ -43,7 +47,7 @@ class PasswordReuseDetector : public PasswordStoreConsumer { void OnLoginsChanged(const PasswordStoreChangeList& changes); // Checks that some suffix of |input| equals to a password saved on another - // registry controlled domain than |domain|. + // registry controlled domain than |domain| or to a sync password. // If such suffix is found, |consumer|->OnReuseFound() is called on the same // thread on which this method is called. // |consumer| should not be null. @@ -51,6 +55,9 @@ class PasswordReuseDetector : public PasswordStoreConsumer { const std::string& domain, PasswordReuseDetectorConsumer* consumer); + // Saves a hash of |password| for password reuse checking. + void SaveSyncPasswordHash(const base::string16& password); + private: using passwords_iterator = std::map<base::string16, std::set<std::string>, @@ -59,6 +66,18 @@ class PasswordReuseDetector : public PasswordStoreConsumer { // Add password from |form| to |passwords_|. void AddPassword(const autofill::PasswordForm& form); + // Returns true iff a reuse of a sync password is found. If reuse is found it + // is reported to |consumer|. + bool CheckSyncPasswordReuse(const base::string16& input, + const std::string& domain, + PasswordReuseDetectorConsumer* consumer); + + // Returns true iff a reuse of a saved password is found. If reuse is found it + // is reported to |consumer|. + bool CheckSavedPasswordReuse(const base::string16& input, + const std::string& domain, + PasswordReuseDetectorConsumer* consumer); + // Returns the iterator to |passwords_| that corresponds to the longest key in // |passwords_| that is a suffix of |input|. Returns passwords_.end() in case // when no key in |passwords_| is a prefix of |input|. @@ -74,6 +93,9 @@ class PasswordReuseDetector : public PasswordStoreConsumer { // of times how many different sites it's saved on. int saved_passwords_ = 0; + base::Optional<uint64_t> sync_password_hash_; + PrefService* const prefs_; + DISALLOW_COPY_AND_ASSIGN(PasswordReuseDetector); }; diff --git a/chromium/components/password_manager/core/browser/password_reuse_detector_consumer.h b/chromium/components/password_manager/core/browser/password_reuse_detector_consumer.h index 254f29f79d9..b621b777e4f 100644 --- a/chromium/components/password_manager/core/browser/password_reuse_detector_consumer.h +++ b/chromium/components/password_manager/core/browser/password_reuse_detector_consumer.h @@ -21,11 +21,12 @@ class PasswordReuseDetectorConsumer virtual ~PasswordReuseDetectorConsumer(); // Called when a password reuse is found. - // |saved_domain| is the domain on which |password| is saved. + // |legitimate_domain| is the domain on which |password| is saved or the sync + // domain if |password| is a sync password. // |saved_passwords| is total number of passwords stored in Password Manager. // |number_matches| is a number of sites on which |password| is saved. virtual void OnReuseFound(const base::string16& password, - const std::string& saved_domain, + const std::string& legitimate_domain, int saved_passwords, int number_matches) = 0; }; diff --git a/chromium/components/password_manager/core/browser/password_reuse_detector_unittest.cc b/chromium/components/password_manager/core/browser/password_reuse_detector_unittest.cc index dac4dd694cb..94d2fa82665 100644 --- a/chromium/components/password_manager/core/browser/password_reuse_detector_unittest.cc +++ b/chromium/components/password_manager/core/browser/password_reuse_detector_unittest.cc @@ -11,6 +11,9 @@ #include "base/strings/utf_string_conversions.h" #include "components/autofill/core/common/password_form.h" #include "components/password_manager/core/browser/password_manager_test_utils.h" +#include "components/password_manager/core/common/password_manager_pref_names.h" +#include "components/prefs/pref_registry_simple.h" +#include "components/prefs/testing_pref_service.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -60,7 +63,7 @@ PasswordStoreChangeList GetChangeList( } TEST(PasswordReuseDetectorTest, TypingPasswordOnDifferentSite) { - PasswordReuseDetector reuse_detector; + PasswordReuseDetector reuse_detector(nullptr); reuse_detector.OnGetPasswordStoreResults(GetForms(GetTestDomainsPasswords())); MockPasswordReuseDetectorConsumer mockConsumer; @@ -91,7 +94,7 @@ TEST(PasswordReuseDetectorTest, TypingPasswordOnDifferentSite) { } TEST(PasswordReuseDetectorTest, PSLMatchNoReuseEvent) { - PasswordReuseDetector reuse_detector; + PasswordReuseDetector reuse_detector(nullptr); reuse_detector.OnGetPasswordStoreResults(GetForms(GetTestDomainsPasswords())); MockPasswordReuseDetectorConsumer mockConsumer; @@ -101,7 +104,7 @@ TEST(PasswordReuseDetectorTest, PSLMatchNoReuseEvent) { } TEST(PasswordReuseDetectorTest, NoPSLMatchReuseEvent) { - PasswordReuseDetector reuse_detector; + PasswordReuseDetector reuse_detector(nullptr); reuse_detector.OnGetPasswordStoreResults(GetForms(GetTestDomainsPasswords())); MockPasswordReuseDetectorConsumer mockConsumer; @@ -114,7 +117,7 @@ TEST(PasswordReuseDetectorTest, NoPSLMatchReuseEvent) { } TEST(PasswordReuseDetectorTest, TooShortPasswordNoReuseEvent) { - PasswordReuseDetector reuse_detector; + PasswordReuseDetector reuse_detector(nullptr); reuse_detector.OnGetPasswordStoreResults(GetForms(GetTestDomainsPasswords())); MockPasswordReuseDetectorConsumer mockConsumer; @@ -123,7 +126,7 @@ TEST(PasswordReuseDetectorTest, TooShortPasswordNoReuseEvent) { } TEST(PasswordReuseDetectorTest, PasswordNotInputSuffixNoReuseEvent) { - PasswordReuseDetector reuse_detector; + PasswordReuseDetector reuse_detector(nullptr); reuse_detector.OnGetPasswordStoreResults(GetForms(GetTestDomainsPasswords())); MockPasswordReuseDetectorConsumer mockConsumer; @@ -138,7 +141,7 @@ TEST(PasswordReuseDetectorTest, OnLoginsChanged) { for (PasswordStoreChange::Type type : {PasswordStoreChange::ADD, PasswordStoreChange::UPDATE, PasswordStoreChange::REMOVE}) { - PasswordReuseDetector reuse_detector; + PasswordReuseDetector reuse_detector(nullptr); PasswordStoreChangeList changes = GetChangeList(type, GetForms(GetTestDomainsPasswords())); reuse_detector.OnLoginsChanged(changes); @@ -162,7 +165,7 @@ TEST(PasswordReuseDetectorTest, CheckLongestPasswordMatchReturn) { {"https://example3.com", "1234567890"}, }; - PasswordReuseDetector reuse_detector; + PasswordReuseDetector reuse_detector(nullptr); reuse_detector.OnGetPasswordStoreResults(GetForms(domain_passwords)); MockPasswordReuseDetectorConsumer mockConsumer; @@ -184,6 +187,59 @@ TEST(PasswordReuseDetectorTest, CheckLongestPasswordMatchReturn) { &mockConsumer); } +TEST(PasswordReuseDetectorTest, SyncPasswordNoReuse) { + PasswordReuseDetector reuse_detector(nullptr); + reuse_detector.OnGetPasswordStoreResults(GetForms(GetTestDomainsPasswords())); + MockPasswordReuseDetectorConsumer mockConsumer; + + reuse_detector.SaveSyncPasswordHash(ASCIIToUTF16("sync_password")); + + EXPECT_CALL(mockConsumer, OnReuseFound(_, _, _, _)).Times(0); + reuse_detector.CheckReuse(ASCIIToUTF16("sync_password"), + "https://accounts.google.com", &mockConsumer); + // Only suffixes are verifed. + reuse_detector.CheckReuse(ASCIIToUTF16("sync_password123"), + "https://evil.com", &mockConsumer); +} + +TEST(PasswordReuseDetectorTest, SyncPasswordReuseFound) { + PasswordReuseDetector reuse_detector(nullptr); + reuse_detector.OnGetPasswordStoreResults(GetForms(GetTestDomainsPasswords())); + MockPasswordReuseDetectorConsumer mockConsumer; + + reuse_detector.SaveSyncPasswordHash(ASCIIToUTF16("sync_password")); + + EXPECT_CALL(mockConsumer, OnReuseFound(ASCIIToUTF16("sync_password"), + "accounts.google.com", 1, 0)); + reuse_detector.CheckReuse(ASCIIToUTF16("sync_password"), "https://evil.com", + &mockConsumer); +} + +TEST(PasswordReuseDetectorTest, SavedPasswordsReuseSyncPasswordAvailable) { + // Check that reuse of saved passwords is detected also if the sync password + // hash is saved. + PasswordReuseDetector reuse_detector(nullptr); + reuse_detector.OnGetPasswordStoreResults(GetForms(GetTestDomainsPasswords())); + MockPasswordReuseDetectorConsumer mockConsumer; + + reuse_detector.SaveSyncPasswordHash(ASCIIToUTF16("sync_password")); + + EXPECT_CALL(mockConsumer, + OnReuseFound(ASCIIToUTF16("password"), "google.com", 5, 1)); + reuse_detector.CheckReuse(ASCIIToUTF16("password"), "https://evil.com", + &mockConsumer); +} + +TEST(PasswordReuseDetectorTest, CheckThatSyncPasswordIsStoredIntoPreferences) { + TestingPrefServiceSimple prefs; + prefs.registry()->RegisterStringPref(prefs::kSyncPasswordHash, std::string(), + PrefRegistry::NO_REGISTRATION_FLAGS); + ASSERT_FALSE(prefs.HasPrefPath(prefs::kSyncPasswordHash)); + PasswordReuseDetector reuse_detector(&prefs); + reuse_detector.SaveSyncPasswordHash(ASCIIToUTF16("sync_password")); + EXPECT_TRUE(prefs.HasPrefPath(prefs::kSyncPasswordHash)); +} + } // namespace } // namespace password_manager diff --git a/chromium/components/password_manager/core/browser/password_store.cc b/chromium/components/password_manager/core/browser/password_store.cc index 3597674b5ba..0575e5de99a 100644 --- a/chromium/components/password_manager/core/browser/password_store.cc +++ b/chromium/components/password_manager/core/browser/password_store.cc @@ -67,13 +67,13 @@ PasswordStore::CheckReuseRequest::~CheckReuseRequest() {} void PasswordStore::CheckReuseRequest::OnReuseFound( const base::string16& password, - const std::string& saved_domain, + const std::string& legitimate_domain, int saved_passwords, int number_matches) { origin_task_runner_->PostTask( FROM_HERE, base::Bind(&PasswordReuseDetectorConsumer::OnReuseFound, consumer_weak_, - password, saved_domain, saved_passwords, number_matches)); + password, legitimate_domain, saved_passwords, number_matches)); } #endif @@ -111,8 +111,10 @@ PasswordStore::PasswordStore( is_propagating_password_changes_to_web_credentials_enabled_(false), shutdown_called_(false) {} -bool PasswordStore::Init(const syncer::SyncableService::StartSyncFlare& flare) { - ScheduleTask(base::Bind(&PasswordStore::InitOnBackgroundThread, this, flare)); +bool PasswordStore::Init(const syncer::SyncableService::StartSyncFlare& flare, + PrefService* prefs) { + ScheduleTask( + base::Bind(&PasswordStore::InitOnBackgroundThread, this, flare, prefs)); return true; } @@ -223,6 +225,14 @@ void PasswordStore::GetLogins(const FormDigest& form, } } +void PasswordStore::GetLoginsForSameOrganizationName( + const std::string& signon_realm, + PasswordStoreConsumer* consumer) { + std::unique_ptr<GetLoginsRequest> request(new GetLoginsRequest(consumer)); + ScheduleTask(base::Bind(&PasswordStore::GetLoginsForSameOrganizationNameImpl, + this, signon_realm, base::Passed(&request))); +} + void PasswordStore::GetAutofillableLogins(PasswordStoreConsumer* consumer) { Schedule(&PasswordStore::GetAutofillableLoginsImpl, consumer); } @@ -317,6 +327,11 @@ void PasswordStore::CheckReuse(const base::string16& input, ScheduleTask(base::Bind(&PasswordStore::CheckReuseImpl, this, base::Passed(&check_reuse_request), input, domain)); } + +void PasswordStore::SaveSyncPasswordHash(const base::string16& password) { + ScheduleTask( + base::Bind(&PasswordStore::SaveSyncPasswordHashImpl, this, password)); +} #endif PasswordStore::~PasswordStore() { @@ -399,6 +414,11 @@ void PasswordStore::CheckReuseImpl(std::unique_ptr<CheckReuseRequest> request, if (reuse_detector_) reuse_detector_->CheckReuse(input, domain, request.get()); } + +void PasswordStore::SaveSyncPasswordHashImpl(const base::string16& password) { + if (reuse_detector_) + reuse_detector_->SaveSyncPasswordHash(password); +} #endif void PasswordStore::Schedule( @@ -487,6 +507,13 @@ void PasswordStore::DisableAutoSignInForOriginsInternal( main_thread_runner_->PostTask(FROM_HERE, completion); } +void PasswordStore::GetLoginsForSameOrganizationNameImpl( + const std::string& signon_realm, + std::unique_ptr<GetLoginsRequest> request) { + request->NotifyConsumerWithResults( + FillLoginsForSameOrganizationName(signon_realm)); +} + void PasswordStore::GetAutofillableLoginsImpl( std::unique_ptr<GetLoginsRequest> request) { std::vector<std::unique_ptr<PasswordForm>> obtained_forms; @@ -708,14 +735,15 @@ void PasswordStore::ScheduleUpdateAffiliatedWebLoginsImpl( } void PasswordStore::InitOnBackgroundThread( - const syncer::SyncableService::StartSyncFlare& flare) { + const syncer::SyncableService::StartSyncFlare& flare, + PrefService* prefs) { DCHECK(GetBackgroundTaskRunner()->BelongsToCurrentThread()); DCHECK(!syncable_service_); syncable_service_.reset(new PasswordSyncableService(this)); syncable_service_->InjectStartSyncFlare(flare); // TODO(crbug.com/706392): Fix password reuse detection for Android. #if !defined(OS_ANDROID) && !defined(OS_IOS) - reuse_detector_.reset(new PasswordReuseDetector); + reuse_detector_ = base::MakeUnique<PasswordReuseDetector>(prefs); GetAutofillableLoginsImpl( base::MakeUnique<GetLoginsRequest>(reuse_detector_.get())); #endif diff --git a/chromium/components/password_manager/core/browser/password_store.h b/chromium/components/password_manager/core/browser/password_store.h index 0a0b915bbef..6c2f104e326 100644 --- a/chromium/components/password_manager/core/browser/password_store.h +++ b/chromium/components/password_manager/core/browser/password_store.h @@ -28,6 +28,7 @@ #endif class PasswordStoreProxyMac; +class PrefService; namespace autofill { struct PasswordForm; @@ -92,7 +93,8 @@ class PasswordStore : protected PasswordStoreSync, scoped_refptr<base::SingleThreadTaskRunner> db_thread_runner); // Reimplement this to add custom initialization. Always call this too. - virtual bool Init(const syncer::SyncableService::StartSyncFlare& flare); + virtual bool Init(const syncer::SyncableService::StartSyncFlare& flare, + PrefService* prefs); // RefcountedKeyedService: void ShutdownOnUIThread() override; @@ -180,6 +182,21 @@ class PasswordStore : protected PasswordStoreSync, virtual void GetLogins(const FormDigest& form, PasswordStoreConsumer* consumer); + // Returns all stored credentials with SCHEME_HTTP that have a realm whose + // organization-identifying name -- that is, the first domain name label below + // the effective TLD -- matches that of |signon_realm|. Notifies |consumer| on + // completion. The request will be cancelled if the consumer is destroyed. + // + // WARNING: This is *NOT* PSL (Public Suffix List) matching. The logins + // returned by this method are not safe to be filled into the observed form. + // + // For example, the organization-identifying name of "https://foo.example.org" + // is `example`, and logins will be returned for "http://bar.example.co.uk", + // but not for "http://notexample.com" or "https://example.foo.com". + virtual void GetLoginsForSameOrganizationName( + const std::string& signon_realm, + PasswordStoreConsumer* consumer); + // Gets the complete list of PasswordForms that are not blacklist entries--and // are thus auto-fillable. |consumer| will be notified on completion. // The request will be cancelled if the consumer is destroyed. @@ -241,6 +258,9 @@ class PasswordStore : protected PasswordStoreSync, virtual void CheckReuse(const base::string16& input, const std::string& domain, PasswordReuseDetectorConsumer* consumer); + + // Saves a hash of |password| for password reuse checking. + void SaveSyncPasswordHash(const base::string16& password); #endif protected: @@ -291,7 +311,7 @@ class PasswordStore : protected PasswordStoreSync, // PasswordReuseDetectorConsumer void OnReuseFound(const base::string16& password, - const std::string& saved_domain, + const std::string& legitimate_domain, int saved_passwords, int number_matches) override; @@ -369,6 +389,11 @@ class PasswordStore : protected PasswordStoreSync, virtual std::vector<std::unique_ptr<autofill::PasswordForm>> FillMatchingLogins(const FormDigest& form) = 0; + // Finds and returns all organization-name-matching logins, or returns an + // empty list on error. + virtual std::vector<std::unique_ptr<autofill::PasswordForm>> + FillLoginsForSameOrganizationName(const std::string& signon_realm) = 0; + // Synchronous implementation for manipulating with statistics. virtual void AddSiteStatsImpl(const InteractionsStats& stats) = 0; virtual void RemoveSiteStatsImpl(const GURL& origin_domain) = 0; @@ -402,6 +427,9 @@ class PasswordStore : protected PasswordStoreSync, void CheckReuseImpl(std::unique_ptr<CheckReuseRequest> request, const base::string16& input, const std::string& domain); + + // Synchronous implementation of SaveSyncPasswordHash(). + void SaveSyncPasswordHashImpl(const base::string16& password); #endif // TaskRunner for tasks that run on the main thread (usually the UI thread). @@ -456,6 +484,12 @@ class PasswordStore : protected PasswordStoreSync, const base::Callback<bool(const GURL&)>& origin_filter, const base::Closure& completion); + // Finds all logins organization-name-matching |signon_realm| and notifies the + // consumer. + void GetLoginsForSameOrganizationNameImpl( + const std::string& signon_realm, + std::unique_ptr<GetLoginsRequest> request); + // Finds all non-blacklist PasswordForms, and notifies the consumer. void GetAutofillableLoginsImpl(std::unique_ptr<GetLoginsRequest> request); @@ -544,7 +578,8 @@ class PasswordStore : protected PasswordStoreSync, // Creates PasswordSyncableService and PasswordReuseDetector instances on the // background thread. void InitOnBackgroundThread( - const syncer::SyncableService::StartSyncFlare& flare); + const syncer::SyncableService::StartSyncFlare& flare, + PrefService* prefs); // Deletes objest that should be destroyed on the background thread. void DestroyOnBackgroundThread(); diff --git a/chromium/components/password_manager/core/browser/password_store_default.cc b/chromium/components/password_manager/core/browser/password_store_default.cc index 23debe3534b..2e05c0a7c3d 100644 --- a/chromium/components/password_manager/core/browser/password_store_default.cc +++ b/chromium/components/password_manager/core/browser/password_store_default.cc @@ -27,9 +27,10 @@ PasswordStoreDefault::~PasswordStoreDefault() { } bool PasswordStoreDefault::Init( - const syncer::SyncableService::StartSyncFlare& flare) { + const syncer::SyncableService::StartSyncFlare& flare, + PrefService* prefs) { ScheduleTask(base::Bind(&PasswordStoreDefault::InitOnDBThread, this)); - return PasswordStore::Init(flare); + return PasswordStore::Init(flare, prefs); } void PasswordStoreDefault::ShutdownOnUIThread() { @@ -181,6 +182,16 @@ PasswordStoreDefault::FillMatchingLogins(const FormDigest& form) { return matched_forms; } +std::vector<std::unique_ptr<PasswordForm>> +PasswordStoreDefault::FillLoginsForSameOrganizationName( + const std::string& signon_realm) { + std::vector<std::unique_ptr<PasswordForm>> forms; + if (login_db_ && + !login_db_->GetLoginsForSameOrganizationName(signon_realm, &forms)) + return std::vector<std::unique_ptr<PasswordForm>>(); + return forms; +} + bool PasswordStoreDefault::FillAutofillableLogins( std::vector<std::unique_ptr<PasswordForm>>* forms) { DCHECK(GetBackgroundTaskRunner()->BelongsToCurrentThread()); diff --git a/chromium/components/password_manager/core/browser/password_store_default.h b/chromium/components/password_manager/core/browser/password_store_default.h index 71b93656292..0c7faebef2d 100644 --- a/chromium/components/password_manager/core/browser/password_store_default.h +++ b/chromium/components/password_manager/core/browser/password_store_default.h @@ -10,9 +10,12 @@ #include <vector> #include "base/macros.h" +#include "base/single_thread_task_runner.h" #include "components/password_manager/core/browser/login_database.h" #include "components/password_manager/core/browser/password_store.h" +class PrefService; + namespace password_manager { // Simple password store implementation that delegates everything to @@ -26,7 +29,8 @@ class PasswordStoreDefault : public PasswordStore { scoped_refptr<base::SingleThreadTaskRunner> db_thread_runner, std::unique_ptr<LoginDatabase> login_db); - bool Init(const syncer::SyncableService::StartSyncFlare& flare) override; + bool Init(const syncer::SyncableService::StartSyncFlare& flare, + PrefService* prefs) override; void ShutdownOnUIThread() override; @@ -66,6 +70,8 @@ class PasswordStoreDefault : public PasswordStore { base::Time delete_end) override; std::vector<std::unique_ptr<autofill::PasswordForm>> FillMatchingLogins( const FormDigest& form) override; + std::vector<std::unique_ptr<autofill::PasswordForm>> + FillLoginsForSameOrganizationName(const std::string& signon_realm) override; bool FillAutofillableLogins( std::vector<std::unique_ptr<autofill::PasswordForm>>* forms) override; bool FillBlacklistLogins( diff --git a/chromium/components/password_manager/core/browser/password_store_default_unittest.cc b/chromium/components/password_manager/core/browser/password_store_default_unittest.cc index f8239e5ceba..cc2e8afd92e 100644 --- a/chromium/components/password_manager/core/browser/password_store_default_unittest.cc +++ b/chromium/components/password_manager/core/browser/password_store_default_unittest.cc @@ -15,6 +15,7 @@ #include "base/run_loop.h" #include "base/strings/string_util.h" #include "base/strings/utf_string_conversions.h" +#include "base/test/scoped_task_environment.h" #include "base/threading/thread_task_runner_handle.h" #include "base/time/time.h" #include "components/password_manager/core/browser/login_database.h" @@ -97,21 +98,25 @@ class PasswordStoreDefaultTestDelegate { base::FilePath test_login_db_file_path() const; - base::MessageLoopForUI message_loop_; + base::test::ScopedTaskEnvironment scoped_task_environment_; base::ScopedTempDir temp_dir_; scoped_refptr<PasswordStoreDefault> store_; DISALLOW_COPY_AND_ASSIGN(PasswordStoreDefaultTestDelegate); }; -PasswordStoreDefaultTestDelegate::PasswordStoreDefaultTestDelegate() { +PasswordStoreDefaultTestDelegate::PasswordStoreDefaultTestDelegate() + : scoped_task_environment_( + base::test::ScopedTaskEnvironment::MainThreadType::UI) { SetupTempDir(); store_ = CreateInitializedStore( base::MakeUnique<LoginDatabase>(test_login_db_file_path())); } PasswordStoreDefaultTestDelegate::PasswordStoreDefaultTestDelegate( - std::unique_ptr<LoginDatabase> database) { + std::unique_ptr<LoginDatabase> database) + : scoped_task_environment_( + base::test::ScopedTaskEnvironment::MainThreadType::UI) { SetupTempDir(); store_ = CreateInitializedStore(std::move(database)); } @@ -140,7 +145,7 @@ PasswordStoreDefaultTestDelegate::CreateInitializedStore( scoped_refptr<PasswordStoreDefault> store(new PasswordStoreDefault( base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get(), std::move(database))); - store->Init(syncer::SyncableService::StartSyncFlare()); + store->Init(syncer::SyncableService::StartSyncFlare(), nullptr); return store; } diff --git a/chromium/components/password_manager/core/browser/password_store_unittest.cc b/chromium/components/password_manager/core/browser/password_store_unittest.cc index 527770396f7..53835261704 100644 --- a/chromium/components/password_manager/core/browser/password_store_unittest.cc +++ b/chromium/components/password_manager/core/browser/password_store_unittest.cc @@ -15,10 +15,12 @@ #include "base/files/scoped_temp_dir.h" #include "base/macros.h" #include "base/memory/ptr_util.h" +#include "base/message_loop/message_loop.h" #include "base/run_loop.h" #include "base/strings/string_util.h" #include "base/strings/utf_string_conversions.h" #include "base/synchronization/waitable_event.h" +#include "base/test/scoped_task_environment.h" #include "base/threading/thread_task_runner_handle.h" #include "base/time/time.h" #include "build/build_config.h" @@ -45,24 +47,32 @@ namespace password_manager { namespace { -const char kTestWebRealm1[] = "https://one.example.com/"; -const char kTestWebOrigin1[] = "https://one.example.com/origin"; -const char kTestWebRealm2[] = "https://two.example.com/"; -const char kTestWebOrigin2[] = "https://two.example.com/origin"; -const char kTestWebRealm3[] = "https://three.example.com/"; -const char kTestWebOrigin3[] = "https://three.example.com/origin"; -const char kTestWebRealm5[] = "https://five.example.com/"; -const char kTestWebOrigin5[] = "https://five.example.com/origin"; -const char kTestPSLMatchingWebRealm[] = "https://psl.example.com/"; -const char kTestPSLMatchingWebOrigin[] = "https://psl.example.com/origin"; -const char kTestUnrelatedWebRealm[] = "https://notexample.com/"; -const char kTestUnrelatedWebOrigin[] = "https:/notexample.com/origin"; -const char kTestInsecureWebRealm[] = "http://one.example.com/"; -const char kTestInsecureWebOrigin[] = "http://one.example.com/origin"; -const char kTestAndroidRealm1[] = "android://hash@com.example.android/"; -const char kTestAndroidRealm2[] = "android://hash@com.example.two.android/"; -const char kTestAndroidRealm3[] = "android://hash@com.example.three.android/"; -const char kTestUnrelatedAndroidRealm[] = +constexpr const char kTestWebRealm1[] = "https://one.example.com/"; +constexpr const char kTestWebOrigin1[] = "https://one.example.com/origin"; +constexpr const char kTestWebRealm2[] = "https://two.example.com/"; +constexpr const char kTestWebOrigin2[] = "https://two.example.com/origin"; +constexpr const char kTestWebRealm3[] = "https://three.example.com/"; +constexpr const char kTestWebOrigin3[] = "https://three.example.com/origin"; +constexpr const char kTestWebRealm5[] = "https://five.example.com/"; +constexpr const char kTestWebOrigin5[] = "https://five.example.com/origin"; +constexpr const char kTestPSLMatchingWebRealm[] = "https://psl.example.com/"; +constexpr const char kTestPSLMatchingWebOrigin[] = + "https://psl.example.com/origin"; +constexpr const char kTestUnrelatedWebRealm[] = "https://notexample.com/"; +constexpr const char kTestUnrelatedWebOrigin[] = "https:/notexample.com/origin"; +constexpr const char kTestSameOrganizationNameWebRealm[] = + "https://example.appspot.com/"; +constexpr const char kTestSameOrganizationNameWebOrigin[] = + "https://example.appspot.com/origin"; +constexpr const char kTestInsecureWebRealm[] = "http://one.example.com/"; +constexpr const char kTestInsecureWebOrigin[] = "http://one.example.com/origin"; +constexpr const char kTestAndroidRealm1[] = + "android://hash@com.example.android/"; +constexpr const char kTestAndroidRealm2[] = + "android://hash@com.example.two.android/"; +constexpr const char kTestAndroidRealm3[] = + "android://hash@com.example.three.android/"; +constexpr const char kTestUnrelatedAndroidRealm[] = "android://hash@com.notexample.android/"; class MockPasswordStoreConsumer : public PasswordStoreConsumer { @@ -89,6 +99,10 @@ class StartSyncFlareMock { class PasswordStoreTest : public testing::Test { protected: + PasswordStoreTest() + : scoped_task_environment_( + base::test::ScopedTaskEnvironment::MainThreadType::UI) {} + void SetUp() override { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); } void TearDown() override { ASSERT_TRUE(temp_dir_.Delete()); } @@ -97,7 +111,7 @@ class PasswordStoreTest : public testing::Test { return temp_dir_.GetPath().Append(FILE_PATH_LITERAL("login_test")); } - base::MessageLoopForUI message_loop_; + base::test::ScopedTaskEnvironment scoped_task_environment_; base::ScopedTempDir temp_dir_; }; @@ -105,7 +119,7 @@ TEST_F(PasswordStoreTest, IgnoreOldWwwGoogleLogins) { scoped_refptr<PasswordStoreDefault> store(new PasswordStoreDefault( base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get(), base::MakeUnique<LoginDatabase>(test_login_db_file_path()))); - store->Init(syncer::SyncableService::StartSyncFlare()); + store->Init(syncer::SyncableService::StartSyncFlare(), nullptr); const time_t cutoff = 1325376000; // 00:00 Jan 1 2012 UTC static const PasswordFormData form_data[] = { @@ -201,7 +215,8 @@ TEST_F(PasswordStoreTest, StartSyncFlare) { base::MakeUnique<LoginDatabase>(test_login_db_file_path()))); StartSyncFlareMock mock; store->Init( - base::Bind(&StartSyncFlareMock::StartSyncFlare, base::Unretained(&mock))); + base::Bind(&StartSyncFlareMock::StartSyncFlare, base::Unretained(&mock)), + nullptr); { PasswordForm form; form.origin = GURL("http://accounts.google.com/LoginAuth"); @@ -228,7 +243,7 @@ TEST_F(PasswordStoreTest, GetLoginImpl) { scoped_refptr<PasswordStoreDefault> store(new PasswordStoreDefault( base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get(), base::MakeUnique<LoginDatabase>(test_login_db_file_path()))); - store->Init(syncer::SyncableService::StartSyncFlare()); + store->Init(syncer::SyncableService::StartSyncFlare(), nullptr); // For each attribute in the primary key, create one form that mismatches on // that attribute. @@ -291,7 +306,7 @@ TEST_F(PasswordStoreTest, UpdateLoginPrimaryKeyFields) { scoped_refptr<PasswordStoreDefault> store(new PasswordStoreDefault( base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get(), base::MakeUnique<LoginDatabase>(test_login_db_file_path()))); - store->Init(syncer::SyncableService::StartSyncFlare()); + store->Init(syncer::SyncableService::StartSyncFlare(), nullptr); std::unique_ptr<PasswordForm> old_form( CreatePasswordFormFromDataForTesting(kTestCredentials[0])); @@ -344,7 +359,7 @@ TEST_F(PasswordStoreTest, RemoveLoginsCreatedBetweenCallbackIsCalled) { scoped_refptr<PasswordStoreDefault> store(new PasswordStoreDefault( base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get(), base::WrapUnique(new LoginDatabase(test_login_db_file_path())))); - store->Init(syncer::SyncableService::StartSyncFlare()); + store->Init(syncer::SyncableService::StartSyncFlare(), nullptr); std::unique_ptr<PasswordForm> test_form( CreatePasswordFormFromDataForTesting(kTestCredential)); @@ -397,7 +412,7 @@ TEST_F(PasswordStoreTest, GetLoginsWithoutAffiliations) { scoped_refptr<PasswordStoreDefault> store(new PasswordStoreDefault( base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get(), base::WrapUnique(new LoginDatabase(test_login_db_file_path())))); - store->Init(syncer::SyncableService::StartSyncFlare()); + store->Init(syncer::SyncableService::StartSyncFlare(), nullptr); MockAffiliatedMatchHelper* mock_helper = new MockAffiliatedMatchHelper; store->SetAffiliatedMatchHelper(base::WrapUnique(mock_helper)); @@ -502,7 +517,7 @@ TEST_F(PasswordStoreTest, GetLoginsWithAffiliations) { scoped_refptr<PasswordStoreDefault> store(new PasswordStoreDefault( base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get(), base::WrapUnique(new LoginDatabase(test_login_db_file_path())))); - store->Init(syncer::SyncableService::StartSyncFlare()); + store->Init(syncer::SyncableService::StartSyncFlare(), nullptr); MockAffiliatedMatchHelper* mock_helper = new MockAffiliatedMatchHelper; store->SetAffiliatedMatchHelper(base::WrapUnique(mock_helper)); @@ -686,7 +701,7 @@ TEST_F(PasswordStoreTest, MAYBE_UpdatePasswordsStoredForAffiliatedWebsites) { base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get(), base::WrapUnique(new LoginDatabase(test_login_db_file_path())))); - store->Init(syncer::SyncableService::StartSyncFlare()); + store->Init(syncer::SyncableService::StartSyncFlare(), nullptr); store->RemoveLoginsCreatedBetween(base::Time(), base::Time::Max(), base::Closure()); @@ -795,7 +810,7 @@ TEST_F(PasswordStoreTest, GetLoginsWithAffiliatedRealms) { base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get(), base::MakeUnique<LoginDatabase>(test_login_db_file_path()))); - store->Init(syncer::SyncableService::StartSyncFlare()); + store->Init(syncer::SyncableService::StartSyncFlare(), nullptr); store->RemoveLoginsCreatedBetween(base::Time(), base::Time::Max(), base::Closure()); @@ -844,6 +859,61 @@ TEST_F(PasswordStoreTest, GetLoginsWithAffiliatedRealms) { } } +TEST_F(PasswordStoreTest, GetLoginsForSameOrganizationName) { + static constexpr const PasswordFormData kSameOrganizationCredentials[] = { + // Credential that is an exact match of the observed form. + {PasswordForm::SCHEME_HTML, kTestWebRealm1, kTestWebOrigin1, "", L"", L"", + L"", L"username_value_1", L"", true, 1}, + // Credential that is a PSL match of the observed form. + {PasswordForm::SCHEME_HTML, kTestPSLMatchingWebRealm, + kTestPSLMatchingWebOrigin, "", L"", L"", L"", L"username_value_2", L"", + true, 1}, + // Credential for the HTTP version of the observed form. (Should not be + // filled, but returned as part of same-organization-name matches). + {PasswordForm::SCHEME_HTML, kTestInsecureWebRealm, kTestInsecureWebOrigin, + "", L"", L"", L"", L"username_value_3", L"", true, 1}, + // Credential for a signon realm with a different TLD, but same + // organization identifying name. + {PasswordForm::SCHEME_HTML, kTestSameOrganizationNameWebRealm, + kTestSameOrganizationNameWebOrigin, "", L"", L"", L"", + L"username_value_4", L"", true, 1}, + }; + + static constexpr const PasswordFormData kNotSameOrganizationCredentials[] = { + // Unrelated Web credential. + {PasswordForm::SCHEME_HTML, kTestUnrelatedWebRealm, + kTestUnrelatedWebOrigin, "", L"", L"", L"", L"username_value_5", L"", + true, 1}, + // Credential for an affiliated Android application. + {PasswordForm::SCHEME_HTML, kTestAndroidRealm1, "", "", L"", L"", L"", + L"username_value_6", L"", true, 1}}; + + scoped_refptr<PasswordStoreDefault> store(new PasswordStoreDefault( + base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get(), + base::MakeUnique<LoginDatabase>(test_login_db_file_path()))); + store->Init(syncer::SyncableService::StartSyncFlare(), nullptr); + + std::vector<std::unique_ptr<PasswordForm>> expected_results; + for (const auto& form_data : kSameOrganizationCredentials) { + expected_results.push_back(CreatePasswordFormFromDataForTesting(form_data)); + store->AddLogin(*expected_results.back()); + } + + for (const auto& form_data : kNotSameOrganizationCredentials) { + store->AddLogin(*CreatePasswordFormFromDataForTesting(form_data)); + } + + const std::string observed_form_realm = kTestWebRealm1; + MockPasswordStoreConsumer mock_consumer; + EXPECT_CALL(mock_consumer, + OnGetPasswordStoreResultsConstRef( + UnorderedPasswordFormElementsAre(&expected_results))); + store->GetLoginsForSameOrganizationName(observed_form_realm, &mock_consumer); + base::RunLoop().RunUntilIdle(); + store->ShutdownOnUIThread(); + base::RunLoop().RunUntilIdle(); +} + // TODO(crbug.com/706392): Fix password reuse detection for Android. #if !defined(OS_ANDROID) && !defined(OS_IOS) TEST_F(PasswordStoreTest, CheckPasswordReuse) { @@ -861,7 +931,7 @@ TEST_F(PasswordStoreTest, CheckPasswordReuse) { scoped_refptr<PasswordStoreDefault> store(new PasswordStoreDefault( base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get(), base::MakeUnique<LoginDatabase>(test_login_db_file_path()))); - store->Init(syncer::SyncableService::StartSyncFlare()); + store->Init(syncer::SyncableService::StartSyncFlare(), nullptr); for (const auto& test_credentials : kTestCredentials) { auto credentials = CreatePasswordFormFromDataForTesting(test_credentials); diff --git a/chromium/components/password_manager/core/browser/psl_matching_helper.cc b/chromium/components/password_manager/core/browser/psl_matching_helper.cc index b4c4bef165c..6cbb305c764 100644 --- a/chromium/components/password_manager/core/browser/psl_matching_helper.cc +++ b/chromium/components/password_manager/core/browser/psl_matching_helper.cc @@ -116,4 +116,28 @@ std::string GetRegistryControlledDomain(const GURL& signon_realm) { signon_realm, net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES); } + +std::string GetOrganizationIdentifyingName(const GURL& url) { + if (!url.is_valid()) + return std::string(); + + const std::string organization_and_registrar = + net::registry_controlled_domains::GetDomainAndRegistry( + url, net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES); + const size_t registrar_length = + net::registry_controlled_domains::GetRegistryLength( + url, net::registry_controlled_domains::INCLUDE_UNKNOWN_REGISTRIES, + net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES); + + if (organization_and_registrar.empty() || !registrar_length || + registrar_length == std::string::npos) { + return std::string(); + } + + // No CHECK, std::string::substr gracefully handles an underflow there. + DCHECK_LT(registrar_length, organization_and_registrar.size()); + return organization_and_registrar.substr( + 0, organization_and_registrar.size() - registrar_length - 1); +} + } // namespace password_manager diff --git a/chromium/components/password_manager/core/browser/psl_matching_helper.h b/chromium/components/password_manager/core/browser/psl_matching_helper.h index 7bfea29daa9..1d0e6596927 100644 --- a/chromium/components/password_manager/core/browser/psl_matching_helper.h +++ b/chromium/components/password_manager/core/browser/psl_matching_helper.h @@ -36,6 +36,8 @@ enum class MatchResult { #if defined(UNIT_TEST) std::ostream& operator<<(std::ostream& out, MatchResult result); +// These functions are used in production internally but exposed for testing. + // Returns true iff |form_signon_realm| designates a federated credential for // |origin|. It doesn't check the port because |form_signon_realm| doesn't have // it. @@ -74,6 +76,11 @@ bool IsPublicSuffixDomainMatch(const std::string& url1, // registry-controlled domain part. std::string GetRegistryControlledDomain(const GURL& signon_realm); +// Returns the organization-identifying name of the host of |url|, that is, the +// first domain name label below the effective TLD. Returns the empty string for +// URLs where these concepts are ill-defined, as well as for invalid URLs. +std::string GetOrganizationIdentifyingName(const GURL& url); + } // namespace password_manager #endif // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PSL_MATCHING_HELPER_H_ diff --git a/chromium/components/password_manager/core/browser/psl_matching_helper_unittest.cc b/chromium/components/password_manager/core/browser/psl_matching_helper_unittest.cc index 66c798df366..3f6a25b041d 100644 --- a/chromium/components/password_manager/core/browser/psl_matching_helper_unittest.cc +++ b/chromium/components/password_manager/core/browser/psl_matching_helper_unittest.cc @@ -5,10 +5,15 @@ #include "components/password_manager/core/browser/psl_matching_helper.h" #include <stddef.h> +#include <cctype> #include "base/macros.h" +#include "base/stl_util.h" +#include "base/strings/string_util.h" +#include "base/strings/stringprintf.h" #include "components/autofill/core/common/password_form.h" #include "testing/gtest/include/gtest/gtest.h" +#include "url/gurl.h" namespace password_manager { @@ -387,6 +392,102 @@ TEST(PSLMatchingUtilsTest, IsFederatedPSLMatch) { } } +TEST(PSLMatchingUtilsTest, GetOrganizationIdentifyingName) { + static constexpr const struct { + const char* url; + const char* expected_organization_name; + } kTestCases[] = { + {"http://example.com/login", "example"}, + {"https://example.com", "example"}, + {"ftp://example.com/ftp_realm", "example"}, + + {"http://foo.bar.example.com", "example"}, + {"http://example.co.uk", "example"}, + {"http://bar.example.appspot.com", "example"}, + {"http://foo.bar", "foo"}, + {"https://user:pass@www.example.com:80/path?query#ref", "example"}, + + {"http://www.foo+bar.com", "foo+bar"}, + {"http://www.foo-bar.com", "foo-bar"}, + {"https://foo_bar.com", "foo_bar"}, + {"http://www.foo%2Bbar.com", "foo+bar"}, + {"http://www.foo%2Dbar.com", "foo-bar"}, + {"https://foo%5Fbar.com", "foo_bar"}, + {"http://www.foo%2Ebar.com", "bar"}, + + // Internationalized Domain Names: each dot-separated label of the domain + // name is individually Punycode-encoded, so the organization-identifying + // name is still well-defined and can be determined as normal. + // , , + // szotar = sz\xc3\xb3t\xc3\xa1r (UTF-8) = xn--sztr-7na0i (IDN) + // | | + // U+00E1 U+00F3 + {"https://www.sz\xc3\xb3t\xc3\xa1r.appspot.com", "xn--sztr-7na0i"}, + + {"http://www.foo!bar.com", "foo%21bar"}, + {"http://www.foo%21bar.com", "foo%21bar"}, + {"http://www.foo$bar.com", "foo%24bar"}, + {"http://www.foo&bar.com", "foo%26bar"}, + {"http://www.foo\'bar.com", "foo%27bar"}, + {"http://www.foo(bar.com", "foo%28bar"}, + {"http://www.foo)bar.com", "foo%29bar"}, + {"http://www.foo*bar.com", "foo%2Abar"}, + {"http://www.foo,bar.com", "foo%2Cbar"}, + {"http://www.foo=bar.com", "foo%3Dbar"}, + + // URLs without host portions, hosts without registry controlled domains + // should, or hosts consisting of a registry yield the empty string. + {"http://localhost", ""}, + {"http://co.uk", ""}, + {"http://google", ""}, + {"http://127.0.0.1", ""}, + {"file:///usr/bin/stuff", ""}, + {"federation://example.com/google.com", ""}, + {"android://hash@com.example/", ""}, + {"http://[1080:0:0:0:8:800:200C:417A]/", ""}, + {"http://[3ffe:2a00:100:7031::1]", ""}, + {"http://[::192.9.5.5]/", ""}, + + // Invalid URLs should yield the empty string. + {"", ""}, + {"http://", ""}, + {"bad url", ""}, + {"http://www.example.com/%00", ""}, + {"http://www.foo;bar.com", ""}, + {"http://www.foo~bar.com", ""}, + }; + + for (const auto& test_case : kTestCases) { + SCOPED_TRACE(test_case.url); + GURL url(test_case.url); + EXPECT_EQ(test_case.expected_organization_name, + GetOrganizationIdentifyingName(url)); + } +} + +// Apart from alphanumeric characters and '.', only |kExpectedUnescapedChars| +// are expected to appear without percent-encoding in the domain of a valid, +// canonicalized URL. +// +// The purpose of this test is to ensure that the test cases around unescaped +// special characters in `GetOrganizationIdentifyingName` are exhaustive. +TEST(PSLMatchingUtilsTest, + GetOrganizationIdentifyingName_UnescapedSpecialChars) { + static constexpr const char kExpectedNonAlnumChars[] = {'+', '-', '_', '.'}; + for (int chr = 0; chr <= 255; ++chr) { + const auto percent_encoded = base::StringPrintf("http://a%%%02Xb.hu/", chr); + const GURL url(percent_encoded); + if (isalnum(chr) || base::ContainsValue(kExpectedNonAlnumChars, chr)) { + ASSERT_TRUE(url.is_valid()); + const auto percent_decoded = base::StringPrintf( + "http://a%cb.hu/", base::ToLowerASCII(static_cast<char>(chr))); + EXPECT_EQ(percent_decoded, url.spec()); + } else if (url.is_valid()) { + EXPECT_EQ(percent_encoded, url.spec()); + } + } +} + } // namespace } // namespace password_manager diff --git a/chromium/components/password_manager/core/browser/stub_password_manager_client.cc b/chromium/components/password_manager/core/browser/stub_password_manager_client.cc index c4c9658ea66..309c877e946 100644 --- a/chromium/components/password_manager/core/browser/stub_password_manager_client.cc +++ b/chromium/components/password_manager/core/browser/stub_password_manager_client.cc @@ -69,6 +69,13 @@ safe_browsing::PasswordProtectionService* StubPasswordManagerClient::GetPasswordProtectionService() const { return nullptr; } + +void StubPasswordManagerClient::CheckSafeBrowsingReputation( + const GURL& form_action, + const GURL& frame_url) {} + +void StubPasswordManagerClient::CheckProtectedPasswordEntry( + const std::string& password_saved_domain) {} #endif } // namespace password_manager diff --git a/chromium/components/password_manager/core/browser/stub_password_manager_client.h b/chromium/components/password_manager/core/browser/stub_password_manager_client.h index a7a6b49588c..77c740befb5 100644 --- a/chromium/components/password_manager/core/browser/stub_password_manager_client.h +++ b/chromium/components/password_manager/core/browser/stub_password_manager_client.h @@ -46,6 +46,10 @@ class StubPasswordManagerClient : public PasswordManagerClient { #if defined(SAFE_BROWSING_DB_LOCAL) safe_browsing::PasswordProtectionService* GetPasswordProtectionService() const override; + void CheckSafeBrowsingReputation(const GURL& form_action, + const GURL& frame_url) override; + void CheckProtectedPasswordEntry( + const std::string& password_saved_domain) override; #endif private: diff --git a/chromium/components/password_manager/core/browser/stub_password_manager_driver.cc b/chromium/components/password_manager/core/browser/stub_password_manager_driver.cc index 18b7dc56a7c..4a9145faa38 100644 --- a/chromium/components/password_manager/core/browser/stub_password_manager_driver.cc +++ b/chromium/components/password_manager/core/browser/stub_password_manager_driver.cc @@ -60,4 +60,8 @@ autofill::AutofillDriver* StubPasswordManagerDriver::GetAutofillDriver() { return nullptr; } +bool StubPasswordManagerDriver::IsMainFrame() const { + return true; +} + } // namespace password_manager diff --git a/chromium/components/password_manager/core/browser/stub_password_manager_driver.h b/chromium/components/password_manager/core/browser/stub_password_manager_driver.h index f7a1703d1d9..406190aef35 100644 --- a/chromium/components/password_manager/core/browser/stub_password_manager_driver.h +++ b/chromium/components/password_manager/core/browser/stub_password_manager_driver.h @@ -37,6 +37,7 @@ class StubPasswordManagerDriver : public PasswordManagerDriver { PasswordManager* GetPasswordManager() override; PasswordAutofillManager* GetPasswordAutofillManager() override; autofill::AutofillDriver* GetAutofillDriver() override; + bool IsMainFrame() const override; private: DISALLOW_COPY_AND_ASSIGN(StubPasswordManagerDriver); diff --git a/chromium/components/password_manager/core/browser/suppressed_form_fetcher.cc b/chromium/components/password_manager/core/browser/suppressed_form_fetcher.cc new file mode 100644 index 00000000000..1a52b08999e --- /dev/null +++ b/chromium/components/password_manager/core/browser/suppressed_form_fetcher.cc @@ -0,0 +1,41 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "components/password_manager/core/browser/suppressed_form_fetcher.h" + +#include "base/logging.h" +#include "base/stl_util.h" +#include "components/password_manager/core/browser/password_manager_client.h" +#include "components/password_manager/core/browser/password_store.h" +#include "url/gurl.h" + +namespace password_manager { + +SuppressedFormFetcher::SuppressedFormFetcher( + const std::string& observed_signon_realm, + const PasswordManagerClient* client, + Consumer* consumer) + : client_(client), + consumer_(consumer), + observed_signon_realm_(observed_signon_realm) { + DCHECK(client_); + DCHECK(consumer_); + DCHECK(GURL(observed_signon_realm_).SchemeIsHTTPOrHTTPS()); + client_->GetPasswordStore()->GetLoginsForSameOrganizationName( + observed_signon_realm_, this); +} + +SuppressedFormFetcher::~SuppressedFormFetcher() = default; + +void SuppressedFormFetcher::OnGetPasswordStoreResults( + std::vector<std::unique_ptr<autofill::PasswordForm>> results) { + base::EraseIf(results, + [this](const std::unique_ptr<autofill::PasswordForm>& form) { + return form->signon_realm == observed_signon_realm_; + }); + + consumer_->ProcessSuppressedForms(std::move(results)); +} + +} // namespace password_manager diff --git a/chromium/components/password_manager/core/browser/suppressed_form_fetcher.h b/chromium/components/password_manager/core/browser/suppressed_form_fetcher.h new file mode 100644 index 00000000000..7f663494b8f --- /dev/null +++ b/chromium/components/password_manager/core/browser/suppressed_form_fetcher.h @@ -0,0 +1,62 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_SUPPRESSED_FORM_FETCHER_H_ +#define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_SUPPRESSED_FORM_FETCHER_H_ + +#include <memory> +#include <vector> + +#include "base/gtest_prod_util.h" +#include "base/macros.h" +#include "components/autofill/core/common/password_form.h" +#include "components/password_manager/core/browser/password_store_consumer.h" + +namespace password_manager { + +class PasswordManagerClient; + +// Fetches credentials saved for the HTTPS counterpart of the given HTTP realm. +// +// Filling these HTTPS credentials into forms served over HTTP is obviously +// suppressed, the purpose of doing such a query is to collect metrics on how +// often this happens and inconveniences the user. +// +// This logic is implemented by this class, a separate PasswordStore consumer, +// to make it very sure that these credentials will not get mistakenly filled. +class SuppressedFormFetcher : public PasswordStoreConsumer { + public: + // Interface to be implemented by the consumer of this class. + class Consumer { + public: + virtual void ProcessSuppressedForms( + std::vector<std::unique_ptr<autofill::PasswordForm>> forms) = 0; + }; + + SuppressedFormFetcher(const std::string& observed_signon_realm, + const PasswordManagerClient* client, + Consumer* consumer); + ~SuppressedFormFetcher() override; + + protected: + // PasswordStoreConsumer: + void OnGetPasswordStoreResults( + std::vector<std::unique_ptr<autofill::PasswordForm>> results) override; + + private: + FRIEND_TEST_ALL_PREFIXES(SuppressedFormFetcherTest, EmptyStore); + FRIEND_TEST_ALL_PREFIXES(SuppressedFormFetcherTest, FullStore); + + // The client and the consumer should outlive |this|. + const PasswordManagerClient* client_; + Consumer* consumer_; + + const std::string observed_signon_realm_; + + DISALLOW_COPY_AND_ASSIGN(SuppressedFormFetcher); +}; + +} // namespace password_manager + +#endif // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_SUPPRESSED_FORM_FETCHER_H_ diff --git a/chromium/components/password_manager/core/browser/suppressed_form_fetcher_unittest.cc b/chromium/components/password_manager/core/browser/suppressed_form_fetcher_unittest.cc new file mode 100644 index 00000000000..d4caa0422a2 --- /dev/null +++ b/chromium/components/password_manager/core/browser/suppressed_form_fetcher_unittest.cc @@ -0,0 +1,157 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "components/password_manager/core/browser/suppressed_form_fetcher.h" + +#include "base/macros.h" +#include "base/memory/ptr_util.h" +#include "base/message_loop/message_loop.h" +#include "components/password_manager/core/browser/mock_password_store.h" +#include "components/password_manager/core/browser/password_manager_test_utils.h" +#include "components/password_manager/core/browser/stub_password_manager_client.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace password_manager { +namespace { + +using autofill::PasswordForm; +using testing::_; + +constexpr const char kTestHttpURL[] = "http://one.example.com/"; +constexpr const char kTestHttpsURL[] = "https://one.example.com/"; +constexpr const char kTestPSLMatchingHttpURL[] = "http://psl.example.com/"; +constexpr const char kTestPSLMatchingHttpsURL[] = "https://psl.example.com/"; +constexpr const char kTestHttpSameOrgNameURL[] = "http://login.example.co.uk/"; +constexpr const char kTestHttpsSameOrgNameURL[] = + "https://login.example.co.uk/"; + +class MockConsumer : public SuppressedFormFetcher::Consumer { + public: + MockConsumer() = default; + ~MockConsumer() = default; + + // GMock still cannot mock methods with move-only args. + MOCK_METHOD1(ProcessSuppressedHTTPSFormsConstRef, + void(const std::vector<std::unique_ptr<PasswordForm>>&)); + + protected: + // SuppressedFormFetcher::Consumer: + void ProcessSuppressedForms( + std::vector<std::unique_ptr<PasswordForm>> forms) override { + ProcessSuppressedHTTPSFormsConstRef(forms); + } + + private: + DISALLOW_COPY_AND_ASSIGN(MockConsumer); +}; + +class PasswordManagerClientWithMockStore : public StubPasswordManagerClient { + public: + PasswordManagerClientWithMockStore() + : mock_store_(new ::testing::StrictMock<MockPasswordStore>()) {} + ~PasswordManagerClientWithMockStore() override { + mock_store_->ShutdownOnUIThread(); + } + + MockPasswordStore& mock_password_store() const { return *mock_store_.get(); } + + protected: + // StubPasswordManagerClient: + PasswordStore* GetPasswordStore() const override { return mock_store_.get(); } + + private: + scoped_refptr<MockPasswordStore> mock_store_; + + DISALLOW_COPY_AND_ASSIGN(PasswordManagerClientWithMockStore); +}; + +} // namespace + +class SuppressedFormFetcherTest : public testing::Test { + public: + SuppressedFormFetcherTest() = default; + ~SuppressedFormFetcherTest() override = default; + + MockConsumer* mock_consumer() { return &consumer_; } + MockPasswordStore* mock_store() { return &client_.mock_password_store(); } + PasswordManagerClientWithMockStore* mock_client() { return &client_; } + + private: + base::MessageLoop message_loop_; // Needed by the MockPasswordStore. + + MockConsumer consumer_; + PasswordManagerClientWithMockStore client_; + + DISALLOW_COPY_AND_ASSIGN(SuppressedFormFetcherTest); +}; + +TEST_F(SuppressedFormFetcherTest, EmptyStore) { + EXPECT_CALL(*mock_store(), GetLoginsForSameOrganizationName(kTestHttpURL, _)); + SuppressedFormFetcher suppressed_form_fetcher(kTestHttpURL, mock_client(), + mock_consumer()); + EXPECT_CALL(*mock_consumer(), + ProcessSuppressedHTTPSFormsConstRef(::testing::IsEmpty())); + suppressed_form_fetcher.OnGetPasswordStoreResults( + std::vector<std::unique_ptr<PasswordForm>>()); +} + +TEST_F(SuppressedFormFetcherTest, FullStore) { + static constexpr const PasswordFormData kSuppressedCredentials[] = { + // Credential that is for the HTTPS counterpart of the observed form. + {PasswordForm::SCHEME_HTML, kTestHttpsURL, kTestHttpsURL, "", L"", L"", + L"", L"username_value_1", L"password_value_1", true, 1}, + // Once again, but with a different username/password. + {PasswordForm::SCHEME_HTML, kTestHttpsURL, kTestHttpsURL, "", L"", L"", + L"", L"username_value_2", L"password_value_2", true, 1}, + // A PSL match to the observed form. + {PasswordForm::SCHEME_HTML, kTestPSLMatchingHttpURL, + kTestPSLMatchingHttpURL, "", L"", L"", L"", L"username_value_2", + L"password_value_2", true, 1}, + // A PSL match to the HTTPS counterpart of the observed form. Note that + // this is *not* a PSL match to the observed form, but a same organization + // name match. + {PasswordForm::SCHEME_HTML, kTestPSLMatchingHttpsURL, + kTestPSLMatchingHttpsURL, "", L"", L"", L"", L"username_value_3", + L"password_value_3", true, 1}, + // Credentials for a HTTP origin with the same organization + // identifying name. + {PasswordForm::SCHEME_HTML, kTestHttpSameOrgNameURL, + kTestHttpSameOrgNameURL, "", L"", L"", L"", L"username_value_4", + L"password_value_4", true, 1}, + // Credentials for a HTTPS origin with the same organization + // identifying name. + {PasswordForm::SCHEME_HTML, kTestHttpsSameOrgNameURL, + kTestHttpsSameOrgNameURL, "", L"", L"", L"", L"username_value_5", + L"password_value_5", true, 1}}; + + static const PasswordFormData kNotSuppressedCredentials[] = { + // Credential exactly matching the observed form. + {PasswordForm::SCHEME_HTML, kTestHttpURL, kTestHttpURL, "", L"", L"", L"", + L"username_value_1", L"password_value_1", true, 1}, + }; + + std::vector<std::unique_ptr<PasswordForm>> simulated_store_results; + std::vector<std::unique_ptr<PasswordForm>> expected_results; + for (const auto& form_data : kSuppressedCredentials) { + expected_results.push_back(CreatePasswordFormFromDataForTesting(form_data)); + simulated_store_results.push_back( + CreatePasswordFormFromDataForTesting(form_data)); + } + for (const auto& form_data : kNotSuppressedCredentials) { + simulated_store_results.push_back( + CreatePasswordFormFromDataForTesting(form_data)); + } + + EXPECT_CALL(*mock_store(), GetLoginsForSameOrganizationName(kTestHttpURL, _)); + SuppressedFormFetcher suppressed_form_fetcher(kTestHttpURL, mock_client(), + mock_consumer()); + EXPECT_CALL(*mock_consumer(), + ProcessSuppressedHTTPSFormsConstRef( + UnorderedPasswordFormElementsAre(&expected_results))); + suppressed_form_fetcher.OnGetPasswordStoreResults( + std::move(simulated_store_results)); +} + +} // namespace password_manager diff --git a/chromium/components/password_manager/core/browser/test_password_store.cc b/chromium/components/password_manager/core/browser/test_password_store.cc index 6ab4b775e25..1e11a3241d4 100644 --- a/chromium/components/password_manager/core/browser/test_password_store.cc +++ b/chromium/components/password_manager/core/browser/test_password_store.cc @@ -87,7 +87,7 @@ TestPasswordStore::FillMatchingLogins(const FormDigest& form) { std::vector<std::unique_ptr<autofill::PasswordForm>> matched_forms; for (const auto& elements : stored_passwords_) { // The code below doesn't support PSL federated credential. It's doable but - // no test need it so far. + // no tests need it so far. const bool realm_matches = elements.first == form.signon_realm; const bool realm_psl_matches = IsPublicSuffixDomainMatch(elements.first, form.signon_realm); @@ -112,6 +112,13 @@ TestPasswordStore::FillMatchingLogins(const FormDigest& form) { return matched_forms; } +std::vector<std::unique_ptr<autofill::PasswordForm>> +TestPasswordStore::FillLoginsForSameOrganizationName( + const std::string& signon_realm) { + // TODO: Implement when needed. + return std::vector<std::unique_ptr<autofill::PasswordForm>>(); +} + void TestPasswordStore::ReportMetricsImpl(const std::string& sync_username, bool custom_passphrase_sync_enabled) { } diff --git a/chromium/components/password_manager/core/browser/test_password_store.h b/chromium/components/password_manager/core/browser/test_password_store.h index 8a644a6deb1..837dfaa6c7d 100644 --- a/chromium/components/password_manager/core/browser/test_password_store.h +++ b/chromium/components/password_manager/core/browser/test_password_store.h @@ -46,6 +46,8 @@ class TestPasswordStore : public PasswordStore { const autofill::PasswordForm& form) override; std::vector<std::unique_ptr<autofill::PasswordForm>> FillMatchingLogins( const FormDigest& form) override; + std::vector<std::unique_ptr<autofill::PasswordForm>> + FillLoginsForSameOrganizationName(const std::string& signon_realm) override; // Unused portions of PasswordStore interface void ReportMetricsImpl(const std::string& sync_username, diff --git a/chromium/components/password_manager/core/browser/webdata/password_web_data_service_win.cc b/chromium/components/password_manager/core/browser/webdata/password_web_data_service_win.cc index b063667be7a..8a432564fb4 100644 --- a/chromium/components/password_manager/core/browser/webdata/password_web_data_service_win.cc +++ b/chromium/components/password_manager/core/browser/webdata/password_web_data_service_win.cc @@ -5,6 +5,7 @@ #include "components/password_manager/core/browser/webdata/password_web_data_service_win.h" #include "base/bind.h" +#include "base/single_thread_task_runner.h" #include "components/os_crypt/ie7_password_win.h" #include "components/password_manager/core/browser/webdata/logins_table.h" #include "components/webdata/common/web_database_service.h" diff --git a/chromium/components/password_manager/core/common/credential_manager_types.h b/chromium/components/password_manager/core/common/credential_manager_types.h index 06e7f0096ce..b9fffab9380 100644 --- a/chromium/components/password_manager/core/common/credential_manager_types.h +++ b/chromium/components/password_manager/core/common/credential_manager_types.h @@ -33,6 +33,8 @@ enum class CredentialType { CREDENTIAL_TYPE_LAST = CREDENTIAL_TYPE_FEDERATED }; +enum class CredentialMediationRequirement { kSilent, kOptional, kRequired }; + std::string CredentialTypeToString(CredentialType value); std::ostream& operator<<(std::ostream& os, CredentialType value); diff --git a/chromium/components/password_manager/core/common/password_manager_pref_names.cc b/chromium/components/password_manager/core/common/password_manager_pref_names.cc index fd70a41fe4b..34f10eae2a8 100644 --- a/chromium/components/password_manager/core/common/password_manager_pref_names.cc +++ b/chromium/components/password_manager/core/common/password_manager_pref_names.cc @@ -39,5 +39,7 @@ const char kWasSignInPasswordPromoClicked[] = const char kNumberSignInPasswordPromoShown[] = "profile.number_sign_in_password_promo_shown"; +const char kSyncPasswordHash[] = "profile.sync_password_hash"; + } // namespace prefs } // namespace password_manager diff --git a/chromium/components/password_manager/core/common/password_manager_pref_names.h b/chromium/components/password_manager/core/common/password_manager_pref_names.h index dba5fb451e4..9f05a765f1e 100644 --- a/chromium/components/password_manager/core/common/password_manager_pref_names.h +++ b/chromium/components/password_manager/core/common/password_manager_pref_names.h @@ -68,6 +68,9 @@ extern const char kWasSignInPasswordPromoClicked[]; // Number of times the Chrome Sign in promo popped up. extern const char kNumberSignInPasswordPromoShown[]; +// String that represent the sync password hash. +extern const char kSyncPasswordHash[]; + } // namespace prefs } // namespace password_manager diff --git a/chromium/components/password_manager/sync/browser/password_manager_setting_migrator_service_unittest.cc b/chromium/components/password_manager/sync/browser/password_manager_setting_migrator_service_unittest.cc index 6e432dfb60f..3ba8c194cfa 100644 --- a/chromium/components/password_manager/sync/browser/password_manager_setting_migrator_service_unittest.cc +++ b/chromium/components/password_manager/sync/browser/password_manager_setting_migrator_service_unittest.cc @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include <utility> + #include "base/json/json_writer.h" #include "base/macros.h" #include "base/metrics/field_trial.h" @@ -192,7 +194,7 @@ class PasswordManagerSettingMigratorServiceTest : public testing::Test { pref_service_syncable = factory.CreateSyncable(pref_registry.get()); migration_service_.reset( new PasswordManagerSettingMigratorService(pref_service_syncable.get())); - pref_service_.reset(pref_service_syncable.release()); + pref_service_ = std::move(pref_service_syncable); } void ExpectValuesForBothPrefValues(bool new_pref_value, bool old_pref_value) { |