Do not involve PasswordManagerDriver in filling HTTP-auth forms; also check realm

components/password_manager/core/browser/password_manager.cc

Index: components/password_manager/core/browser/password_manager.cc
diff --git a/components/password_manager/core/browser/password_manager.cc b/components/password_manager/core/browser/password_manager.cc
index d30b10acaedce1e911444932c3fa8978e7ea88a2..3529e88657ea53cf9dd9de2a9252978f15a6baf0 100644
--- a/components/password_manager/core/browser/password_manager.cc
+++ b/components/password_manager/core/browser/password_manager.cc
@@ -4,6 +4,8 @@
 
 #include "components/password_manager/core/browser/password_manager.h"
 
+#include <map>
+
 #include "base/command_line.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_macros.h"
@@ -395,8 +397,16 @@ void PasswordManager::AddSubmissionCallback(
   submission_callbacks_.push_back(callback);
 }
 
-void PasswordManager::AddObserver(LoginModelObserver* observer) {
+void PasswordManager::AddObserverAndDeliverCredentials(
+    LoginModelObserver* observer,
+    const PasswordForm& observed_form) {

[davidben, 2015/10/07 23:13:54]

In a vacuum, I think an API like perhaps

  void PasswordManager::GetPreferredCredentials(
      const PasswordForm& observed_form,
      const base::Callback<std::string, std::string>& callback) {
    // Call |callback| if credentials are found.
  }

would be cleaner. (If you get two copies of the same prompt, I think this
observer mechanism will still double-signal. It just won't matter because the
realms will match.) But that seems a much more involved job and I'm not familiar
with this code, so I'll leave that judgement to you.

[vabr (Chromium), 2015/10/08 10:24:12]

I agree with you that what you suggest would be a cleaner approach. On the other
hand, the observer list currently is there to guarantee that observers can get
removed and added any time in a safe way, so to use the callback, we would need
a weak pointer to the observer or something analogous.

The double-signal is not a big issue (not a security one, and performance-wise
we are not too concerned, given the expected number of the observers). We might
have a major redesign of the code during the coming year or so, so at this point
I'd prefer not to invest too much effort in clearing this particular issue. I'm
noting a TODO, nevertheless, so that your observation is kept once we get to the
redesign.

   observers_.AddObserver(observer);
+
+  observer->set_signon_realm(observed_form.signon_realm);
+
+  std::vector<PasswordForm> observed_forms;
+  observed_forms.push_back(observed_form);
+  OnPasswordFormsParsed(nullptr, observed_forms);
 }
 
 void PasswordManager::RemoveObserver(LoginModelObserver* observer) {
@@ -470,7 +480,8 @@ void PasswordManager::CreatePendingLoginManagers(
         continue;
       }
       old_manager_found = true;
-      old_manager->ProcessFrame(driver->AsWeakPtr());
+      if (driver)
+        old_manager->ProcessFrame(driver->AsWeakPtr());
       break;
     }
     if (old_manager_found)
@@ -492,7 +503,9 @@ void PasswordManager::CreatePendingLoginManagers(
       logger->LogFormSignatures(Logger::STRING_ADDING_SIGNATURE, *iter);
     bool ssl_valid = iter->origin.SchemeIsCryptographic();
     PasswordFormManager* manager = new PasswordFormManager(
-        this, client_, driver->AsWeakPtr(), *iter, ssl_valid);
+        this, client_,
+        (driver ? driver->AsWeakPtr() : base::WeakPtr<PasswordManagerDriver>()),
+        *iter, ssl_valid);
     pending_login_managers_.push_back(manager);
 
     PasswordStore::AuthorizationPromptPolicy prompt_policy =
@@ -703,45 +716,46 @@ void PasswordManager::Autofill(password_manager::PasswordManagerDriver* driver,
                                const PasswordFormMap& best_matches,
                                const PasswordForm& preferred_match,
                                bool wait_for_username) const {
+  DCHECK_EQ(PasswordForm::SCHEME_HTML, preferred_match.scheme);
+
   scoped_ptr<BrowserSavePasswordProgressLogger> logger;
   if (client_->IsLoggingActive()) {
     logger.reset(new BrowserSavePasswordProgressLogger(client_));
     logger->LogMessage(Logger::STRING_PASSWORDMANAGER_AUTOFILL);
   }
-  switch (form_for_autofill.scheme) {
-    case PasswordForm::SCHEME_HTML: {
-      // Note the check above is required because the observers_ for a non-HTML
-      // schemed password form may have been freed, so we need to distinguish.
-      autofill::PasswordFormFillData fill_data;
-      InitPasswordFormFillData(form_for_autofill,
-                               best_matches,
-                               &preferred_match,
-                               wait_for_username,
-                               OtherPossibleUsernamesEnabled(),
-                               &fill_data);
-      if (logger)
-        logger->LogBoolean(Logger::STRING_WAIT_FOR_USERNAME, wait_for_username);
-      UMA_HISTOGRAM_BOOLEAN(
-          "PasswordManager.FillSuggestionsIncludeAndroidAppCredentials",
-          ContainsAndroidCredentials(fill_data));
-      metrics_util::LogFilledCredentialIsFromAndroidApp(
-          PreferredRealmIsFromAndroid(fill_data));
-      driver->FillPasswordForm(fill_data);
-      break;
-    }
-    default:
-      if (logger) {
-        logger->LogBoolean(Logger::STRING_LOGINMODELOBSERVER_PRESENT,
-                           observers_.might_have_observers());
-      }
-      FOR_EACH_OBSERVER(
-          LoginModelObserver,
-          observers_,
-          OnAutofillDataAvailable(preferred_match.username_value,
-                                  preferred_match.password_value));
-      break;
+
+  autofill::PasswordFormFillData fill_data;
+  InitPasswordFormFillData(form_for_autofill, best_matches, &preferred_match,
+                           wait_for_username, OtherPossibleUsernamesEnabled(),
+                           &fill_data);
+  if (logger)
+    logger->LogBoolean(Logger::STRING_WAIT_FOR_USERNAME, wait_for_username);
+  UMA_HISTOGRAM_BOOLEAN(
+      "PasswordManager.FillSuggestionsIncludeAndroidAppCredentials",
+      ContainsAndroidCredentials(fill_data));
+  metrics_util::LogFilledCredentialIsFromAndroidApp(
+      PreferredRealmIsFromAndroid(fill_data));
+  driver->FillPasswordForm(fill_data);
+
+  client_->PasswordWasAutofilled(best_matches);
+}
+
+void PasswordManager::AutofillHttpAuth(
+    const PasswordFormMap& best_matches,
+    const PasswordForm& preferred_match) const {
+  DCHECK_NE(PasswordForm::SCHEME_HTML, preferred_match.scheme);
+
+  scoped_ptr<BrowserSavePasswordProgressLogger> logger;
+  if (client_->IsLoggingActive()) {
+    logger.reset(new BrowserSavePasswordProgressLogger(client_));
+    logger->LogMessage(Logger::STRING_PASSWORDMANAGER_AUTOFILLHTTPAUTH);

[davidben, 2015/10/07 23:13:54]

[I have no idea what this logger thing is for and will defer to you on whether
havnig a separate one for HTTP is what you want. :-) ]

[vabr (Chromium), 2015/10/08 10:24:12]

This is for the chrome://password-manager-internals page. It has been useful to
have the logs indicate the code path taken by signatures of notable methods.
While it would probably be possible to deduce this from the other messages
currently, I don't see a harm in having it explicit.

+    logger->LogBoolean(Logger::STRING_LOGINMODELOBSERVER_PRESENT,
+                       observers_.might_have_observers());
   }
 
+  FOR_EACH_OBSERVER(LoginModelObserver, observers_,
+                    OnAutofillDataAvailable(preferred_match));
+
   client_->PasswordWasAutofilled(best_matches);
 }