[Password Manager] Implements entries sorting and duplicates omitting on chrome://settings/passwords

chrome/browser/ui/passwords/password_manager_presenter.cc

Index: chrome/browser/ui/passwords/password_manager_presenter.cc
diff --git a/chrome/browser/ui/passwords/password_manager_presenter.cc b/chrome/browser/ui/passwords/password_manager_presenter.cc
index 1b30d119803487ee8b2aeea6968e618c7c7c9078..196f2b0bb037b4100fc21a3047c50e4925d1d6f8 100644
--- a/chrome/browser/ui/passwords/password_manager_presenter.cc
+++ b/chrome/browser/ui/passwords/password_manager_presenter.cc
@@ -9,6 +9,9 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/metrics/user_metrics_action.h"
+#include "base/sha1.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "base/values.h"
@@ -26,12 +29,14 @@
 #include "components/browser_sync/browser/profile_sync_service.h"
 #include "components/password_manager/core/browser/affiliation_utils.h"
 #include "components/password_manager/core/browser/password_manager_util.h"
+#include "components/password_manager/core/browser/password_ui_utils.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "components/password_manager/sync/browser/password_sync_util.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_contents.h"
 
+#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #if defined(OS_WIN)
 #include "chrome/browser/password_manager/password_manager_util_win.h"
 #elif defined(OS_MACOSX)
@@ -40,6 +45,89 @@
 
 using password_manager::PasswordStore;
 
+namespace {
+
+const int kAndroidAppSchemeAndDelimeterLength = 10;  // Length of 'android://'.

[vabr (Chromium), 2016/03/08 13:09:28]

typo: delimeter -> delimiter

[kolos1, 2016/03/08 15:14:23]

Done.

+
+const char kSortKeyPartsSeparator = ' ';
+
+// Reverse order of subdomains in hostname.
+std::string SplitByDotAndReverse(const std::string& host) {

[vabr (Chromium), 2016/03/08 13:09:28]

To improve efficiency, consider passing a StringPiece instead of a const
std::string& -- this allows you to pass in substrings (like on line 89) cheaply,
without string copying.

[kolos1, 2016/03/08 15:14:24]

Yes, it would be nice to replace std::string with StringPiece, but there is no
JoinString for vector<StringPiece> (which is weird). So, we will have to convert
every StringPiece to std::string and then join them, i.e. there is no sense to
pass StringPiece, as I understand. 

[vabr (Chromium), 2016/03/08 16:04:44]

This is not about changing the SplitString/JoinString call, that stays as it is
(note that SplitString takes StringPiece arguments anyway). What happens now is:
at the callsite, a new string is created (by calling string::substr), then
SplitString reads it and creates the vector.
If you use StringPiece, instead of creating the substring as a new string,
SplitByDotAndReverse will get a "viewport" to the full string "covering" the
unnecessary parts. SplitString will then happily use that viewport to create a
vector if strings (not StringPieces) again.

[kolos1, 2016/03/09 09:56:39]

Done.

+  std::vector<std::string> parts =
+      base::SplitString(host, ".", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+  std::reverse(parts.begin(), parts.end());
+  return base::JoinString(parts, ".");
+}
+
+// Helper function that returns the type of the entry (non-Android credentials,
+// Android w/ affiliated web realm (i.e. clickable) or w/o web realm).
+std::string GetEntryTypeCode(bool is_android_uri, bool is_clickable) {
+  if (!is_android_uri)
+    return "0";
+  if (is_clickable)
+    return "1";
+  return "2";
+}
+
+// Creates key for sorting password or password exception entries.
+// The key is eTLD+1 followed by subdomains
+// (e.g. secure.accounts.example.com => example.com.accounts.secure).
+// If |username_and_password_in_key == true|, the key is appended with username

[vabr (Chromium), 2016/03/08 13:09:28]

nit: To avoid misunderstanding of what is appended to what, I suggest replacing
"the key ... password" with "username and hashed password is appended to the
key".

[kolos1, 2016/03/08 15:14:23]

Done.

+// and hashed password. The key is also appended with entry type code
+// (non-Android, Android w/ or w/o affiliated web realm).
+std::string CreateSortKey(const scoped_ptr<autofill::PasswordForm>& form,

[vabr (Chromium), 2016/03/08 13:09:28]

Please replace the const scoped_ptr with just a raw pointer.

See also:
http://dev.chromium.org/developers/coding-style#TOC-Object-ownership-and-call...
https://www.chromium.org/developers/smart-pointer-guidelines

[kolos1, 2016/03/08 15:14:23]

Since we don't modify |form|, I redeclared it as const ref.

Did the same in RemoveDuplicates.

[vabr (Chromium), 2016/03/08 16:04:44]

Acknowledged.

+                          const std::string& languages,
+                          bool username_and_password_in_key) {
+  bool is_android_uri = false;
+  bool is_clickable = false;
+  GURL link_url;
+  std::string origin = password_manager::GetShownOriginAndLinkUrl(
+      *form, languages, &is_android_uri, &link_url, &is_clickable);
+
+  if (!is_clickable)  // e.g. android://com.example.r => r.example.com.
+    origin = SplitByDotAndReverse(
+        origin.substr(kAndroidAppSchemeAndDelimeterLength));
+
+  std::string site_name =
+      net::registry_controlled_domains::GetDomainAndRegistry(
+          origin, net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
+  if (site_name.empty())  // e.g. localhost.
+    site_name = origin;
+  std::string key = site_name + SplitByDotAndReverse(origin.substr(
+                                    0, origin.length() - site_name.length()));
+
+  if (username_and_password_in_key)

[vabr (Chromium), 2016/03/08 13:09:28]

nit: Add { and }, this is not a one-liner.

[kolos1, 2016/03/08 15:14:23]

Done.

+    key = key + kSortKeyPartsSeparator +
+          base::UTF16ToUTF8(form->username_value) + kSortKeyPartsSeparator +
+          base::SHA1HashString(base::UTF16ToUTF8(form->password_value));

[vabr (Chromium), 2016/03/08 14:48:24]

Why do we actually hash the passwords? The keys not sent to the renderer, are
they?

[kolos1, 2016/03/08 15:14:23]

Lets somebody want to see one of passwords listed on password page, but he
doesn't know admin password. Lets he is able to add dummy password entries with
specified passwords. Then, based on sorting results of dummy entries and the
real entry, it is possible to know the real password. 

In few words, sorting shows passwords implicitly.

[vabr (Chromium), 2016/03/08 16:04:44]

Oh, that's a interesting good point, especially the use of sorting to guide the
search for passwords.

However, could we get the security team's opinion on this (jww@, mkwst@, or
anybody else)? My concern is that there are far easier ways to discover the
password (e.g., via DevTools) for somebody with so much time that they can add
lots of dummy passwords to the password store, so we might be investing this
effort in a vain.

If the security team recommends hashing, I'd still like to see a quick comment
explaining why is it there (basically what you wrote above).

[kolos1, 2016/03/09 09:56:39]

I discussed this question with security team. They said we could use unhashed
passwords here. It doesn't make any difference. Fixed to unhashed password. 

+
+  // Since Android and non-Android entries shouldn't be merged into one entry,
+  // add the entry type code to the sort key.
+  key +=
+      kSortKeyPartsSeparator + GetEntryTypeCode(is_android_uri, is_clickable);
+  return key;
+}
+
+// Finds duplicates of |form| in |duplicates|, removes them from |store| and
+// from |duplicates|.
+void RemoveDuplicates(const scoped_ptr<autofill::PasswordForm>& form,

[vabr (Chromium), 2016/03/08 13:09:28]

Also here: raw pointer instead of a scoped_ptr.

[kolos1, 2016/03/08 15:14:23]

Since we don't modify form, redeclared as const ref.

[vabr (Chromium), 2016/03/08 16:04:44]

Acknowledged.

+                      const std::string& languages,
+                      autofill::DuplicatesMap* duplicates,
+                      PasswordStore* store,
+                      bool username_and_password_in_key) {
+  std::string key =
+      CreateSortKey(form, languages, username_and_password_in_key);
+  std::pair<autofill::DuplicatesMap::iterator,
+            autofill::DuplicatesMap::iterator>
+      dups = duplicates->equal_range(key);
+  for (autofill::DuplicatesMap::iterator it = dups.first; it != dups.second;

[vabr (Chromium), 2016/03/08 13:09:28]

nit: Use range-based loop, it's shorter and easier to read:
for (const auto& duplicate : dups) {...}

[kolos1, 2016/03/08 15:14:23]

dups is a pair of iterators.

I didn't find a way to iterate over multimap entries with the specific key in
for_each loop.

[vabr (Chromium), 2016/03/08 16:04:44]

Ah right, I got confused by the STL naming. :) The current code is fine, sorry
for the noise.

+       ++it)
+    store->RemoveLogin(*it->second);
+  duplicates->erase(key);
+}
+
+}  // namespace
+
 PasswordManagerPresenter::PasswordManagerPresenter(
     PasswordUIView* password_view)
     : populater_(this),
@@ -98,7 +186,9 @@ void PasswordManagerPresenter::UpdatePasswordLists() {
 
   // Reset the current lists.
   password_list_.clear();
+  password_duplicates_.clear();
   password_exception_list_.clear();
+  password_exception_duplicates_.clear();
 
   populater_.Populate();
   exception_populater_.Populate();
@@ -114,6 +204,9 @@ void PasswordManagerPresenter::RemoveSavedPassword(size_t index) {
   PasswordStore* store = GetPasswordStore();
   if (!store)
     return;
+
+  RemoveDuplicates(password_list_[index], languages_, &password_duplicates_,
+                   store, true);
   store->RemoveLogin(*password_list_[index]);
   content::RecordAction(
       base::UserMetricsAction("PasswordManager_RemoveSavedPassword"));
@@ -129,6 +222,8 @@ void PasswordManagerPresenter::RemovePasswordException(size_t index) {
   PasswordStore* store = GetPasswordStore();
   if (!store)
     return;
+  RemoveDuplicates(password_exception_list_[index], languages_,
+                   &password_exception_duplicates_, store, false);
   store->RemoveLogin(*password_exception_list_[index]);
   content::RecordAction(
       base::UserMetricsAction("PasswordManager_RemovePasswordException"));
@@ -221,6 +316,38 @@ void PasswordManagerPresenter::SetPasswordExceptionList() {
   password_view_->SetPasswordExceptionList(password_exception_list_);
 }
 
+void PasswordManagerPresenter::SortEntriesAndHideDuplicates(
+    const std::string& languages,
+    std::vector<scoped_ptr<autofill::PasswordForm>>* list,
+    autofill::DuplicatesMap* duplicates_,
+    bool username_and_password_in_key) {
+  std::vector<std::pair<std::string, scoped_ptr<autofill::PasswordForm>>> pairs;
+  pairs.reserve(list->size());
+  for (auto& form : *list) {
+    pairs.push_back(std::make_pair(
+        CreateSortKey(form, languages, username_and_password_in_key),
+        std::move(form)));
+  }
+
+  std::sort(
+      pairs.begin(), pairs.end(),
+      [](const std::pair<std::string, scoped_ptr<autofill::PasswordForm>>& left,
+         const std::pair<std::string, scoped_ptr<autofill::PasswordForm>>&
+             right) { return left.first < right.first; });
+
+  list->clear();
+  duplicates_->clear();
+  std::string previous_key;
+  for (auto& pair : pairs) {
+    if (pair.first != previous_key) {
+      list->push_back(std::move(pair.second));
+      previous_key = pair.first;
+    } else {
+      duplicates_->insert(std::make_pair(previous_key, std::move(pair.second)));
+    }
+  }
+}
+
 PasswordManagerPresenter::ListPopulater::ListPopulater(
     PasswordManagerPresenter* page) : page_(page) {
 }
@@ -246,6 +373,9 @@ void PasswordManagerPresenter::PasswordListPopulater::OnGetPasswordStoreResults(
     ScopedVector<autofill::PasswordForm> results) {
   page_->password_list_ =
       password_manager_util::ConvertScopedVector(std::move(results));
+  page_->SortEntriesAndHideDuplicates(page_->languages_, &page_->password_list_,
+                                      &page_->password_duplicates_,
+                                      true /* use username and password */);
   page_->SetPasswordList();
 }
 
@@ -268,5 +398,9 @@ void PasswordManagerPresenter::PasswordExceptionListPopulater::
     OnGetPasswordStoreResults(ScopedVector<autofill::PasswordForm> results) {
   page_->password_exception_list_ =
       password_manager_util::ConvertScopedVector(std::move(results));
+  page_->SortEntriesAndHideDuplicates(
+      page_->languages_, &page_->password_exception_list_,
+      &page_->password_exception_duplicates_,
+      false /* don't use username and password*/);
   page_->SetPasswordExceptionList();
 }