Created new struct CoreAccountId to be used in CoreAccountInfo

The goal is to introduce a proper type for CoreAccountInfo's
account_id to prevent incorrect uses. Before this CL it was a
std::string and sometimes was used as an email or a gaia id
which was incorrect but sometimes working.

The CL makes the struct CoreAccountId implicitly convertible
to/from std::string as using a proper type would make the
size of the CL intractible. Multiple followup CLs will fix
the code and eventually the conversion will be explicit.

Bug: 959158
Change-Id: Ia8295b71fa212118115db5f53b5fe845c7992a83
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1596749Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Tanmoy Mollik <triploblastic@google.com>
Cr-Commit-Position: refs/heads/master@{#659001}

chrome/browser/extensions/updater/extension_updater_unittest.cc

@@ -270,7 +270,7 @@ class MockService : public TestExtensionService {
     return identity_test_env_.get();
   }
 
-  const std::string& account_id() { return account_info_.account_id; }
+  const std::string& account_id() { return account_info_.account_id.id; }
 
   // Creates test extensions and inserts them into list. The name and
   // version are all based on their index. If |update_url| is non-null, it

components/gcm_driver/account_tracker.cc

@@ -69,7 +69,7 @@ std::vector<AccountIds> AccountTracker::GetAccounts() const {
 void AccountTracker::OnRefreshTokenUpdatedForAccount(
     const CoreAccountInfo& account_info) {
   TRACE_EVENT1("identity", "AccountTracker::OnRefreshTokenUpdatedForAccount",
-               "account_id", account_info.account_id);
+               "account_id", account_info.account_id.id);
 
   // Ignore refresh tokens if there is no active account ID at all.
   if (!identity_manager_->HasPrimaryAccount())

components/signin/core/browser/about_signin_internals.cc

@@ -690,7 +690,7 @@ AboutSigninInternals::SigninStatus::ToValue(
   } else {
     for (const CoreAccountInfo& account_info : accounts_with_refresh_tokens) {
       auto entry = std::make_unique<base::DictionaryValue>();
-      entry->SetString("accountId", account_info.account_id);
+      entry->SetString("accountId", account_info.account_id.id);
       // TODO(https://crbug.com/919793): Remove this field once the token
       // service is internally consistent on all platforms.
       entry->SetBoolean("hasRefreshToken",

components/signin/core/browser/account_info.h

@@ -8,6 +8,7 @@
 #include <string>
 
 #include "components/account_id/account_id.h"
+#include "google_apis/gaia/core_account_id.h"
 #include "ui/gfx/image/image.h"
 
 // Value representing no hosted domain associated with an account.
@@ -29,7 +30,7 @@ struct CoreAccountInfo {
   CoreAccountInfo& operator=(const CoreAccountInfo& other);
   CoreAccountInfo& operator=(CoreAccountInfo&& other) noexcept;
 
-  std::string account_id;
+  CoreAccountId account_id;
   std::string gaia;
   std::string email;
 

components/signin/core/browser/account_tracker_service.cc

@@ -572,12 +572,12 @@ void AccountTrackerService::SaveToPrefs(const AccountInfo& account_info) {
     return;
 
   base::DictionaryValue* dict = nullptr;
-  base::string16 account_id_16 = base::UTF8ToUTF16(account_info.account_id);
   ListPrefUpdate update(pref_service_, prefs::kAccountInfo);
   for (size_t i = 0; i < update->GetSize(); ++i, dict = nullptr) {
     if (update->GetDictionary(i, &dict)) {
-      base::string16 value;
-      if (dict->GetString(kAccountKeyPath, &value) && value == account_id_16)
+      std::string value;
+      if (dict->GetString(kAccountKeyPath, &value) &&
+          value == account_info.account_id.id)
         break;
     }
   }
@@ -587,7 +587,7 @@ void AccountTrackerService::SaveToPrefs(const AccountInfo& account_info) {
     update->Append(base::WrapUnique(dict));
     // |dict| is invalidated at this point, so it needs to be reset.
     update->GetDictionary(update->GetSize() - 1, &dict);
-    dict->SetString(kAccountKeyPath, account_id_16);
+    dict->SetString(kAccountKeyPath, account_info.account_id.id);
   }
 
   dict->SetString(kAccountEmailPath, account_info.email);
@@ -607,13 +607,13 @@ void AccountTrackerService::RemoveFromPrefs(const AccountInfo& account_info) {
   if (!pref_service_)
     return;
 
-  base::string16 account_id_16 = base::UTF8ToUTF16(account_info.account_id);
   ListPrefUpdate update(pref_service_, prefs::kAccountInfo);
   for (size_t i = 0; i < update->GetSize(); ++i) {
     base::DictionaryValue* dict = nullptr;
     if (update->GetDictionary(i, &dict)) {
-      base::string16 value;
-      if (dict->GetString(kAccountKeyPath, &value) && value == account_id_16) {
+      std::string value;
+      if (dict->GetString(kAccountKeyPath, &value) &&
+          value == account_info.account_id.id) {
         update->Remove(i, nullptr);
         break;
       }

components/signin/core/browser/mutable_profile_oauth2_token_service_delegate_unittest.cc

@@ -541,9 +541,9 @@ TEST_F(MutableProfileOAuth2TokenServiceDelegateTest,
   account_tracker_service_.SeedAccountInfo(secondary_account);
 
   ResetObserverCounts();
-  AddAuthTokenManually("AccountId-" + primary_account.account_id,
+  AddAuthTokenManually("AccountId-" + primary_account.account_id.id,
                        "refresh_token");
-  AddAuthTokenManually("AccountId-" + secondary_account.account_id,
+  AddAuthTokenManually("AccountId-" + secondary_account.account_id.id,
                        "refresh_token");
   oauth2_service_delegate_->LoadCredentials(primary_account.account_id);
   base::RunLoop().RunUntilIdle();
@@ -582,9 +582,9 @@ TEST_F(MutableProfileOAuth2TokenServiceDelegateTest,
   account_tracker_service_.SeedAccountInfo(secondary_account);
 
   ResetObserverCounts();
-  AddAuthTokenManually("AccountId-" + primary_account.account_id,
+  AddAuthTokenManually("AccountId-" + primary_account.account_id.id,
                        "refresh_token");
-  AddAuthTokenManually("AccountId-" + secondary_account.account_id,
+  AddAuthTokenManually("AccountId-" + secondary_account.account_id.id,
                        "refresh_token");
   oauth2_service_delegate_->LoadCredentials(primary_account.account_id);
   base::RunLoop().RunUntilIdle();
@@ -623,9 +623,9 @@ TEST_F(MutableProfileOAuth2TokenServiceDelegateTest,
   account_tracker_service_.SeedAccountInfo(secondary_account);
 
   ResetObserverCounts();
-  AddAuthTokenManually("AccountId-" + primary_account.account_id,
+  AddAuthTokenManually("AccountId-" + primary_account.account_id.id,
                        "refresh_token");
-  AddAuthTokenManually("AccountId-" + secondary_account.account_id,
+  AddAuthTokenManually("AccountId-" + secondary_account.account_id.id,
                        "refresh_token");
   oauth2_service_delegate_->LoadCredentials(primary_account.account_id);
   base::RunLoop().RunUntilIdle();
@@ -659,7 +659,7 @@ TEST_F(MutableProfileOAuth2TokenServiceDelegateTest,
   account_tracker_service_.SeedAccountInfo(primary_account);
 
   ResetObserverCounts();
-  AddAuthTokenManually("AccountId-" + primary_account.account_id,
+  AddAuthTokenManually("AccountId-" + primary_account.account_id.id,
                        "refresh_token");
   oauth2_service_delegate_->LoadCredentials(primary_account.account_id);
   base::RunLoop().RunUntilIdle();

google_apis/BUILD.gn

@@ -84,6 +84,8 @@ config("key_defines") {
 template("google_apis_tmpl") {
   source_set(target_name) {
     sources = [
+      "gaia/core_account_id.cc",
+      "gaia/core_account_id.h",
       "gaia/gaia_auth_consumer.cc",
       "gaia/gaia_auth_consumer.h",
       "gaia/gaia_auth_fetcher.cc",

google_apis/gaia/core_account_id.cc

+// Copyright 2019 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 "google_apis/gaia/core_account_id.h"
+
+CoreAccountId::CoreAccountId() = default;
+
+CoreAccountId::~CoreAccountId() = default;
+
+CoreAccountId::CoreAccountId(const char* id) : id(id) {}
+
+CoreAccountId::CoreAccountId(std::string&& id) : id(std::move(id)) {}
+
+CoreAccountId::CoreAccountId(const std::string& id) : id(id) {}
+
+CoreAccountId::operator std::string() const {
+  return id;
+}
+
+bool CoreAccountId::empty() const {
+  return id.empty();
+}
+
+bool operator<(const CoreAccountId& lhs, const CoreAccountId& rhs) {
+  return lhs.id < rhs.id;
+}
+
+bool operator==(const CoreAccountId& lhs, const CoreAccountId& rhs) {
+  return lhs.id == rhs.id;
+}
+
+bool operator!=(const CoreAccountId& lhs, const CoreAccountId& rhs) {
+  return lhs.id != rhs.id;
+}
+
+std::ostream& operator<<(std::ostream& out, const CoreAccountId& a) {
+  return out << a.id;
+}

google_apis/gaia/core_account_id.h

+// Copyright 2019 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 GOOGLE_APIS_GAIA_CORE_ACCOUNT_ID_H_
+#define GOOGLE_APIS_GAIA_CORE_ACCOUNT_ID_H_
+
+#include <ostream>
+#include <string>
+
+// Represent the id of an account for interaction with GAIA. It is
+// currently implicitly convertible to and from std::string to allow
+// progressive migration of the code (see https://crbug.com/959157
+// for design and tracking).
+struct CoreAccountId {
+  CoreAccountId();
+  ~CoreAccountId();
+
+  // Those implicit constructor and conversion operator allow to
+  // progressively migrate the code to use this struct. Removing
+  // them is tracked by https://crbug.com/959161
+  CoreAccountId(const char* id);
+  CoreAccountId(std::string&& id);
+  CoreAccountId(const std::string& id);
+  operator std::string() const;
+
+  // Checks if the account is valid or not.
+  // TODO(triploblastic): Possibly rename of remove this after
+  // refactoring.
+  bool empty() const;
+
+  std::string id;
+};
+
+bool operator<(const CoreAccountId& lhs, const CoreAccountId& rhs);
+
+bool operator==(const CoreAccountId& lhs, const CoreAccountId& rhs);
+
+bool operator!=(const CoreAccountId& lhs, const CoreAccountId& rhs);
+
+std::ostream& operator<<(std::ostream& out, const CoreAccountId& a);
+
+#endif  // GOOGLE_APIS_GAIA_CORE_ACCOUNT_ID_H_

ios/chrome/browser/signin/authentication_service.mm

@@ -274,7 +274,7 @@ void AuthenticationService::StoreAccountsInPrefs() {
       identity_manager_->GetAccountsWithRefreshTokens());
   std::vector<base::Value> accounts_pref_value;
   for (const CoreAccountInfo& account_info : accounts)
-    accounts_pref_value.emplace_back(account_info.account_id);
+    accounts_pref_value.emplace_back(account_info.account_id.id);
   pref_service_->Set(prefs::kSigninLastAccounts,
                      base::Value(std::move(accounts_pref_value)));
 }

services/identity/public/cpp/core_account_info_mojom_traits.h

@@ -16,7 +16,7 @@ template <>
 struct StructTraits<identity::mojom::CoreAccountInfo::DataView,
                     ::CoreAccountInfo> {
   static const std::string& account_id(const ::CoreAccountInfo& r) {
-    return r.account_id;
+    return r.account_id.id;
   }
 
   static const std::string& gaia(const ::CoreAccountInfo& r) { return r.gaia; }

services/identity/public/cpp/primary_account_mutator_unittest.cc

@@ -187,9 +187,9 @@ void RunClearPrimaryAccountTest(
       EXPECT_TRUE(identity_manager->HasAccountWithRefreshToken(
           secondary_account_info.account_id));
       EXPECT_TRUE(base::ContainsKey(observed_removals,
-                                    former_primary_account.account_id));
+                                    former_primary_account.account_id.id));
       EXPECT_FALSE(base::ContainsKey(observed_removals,
-                                     secondary_account_info.account_id));
+                                     secondary_account_info.account_id.id));
       break;
     case RemoveAccountExpectation::kRemoveAll:
       EXPECT_FALSE(identity_manager->HasAccountWithRefreshToken(
@@ -197,9 +197,9 @@ void RunClearPrimaryAccountTest(
       EXPECT_FALSE(identity_manager->HasAccountWithRefreshToken(
           secondary_account_info.account_id));
       EXPECT_TRUE(base::ContainsKey(observed_removals,
-                                    former_primary_account.account_id));
+                                    former_primary_account.account_id.id));
       EXPECT_TRUE(base::ContainsKey(observed_removals,
-                                    secondary_account_info.account_id));
+                                    secondary_account_info.account_id.id));
       break;
   }
 }