[signin] OAuthMultilogin maintains current account ordering

OAuth multilogin now tries to keep the current account ordering as
much as possible.
It also handles the cases where there are more than 10 accounts in
Chrome (which happens in real life), and where there are more than
10 accounts in the cookie (should not happen in real life, but
should be handled gracefully).

Bug: 899472
Change-Id: I04b6ab195beb2691ce23f25e1a645b19b0e2c9d9
Reviewed-on: https://chromium-review.googlesource.com/c/1335598
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608723}

components/signin/core/browser/BUILD.gn

@@ -265,6 +265,7 @@ source_set("unit_tests") {
   sources = [
     "account_info_unittest.cc",
     "account_investigator_unittest.cc",
+    "account_reconcilor_delegate_unittest.cc",
     "account_reconcilor_unittest.cc",
     "account_tracker_service_unittest.cc",
     "avatar_icon_util_unittest.cc",

components/signin/core/browser/account_reconcilor_delegate.cc

@@ -46,8 +46,8 @@ AccountReconcilorDelegate::CalculateParametersForMultilogin(
   const MultiloginMode mode = CalculateModeForReconcile(
       gaia_accounts, primary_account, first_execution, primary_has_error);
   const std::vector<std::string> accounts_to_send =
-      ReorderChromeAccountsForReconcile(chrome_accounts, primary_account,
-                                        gaia_accounts, mode);
+      GetChromeAccountsForReconcile(chrome_accounts, primary_account,
+                                    gaia_accounts, mode);
   return {mode, accounts_to_send};
 }
 
@@ -61,6 +61,98 @@ MultiloginMode AccountReconcilorDelegate::CalculateModeForReconcile(
 
 std::vector<std::string>
 AccountReconcilorDelegate::ReorderChromeAccountsForReconcile(
+    const std::vector<std::string>& chrome_accounts,
+    const std::string& first_account,
+    const std::vector<gaia::ListedAccount>& gaia_accounts) const {
+  // Gaia only supports kMaxGaiaAccounts. Multilogin and MergeSession calls
+  // which go above this count will fail.
+  const int kMaxGaiaAccounts = 10;
+  DCHECK(!first_account.empty());
+  DCHECK(base::ContainsValue(chrome_accounts, first_account));
+
+  // Ordered list of accounts, this is the result of this function.
+  std::vector<std::string> ordered_accounts;
+  ordered_accounts.reserve(gaia_accounts.size());
+  // Set of accounts that must be added to ordered_accounts.
+  std::set<std::string> chrome_accounts_set(chrome_accounts.begin(),
+                                            chrome_accounts.end());
+
+  // Start from the gaia accounts.
+  for (const gaia::ListedAccount& account : gaia_accounts)
+    ordered_accounts.push_back(account.id);
+
+  // Keep only accounts that are in chrome_accounts_set.
+  for (std::string& account : ordered_accounts) {
+    if (chrome_accounts_set.find(account) == chrome_accounts_set.end())
+      account = std::string();
+    else
+      chrome_accounts_set.erase(account);
+  }
+
+  // At this point, ordered_accounts only contains accounts that must be kept,
+  // and chrome_accounts_set only contains account that are not yet in
+  // ordered_accounts.
+
+  // Put first_account in first position if needed, using swap to avoid changing
+  // the order of existing cookies.
+  auto first_account_it = std::find(ordered_accounts.begin(),
+                                    ordered_accounts.end(), first_account);
+  if (first_account_it == ordered_accounts.end()) {
+    // The first account was not already in the cookies, add it in the first
+    // empty spot, or at the end if there is no available spot.
+    first_account_it = ordered_accounts.insert(
+        std::find(ordered_accounts.begin(), ordered_accounts.end(),
+                  std::string()),
+        first_account);
+    chrome_accounts_set.erase(first_account);
+  }
+  std::iter_swap(ordered_accounts.begin(), first_account_it);
+
+  // Add the remaining chrome accounts.
+  // First in empty spots.
+  auto remaining_accounts_it = chrome_accounts_set.begin();
+  for (std::string& account : ordered_accounts) {
+    if (remaining_accounts_it == chrome_accounts_set.end())
+      break;
+    if (account.empty()) {
+      account = *remaining_accounts_it;
+      ++remaining_accounts_it;
+    }
+  }
+  // Then at the end.
+  while (remaining_accounts_it != chrome_accounts_set.end()) {
+    ordered_accounts.push_back(*remaining_accounts_it);
+    ++remaining_accounts_it;
+  }
+
+  // There may still be empty spots left. Compact the vector by moving accounts
+  // from the end of the vector into the empty spots.
+  auto compacting_it = ordered_accounts.begin();
+  while (compacting_it != ordered_accounts.end()) {
+    // Remove all the empty accounts at the end.
+    while (ordered_accounts.back().empty())
+      ordered_accounts.pop_back();
+    // Find next empty slot.
+    compacting_it =
+        std::find(compacting_it, ordered_accounts.end(), std::string());
+    // Swap it with the last element.
+    if (compacting_it != ordered_accounts.end())
+      std::swap(*compacting_it, ordered_accounts.back());
+  }
+
+  // All accounts have been added, and ordered_accounts now has the same
+  // accounts as chrome_accounts, but in a different order.
+  DCHECK_EQ(ordered_accounts.size(), chrome_accounts.size());
+
+  // Keep only kMaxGaiaAccounts.
+  if (ordered_accounts.size() > kMaxGaiaAccounts)
+    ordered_accounts.resize(kMaxGaiaAccounts);
+
+  return ordered_accounts;
+}
+
+std::vector<std::string>
+AccountReconcilorDelegate::GetChromeAccountsForReconcile(
     const std::vector<std::string>& chrome_accounts,
     const std::string& primary_account,
     const std::vector<gaia::ListedAccount>& gaia_accounts,

components/signin/core/browser/account_reconcilor_delegate.h

@@ -108,10 +108,26 @@ class AccountReconcilorDelegate {
   }
   AccountReconcilor* reconcilor() { return reconcilor_; }
 
+ protected:
+  // Computes a new ordering for chrome_accounts. |first_account| must be in
+  // |chrome_accounts|. The returned order has the following properties:
+  // - first_account will be first.
+  // - if a chrome account is also in gaia_accounts, the function tries to keep
+  //   it at the same index. The function mimimizes account re-numbering.
+  // - if there are too many accounts, some accounts will be discarded.
+  //   |first_account| and accounts already in cookies will be kept in priority.
+  //   Aplhabetical order is used to break ties.
+  // Note: the input order of the accounts in |chrome_accounts| does not matter
+  // (different orders yield to the same result).
+  std::vector<std::string> ReorderChromeAccountsForReconcile(
+      const std::vector<std::string>& chrome_accounts,
+      const std::string& first_account,
+      const std::vector<gaia::ListedAccount>& gaia_accounts) const;
+
  private:
   // Reorders chrome accounts in the order they should appear in cookies with
   // respect to existing cookies.
-  virtual std::vector<std::string> ReorderChromeAccountsForReconcile(
+  virtual std::vector<std::string> GetChromeAccountsForReconcile(
       const std::vector<std::string>& chrome_accounts,
       const std::string& primary_account,
       const std::vector<gaia::ListedAccount>& gaia_accounts,

components/signin/core/browser/account_reconcilor_delegate_unittest.cc

+// Copyright 2018 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/signin/core/browser/account_reconcilor_delegate.h"
+
+#include <ostream>
+#include <string>
+#include <vector>
+
+#include "google_apis/gaia/gaia_auth_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace signin {
+
+struct AccountReconcilorDelegateTestParam {
+  const char* chrome_accounts;
+  const char* gaia_accounts;
+  char first_account;
+  const char* expected_order;
+};
+
+// clang-format off
+static const AccountReconcilorDelegateTestParam kReorderParams[] = {
+// | Tokens          | Cookies       | First Acc. | Expected cookies |
+// |------------ Basic cases ----------------------------------------|
+   // Nothing to do.
+   { "A",              "A",            'A',         "A"              },
+   { "ABCD",           "ABCD",         'A',         "ABCD"           },
+   // Token ordering does not matter.
+   { "DBCA",           "ABCD",         'A',         "ABCD"           },
+   // Simple reordering of cookies.
+   { "AB",             "BA",           'A',         "AB"             },
+// |------------ Extra accounts in cookie ---------------------------|
+   // Extra secondary account.
+   { "A",              "AB",           'A',         "A"              },
+   // Extra primary account.
+   { "A",              "BA",           'A',         "A"              },
+   // Multiple extra accounts.
+   { "AE",             "ABCDEF",       'A',         "AE"             },
+   { "AE",             "GABCDEF",      'A',         "AE"             },
+   // C is kept in place.
+   { "ACF",            "ABCDEF",       'A',         "AFC"            },
+// |------------ Missing accounts in cookie -------------------------|
+   // Cookie was lost.
+   { "A",              "",             'A',         "A"              },
+   { "ABCD",           "",             'A',         "ABCD"           },
+   // ACEG kept in place.
+   { "ABCDEFGH",       "ACEG",         'A',         "ACEGBDFH"       },
+   // C kept in place, but not B.
+   { "ABCD",           "BC",           'A',         "ACBD"           },
+   // D not kept in place.
+   { "AD",             "ABCD",         'A',         "AD"             },
+// |------------ Both extra accounts and missing accounts -----------|
+   // Simple account mismatch.
+   { "A",              "B",            'A',         "A"              },
+   // ADE kept in place, BG removed.
+   { "ADEH",           "ABDEG",        'A',         "AHDE"           },
+   // E kept in place, BG removed, AD swapped.
+   { "ADEH",           "ABDEG",        'D',         "DHAE"           },
+   // Missing first account.
+   { "ADE",            "BCDE",         'A',         "AED"            },
+   // Three-ways swap A-B-D.
+   { "ABCE",           "BCDE",         'A',         "ACBE"           },
+   // Extreme example.
+   { "ACJKL",          "ABCDEFGHIJ",   'A',         "AKCLJ"          },
+// |------------ More than 10 accounts in chrome --------------------|
+   // Trim extra accounts.
+   { "ABCDEFGHIJKLM",  "ABCDEFGHIJ",   'A',         "ABCDEFGHIJ"     },
+   // D missing.
+   { "ABCEFGHIJKLMN",  "ABCDEFGHIJ",   'A',         "ABCKEFGHIJ"     },
+   // DG missing.
+   { "ABCEFHIJKLMOP",  "ABCDEFGHIJ",   'A',         "ABCKEFLHIJ"     },
+   // Primary swapped in.
+   { "ABCDEFGHIJKLM",  "ABCDEFGHIJ",   'K',         "KBCDEFGHIJ"     },
+// |------------ More than 10 accounts in cookie --------------------|
+   // Trim extra account.
+   { "ABCDEFGHIJK",    "ABCDEFGHIJK",  'A',         "ABCDEFGHIJ"     },
+   // Other edge cases.
+   { "BE",             "ABCDEFGHIJK",  'B',         "BE"             },
+   { "AE",             "ABCDEFGHIJK",  'A',         "AE"             },
+   { "AK",             "ABCDEFGHIJK",  'A',         "AK"             },
+   { "K",              "ABCDEFGHIJK",  'K',         "K"              },
+};
+// clang-format on
+
+// Pretty prints a AccountReconcilorDelegateTestParam. Used by gtest.
+static void PrintTo(const AccountReconcilorDelegateTestParam& param,
+                    ::std::ostream* os) {
+  *os << "gaia_accounts: \"" << param.gaia_accounts << "\". chrome_accounts: \""
+      << param.chrome_accounts << "\". first_account: \"" << param.first_account
+      << "\".";
+}
+
+class AccountReconcilorDelegateTest
+    : public AccountReconcilorDelegate,
+      public ::testing::TestWithParam<AccountReconcilorDelegateTestParam> {
+ public:
+  AccountReconcilorDelegateTest() {}
+  ~AccountReconcilorDelegateTest() override {}
+
+  // Parses a cookie string and converts it into ListedAccounts.
+  std::vector<gaia::ListedAccount> GaiaAccountsFromString(
+      const std::string& account_string) {
+    std::vector<gaia::ListedAccount> gaia_accounts;
+    for (const char& c : account_string) {
+      gaia::ListedAccount account;
+      account.id = std::string(1, c);
+      gaia_accounts.push_back(account);
+    }
+    return gaia_accounts;
+  }
+};
+
+TEST_P(AccountReconcilorDelegateTest, ReorderChromeAccountsForReconcile) {
+  // Decode test parameters.
+  std::string first_account = std::string(1, GetParam().first_account);
+  std::vector<std::string> chrome_accounts;
+  for (int i = 0; GetParam().chrome_accounts[i] != '\0'; ++i)
+    chrome_accounts.push_back(std::string(1, GetParam().chrome_accounts[i]));
+  ASSERT_TRUE(base::ContainsValue(chrome_accounts, first_account))
+      << "Invalid test parameter.";
+  std::vector<gaia::ListedAccount> gaia_accounts =
+      GaiaAccountsFromString(GetParam().gaia_accounts);
+
+  // Reorder the accounts.
+  std::vector<std::string> order = ReorderChromeAccountsForReconcile(
+      chrome_accounts, first_account, gaia_accounts);
+
+  // Check results.
+  std::string order_as_string;
+  for (const std::string& account : order) {
+    ASSERT_EQ(1u, account.size());
+    order_as_string += account;
+  }
+  EXPECT_EQ(GetParam().expected_order, order_as_string);
+
+  // Check that the result is idempotent (re-ordering again is a no-op).
+  EXPECT_EQ(order, ReorderChromeAccountsForReconcile(
+                       chrome_accounts, first_account,
+                       GaiaAccountsFromString(order_as_string)));
+}
+
+INSTANTIATE_TEST_CASE_P(,
+                        AccountReconcilorDelegateTest,
+                        ::testing::ValuesIn(kReorderParams));
+
+}  // namespace signin

components/signin/core/browser/account_reconcilor_unittest.cc

@@ -853,8 +853,8 @@ const std::vector<AccountReconcilorTestTableParam> kDiceParams = {
     {  "*A",   "AB",   IsFirstReconcile::kBoth,       "XA", "*A",     "A",    "PA", "*A",   "AxB"},
     // Check that Gaia default account is kept in first position.
     {  "AB",   "BC",   IsFirstReconcile::kBoth,       "XBA","AB",     "BA",   "PAB","AB",   "BxCA"},
-    // Check that Gaia cookie order preserved except for the first one.
-    {  "*ABC", "CB",   IsFirstReconcile::kFirst,      "XABC","*ABC",  "ABC",  "UACB","*ABC","ACB"},
+    // Check that Gaia cookie order is preserved for B.
+    {  "*ABC", "CB",   IsFirstReconcile::kFirst,      "XABC","*ABC",  "ABC",  "UABC","*ABC","ABC"},
     // Required for idempotency check.
     {  "",     "",     IsFirstReconcile::kNotFirst,   "",   "",       "",     "",   "",     ""},
     {  "",     "xA",   IsFirstReconcile::kNotFirst,   "",   "",       "xA",   "",   "",     "xA"},

components/signin/core/browser/dice_account_reconcilor_delegate.cc

@@ -13,42 +13,6 @@
 #include "components/signin/core/browser/signin_client.h"
 #include "components/signin/core/browser/signin_pref_names.h"
 
-namespace {
-// Outputs accounts in the following order: first account, gaia accounts that
-// are present in chrome_accounts, rest of chrome_accounts.
-std::vector<std::string> ReorderChromeAccountsInternal(
-    const std::vector<std::string>& chrome_accounts,
-    const std::string& first_account,
-    const std::vector<gaia::ListedAccount>& gaia_accounts) {
-  // Reordering should only happen if there is first account with a valid token.
-  DCHECK(!first_account.empty());
-
-  std::set<std::string> chrome_accounts_set(chrome_accounts.begin(),
-                                            chrome_accounts.end());
-  DCHECK(base::ContainsKey(chrome_accounts_set, first_account));
-
-  std::vector<std::string> accounts_to_send;
-  accounts_to_send.reserve(chrome_accounts.size());
-
-  accounts_to_send.push_back(first_account);
-
-  chrome_accounts_set.erase(first_account);
-  for (const gaia::ListedAccount& gaia_account : gaia_accounts) {
-    if (gaia_account.id == first_account ||
-        chrome_accounts_set.find(gaia_account.id) == chrome_accounts_set.end())
-      continue;
-    accounts_to_send.push_back(gaia_account.id);
-    chrome_accounts_set.erase(gaia_account.id);
-  }
-
-  for (const std::string& chrome_account : chrome_accounts_set) {
-    accounts_to_send.push_back(chrome_account);
-  }
-  DCHECK(!accounts_to_send.empty());
-  return accounts_to_send;
-}
-}  // namespace
-
 namespace signin {
 
 DiceAccountReconcilorDelegate::DiceAccountReconcilorDelegate(
@@ -172,21 +136,21 @@ MultiloginMode DiceAccountReconcilorDelegate::CalculateModeForReconcile(
 }
 
 std::vector<std::string>
-DiceAccountReconcilorDelegate::ReorderChromeAccountsForReconcile(
+DiceAccountReconcilorDelegate::GetChromeAccountsForReconcile(
     const std::vector<std::string>& chrome_accounts,
     const std::string& primary_account,
     const std::vector<gaia::ListedAccount>& gaia_accounts,
     const signin::MultiloginMode mode) const {
   if (mode == signin::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER) {
-    return ReorderChromeAccountsInternal(chrome_accounts, primary_account,
-                                         gaia_accounts);
+    return ReorderChromeAccountsForReconcile(chrome_accounts, primary_account,
+                                             gaia_accounts);
   }
   if (gaia_accounts.empty() &&
       base::ContainsValue(chrome_accounts, last_known_first_account_)) {
     // In PRESERVE mode in case accounts in cookies are accidentally lost we
     // should put cached first account first since Gaia has no information about
     // it.
-    return ReorderChromeAccountsInternal(
+    return ReorderChromeAccountsForReconcile(
         chrome_accounts, last_known_first_account_, gaia_accounts);
   }
   return chrome_accounts;

components/signin/core/browser/dice_account_reconcilor_delegate.h

@@ -39,7 +39,7 @@ class DiceAccountReconcilorDelegate : public AccountReconcilorDelegate {
   bool ShouldRevokeTokensOnCookieDeleted() override;
 
  private:
-  std::vector<std::string> ReorderChromeAccountsForReconcile(
+  std::vector<std::string> GetChromeAccountsForReconcile(
       const std::vector<std::string>& chrome_accounts,
       const std::string& primary_account,
       const std::vector<gaia::ListedAccount>& gaia_accounts,

components/signin/core/browser/mirror_account_reconcilor_delegate.cc

@@ -50,27 +50,16 @@ std::string MirrorAccountReconcilorDelegate::GetFirstGaiaAccountForReconcile(
 }
 
 std::vector<std::string>
-MirrorAccountReconcilorDelegate::ReorderChromeAccountsForReconcile(
+MirrorAccountReconcilorDelegate::GetChromeAccountsForReconcile(
     const std::vector<std::string>& chrome_accounts,
     const std::string& primary_account,
     const std::vector<gaia::ListedAccount>& gaia_accounts,
     const signin::MultiloginMode mode) const {
   DCHECK(mode ==
          signin::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER);
-  std::vector<std::string> accounts_to_send;
-  accounts_to_send.reserve(chrome_accounts.size());
-
-  DCHECK(!primary_account.empty());
-  accounts_to_send.push_back(primary_account);
-
-  for (const std::string& chrome_account : chrome_accounts) {
-    if (chrome_account == primary_account)
-      continue;
-    accounts_to_send.push_back(chrome_account);
+  return ReorderChromeAccountsForReconcile(chrome_accounts, primary_account,
+                                           gaia_accounts);
   }
-  DCHECK(!accounts_to_send.empty());
-  return accounts_to_send;
-}
 
 void MirrorAccountReconcilorDelegate::OnPrimaryAccountSet(
     const AccountInfo& primary_account_info) {

components/signin/core/browser/mirror_account_reconcilor_delegate.h

@@ -31,8 +31,7 @@ class MirrorAccountReconcilorDelegate
       const std::string& primary_account,
       bool first_execution,
       bool will_logout) const override;
-
-  std::vector<std::string> ReorderChromeAccountsForReconcile(
+  std::vector<std::string> GetChromeAccountsForReconcile(
       const std::vector<std::string>& chrome_accounts,
       const std::string& primary_account,
       const std::vector<gaia::ListedAccount>& gaia_accounts,