Merge to branch 2272 [Password Manager] Fix bug with Mac and Android credentials

Previously syncing Android credentials to a Mac would delete them, as
these credentials would be managled on insertion to the Keychain. Later
lookups (e.g. for accessing chrome://settings/passwords) would delete the
credentials because unmatched credentials are deleted on the assumption
that they were removed in the Keychain already.

This CL also adds tests to verify that similar bugs do not exist on other platforms
as well.

BUG=455551

Review URL: https://codereview.chromium.org/911743002

Cr-Commit-Position: refs/heads/master@{#315887}
(cherry picked from commit c5e83abcfa345d233d852e80bd196bfee913612c)

Committed: https://chromium.googlesource.com/chromium/src/+/559f24b7d96571db6e6689d3c71085a842de784e

Review URL: https://codereview.chromium.org/925983004

Cr-Commit-Position: refs/branch-heads/2272@{#298}
Cr-Branched-From: 827a380cfdb31aa54c8d56e63ce2c3fd8c3ba4d4-refs/heads/master@{#310958}

diff --git a/chrome/browser/password_manager/native_backend_gnome_x_unittest.cc b/chrome/browser/password_manager/native_backend_gnome_x_unittest.cc
index 50b5a4a..40d0901 100644
--- a/chrome/browser/password_manager/native_backend_gnome_x_unittest.cc
+++ b/chrome/browser/password_manager/native_backend_gnome_x_unittest.cc

@@ -1069,6 +1069,37 @@
     CheckMockKeyringItem(&mock_keyring_items[0], form_google_, "chrome-42");
 }
 
+TEST_F(NativeBackendGnomeTest, AndroidCredentials) {
+  NativeBackendGnome backend(42);
+  backend.Init();
+
+  PasswordForm observed_android_form;
+  observed_android_form.scheme = PasswordForm::SCHEME_HTML;
+  observed_android_form.signon_realm =
+      "android://7x7IDboo8u9YKraUsbmVkuf1-@net.rateflix.app/";
+  PasswordForm saved_android_form = observed_android_form;
+  saved_android_form.username_value = base::UTF8ToUTF16("randomusername");
+  saved_android_form.password_value = base::UTF8ToUTF16("password");
+  saved_android_form.date_created = base::Time::FromTimeT(1);
+
+  BrowserThread::PostTask(
+      BrowserThread::DB, FROM_HERE,
+      base::Bind(base::IgnoreResult(&NativeBackendGnome::AddLogin),
+                 base::Unretained(&backend), saved_android_form));
+
+  std::vector<PasswordForm*> form_list;
+  BrowserThread::PostTask(
+      BrowserThread::DB, FROM_HERE,
+      base::Bind(base::IgnoreResult(&NativeBackendGnome::GetLogins),
+                 base::Unretained(&backend), observed_android_form,
+                 &form_list));
+
+  RunBothThreads();
+
+  EXPECT_EQ(1u, form_list.size());
+  EXPECT_EQ(saved_android_form, *form_list[0]);
+}
+
 TEST_F(NativeBackendGnomeTest, RemoveLoginsCreatedBetween) {
   CheckRemoveLoginsBetween(CREATED);
 }

diff --git a/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc b/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
index 5960c13..1c8a468 100644
--- a/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
+++ b/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc

@@ -899,6 +899,42 @@
   CheckPasswordForms("Chrome Form Data (42)", expected);
 }
 
+TEST_F(NativeBackendKWalletTest, AndroidCredentials) {
+  NativeBackendKWalletStub backend(42);
+  EXPECT_TRUE(backend.InitWithBus(mock_session_bus_));
+
+  PasswordForm observed_android_form;
+  observed_android_form.scheme = PasswordForm::SCHEME_HTML;
+  observed_android_form.signon_realm =
+      "android://7x7IDboo8u9YKraUsbmVkuf1-@net.rateflix.app/";
+  PasswordForm saved_android_form = observed_android_form;
+  saved_android_form.username_value = base::UTF8ToUTF16("randomusername");
+  saved_android_form.password_value = base::UTF8ToUTF16("password");
+
+  BrowserThread::PostTask(
+      BrowserThread::DB, FROM_HERE,
+      base::Bind(base::IgnoreResult(&NativeBackendKWalletStub::AddLogin),
+                 base::Unretained(&backend), saved_android_form));
+
+  std::vector<PasswordForm*> form_list;
+  BrowserThread::PostTask(
+      BrowserThread::DB, FROM_HERE,
+      base::Bind(
+          base::IgnoreResult(&NativeBackendKWalletStub::GetLogins),
+          base::Unretained(&backend), observed_android_form, &form_list));
+
+  RunDBThread();
+
+  EXPECT_EQ(1u, form_list.size());
+
+  std::vector<const PasswordForm*> forms;
+  forms.push_back(&saved_android_form);
+  ExpectationArray expected;
+  expected.push_back(
+      make_pair(std::string(saved_android_form.signon_realm), forms));
+  CheckPasswordForms("Chrome Form Data (42)", expected);
+}
+
 TEST_F(NativeBackendKWalletTest, RemoveLoginsCreatedBetween) {
   TestRemoveLoginsBetween(CREATED);
 }

diff --git a/chrome/browser/password_manager/password_store_mac.cc b/chrome/browser/password_manager/password_store_mac.cc
index 416bab5..8418752 100644
--- a/chrome/browser/password_manager/password_store_mac.cc
+++ b/chrome/browser/password_manager/password_store_mac.cc

@@ -21,6 +21,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/mac/security_wrappers.h"
+#include "components/password_manager/core/browser/affiliation_utils.h"
 #include "components/password_manager/core/browser/login_database.h"
 #include "components/password_manager/core/browser/password_store_change.h"
 #include "content/public/browser/browser_thread.h"
@@ -370,12 +371,19 @@
     form->blacklisted_by_user = true;
   }
 
-  form->origin = URLFromComponents(form->ssl_valid, server, port, path);
-  // TODO(stuartmorgan): Handle proxies, which need a different signon_realm
-  // format.
-  form->signon_realm = form->origin.GetOrigin().spec();
-  if (form->scheme != PasswordForm::SCHEME_HTML) {
-    form->signon_realm.append(security_domain);
+  // Android facet URLs aren't parsed correctly by GURL and need to be handled
+  // separately.
+  if (password_manager::IsValidAndroidFacetURI(server)) {
+    form->signon_realm = server;
+    form->origin = GURL();
+  } else {
+    form->origin = URLFromComponents(form->ssl_valid, server, port, path);
+    // TODO(stuartmorgan): Handle proxies, which need a different signon_realm
+    // format.
+    form->signon_realm = form->origin.GetOrigin().spec();
+    if (form->scheme != PasswordForm::SCHEME_HTML) {
+      form->signon_realm.append(security_domain);
+    }
   }
   return true;
 }
@@ -553,6 +561,19 @@
                                   UInt32* port,
                                   bool* is_secure,
                                   std::string* security_domain) {
+  // GURL does not parse Android facet URIs correctly.
+  if (password_manager::IsValidAndroidFacetURI(signon_realm)) {
+    if (server)
+      *server = signon_realm;
+    if (is_secure)
+      *is_secure = true;
+    if (port)
+      *port = 0;
+    if (security_domain)
+      security_domain->clear();
+    return true;
+  }
+
   // The signon_realm will be the Origin portion of a URL for an HTML form,
   // and the same but with the security domain as a path for HTTP auth.
   GURL realm_as_url(signon_realm);
@@ -699,9 +720,12 @@
            form.signon_realm, &server, &port, &is_secure, &security_domain)) {
     return false;
   }
+  std::string path;
+  // Path doesn't make sense for Android app credentials.
+  if (!password_manager::IsValidAndroidFacetURI(form.signon_realm))
+    path = form.origin.path();
   std::string username = base::UTF16ToUTF8(form.username_value);
   std::string password = base::UTF16ToUTF8(form.password_value);
-  std::string path = form.origin.path();
   SecProtocolType protocol = is_secure ? kSecProtocolTypeHTTPS
                                        : kSecProtocolTypeHTTP;
   SecKeychainItemRef new_item = NULL;

diff --git a/chrome/browser/password_manager/password_store_mac_unittest.cc b/chrome/browser/password_manager/password_store_mac_unittest.cc
index a3e8810..6eb6f573 100644
--- a/chrome/browser/password_manager/password_store_mac_unittest.cc
+++ b/chrome/browser/password_manager/password_store_mac_unittest.cc

@@ -1442,3 +1442,40 @@
       "http://some.domain.com/insecure.html", PasswordForm::SCHEME_HTML);
   ASSERT_EQ(1u, matching_items.size());
 }
+
+// Verify that Android app passwords are retrievable.
+// Regression test for http://crbug.com/455551
+TEST_F(PasswordStoreMacTest, AndroidCredentialsMatchAfterInsertion) {
+  PasswordForm form;
+  form.signon_realm = "android://7x7IDboo8u9YKraUsbmVkuf1@net.rateflix.app/";
+  form.username_value = base::UTF8ToUTF16("randomusername");
+  form.password_value = base::UTF8ToUTF16("password");
+  store()->AddLogin(form);
+  WaitForStoreUpdate();
+
+  PasswordForm returned_form;
+  MockPasswordStoreConsumer consumer;
+  EXPECT_CALL(consumer, OnGetPasswordStoreResults(_)).WillOnce(DoAll(
+      WithArg<0>(Invoke(&consumer, &MockPasswordStoreConsumer::CopyElements)),
+      WithArg<0>(STLDeleteElements0()),
+      QuitUIMessageLoop()));
+
+  store()->GetAutofillableLogins(&consumer);
+  base::MessageLoop::current()->Run();
+  ::testing::Mock::VerifyAndClearExpectations(&consumer);
+  EXPECT_EQ(1u, consumer.last_result.size());
+  EXPECT_EQ(form, consumer.last_result[0]);
+
+  PasswordForm query_form = form;
+  query_form.password_value.clear();
+  query_form.username_value.clear();
+  EXPECT_CALL(consumer, OnGetPasswordStoreResults(_)).WillOnce(DoAll(
+      WithArg<0>(Invoke(&consumer, &MockPasswordStoreConsumer::CopyElements)),
+      WithArg<0>(STLDeleteElements0()),
+      QuitUIMessageLoop()));
+  store()->GetLogins(query_form, PasswordStore::ALLOW_PROMPT, &consumer);
+  base::MessageLoop::current()->Run();
+  ::testing::Mock::VerifyAndClearExpectations(&consumer);
+  EXPECT_EQ(1u, consumer.last_result.size());
+  EXPECT_EQ(form, consumer.last_result[0]);
+}

diff --git a/components/autofill/core/common/password_form.h b/components/autofill/core/common/password_form.h
index 039e644..bcacbdd 100644
--- a/components/autofill/core/common/password_form.h
+++ b/components/autofill/core/common/password_form.h

@@ -49,8 +49,10 @@
     SCHEME_LAST = SCHEME_OTHER
   } scheme;
 
-  // The "Realm" for the sign-on (scheme, host, port for SCHEME_HTML, and
-  // contains the HTTP realm for dialog-based forms).
+  // The "Realm" for the sign-on. This is scheme, host, port for SCHEME_HTML.
+  // Dialog based forms also contain the HTTP realm. Android based forms will
+  // contain a string of the form "android://<hash of cert>@<package name>"
+  //
   // The signon_realm is effectively the primary key used for retrieving
   // data from the database, so it must not be empty.
   std::string signon_realm;
@@ -76,8 +78,8 @@
   // An origin URL consists of the scheme, host, port and path; the rest is
   // stripped. This is the primary data used by the PasswordManager to decide
   // (in longest matching prefix fashion) whether or not a given PasswordForm
-  // result from the database is a good fit for a particular form on a page, so
-  // it must not be empty.
+  // result from the database is a good fit for a particular form on a page.
+  // This should not be empty except for Android based credentials.
   GURL origin;
 
   // The action target of the form; like |origin| URL consists of the scheme,

diff --git a/components/password_manager/core/browser/affiliation_utils.cc b/components/password_manager/core/browser/affiliation_utils.cc
index d34abd8..4f0d978 100644
--- a/components/password_manager/core/browser/affiliation_utils.cc
+++ b/components/password_manager/core/browser/affiliation_utils.cc

@@ -266,4 +266,9 @@
   return std::equal(a_sorted.begin(), a_sorted.end(), b_sorted.begin());
 }
 
+bool IsValidAndroidFacetURI(const std::string& url) {
+  FacetURI facet = FacetURI::FromPotentiallyInvalidSpec(url);
+  return facet.IsValidAndroidFacetURI();
+}
+
 }  // namespace password_manager

diff --git a/components/password_manager/core/browser/affiliation_utils.h b/components/password_manager/core/browser/affiliation_utils.h
index af153b9b..61ef77a 100644
--- a/components/password_manager/core/browser/affiliation_utils.h
+++ b/components/password_manager/core/browser/affiliation_utils.h

@@ -144,6 +144,9 @@
 bool AreEquivalenceClassesEqual(const AffiliatedFacets& a,
                                 const AffiliatedFacets& b);
 
+// A shorter way to spell FacetURI::IsValidAndroidFacetURI().
+bool IsValidAndroidFacetURI(const std::string& uri);
+
 }  // namespace password_manager
 
 #endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_AFFILIATION_UTILS_H_

diff --git a/components/password_manager/core/browser/login_database.cc b/components/password_manager/core/browser/login_database.cc
index c769481..5031d2da 100644
--- a/components/password_manager/core/browser/login_database.cc
+++ b/components/password_manager/core/browser/login_database.cc

@@ -17,6 +17,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "components/autofill/core/common/password_form.h"
+#include "components/password_manager/core/browser/affiliation_utils.h"
 #include "components/password_manager/core/browser/password_manager_client.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 #include "google_apis/gaia/gaia_urls.h"
@@ -119,7 +120,7 @@
 }
 
 bool DoesMatchConstraints(const PasswordForm& form) {
-  if (form.origin.is_empty()) {
+  if (!IsValidAndroidFacetURI(form.signon_realm) && form.origin.is_empty()) {
     DLOG(ERROR) << "Constraint violation: form.origin is empty";
     return false;
   }