diff options
author | Allan Sandfeld Jensen <allan.jensen@qt.io> | 2022-09-29 16:16:15 +0200 |
---|---|---|
committer | Allan Sandfeld Jensen <allan.jensen@qt.io> | 2022-11-09 10:04:06 +0000 |
commit | a95a7417ad456115a1ef2da4bb8320531c0821f1 (patch) | |
tree | edcd59279e486d2fd4a8f88a7ed025bcf925c6e6 /chromium/components/signin | |
parent | 33fc33aa94d4add0878ec30dc818e34e1dd3cc2a (diff) | |
download | qtwebengine-chromium-a95a7417ad456115a1ef2da4bb8320531c0821f1.tar.gz |
BASELINE: Update Chromium to 106.0.5249.126
Change-Id: Ib0bb21c437a7d1686e21c33f2d329f2ac425b7ab
Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/438936
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
Diffstat (limited to 'chromium/components/signin')
62 files changed, 1668 insertions, 2392 deletions
diff --git a/chromium/components/signin/DEPS b/chromium/components/signin/DEPS index 6a3669d4d81..8da8c60110e 100644 --- a/chromium/components/signin/DEPS +++ b/chromium/components/signin/DEPS @@ -1,5 +1,5 @@ include_rules = [ - "+ash/components/account_manager", + "+chromeos/ash/components/account_manager", "+components/account_manager_core", "+components/content_settings", "+components/google/core", diff --git a/chromium/components/signin/core/browser/BUILD.gn b/chromium/components/signin/core/browser/BUILD.gn index df0893c6b90..0624c7c16f0 100644 --- a/chromium/components/signin/core/browser/BUILD.gn +++ b/chromium/components/signin/core/browser/BUILD.gn @@ -40,8 +40,6 @@ static_library("browser") { "signin_header_helper.h", "signin_internals_util.cc", "signin_internals_util.h", - "signin_investigator.cc", - "signin_investigator.h", "signin_status_metrics_provider_helpers.cc", "signin_status_metrics_provider_helpers.h", ] @@ -80,7 +78,7 @@ static_library("browser") { "active_directory_account_reconcilor_delegate.h", ] deps += [ - "//ash/components/tpm", + "//chromeos/ash/components/install_attributes", "//chromeos/crosapi/mojom", ] } @@ -116,7 +114,6 @@ source_set("unit_tests") { "account_reconcilor_unittest.cc", "signin_error_controller_unittest.cc", "signin_header_helper_unittest.cc", - "signin_investigator_unittest.cc", "signin_status_metrics_provider_helpers_unittest.cc", ] @@ -144,7 +141,7 @@ source_set("unit_tests") { if (is_chromeos_ash) { deps += [ - "//ash/components/tpm:test_support", + "//chromeos/ash/components/install_attributes:test_support", "//chromeos/crosapi/mojom", ] sources -= [ @@ -173,10 +170,7 @@ if (is_android) { } java_cpp_enum("signin_enums_javagen") { - sources = [ - "signin_header_helper.h", - "signin_investigator.h", - ] + sources = [ "signin_header_helper.h" ] visibility = [ ":*" ] # Depend on through :signin_enums_java } } diff --git a/chromium/components/signin/core/browser/DEPS b/chromium/components/signin/core/browser/DEPS index b166d8a8fea..2d38d7dcbbc 100644 --- a/chromium/components/signin/core/browser/DEPS +++ b/chromium/components/signin/core/browser/DEPS @@ -6,10 +6,10 @@ include_rules = [ specific_include_rules = { "active_directory_account_reconcilor_delegate.cc": [ - "+ash/components/tpm/install_attributes.h" + "+chromeos/ash/components/install_attributes/install_attributes.h" ], "account_reconcilor_unittest.cc": [ - "+ash/components/tpm/stub_install_attributes.h" + "+chromeos/ash/components/install_attributes/stub_install_attributes.h" ], # TODO(crbug.com/1198528): remove Lacros deps after the rollout. "chrome_connected_header_helper.cc": [ diff --git a/chromium/components/signin/core/browser/about_signin_internals.cc b/chromium/components/signin/core/browser/about_signin_internals.cc index 7b14cdcc988..4b04265f310 100644 --- a/chromium/components/signin/core/browser/about_signin_internals.cc +++ b/chromium/components/signin/core/browser/about_signin_internals.cc @@ -64,35 +64,35 @@ std::string GetGaiaCookiesStateAsString(const GaiaCookiesState state) { } } -void AddSection(base::Value::ListStorage& parent_list, - base::Value section_content, +void AddSection(base::Value::List& parent_list, + base::Value::List section_content, const std::string& title) { - base::Value section(base::Value::Type::DICTIONARY); - section.SetStringKey("title", title); - section.SetKey("data", std::move(section_content)); - parent_list.push_back(std::move(section)); + base::Value::Dict section; + section.Set("title", title); + section.Set("data", std::move(section_content)); + parent_list.Append(std::move(section)); } -void AddSectionEntry(base::Value::ListStorage& section_list, +void AddSectionEntry(base::Value::List& section_list, const std::string& field_name, const std::string& field_status, const std::string& field_time = "") { - base::Value entry(base::Value::Type::DICTIONARY); - entry.SetStringKey("label", field_name); - entry.SetStringKey("status", field_status); - entry.SetStringKey("time", field_time); - section_list.push_back(std::move(entry)); + base::Value::Dict entry; + entry.Set("label", field_name); + entry.Set("status", field_status); + entry.Set("time", field_time); + section_list.Append(std::move(entry)); } -void AddCookieEntry(base::Value::ListStorage& accounts_list, +void AddCookieEntry(base::Value::List& accounts_list, const std::string& field_email, const std::string& field_gaia_id, const std::string& field_valid) { - base::Value entry(base::Value::Type::DICTIONARY); - entry.SetStringKey("email", field_email); - entry.SetStringKey("gaia_id", field_gaia_id); - entry.SetStringKey("valid", field_valid); - accounts_list.push_back(std::move(entry)); + base::Value::Dict entry; + entry.Set("email", field_email); + entry.Set("gaia_id", field_gaia_id); + entry.Set("valid", field_valid); + accounts_list.Append(std::move(entry)); } std::string SigninStatusFieldToLabel( @@ -353,15 +353,15 @@ void AboutSigninInternals::NotifyObservers() { if (signin_observers_.empty()) return; - base::Value signin_status_value = signin_status_.ToValue( + base::Value::Dict signin_status_value = signin_status_.ToValue( identity_manager_, signin_error_controller_, client_, account_consistency_, account_reconcilor_); for (auto& observer : signin_observers_) - observer.OnSigninStateChanged(&signin_status_value); + observer.OnSigninStateChanged(signin_status_value); } -base::Value AboutSigninInternals::GetSigninStatus() { +base::Value::Dict AboutSigninInternals::GetSigninStatus() { return signin_status_.ToValue(identity_manager_, signin_error_controller_, client_, account_consistency_, account_reconcilor_); @@ -482,7 +482,7 @@ void AboutSigninInternals::OnAccountsInCookieUpdated( if (error.state() != GoogleServiceAuthError::NONE) return; - base::Value::ListStorage cookie_info; + base::Value::List cookie_info; for (const auto& signed_in_account : accounts_in_cookie_jar_info.signed_in_accounts) { AddCookieEntry(cookie_info, signed_in_account.raw_email, @@ -495,11 +495,11 @@ void AboutSigninInternals::OnAccountsInCookieUpdated( std::string()); } - base::Value cookie_status(base::Value::Type::DICTIONARY); - cookie_status.SetKey("cookie_info", base::Value(std::move(cookie_info))); + base::Value::Dict cookie_status_dict; + cookie_status_dict.Set("cookie_info", std::move(cookie_info)); // Update the observers that the cookie's accounts are updated. for (auto& observer : signin_observers_) - observer.OnCookieAccountsFetched(&cookie_status); + observer.OnCookieAccountsFetched(cookie_status_dict); } AboutSigninInternals::TokenInfo::TokenInfo(const std::string& consumer_id, @@ -521,19 +521,19 @@ bool AboutSigninInternals::TokenInfo::LessThan( void AboutSigninInternals::TokenInfo::Invalidate() { removed_ = true; } -base::Value AboutSigninInternals::TokenInfo::ToValue() const { - base::Value token_info(base::Value::Type::DICTIONARY); - token_info.SetStringKey("service", consumer_id); +base::Value::Dict AboutSigninInternals::TokenInfo::ToValue() const { + base::Value::Dict token_info; + token_info.Set("service", consumer_id); std::string scopes_str; for (auto it = scopes.begin(); it != scopes.end(); ++it) { scopes_str += *it + "\n"; } - token_info.SetStringKey("scopes", scopes_str); - token_info.SetStringKey("request_time", base::TimeToISO8601(request_time)); + token_info.Set("scopes", scopes_str); + token_info.Set("request_time", base::TimeToISO8601(request_time)); if (removed_) { - token_info.SetStringKey("status", "Token was revoked."); + token_info.Set("status", "Token was revoked."); } else if (!receive_time.is_null()) { if (error == GoogleServiceAuthError::AuthErrorNone()) { bool token_expired = expiration_time < base::Time::Now(); @@ -553,14 +553,13 @@ base::Value AboutSigninInternals::TokenInfo::ToValue() const { // JS code looks for `Expired at` string in order to mark // specific status row red color. Changing `Exired at` status // requires a change in JS code too. - token_info.SetStringKey("status", status_str); + token_info.Set("status", status_str); } else { - token_info.SetStringKey( - "status", - base::StringPrintf("Failure: %s", error.ToString().c_str())); + token_info.Set("status", base::StringPrintf("Failure: %s", + error.ToString().c_str())); } } else { - token_info.SetStringKey("status", "Waiting for response"); + token_info.Set("status", "Waiting for response"); } return token_info; @@ -606,17 +605,17 @@ void AboutSigninInternals::SigninStatus::AddRefreshTokenEvent( refresh_token_events.push_back(event); } -base::Value AboutSigninInternals::SigninStatus::ToValue( +base::Value::Dict AboutSigninInternals::SigninStatus::ToValue( signin::IdentityManager* identity_manager, SigninErrorController* signin_error_controller, SigninClient* signin_client, signin::AccountConsistencyMethod account_consistency, AccountReconcilor* account_reconcilor) { - base::Value::ListStorage signin_info; + base::Value::List signin_info; // A summary of signin related info first. { - base::Value::ListStorage basic_info; + base::Value::List basic_info; AddSectionEntry(basic_info, "Account Consistency", GetAccountConsistencyDescription(account_consistency)); AddSectionEntry(basic_info, "Signin Status", @@ -664,14 +663,13 @@ base::Value AboutSigninInternals::SigninStatus::ToValue( basic_info, "Account Reconcilor blocked", account_reconcilor->IsReconcileBlocked() ? "True" : "False"); - AddSection(signin_info, base::Value(std::move(basic_info)), - "Basic Information"); + AddSection(signin_info, std::move(basic_info), "Basic Information"); } #if !BUILDFLAG(IS_CHROMEOS_ASH) // Time and status information of the possible sign in types. { - base::Value::ListStorage detailed_info; + base::Value::List detailed_info; for (signin_internals_util::TimedSigninStatusField i = signin_internals_util::TIMED_FIELDS_BEGIN; i < signin_internals_util::TIMED_FIELDS_END; ++i) { @@ -707,66 +705,62 @@ base::Value AboutSigninInternals::SigninStatus::ToValue( base::TimeToISO8601(next_retry_time), ""); } - AddSection(signin_info, base::Value(std::move(detailed_info)), - "Last Signin Details"); + AddSection(signin_info, std::move(detailed_info), "Last Signin Details"); } #endif // !BUILDFLAG(IS_CHROMEOS_ASH) - base::Value signin_status(base::Value::Type::DICTIONARY); - signin_status.SetKey("signin_info", base::Value(std::move(signin_info))); + base::Value::Dict signin_status; + signin_status.Set("signin_info", std::move(signin_info)); // Token information for all services. - base::Value::ListStorage token_info; + base::Value::List token_info; for (auto& it : token_info_map) { - base::Value::ListStorage token_details; + base::Value::List token_details; std::sort(it.second.begin(), it.second.end(), TokenInfo::LessThan); for (const std::unique_ptr<TokenInfo>& token : it.second) - token_details.push_back(token->ToValue()); + token_details.Append(token->ToValue()); - AddSection(token_info, base::Value(std::move(token_details)), - it.first.ToString()); + AddSection(token_info, std::move(token_details), it.first.ToString()); } - signin_status.SetKey("token_info", base::Value(std::move(token_info))); + signin_status.Set("token_info", std::move(token_info)); // Account info section - base::Value::ListStorage account_info_section; + base::Value::List account_info_section; const std::vector<CoreAccountInfo>& accounts_with_refresh_tokens = identity_manager->GetAccountsWithRefreshTokens(); if (accounts_with_refresh_tokens.size() == 0) { - base::Value no_token_entry(base::Value::Type::DICTIONARY); - no_token_entry.SetStringKey("accountId", "No token in Token Service."); - account_info_section.push_back(std::move(no_token_entry)); + base::Value::Dict no_token_entry; + no_token_entry.Set("accountId", "No token in Token Service."); + account_info_section.Append(std::move(no_token_entry)); } else { for (const CoreAccountInfo& account_info : accounts_with_refresh_tokens) { - base::Value entry(base::Value::Type::DICTIONARY); - entry.SetStringKey("accountId", account_info.account_id.ToString()); + base::Value::Dict entry; + entry.Set("accountId", account_info.account_id.ToString()); // TODO(https://crbug.com/919793): Remove this field once the token // service is internally consistent on all platforms. - entry.SetBoolKey("hasRefreshToken", - identity_manager->HasAccountWithRefreshToken( - account_info.account_id)); - entry.SetBoolKey( + entry.Set("hasRefreshToken", identity_manager->HasAccountWithRefreshToken( + account_info.account_id)); + entry.Set( "hasAuthError", identity_manager->HasAccountWithRefreshTokenInPersistentErrorState( account_info.account_id)); - account_info_section.push_back(std::move(entry)); + account_info_section.Append(std::move(entry)); } } - signin_status.SetKey("accountInfo", - base::Value(std::move(account_info_section))); + signin_status.Set("accountInfo", std::move(account_info_section)); // Refresh token events section - base::Value::ListStorage refresh_token_events_value; + base::Value::List refresh_token_events_value; for (const auto& event : refresh_token_events) { - base::Value entry(base::Value::Type::DICTIONARY); - entry.SetStringKey("accountId", event.account_id.ToString()); - entry.SetStringKey("timestamp", base::TimeToISO8601(event.timestamp)); - entry.SetStringKey("type", event.GetTypeAsString()); - entry.SetStringKey("source", event.source); - refresh_token_events_value.push_back(std::move(entry)); + base::Value::Dict entry; + entry.Set("accountId", event.account_id.ToString()); + entry.Set("timestamp", base::TimeToISO8601(event.timestamp)); + entry.Set("type", event.GetTypeAsString()); + entry.Set("source", event.source); + refresh_token_events_value.Append(std::move(entry)); } - signin_status.SetKey("refreshTokenEvents", - base::Value(std::move(refresh_token_events_value))); + signin_status.Set("refreshTokenEvents", + std::move(refresh_token_events_value)); return signin_status; } diff --git a/chromium/components/signin/core/browser/about_signin_internals.h b/chromium/components/signin/core/browser/about_signin_internals.h index a8460812666..ef2f9191d9f 100644 --- a/chromium/components/signin/core/browser/about_signin_internals.h +++ b/chromium/components/signin/core/browser/about_signin_internals.h @@ -49,10 +49,10 @@ class AboutSigninInternals : public KeyedService, public: // |info| will contain the dictionary of signin_status_ values as indicated // in the comments for GetSigninStatus() below. - virtual void OnSigninStateChanged(const base::Value* info) = 0; + virtual void OnSigninStateChanged(const base::Value::Dict& info) = 0; // Notification that the cookie accounts are ready to be displayed. - virtual void OnCookieAccountsFetched(const base::Value* info) = 0; + virtual void OnCookieAccountsFetched(const base::Value::Dict& info) = 0; }; AboutSigninInternals(signin::IdentityManager* identity_manager, @@ -97,7 +97,7 @@ class AboutSigninInternals : public KeyedService, // [ List of {"name": "foo-name", "token" : "foo-token", // "status": "foo_stat", "time" : "foo_time"} elems] // } - base::Value GetSigninStatus(); + base::Value::Dict GetSigninStatus(); // signin::IdentityManager::Observer implementations. void OnAccountsInCookieUpdated( @@ -109,7 +109,7 @@ class AboutSigninInternals : public KeyedService, struct TokenInfo { TokenInfo(const std::string& consumer_id, const signin::ScopeSet& scopes); ~TokenInfo(); - base::Value ToValue() const; + base::Value::Dict ToValue() const; static bool LessThan(const std::unique_ptr<TokenInfo>& a, const std::unique_ptr<TokenInfo>& b); @@ -182,11 +182,12 @@ class AboutSigninInternals : public KeyedService, // "status" : request status} elems] // }], // } - base::Value ToValue(signin::IdentityManager* identity_manager, - SigninErrorController* signin_error_controller, - SigninClient* signin_client, - signin::AccountConsistencyMethod account_consistency, - AccountReconcilor* account_reconcilor); + base::Value::Dict ToValue( + signin::IdentityManager* identity_manager, + SigninErrorController* signin_error_controller, + SigninClient* signin_client, + signin::AccountConsistencyMethod account_consistency, + AccountReconcilor* account_reconcilor); }; // IdentityManager::DiagnosticsObserver implementations. diff --git a/chromium/components/signin/core/browser/account_reconcilor.cc b/chromium/components/signin/core/browser/account_reconcilor.cc index 562d86f0f4b..9d8aa9a2db6 100644 --- a/chromium/components/signin/core/browser/account_reconcilor.cc +++ b/chromium/components/signin/core/browser/account_reconcilor.cc @@ -14,7 +14,6 @@ #include "base/bind.h" #include "base/callback_helpers.h" #include "base/containers/contains.h" -#include "base/feature_list.h" #include "base/location.h" #include "base/logging.h" #include "base/memory/ptr_util.h" @@ -24,12 +23,12 @@ #include "base/strings/string_util.h" #include "base/task/single_thread_task_runner.h" #include "base/threading/thread_task_runner_handle.h" +#include "base/time/time.h" #include "components/content_settings/core/browser/content_settings_observer.h" #include "components/signin/core/browser/account_reconcilor_delegate.h" #include "components/signin/public/base/account_consistency_method.h" #include "components/signin/public/base/signin_client.h" #include "components/signin/public/base/signin_metrics.h" -#include "components/signin/public/base/signin_switches.h" #include "components/signin/public/identity_manager/accounts_cookie_mutator.h" #include "components/signin/public/identity_manager/accounts_in_cookie_jar_info.h" #include "components/signin/public/identity_manager/accounts_mutator.h" @@ -48,6 +47,13 @@ using signin_metrics::AccountReconcilorState; namespace { +#if BUILDFLAG(ENABLE_MIRROR) +// Number of seconds to wait before trying to force another reconciliation +// cycle. The value roughly represents the 95 percentile success rate of +// `Signin.Reconciler.Duration.UpTo3mins.Success` histogram. +const int kForcedReconciliationWaitTimeInSeconds = 15; +#endif // BUILDFLAG(ENABLE_MIRROR) + // Returns a copy of |accounts| without the unverified accounts. std::vector<gaia::ListedAccount> FilterUnverifiedAccounts( const std::vector<gaia::ListedAccount>& accounts) { @@ -135,10 +141,8 @@ AccountReconcilor::AccountReconcilor( timeout_ = delegate_->GetReconcileTimeout(); #if BUILDFLAG(IS_CHROMEOS_LACROS) - if (base::FeatureList::IsEnabled(switches::kLacrosNonSyncingProfiles)) { - consistency_cookie_manager_ = - std::make_unique<signin::ConsistencyCookieManager>(client_, this); - } + consistency_cookie_manager_ = + std::make_unique<signin::ConsistencyCookieManager>(client_, this); #endif } @@ -385,8 +389,21 @@ void AccountReconcilor::StartReconcile(Trigger trigger) { return; } + // In the case of a forced reconciliation, we will not rely on ListAccounts, + // and consider the cookie jar to be empty. + if (trigger_ == Trigger::kForcedReconcile) { + OnAccountsInCookieUpdated( + /*accounts_in_cookie_jar_info=*/signin::AccountsInCookieJarInfo( + /*accounts_are_fresh_param=*/true, + /*signed_in_accounts_param=*/{}, + /*signed_out_accounts_param=*/{}), + /*error=*/GoogleServiceAuthError(GoogleServiceAuthError::NONE)); + return; + } + // Rely on the IdentityManager to manage calls to and responses from - // ListAccounts. + // ListAccounts - except when a forced reconciliation is requested (handled + // above). signin::AccountsInCookieJarInfo accounts_in_cookie_jar = identity_manager_->GetAccountsInCookieJar(); if (accounts_in_cookie_jar.accounts_are_fresh) { @@ -625,6 +642,38 @@ void AccountReconcilor::ScheduleStartReconcileIfChromeAccountsChanged() { } } +#if BUILDFLAG(ENABLE_MIRROR) +void AccountReconcilor::ForceReconcile() { + if (state_ == + signin_metrics::AccountReconcilorState::ACCOUNT_RECONCILOR_INACTIVE) { + VLOG(1) << "Ignoring ForceReconcile request because AccountReconcilor is " + "inactive"; + return; + } + + if (!is_reconcile_started_ && + (state_ == + signin_metrics::AccountReconcilorState::ACCOUNT_RECONCILOR_OK || + state_ == + signin_metrics::AccountReconcilorState::ACCOUNT_RECONCILOR_ERROR)) { + // Reconcilor is not running. Force start it. + StartReconcile(Trigger::kForcedReconcile); + return; + } + + // For all other cases, wait for some time and retry forcing a reconciliation. + // Note that we cannot simply rely on the current reconciliation cycle because + // `kForcedReconcile` is handled differently by `StartReconcile` - it leads to + // ListAccounts being ignored - something that doesn't happen in a regular + // reconciliation cycle. + base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( + FROM_HERE, + base::BindOnce(&AccountReconcilor::ForceReconcile, + weak_factory_.GetWeakPtr()), + base::Seconds(kForcedReconciliationWaitTimeInSeconds)); +} +#endif // BUILDFLAG(ENABLE_MIRROR) + bool AccountReconcilor::IsIdentityManagerReady() { return identity_manager_->AreRefreshTokensLoaded(); } diff --git a/chromium/components/signin/core/browser/account_reconcilor.h b/chromium/components/signin/core/browser/account_reconcilor.h index 92d03784d4d..5facbbb0dc8 100644 --- a/chromium/components/signin/core/browser/account_reconcilor.h +++ b/chromium/components/signin/core/browser/account_reconcilor.h @@ -163,6 +163,19 @@ class AccountReconcilor : public KeyedService, friend class signin::ConsistencyCookieManagerTest; #endif +#if BUILDFLAG(ENABLE_MIRROR) + FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, + ForceReconcileEarlyExitsForInactiveReconcilor); + FRIEND_TEST_ALL_PREFIXES(AccountReconcilorMirrorTest, + ForceReconcileImmediatelyStartsForIdleReconcilor); + FRIEND_TEST_ALL_PREFIXES( + AccountReconcilorMirrorTest, + ForceReconcileImmediatelyStartsForErroredOutReconcilor); + FRIEND_TEST_ALL_PREFIXES( + AccountReconcilorMirrorTest, + ForceReconcileSchedulesReconciliationIfReconcilorIsAlreadyRunning); +#endif // BUILDFLAG(ENABLE_MIRROR) + FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTestForceDiceMigration, TableRowTestCheckNoOp); FRIEND_TEST_ALL_PREFIXES(AccountReconcilorMirrorTest, @@ -243,6 +256,10 @@ class AccountReconcilor : public KeyedService, FRIEND_TEST_ALL_PREFIXES(AccountReconcilorMirrorTest, DelegateTimeoutIsNotCalled); FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, + ForcedReconcileTriggerShouldNotCallListAccounts); + FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, + ForcedReconcileTriggerShouldNotResultInNoop); + FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, DelegateTimeoutIsNotCalledIfTimeoutIsNotReached); FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, MultiloginLogout); FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTestForceDiceMigration, @@ -283,8 +300,9 @@ class AccountReconcilor : public KeyedService, kTokenChangeDuringReconcile = 5, kCookieChange = 6, kCookieSettingChange = 7, + kForcedReconcile = 8, - kMaxValue = kCookieSettingChange + kMaxValue = kForcedReconcile }; void set_timer_for_testing(std::unique_ptr<base::OneShotTimer> timer); @@ -316,6 +334,13 @@ class AccountReconcilor : public KeyedService, void AbortReconcile(); void ScheduleStartReconcileIfChromeAccountsChanged(); +#if BUILDFLAG(ENABLE_MIRROR) + // Forces reconciliation. A reconciliation cycle is started immediately if it + // is not already running, otherwise another forced reconciliation is + // attempted after some time. + void ForceReconcile(); +#endif // BUILDFLAG(ENABLE_MIRROR) + // Returns the list of valid accounts from the TokenService. std::vector<CoreAccountId> LoadValidAccountsFromTokenService() const; diff --git a/chromium/components/signin/core/browser/account_reconcilor_unittest.cc b/chromium/components/signin/core/browser/account_reconcilor_unittest.cc index 3af02870985..f1835003db6 100644 --- a/chromium/components/signin/core/browser/account_reconcilor_unittest.cc +++ b/chromium/components/signin/core/browser/account_reconcilor_unittest.cc @@ -8,13 +8,13 @@ #include <memory> #include <string> #include <utility> +#include <vector> #include "base/containers/contains.h" #include "base/run_loop.h" #include "base/scoped_observation.h" #include "base/strings/utf_string_conversions.h" #include "base/test/metrics/histogram_tester.h" -#include "base/test/scoped_feature_list.h" #include "base/test/task_environment.h" #include "base/time/time.h" #include "base/timer/mock_timer.h" @@ -37,6 +37,7 @@ #include "components/signin/public/identity_manager/primary_account_mutator.h" #include "components/signin/public/identity_manager/set_accounts_in_cookie_result.h" #include "components/sync_preferences/testing_pref_service_syncable.h" +#include "google_apis/gaia/core_account_id.h" #include "google_apis/gaia/gaia_constants.h" #include "google_apis/gaia/gaia_urls.h" #include "google_apis/gaia/google_service_auth_error.h" @@ -48,13 +49,12 @@ #endif #if BUILDFLAG(IS_CHROMEOS_ASH) -#include "ash/components/tpm/stub_install_attributes.h" +#include "chromeos/ash/components/install_attributes/stub_install_attributes.h" #include "components/signin/core/browser/active_directory_account_reconcilor_delegate.h" #endif #if BUILDFLAG(IS_CHROMEOS_LACROS) #include "components/signin/core/browser/mirror_landing_account_reconcilor_delegate.h" -#include "components/signin/public/base/signin_switches.h" #endif using signin_metrics::AccountReconcilorState; @@ -62,6 +62,15 @@ using testing::_; namespace { +#if BUILDFLAG(ENABLE_MIRROR) +// This should match the variable in the .cc file. +const int kForcedReconciliationWaitTimeInSeconds = 15; +#endif // BUILDFLAG(ENABLE_MIRROR) + +const char kFakeEmail[] = "user@gmail.com"; +const char kFakeEmail2[] = "other@gmail.com"; +const char kFakeGaiaId[] = "12345"; + // An AccountReconcilorDelegate that records all calls (Spy pattern). class SpyReconcilorDelegate : public signin::AccountReconcilorDelegate { public: @@ -139,15 +148,12 @@ class DummyAccountReconcilorWithDelegate : public AccountReconcilor { switch (account_consistency) { case signin::AccountConsistencyMethod::kMirror: #if BUILDFLAG(IS_CHROMEOS_LACROS) - if (base::FeatureList::IsEnabled(switches::kLacrosNonSyncingProfiles)) { - bool is_main_profile = client->GetInitialPrimaryAccount().has_value(); - return std::make_unique< - signin::MirrorLandingAccountReconcilorDelegate>(identity_manager, - is_main_profile); - } -#endif // BUILDFLAG(IS_CHROMEOS_LACROS) + return std::make_unique<signin::MirrorLandingAccountReconcilorDelegate>( + identity_manager, client->GetInitialPrimaryAccount().has_value()); +#else return std::make_unique<signin::MirrorAccountReconcilorDelegate>( identity_manager); +#endif // BUILDFLAG(IS_CHROMEOS_LACROS) case signin::AccountConsistencyMethod::kDisabled: return std::make_unique<signin::AccountReconcilorDelegate>(); case signin::AccountConsistencyMethod::kDice: @@ -417,8 +423,6 @@ struct AccountReconcilorTestTableParam { const char* gaia_api_calls; const char* tokens_after_reconcile; const char* cookies_after_reconcile; - // Int represents AccountReconcilorDelegate::InconsistencyReason. - const int inconsistency_reason; }; std::vector<AccountReconcilorTestTableParam> GenerateTestCasesFromParams( @@ -470,6 +474,8 @@ class BaseAccountReconcilorTestTable : public AccountReconcilorTest { bool has_error; }; + virtual void CreateReconclior() { GetMockReconcilor(); } + // Build Tokens from string. std::vector<Token> ParseTokenString(const char* token_string) { std::vector<Token> parsed_tokens; @@ -584,8 +590,134 @@ class BaseAccountReconcilorTestTable : public AccountReconcilorTest { identity_test_env()->SetFreshnessOfAccountsInGaiaCookie(false); } - std::string GaiaIdForAccountKey(char account_key) { - return accounts_[account_key].gaia_id; + Account GetAccount(const CoreAccountId& account_id) { + for (const auto& pair : accounts_) { + const Account& account = pair.second; + if (PickAccountIdForAccount(account.gaia_id, account.email) == account_id) + return account; + } + NOTREACHED(); + return Account(); + } + + // Simulates the effect of a Multilogin call on the cookies. + std::vector<Cookie> FakeSetAccountsInCookie( + const signin::MultiloginParameters& parameters, + const std::vector<Cookie>& cookies_before_reconcile) { + std::vector<Cookie> cookies_after_reconcile; + if (parameters.mode == + gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER) { + for (const CoreAccountId& account_id : parameters.accounts_to_send) { + cookies_after_reconcile.push_back( + {GetAccount(account_id).gaia_id, true}); + } + } else { + std::vector<std::string> gaia_ids; + for (const auto& account_id : parameters.accounts_to_send) + gaia_ids.push_back(GetAccount(account_id).gaia_id); + cookies_after_reconcile = cookies_before_reconcile; + for (Cookie& cookie : cookies_after_reconcile) { + if (base::Contains(gaia_ids, cookie.gaia_id)) { + cookie.is_valid = true; + gaia_ids.erase( + std::find(gaia_ids.begin(), gaia_ids.end(), cookie.gaia_id)); + } else { + DCHECK(!cookie.is_valid); + } + } + for (const std::string& gaia_id : gaia_ids) + cookies_after_reconcile.push_back({gaia_id, true}); + } + return cookies_after_reconcile; + } + + // Runs the test corresponding to one row of the table. + void RunRowTest(const AccountReconcilorTestTableParam& param) { + // Setup cookies. + std::vector<Cookie> cookies = ParseCookieString(param.cookies); + ConfigureCookieManagerService(cookies); + std::vector<Cookie> cookies_after_reconcile = cookies; + + // Call list accounts now so that the next call completes synchronously. + identity_test_env()->identity_manager()->GetAccountsInCookieJar(); + base::RunLoop().RunUntilIdle(); + + // Setup tokens. This triggers listing cookies so we need to setup cookies + // before that. + SetupTokens(param.tokens); + + CreateReconclior(); + + // Setup expectations. + testing::InSequence mock_sequence; + bool should_logout = false; + if (param.gaia_api_calls[0] != '\0') { + if (param.gaia_api_calls[0] == 'X') { + should_logout = true; + EXPECT_CALL(*GetMockReconcilor(), PerformLogoutAllAccountsAction()) + .Times(1); + cookies_after_reconcile.clear(); + } else { + gaia::MultiloginMode mode = + param.gaia_api_calls[0] == 'U' + ? gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER + : gaia::MultiloginMode:: + MULTILOGIN_PRESERVE_COOKIE_ACCOUNTS_ORDER; + // Generate expected array of accounts in cookies and set fake gaia + // response. + std::vector<CoreAccountId> accounts_to_send; + for (int i = 1; param.gaia_api_calls[i] != '\0'; ++i) { + const Account& account = accounts_[param.gaia_api_calls[i]]; + accounts_to_send.push_back( + PickAccountIdForAccount(account.gaia_id, account.email)); + } + DCHECK(!accounts_to_send.empty()); + const signin::MultiloginParameters params(mode, accounts_to_send); + cookies_after_reconcile = FakeSetAccountsInCookie(params, cookies); + EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)) + .Times(1); + } + } + // Reconcile. + AccountReconcilor* reconcilor = GetMockReconcilor(); + ASSERT_TRUE(reconcilor); + ASSERT_TRUE(reconcilor->first_execution_); + reconcilor->first_execution_ = + param.is_first_reconcile == IsFirstReconcile::kFirst ? true : false; + reconcilor->StartReconcile(AccountReconcilor::Trigger::kCookieChange); + if (param.gaia_api_calls[0] != '\0') { + if (should_logout) { + SimulateLogOutFromCookieCompleted( + reconcilor, GoogleServiceAuthError::AuthErrorNone()); + } else { + SimulateSetAccountsInCookieCompleted( + reconcilor, signin::SetAccountsInCookieResult::kSuccess); + } + } + + ASSERT_FALSE(reconcilor->is_reconcile_started_); + if (param.tokens == param.tokens_after_reconcile) { + EXPECT_EQ(signin_metrics::ACCOUNT_RECONCILOR_OK, reconcilor->GetState()); + } else { + // If the tokens were changed by the reconcile, a new reconcile should be + // scheduled. + EXPECT_EQ(signin_metrics::ACCOUNT_RECONCILOR_SCHEDULED, + reconcilor->GetState()); + } + VerifyCurrentTokens(ParseTokenString(param.tokens_after_reconcile)); + + std::vector<Cookie> cookies_after = + ParseCookieString(param.cookies_after_reconcile); + EXPECT_EQ(cookies_after, cookies_after_reconcile); + + testing::Mock::VerifyAndClearExpectations(GetMockReconcilor()); + + // Another reconcile is sometimes triggered if Chrome accounts have + // changed. Allow it to finish. + EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(testing::_)) + .WillRepeatedly(testing::Return()); + ConfigureCookieManagerService({}); + base::RunLoop().RunUntilIdle(); } std::map<char, Account> accounts_; @@ -629,7 +761,7 @@ TEST_F(AccountReconcilorMirrorTest, IdentityManagerRegistration) { ASSERT_TRUE(reconcilor); ASSERT_FALSE(reconcilor->IsRegisteredWithIdentityManager()); - identity_test_env()->MakePrimaryAccountAvailable("user@gmail.com", + identity_test_env()->MakePrimaryAccountAvailable(kFakeEmail, signin::ConsentLevel::kSync); ASSERT_TRUE(reconcilor->IsRegisteredWithIdentityManager()); @@ -640,8 +772,7 @@ TEST_F(AccountReconcilorMirrorTest, IdentityManagerRegistration) { } TEST_F(AccountReconcilorMirrorTest, Reauth) { - const std::string email = "user@gmail.com"; - AccountInfo account_info = ConnectProfileToAccount(email); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); AccountReconcilor* reconcilor = GetMockReconcilor(); ASSERT_TRUE(reconcilor); @@ -660,7 +791,7 @@ TEST_F(AccountReconcilorMirrorTest, Reauth) { #endif // !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_CHROMEOS_LACROS) TEST_F(AccountReconcilorMirrorTest, ProfileAlreadyConnected) { - ConnectProfileToAccount("user@gmail.com"); + ConnectProfileToAccount(kFakeEmail); AccountReconcilor* reconcilor = GetMockReconcilor(); ASSERT_TRUE(reconcilor); @@ -669,37 +800,6 @@ TEST_F(AccountReconcilorMirrorTest, ProfileAlreadyConnected) { #if BUILDFLAG(ENABLE_DICE_SUPPORT) -namespace { -std::vector<Cookie> FakeSetAccountsInCookie( - const signin::MultiloginParameters& parameters, - const std::vector<Cookie>& cookies_before_reconcile) { - std::vector<Cookie> cookies_after_reconcile; - if (parameters.mode == - gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER) { - for (const CoreAccountId& account : parameters.accounts_to_send) { - cookies_after_reconcile.push_back({account.ToString(), true}); - } - } else { - std::vector<CoreAccountId> accounts(parameters.accounts_to_send.begin(), - parameters.accounts_to_send.end()); - cookies_after_reconcile = cookies_before_reconcile; - for (Cookie& param : cookies_after_reconcile) { - CoreAccountId account = CoreAccountId(param.gaia_id); - if (base::Contains(accounts, account)) { - param.is_valid = true; - accounts.erase(std::find(accounts.begin(), accounts.end(), account)); - } else { - DCHECK(!param.is_valid); - } - } - for (const CoreAccountId& account : accounts) { - cookies_after_reconcile.push_back({account.ToString(), true}); - } - } - return cookies_after_reconcile; -} -} // namespace - // clang-format off const std::vector<AccountReconcilorTestTableParam> kDiceParams = { // This table encodes the initial state and expectations of a reconcile. @@ -720,162 +820,163 @@ const std::vector<AccountReconcilorTestTableParam> kDiceParams = { // x: The next cookie is marked "invalid". // - First Run: true if this is the first reconcile (i.e. Chrome startup). // ------------------------------------------------------------------------- - // Tokens|Cookies|First Run|Gaia calls|Tokens aft.|Cookies aft.|AccountReconcilorDelegate::InconsistencyReason | + // Tokens|Cookies| First Run |Gaia calls|Tokens aft.|Cookies aft. // ------------------------------------------------------------------------- // First reconcile (Chrome restart): Rebuild the Gaia cookie to match the // tokens. Make the Sync account the default account in the Gaia cookie. // Sync enabled. - { "", "A", IsFirstReconcile::kBoth, "U", "", "", 3}, - { "*AB", "AB", IsFirstReconcile::kBoth, "", "*AB", "AB", 0}, - { "*A", "A", IsFirstReconcile::kBoth, "", "*A" , "A", 0}, - { "*A", "", IsFirstReconcile::kBoth, "PA", "*A" , "A", 1}, - { "*A", "B", IsFirstReconcile::kBoth, "UA", "*A" , "A", 1}, - { "*A", "AB", IsFirstReconcile::kBoth, "UA", "*A" , "A", 5}, - { "*AB", "BA", IsFirstReconcile::kFirst, "UAB", "*AB", "AB", 7}, - { "*AB", "BA", IsFirstReconcile::kNotFirst,"", "*AB", "BA", 0}, + { "", "A", IsFirstReconcile::kBoth, "X", "", "" }, + { "*AB", "AB", IsFirstReconcile::kBoth, "", "*AB", "AB" }, + { "*A", "A", IsFirstReconcile::kBoth, "", "*A" , "A" }, + { "*A", "", IsFirstReconcile::kBoth, "PA", "*A" , "A" }, + { "*A", "B", IsFirstReconcile::kBoth, "UA", "*A" , "A" }, + { "*A", "AB", IsFirstReconcile::kBoth, "UA", "*A" , "A" }, + { "*AB", "BA", IsFirstReconcile::kFirst, "UAB", "*AB", "AB" }, + { "*AB", "BA", IsFirstReconcile::kNotFirst, "", "*AB", "BA" }, - { "*AB", "A", IsFirstReconcile::kBoth, "PAB", "*AB", "AB", 4}, + { "*AB", "A", IsFirstReconcile::kBoth, "PAB", "*AB", "AB" }, - { "*AB", "B", IsFirstReconcile::kFirst, "UAB", "*AB", "AB", 1}, - { "*AB", "B", IsFirstReconcile::kNotFirst,"PBA", "*AB", "BA", 1}, + { "*AB", "B", IsFirstReconcile::kFirst, "UAB", "*AB", "AB" }, + { "*AB", "B", IsFirstReconcile::kNotFirst, "PBA", "*AB", "BA" }, - { "*AB", "", IsFirstReconcile::kBoth, "PAB", "*AB", "AB", 1}, + { "*AB", "", IsFirstReconcile::kBoth, "PAB", "*AB", "AB" }, // Sync enabled, token error on primary. - { "*xAB", "AB", IsFirstReconcile::kBoth, "U", "*xA", "", 2}, - { "*xAB", "BA", IsFirstReconcile::kBoth, "UB", "*xAB", "B", 2}, - { "*xAB", "A", IsFirstReconcile::kBoth, "U", "*xA", "", 2}, - { "*xAB", "B", IsFirstReconcile::kBoth, "", "*xAB", "B", 0}, - { "*xAB", "", IsFirstReconcile::kBoth, "PB", "*xAB", "B", 0}, + { "*xAB", "AB", IsFirstReconcile::kBoth, "X", "*xA", "" }, + { "*xAB", "BA", IsFirstReconcile::kBoth, "UB", "*xAB", "B" }, + { "*xAB", "A", IsFirstReconcile::kBoth, "X", "*xA", "" }, + { "*xAB", "B", IsFirstReconcile::kBoth, "", "*xAB", "B" }, + { "*xAB", "", IsFirstReconcile::kBoth, "PB", "*xAB", "B" }, // Sync enabled, token error on secondary. - { "*AxB", "AB", IsFirstReconcile::kBoth, "UA", "*A", "A", 5}, - { "*AxB", "A", IsFirstReconcile::kBoth, "", "*A", "A", 0}, - { "*AxB", "", IsFirstReconcile::kBoth, "PA", "*A", "A", 1}, + { "*AxB", "AB", IsFirstReconcile::kBoth, "UA", "*A", "A" }, + { "*AxB", "A", IsFirstReconcile::kBoth, "", "*A", "A" }, + { "*AxB", "", IsFirstReconcile::kBoth, "PA", "*A", "A" }, // The first account in cookies is swapped even when Chrome is running. - // The swap would happen at next startup anyway and doing it earlier avoids signing the user out. - { "*AxB", "BA", IsFirstReconcile::kBoth, "UA", "*A", "A", 5}, - { "*AxB", "B", IsFirstReconcile::kBoth, "UA", "*A", "A", 1}, + // The swap would happen at next startup anyway and doing it earlier avoids + // signing the user out. + { "*AxB", "BA", IsFirstReconcile::kBoth, "UA", "*A", "A" }, + { "*AxB", "B", IsFirstReconcile::kBoth, "UA", "*A", "A" }, // Sync enabled, token error on both accounts. - { "*xAxB", "AB", IsFirstReconcile::kBoth, "U", "*xA", "", 2}, - { "*xAxB", "BA", IsFirstReconcile::kBoth, "U", "*xA", "", 2}, - { "*xAxB", "A", IsFirstReconcile::kBoth, "U", "*xA", "", 2}, - { "*xAxB", "B", IsFirstReconcile::kBoth, "U", "*xA", "", 5}, - { "*xAxB", "", IsFirstReconcile::kBoth, "", "*xA", "", 0}, + { "*xAxB", "AB", IsFirstReconcile::kBoth, "X", "*xA", "" }, + { "*xAxB", "BA", IsFirstReconcile::kBoth, "X", "*xA", "" }, + { "*xAxB", "A", IsFirstReconcile::kBoth, "X", "*xA", "" }, + { "*xAxB", "B", IsFirstReconcile::kBoth, "X", "*xA", "" }, + { "*xAxB", "", IsFirstReconcile::kBoth, "", "*xA", "" }, // Sync disabled. - { "AB", "AB", IsFirstReconcile::kBoth, "", "AB", "AB", 0}, - { "AB", "BA", IsFirstReconcile::kBoth, "", "AB", "BA", 0}, - { "AB", "A", IsFirstReconcile::kBoth, "PAB", "AB", "AB", 4}, - { "AB", "B", IsFirstReconcile::kBoth, "PBA", "AB", "BA", 4}, - { "AB", "", IsFirstReconcile::kBoth, "PAB", "AB", "AB", 0}, + { "AB", "AB", IsFirstReconcile::kBoth, "", "AB", "AB" }, + { "AB", "BA", IsFirstReconcile::kBoth, "", "AB", "BA" }, + { "AB", "A", IsFirstReconcile::kBoth, "PAB", "AB", "AB" }, + { "AB", "B", IsFirstReconcile::kBoth, "PBA", "AB", "BA" }, + { "AB", "", IsFirstReconcile::kBoth, "PAB", "AB", "AB" }, // Sync disabled, token error on first account. - { "xAB", "AB", IsFirstReconcile::kFirst, "UB", "B", "B", 3}, - { "xAB", "AB", IsFirstReconcile::kNotFirst, "U", "", "", 3}, + { "xAB", "AB", IsFirstReconcile::kFirst, "UB", "B", "B" }, + { "xAB", "AB", IsFirstReconcile::kNotFirst, "X", "", "" }, - { "xAB", "BA", IsFirstReconcile::kBoth, "UB", "B", "B", 5}, + { "xAB", "BA", IsFirstReconcile::kBoth, "UB", "B", "B" }, - { "xAB", "A", IsFirstReconcile::kFirst, "UB", "B", "B", 3}, - { "xAB", "A", IsFirstReconcile::kNotFirst, "U", "", "", 3}, + { "xAB", "A", IsFirstReconcile::kFirst, "UB", "B", "B" }, + { "xAB", "A", IsFirstReconcile::kNotFirst, "X", "", "" }, - { "xAB", "B", IsFirstReconcile::kBoth, "", "B", "B", 0}, + { "xAB", "B", IsFirstReconcile::kBoth, "", "B", "B" }, - { "xAB", "", IsFirstReconcile::kBoth, "PB", "B", "B", 0}, + { "xAB", "", IsFirstReconcile::kBoth, "PB", "B", "B" }, // Sync disabled, token error on second account - { "AxB", "AB", IsFirstReconcile::kBoth, "UA", "A", "A", 5}, + { "AxB", "AB", IsFirstReconcile::kBoth, "UA", "A", "A" }, - { "AxB", "BA", IsFirstReconcile::kFirst, "UA", "A", "A", 3}, - { "AxB", "BA", IsFirstReconcile::kNotFirst, "U", "", "", 3}, + { "AxB", "BA", IsFirstReconcile::kFirst, "UA", "A", "A" }, + { "AxB", "BA", IsFirstReconcile::kNotFirst, "X", "", "" }, - { "AxB", "A", IsFirstReconcile::kBoth, "", "A", "A", 0}, + { "AxB", "A", IsFirstReconcile::kBoth, "", "A", "A" }, - { "AxB", "B", IsFirstReconcile::kFirst, "UA", "A", "A", 3}, - { "AxB", "B", IsFirstReconcile::kNotFirst, "U", "", "", 3}, + { "AxB", "B", IsFirstReconcile::kFirst, "UA", "A", "A" }, + { "AxB", "B", IsFirstReconcile::kNotFirst, "X", "", "" }, - { "AxB", "", IsFirstReconcile::kBoth, "PA", "A", "A", 0}, + { "AxB", "", IsFirstReconcile::kBoth, "PA", "A", "A" }, // Sync disabled, token error on both accounts. - { "xAxB", "AB", IsFirstReconcile::kBoth, "U", "", "", 3}, - { "xAxB", "BA", IsFirstReconcile::kBoth, "U", "", "", 3}, - { "xAxB", "A", IsFirstReconcile::kBoth, "U", "", "", 3}, - { "xAxB", "B", IsFirstReconcile::kBoth, "U", "", "", 3}, - { "xAxB", "", IsFirstReconcile::kBoth, "", "", "", 0}, + { "xAxB", "AB", IsFirstReconcile::kBoth, "X", "", "" }, + { "xAxB", "BA", IsFirstReconcile::kBoth, "X", "", "" }, + { "xAxB", "A", IsFirstReconcile::kBoth, "X", "", "" }, + { "xAxB", "B", IsFirstReconcile::kBoth, "X", "", "" }, + { "xAxB", "", IsFirstReconcile::kBoth, "", "", "" }, // Account marked as invalid in cookies. // No difference between cookies and tokens, do not do do anything. // Do not logout. Regression tests for http://crbug.com/854799 - { "", "xA", IsFirstReconcile::kBoth, "", "", "xA", 0}, - { "", "xAxB", IsFirstReconcile::kBoth, "", "", "xAxB", 0}, - { "xA", "xA", IsFirstReconcile::kBoth, "", "", "xA", 0}, - { "xAB", "xAB", IsFirstReconcile::kBoth, "", "B", "xAB", 0}, - { "AxB", "AxC", IsFirstReconcile::kBoth, "", "A", "AxC", 0}, - { "B", "xAB", IsFirstReconcile::kBoth, "", "B", "xAB", 0}, - { "*xA", "xA", IsFirstReconcile::kBoth, "", "*xA", "xA", 0}, - { "*xA", "xB", IsFirstReconcile::kBoth, "", "*xA", "xB", 0}, - { "*xAB", "xAB", IsFirstReconcile::kBoth, "", "*xAB", "xAB", 0}, - { "*AxB", "xBA", IsFirstReconcile::kNotFirst, "", "*A", "xBA", 0}, + { "", "xA", IsFirstReconcile::kBoth, "", "", "xA" }, + { "", "xAxB", IsFirstReconcile::kBoth, "", "", "xAxB" }, + { "xA", "xA", IsFirstReconcile::kBoth, "", "", "xA" }, + { "xAB", "xAB", IsFirstReconcile::kBoth, "", "B", "xAB" }, + { "AxB", "AxC", IsFirstReconcile::kBoth, "", "A", "AxC" }, + { "B", "xAB", IsFirstReconcile::kBoth, "", "B", "xAB" }, + { "*xA", "xA", IsFirstReconcile::kBoth, "", "*xA", "xA" }, + { "*xA", "xB", IsFirstReconcile::kBoth, "", "*xA", "xB" }, + { "*xAB", "xAB", IsFirstReconcile::kBoth, "", "*xAB", "xAB" }, + { "*AxB", "xBA", IsFirstReconcile::kNotFirst, "", "*A", "xBA" }, // Appending a new cookie after the invalid one. - { "B", "xA", IsFirstReconcile::kBoth, "PB", "B", "xAB", 4}, - { "xAB", "xA", IsFirstReconcile::kBoth, "PB", "B", "xAB", 4}, + { "B", "xA", IsFirstReconcile::kBoth, "PB", "B", "xAB" }, + { "xAB", "xA", IsFirstReconcile::kBoth, "PB", "B", "xAB" }, // Refresh existing cookies. - { "AB", "xAB", IsFirstReconcile::kBoth, "PAB", "AB", "AB", 4}, - { "*AB", "xBxA", IsFirstReconcile::kNotFirst, "PBA", "*AB", "BA", 1}, + { "AB", "xAB", IsFirstReconcile::kBoth, "PAB", "AB", "AB" }, + { "*AB", "xBxA", IsFirstReconcile::kNotFirst, "PBA", "*AB", "BA" }, // Appending and invalidating cookies at the same time. - { "xAB", "xAC", IsFirstReconcile::kFirst, "UB", "B", "B", 6}, - { "xAB", "xAC", IsFirstReconcile::kNotFirst, "U", "", "", 6}, + { "xAB", "xAC", IsFirstReconcile::kFirst, "UB", "B", "B" }, + { "xAB", "xAC", IsFirstReconcile::kNotFirst, "X", "", "" }, - { "xAB", "AxC", IsFirstReconcile::kFirst, "UB", "B", "B", 3}, - { "xAB", "AxC", IsFirstReconcile::kNotFirst, "U", "", "", 3}, + { "xAB", "AxC", IsFirstReconcile::kFirst, "UB", "B", "B" }, + { "xAB", "AxC", IsFirstReconcile::kNotFirst, "X", "", "" }, - { "*xAB", "xABC", IsFirstReconcile::kFirst, "UB", "*xAB", "B", 5}, - { "*xAB", "xABC", IsFirstReconcile::kNotFirst, "U", "*xA", "", 5}, + { "*xAB", "xABC", IsFirstReconcile::kFirst, "UB", "*xAB", "B" }, + { "*xAB", "xABC", IsFirstReconcile::kNotFirst, "X", "*xA", "" }, - { "xAB", "xABC", IsFirstReconcile::kFirst, "UB", "B", "B", 5}, - { "xAB", "xABC", IsFirstReconcile::kNotFirst, "U", "", "", 5}, + { "xAB", "xABC", IsFirstReconcile::kFirst, "UB", "B", "B" }, + { "xAB", "xABC", IsFirstReconcile::kNotFirst, "X", "", "" }, // Miscellaneous cases. // Check that unknown Gaia accounts are signed o. - { "*A", "AB", IsFirstReconcile::kBoth, "UA", "*A", "A", 5}, + { "*A", "AB", IsFirstReconcile::kBoth, "UA", "*A", "A" }, // Check that Gaia default account is kept in first position. - { "AB", "BC", IsFirstReconcile::kBoth, "UBA", "AB", "BA", 6}, + { "AB", "BC", IsFirstReconcile::kBoth, "UBA", "AB", "BA" }, // Check that Gaia cookie order is preserved for B. - { "*ABC", "CB", IsFirstReconcile::kFirst, "UABC", "*ABC", "ABC", 1}, + { "*ABC", "CB", IsFirstReconcile::kFirst, "UABC", "*ABC", "ABC" }, // TODO(https://crbug.com/1129931): Merge session should do XCB instead. - { "xABC", "ABC", IsFirstReconcile::kFirst, "UCB", "BC", "CB", 1}, + { "xABC", "ABC", IsFirstReconcile::kFirst, "UCB", "BC", "CB" }, // Check that order in the chrome_accounts is not important. - { "A*B", "", IsFirstReconcile::kBoth, "PBA", "A*B", "BA", 7}, - { "*xBA", "BA", IsFirstReconcile::kFirst, "U", "*xB", "", 2}, + { "A*B", "", IsFirstReconcile::kBoth, "PBA", "A*B", "BA" }, + { "*xBA", "BA", IsFirstReconcile::kFirst, "X", "*xB", "" }, // Required for idempotency check. - { "", "", IsFirstReconcile::kNotFirst, "", "", "", 0}, - { "", "xA", IsFirstReconcile::kNotFirst, "", "", "xA", 0}, - { "", "xB", IsFirstReconcile::kNotFirst, "", "", "xB", 0}, - { "", "xAxB", IsFirstReconcile::kNotFirst, "", "", "xAxB", 0}, - { "", "xBxA", IsFirstReconcile::kNotFirst, "", "", "xBxA", 0}, - { "*A", "A", IsFirstReconcile::kNotFirst, "", "*A", "A", 0}, - { "*A", "xBA", IsFirstReconcile::kNotFirst, "", "*A", "xBA", 0}, - { "*A", "AxB", IsFirstReconcile::kNotFirst, "", "*A", "AxB", 0}, - { "A", "A", IsFirstReconcile::kNotFirst, "", "A", "A", 0}, - { "A", "xBA", IsFirstReconcile::kNotFirst, "", "A", "xBA", 0}, - { "A", "AxB", IsFirstReconcile::kNotFirst, "", "A", "AxB", 0}, - { "B", "B", IsFirstReconcile::kNotFirst, "", "B", "B", 0}, - { "B", "xAB", IsFirstReconcile::kNotFirst, "", "B", "xAB", 0}, - { "B", "BxA", IsFirstReconcile::kNotFirst, "", "B", "BxA", 0}, - { "*xA", "", IsFirstReconcile::kNotFirst, "", "*xA", "", 0}, - { "*xA", "xAxB", IsFirstReconcile::kNotFirst, "", "*xA", "xAxB", 0}, - { "*xA", "xBxA", IsFirstReconcile::kNotFirst, "", "*xA", "xBxA", 0}, - { "*xA", "xA", IsFirstReconcile::kNotFirst, "", "*xA", "xA", 0}, - { "*xA", "xB", IsFirstReconcile::kNotFirst, "", "*xA", "xB", 0}, - { "*xAB", "B", IsFirstReconcile::kNotFirst, "", "*xAB", "B", 0}, - { "*xAB", "BxA", IsFirstReconcile::kNotFirst, "", "*xAB", "BxA", 0}, - { "*xAB", "xAB", IsFirstReconcile::kNotFirst, "", "*xAB", "xAB", 0}, - { "*xAB", "xABxC",IsFirstReconcile::kNotFirst, "", "*xAB", "xABxC", 0}, - { "*xB", "", IsFirstReconcile::kNotFirst, "", "*xB", "", 0}, - { "A*B", "BA", IsFirstReconcile::kNotFirst, "", "A*B", "BA", 0}, - { "A*B", "AB", IsFirstReconcile::kNotFirst, "", "A*B", "AB", 0}, - { "A", "AxC", IsFirstReconcile::kNotFirst, "", "A", "AxC", 0}, - { "AB", "BxCA", IsFirstReconcile::kNotFirst, "", "AB", "BxCA", 0}, - { "B", "xABxC",IsFirstReconcile::kNotFirst, "", "B", "xABxC", 0}, - { "B", "xAxCB",IsFirstReconcile::kNotFirst, "", "B", "xAxCB", 0}, - { "*ABC", "ACB", IsFirstReconcile::kNotFirst, "", "*ABC", "ACB", 0}, - { "*ABC", "ABC", IsFirstReconcile::kNotFirst, "", "*ABC", "ABC", 0}, - { "BC", "BC", IsFirstReconcile::kNotFirst, "", "BC", "BC", 0}, - { "BC", "CB", IsFirstReconcile::kNotFirst, "", "BC", "CB", 0}, + { "", "", IsFirstReconcile::kNotFirst, "", "", "" }, + { "", "xA", IsFirstReconcile::kNotFirst, "", "", "xA" }, + { "", "xB", IsFirstReconcile::kNotFirst, "", "", "xB" }, + { "", "xAxB", IsFirstReconcile::kNotFirst, "", "", "xAxB" }, + { "", "xBxA", IsFirstReconcile::kNotFirst, "", "", "xBxA" }, + { "*A", "A", IsFirstReconcile::kNotFirst, "", "*A", "A" }, + { "*A", "xBA", IsFirstReconcile::kNotFirst, "", "*A", "xBA" }, + { "*A", "AxB", IsFirstReconcile::kNotFirst, "", "*A", "AxB" }, + { "A", "A", IsFirstReconcile::kNotFirst, "", "A", "A" }, + { "A", "xBA", IsFirstReconcile::kNotFirst, "", "A", "xBA" }, + { "A", "AxB", IsFirstReconcile::kNotFirst, "", "A", "AxB" }, + { "B", "B", IsFirstReconcile::kNotFirst, "", "B", "B" }, + { "B", "xAB", IsFirstReconcile::kNotFirst, "", "B", "xAB" }, + { "B", "BxA", IsFirstReconcile::kNotFirst, "", "B", "BxA" }, + { "*xA", "", IsFirstReconcile::kNotFirst, "", "*xA", "" }, + { "*xA", "xAxB", IsFirstReconcile::kNotFirst, "", "*xA", "xAxB" }, + { "*xA", "xBxA", IsFirstReconcile::kNotFirst, "", "*xA", "xBxA" }, + { "*xA", "xA", IsFirstReconcile::kNotFirst, "", "*xA", "xA" }, + { "*xA", "xB", IsFirstReconcile::kNotFirst, "", "*xA", "xB" }, + { "*xAB", "B", IsFirstReconcile::kNotFirst, "", "*xAB", "B" }, + { "*xAB", "BxA", IsFirstReconcile::kNotFirst, "", "*xAB", "BxA" }, + { "*xAB", "xAB", IsFirstReconcile::kNotFirst, "", "*xAB", "xAB" }, + { "*xAB", "xABxC",IsFirstReconcile::kNotFirst, "", "*xAB", "xABxC" }, + { "*xB", "", IsFirstReconcile::kNotFirst, "", "*xB", "" }, + { "A*B", "BA", IsFirstReconcile::kNotFirst, "", "A*B", "BA" }, + { "A*B", "AB", IsFirstReconcile::kNotFirst, "", "A*B", "AB" }, + { "A", "AxC", IsFirstReconcile::kNotFirst, "", "A", "AxC" }, + { "AB", "BxCA", IsFirstReconcile::kNotFirst, "", "AB", "BxCA" }, + { "B", "xABxC",IsFirstReconcile::kNotFirst, "", "B", "xABxC" }, + { "B", "xAxCB",IsFirstReconcile::kNotFirst, "", "B", "xAxCB" }, + { "*ABC", "ACB", IsFirstReconcile::kNotFirst, "", "*ABC", "ACB" }, + { "*ABC", "ABC", IsFirstReconcile::kNotFirst, "", "*ABC", "ABC" }, + { "BC", "BC", IsFirstReconcile::kNotFirst, "", "BC", "BC" }, + { "BC", "CB", IsFirstReconcile::kNotFirst, "", "BC", "CB" }, }; // clang-format on @@ -894,90 +995,8 @@ class AccountReconcilorTestDiceMultilogin : public AccountReconcilorTestTable { // Checks one row of the kDiceParams table above. TEST_P(AccountReconcilorTestDiceMultilogin, TableRowTest) { SetAccountConsistency(signin::AccountConsistencyMethod::kDice); - CheckReconcileIdempotent(kDiceParams, GetParam()); - - // Setup cookies. - std::vector<Cookie> cookies = ParseCookieString(GetParam().cookies); - ConfigureCookieManagerService(cookies); - std::vector<Cookie> cookies_after_reconcile = cookies; - - // Call list accounts now so that the next call completes synchronously. - identity_test_env()->identity_manager()->GetAccountsInCookieJar(); - base::RunLoop().RunUntilIdle(); - - // Setup tokens. This triggers listing cookies so we need to setup cookies - // before that. - SetupTokens(GetParam().tokens); - - // Setup expectations. - testing::InSequence mock_sequence; - bool should_logout = false; - if (GetParam().gaia_api_calls[0] != '\0') { - gaia::MultiloginMode mode = - GetParam().gaia_api_calls[0] == 'U' - ? gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER - : gaia::MultiloginMode::MULTILOGIN_PRESERVE_COOKIE_ACCOUNTS_ORDER; - // Generate expected array of accounts in cookies and set fake gaia - // response. - std::vector<CoreAccountId> accounts_to_send; - for (int i = 1; GetParam().gaia_api_calls[i] != '\0'; ++i) { - accounts_to_send.push_back( - CoreAccountId(accounts_[GetParam().gaia_api_calls[i]].gaia_id)); - } - const signin::MultiloginParameters params(mode, accounts_to_send); - cookies_after_reconcile = FakeSetAccountsInCookie(params, cookies); - should_logout = - accounts_to_send.empty() && - (mode == gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER); - if (should_logout) { - EXPECT_CALL(*GetMockReconcilor(), PerformLogoutAllAccountsAction()) - .Times(1); - } else { - EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)) - .Times(1); - } - } - // Reconcile. - AccountReconcilor* reconcilor = GetMockReconcilor(); - ASSERT_TRUE(reconcilor); - ASSERT_TRUE(reconcilor->first_execution_); - reconcilor->first_execution_ = - GetParam().is_first_reconcile == IsFirstReconcile::kFirst ? true : false; - reconcilor->StartReconcile(AccountReconcilor::Trigger::kCookieChange); - if (GetParam().gaia_api_calls[0] != '\0') { - if (should_logout) { - SimulateLogOutFromCookieCompleted( - reconcilor, GoogleServiceAuthError::AuthErrorNone()); - } else { - SimulateSetAccountsInCookieCompleted( - reconcilor, signin::SetAccountsInCookieResult::kSuccess); - } - } - - ASSERT_FALSE(reconcilor->is_reconcile_started_); - if (GetParam().tokens == GetParam().tokens_after_reconcile) { - EXPECT_EQ(signin_metrics::ACCOUNT_RECONCILOR_OK, reconcilor->GetState()); - } else { - // If the tokens were changed by the reconcile, a new reconcile should be - // scheduled. - EXPECT_EQ(signin_metrics::ACCOUNT_RECONCILOR_SCHEDULED, - reconcilor->GetState()); - } - VerifyCurrentTokens(ParseTokenString(GetParam().tokens_after_reconcile)); - - std::vector<Cookie> cookies_after = - ParseCookieString(GetParam().cookies_after_reconcile); - EXPECT_EQ(cookies_after, cookies_after_reconcile); - - testing::Mock::VerifyAndClearExpectations(GetMockReconcilor()); - - // Another reconcile is sometimes triggered if Chrome accounts have - // changed. Allow it to finish. - EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(testing::_)) - .WillRepeatedly(testing::Return()); - ConfigureCookieManagerService({}); - base::RunLoop().RunUntilIdle(); + RunRowTest(GetParam()); } INSTANTIATE_TEST_SUITE_P( @@ -1002,7 +1021,7 @@ TEST_F(AccountReconcilorDiceTest, DiceTokenServiceRegistration) { ASSERT_TRUE(reconcilor); ASSERT_TRUE(reconcilor->IsRegisteredWithIdentityManager()); - identity_test_env()->MakePrimaryAccountAvailable("user@gmail.com", + identity_test_env()->MakePrimaryAccountAvailable(kFakeEmail, signin::ConsentLevel::kSync); ASSERT_TRUE(reconcilor->IsRegisteredWithIdentityManager()); @@ -1023,13 +1042,13 @@ TEST_F(AccountReconcilorDiceTest, DiceReconcileWithoutSignin) { // before that. signin::SetListAccountsResponseNoAccounts(&test_url_loader_factory_); const CoreAccountId account_id = - identity_test_env()->MakeAccountAvailable("user@gmail.com").account_id; + identity_test_env()->MakeAccountAvailable(kFakeEmail).account_id; - std::vector<CoreAccountId> accounts_to_send = {account_id}; - const signin::MultiloginParameters params( - gaia::MultiloginMode::MULTILOGIN_PRESERVE_COOKIE_ACCOUNTS_ORDER, - accounts_to_send); - EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)); + std::vector<CoreAccountId> accounts_to_send = {account_id}; + const signin::MultiloginParameters params( + gaia::MultiloginMode::MULTILOGIN_PRESERVE_COOKIE_ACCOUNTS_ORDER, + accounts_to_send); + EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)); AccountReconcilor* reconcilor = GetMockReconcilor(); reconcilor->StartReconcile(AccountReconcilor::Trigger::kCookieChange); @@ -1063,15 +1082,15 @@ TEST_F(AccountReconcilorDiceTest, DiceReconcileNoop) { TEST_F(AccountReconcilorDiceTest, DiceReconcileReuseGaiaFirstAccount) { // Add account "other" to the Gaia cookie. signin::SetListAccountsResponseTwoAccounts( - "other@gmail.com", signin::GetTestGaiaIdForEmail("other@gmail.com"), - "foo@gmail.com", "9999", &test_url_loader_factory_); + kFakeEmail2, signin::GetTestGaiaIdForEmail(kFakeEmail2), "foo@gmail.com", + "9999", &test_url_loader_factory_); // Add accounts "user" and "other" to the token service. const AccountInfo account_info_1 = - identity_test_env()->MakeAccountAvailable("user@gmail.com"); + identity_test_env()->MakeAccountAvailable(kFakeEmail); const CoreAccountId account_id_1 = account_info_1.account_id; const AccountInfo account_info_2 = - identity_test_env()->MakeAccountAvailable("other@gmail.com"); + identity_test_env()->MakeAccountAvailable(kFakeEmail2); const CoreAccountId account_id_2 = account_info_2.account_id; auto* identity_manager = identity_test_env()->identity_manager(); @@ -1106,15 +1125,14 @@ TEST_F(AccountReconcilorDiceTest, DiceLastKnownFirstAccount) { // Making account available (setting a refresh token) triggers listing cookies // so we need to setup cookies before that. signin::SetListAccountsResponseTwoAccounts( - "other@gmail.com", signin::GetTestGaiaIdForEmail("other@gmail.com"), - "user@gmail.com", signin::GetTestGaiaIdForEmail("user@gmail.com"), - &test_url_loader_factory_); + kFakeEmail2, signin::GetTestGaiaIdForEmail(kFakeEmail2), kFakeEmail, + signin::GetTestGaiaIdForEmail(kFakeEmail), &test_url_loader_factory_); AccountInfo account_info_1 = - identity_test_env()->MakeAccountAvailable("user@gmail.com"); + identity_test_env()->MakeAccountAvailable(kFakeEmail); const CoreAccountId account_id_1 = account_info_1.account_id; AccountInfo account_info_2 = - identity_test_env()->MakeAccountAvailable("other@gmail.com"); + identity_test_env()->MakeAccountAvailable(kFakeEmail2); const CoreAccountId account_id_2 = account_info_2.account_id; auto* identity_manager = identity_test_env()->identity_manager(); @@ -1167,7 +1185,7 @@ TEST_F(AccountReconcilorDiceTest, DiceLastKnownFirstAccount) { TEST_F(AccountReconcilorDiceTest, UnverifiedAccountNoop) { // Add a unverified account to the Gaia cookie. signin::SetListAccountsResponseOneAccountWithParams( - {"user@gmail.com", "12345", true /* valid */, false /* signed_out */, + {kFakeEmail, kFakeGaiaId, true /* valid */, false /* signed_out */, false /* verified */}, &test_url_loader_factory_); @@ -1189,21 +1207,21 @@ TEST_F(AccountReconcilorDiceTest, UnverifiedAccountNoop) { TEST_F(AccountReconcilorDiceTest, UnverifiedAccountMerge) { // Add a unverified account to the Gaia cookie. signin::SetListAccountsResponseOneAccountWithParams( - {"user@gmail.com", "12345", true /* valid */, false /* signed_out */, + {kFakeEmail, kFakeGaiaId, true /* valid */, false /* signed_out */, false /* verified */}, &test_url_loader_factory_); // Add a token to Chrome. const CoreAccountId chrome_account_id = - identity_test_env()->MakeAccountAvailable("other@gmail.com").account_id; + identity_test_env()->MakeAccountAvailable(kFakeEmail2).account_id; - // In PRESERVE mode it is up to Gaia to not delete existing accounts in - // cookies and not sign out unveridied accounts. - std::vector<CoreAccountId> accounts_to_send = {chrome_account_id}; - const signin::MultiloginParameters params( - gaia::MultiloginMode::MULTILOGIN_PRESERVE_COOKIE_ACCOUNTS_ORDER, - accounts_to_send); - EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)); + // In PRESERVE mode it is up to Gaia to not delete existing accounts in + // cookies and not sign out unveridied accounts. + std::vector<CoreAccountId> accounts_to_send = {chrome_account_id}; + const signin::MultiloginParameters params( + gaia::MultiloginMode::MULTILOGIN_PRESERVE_COOKIE_ACCOUNTS_ORDER, + accounts_to_send); + EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)); AccountReconcilor* reconcilor = GetMockReconcilor(); reconcilor->StartReconcile(AccountReconcilor::Trigger::kCookieChange); @@ -1218,11 +1236,10 @@ TEST_F(AccountReconcilorDiceTest, UnverifiedAccountMerge) { TEST_F(AccountReconcilorDiceTest, DeleteCookie) { const CoreAccountId primary_account_id = identity_test_env() - ->MakePrimaryAccountAvailable("user@gmail.com", - signin::ConsentLevel::kSync) + ->MakePrimaryAccountAvailable(kFakeEmail, signin::ConsentLevel::kSync) .account_id; const CoreAccountId secondary_account_id = - identity_test_env()->MakeAccountAvailable("other@gmail.com").account_id; + identity_test_env()->MakeAccountAvailable(kFakeEmail2).account_id; auto* identity_manager = identity_test_env()->identity_manager(); ASSERT_TRUE(identity_manager->HasAccountWithRefreshToken(primary_account_id)); @@ -1295,33 +1312,44 @@ TEST_F(AccountReconcilorDiceTest, DeleteCookie) { const std::vector<AccountReconcilorTestTableParam> kMirrorParams = { // This table encodes the initial state and expectations of a reconcile. // See kDiceParams for documentation of the syntax. -// ------------------------------------------------------------------------- -// Tokens | Cookies | First Run | Gaia calls | Tokens after | Cookies after -// ------------------------------------------------------------------------- +// ----------------------------------------------------------------------------- +// Tokens | Cookies | First Run |Gaia calls|Tokens after| Cookies after +// ----------------------------------------------------------------------------- // First reconcile (Chrome restart): Rebuild the Gaia cookie to match the // tokens. Make the Sync account the default account in the Gaia cookie. // Sync enabled. { "*AB", "AB", IsFirstReconcile::kBoth, "", "*AB", "AB"}, -{ "*AB", "BA", IsFirstReconcile::kBoth, "U", "*AB", "AB"}, -{ "*AB", "A", IsFirstReconcile::kBoth, "U", "*AB", "AB"}, -{ "*AB", "B", IsFirstReconcile::kBoth, "U", "*AB", "AB"}, -{ "*AB", "", IsFirstReconcile::kBoth, "U", "*AB", "AB"}, +{ "*AB", "BA", IsFirstReconcile::kBoth, "UAB", "*AB", "AB"}, +{ "*AB", "A", IsFirstReconcile::kBoth, "UAB", "*AB", "AB"}, +{ "*AB", "B", IsFirstReconcile::kBoth, "UAB", "*AB", "AB"}, +{ "*AB", "", IsFirstReconcile::kBoth, "UAB", "*AB", "AB"}, // Sync enabled, token error on primary. // Sync enabled, token error on secondary. -{ "*AxB", "AB", IsFirstReconcile::kBoth, "U", "*AxB", "A"}, -{ "*AxB", "BA", IsFirstReconcile::kBoth, "U", "*AxB", "A"}, -{ "*AxB", "A", IsFirstReconcile::kBoth, "", "*AxB", ""}, -{ "*AxB", "B", IsFirstReconcile::kBoth, "U", "*AxB", "A"}, -{ "*AxB", "", IsFirstReconcile::kBoth, "U", "*AxB", "A"}, +{ "*AxB", "AB", IsFirstReconcile::kBoth, "UA", "*AxB", "A"}, +{ "*AxB", "BA", IsFirstReconcile::kBoth, "UA", "*AxB", "A"}, +{ "*AxB", "A", IsFirstReconcile::kBoth, "", "*AxB", "A"}, +{ "*AxB", "B", IsFirstReconcile::kBoth, "UA", "*AxB", "A"}, +{ "*AxB", "", IsFirstReconcile::kBoth, "UA", "*AxB", "A"}, // Cookies can be refreshed in pace, without logout. -{ "*AB", "xBxA", IsFirstReconcile::kBoth, "U", "*AB", "AB"}, +{ "*AB", "xBxA", IsFirstReconcile::kBoth, "UAB", "*AB", "AB"}, // Check that unknown Gaia accounts are signed out. -{ "*A", "AB", IsFirstReconcile::kBoth, "U", "*A", "A"}, +{ "*A", "AB", IsFirstReconcile::kBoth, "UA", "*A", "A"}, // Check that the previous case is idempotent. -{ "*A", "A", IsFirstReconcile::kBoth, "", "*A", ""}, +{ "*A", "A", IsFirstReconcile::kBoth, "", "*A", "A"}, + +// On Lacros, the reconcilor is enabled even if there is no account, or if the +// primary account is in error. +#if BUILDFLAG(IS_CHROMEOS_LACROS) +{ "", "", IsFirstReconcile::kBoth, "", "", ""}, +{ "*xA", "", IsFirstReconcile::kBoth, "", "*xA", ""}, +{ "*xAB", "", IsFirstReconcile::kBoth, "", "*xAB", ""}, +{ "", "A", IsFirstReconcile::kBoth, "X", "", ""}, +{ "*xA", "A", IsFirstReconcile::kBoth, "X", "*xA", ""}, +{ "*xAB", "AB", IsFirstReconcile::kBoth, "X", "*xAB", ""}, +#endif // BUILDFLAG(IS_CHROMEOS_LACROS) }; // clang-format on @@ -1338,79 +1366,12 @@ class AccountReconcilorTestMirrorMultilogin const AccountReconcilorTestMirrorMultilogin&) = delete; }; -// Checks one row of the kDiceParams table above. +// Checks one row of the kMirrorParams table above. TEST_P(AccountReconcilorTestMirrorMultilogin, TableRowTest) { // Enable Mirror. SetAccountConsistency(signin::AccountConsistencyMethod::kMirror); - - // Setup cookies. - std::vector<Cookie> cookies = ParseCookieString(GetParam().cookies); - ConfigureCookieManagerService(cookies); - - // Call list accounts now so that the next call completes synchronously. - identity_test_env()->identity_manager()->GetAccountsInCookieJar(); - base::RunLoop().RunUntilIdle(); - - // Setup tokens. - SetupTokens(GetParam().tokens); - - // Setup expectations. - testing::InSequence mock_sequence; - bool logout_action = false; - for (int i = 0; GetParam().gaia_api_calls[i] != '\0'; ++i) { - if (GetParam().gaia_api_calls[i] == 'X') { - logout_action = true; - EXPECT_CALL(*GetMockReconcilor(), PerformLogoutAllAccountsAction()) - .Times(1); - cookies.clear(); - continue; - } - if (GetParam().gaia_api_calls[i] == 'U') { - std::vector<CoreAccountId> accounts_to_send; - for (int j = 0; GetParam().cookies_after_reconcile[j] != '\0'; ++j) { - char cookie = GetParam().cookies_after_reconcile[j]; - std::string account_to_send = GaiaIdForAccountKey(cookie); - accounts_to_send.push_back(PickAccountIdForAccount( - account_to_send, - accounts_[GetParam().cookies_after_reconcile[j]].email)); - } - const signin::MultiloginParameters ml_params( - gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER, - accounts_to_send); - EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(ml_params)) - .Times(1); - } - } - if (!logout_action) { - EXPECT_CALL(*GetMockReconcilor(), PerformLogoutAllAccountsAction()) - .Times(0); - } - - // Reconcile. - AccountReconcilor* reconcilor = GetMockReconcilor(); - ASSERT_TRUE(reconcilor); - ASSERT_TRUE(reconcilor->first_execution_); - reconcilor->first_execution_ = - GetParam().is_first_reconcile == IsFirstReconcile::kFirst ? true : false; - reconcilor->StartReconcile(AccountReconcilor::Trigger::kCookieChange); - - SimulateSetAccountsInCookieCompleted( - reconcilor, signin::SetAccountsInCookieResult::kSuccess); - - ASSERT_FALSE(reconcilor->is_reconcile_started_); - ASSERT_EQ(signin_metrics::ACCOUNT_RECONCILOR_OK, reconcilor->GetState()); - VerifyCurrentTokens(ParseTokenString(GetParam().tokens_after_reconcile)); - - testing::Mock::VerifyAndClearExpectations(GetMockReconcilor()); - - // Another reconcile is sometimes triggered if Chrome accounts have - // changed. Allow it to finish. - EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(testing::_)) - .WillRepeatedly(testing::Return()); - EXPECT_CALL(*GetMockReconcilor(), PerformLogoutAllAccountsAction()) - .WillRepeatedly(testing::Return()); - ConfigureCookieManagerService({}); - base::RunLoop().RunUntilIdle(); + CheckReconcileIdempotent(kMirrorParams, GetParam()); + RunRowTest(GetParam()); } INSTANTIATE_TEST_SUITE_P( @@ -1432,6 +1393,12 @@ class AccountReconcilorTestActiveDirectory : public AccountReconcilorTestTable { SetAccountConsistency(signin::AccountConsistencyMethod::kMirror); } + void CreateReconclior() override { + DeleteReconcilor(); + CreateMockReconcilor( + std::make_unique<signin::ActiveDirectoryAccountReconcilorDelegate>()); + } + private: ash::ScopedStubInstallAttributes install_attributes_{ ash::StubInstallAttributes::CreateActiveDirectoryManaged("realm.com", @@ -1442,103 +1409,37 @@ class AccountReconcilorTestActiveDirectory : public AccountReconcilorTestTable { const std::vector<AccountReconcilorTestTableParam> kActiveDirectoryParams = { // This table encodes the initial state and expectations of a reconcile. // See kDiceParams for documentation of the syntax. -// ------------------------------------------------------------------------- -// Tokens |Cookies |First Run |Gaia calls|Tokens aft.|Cookies aft | -// ------------------------------------------------------------------------- -{ "ABC", "ABC", IsFirstReconcile::kBoth, "" , "ABC", "ABC" }, -{ "ABC", "", IsFirstReconcile::kBoth, "U", "ABC", "ABC" }, -{ "", "ABC", IsFirstReconcile::kBoth, "X", "", "", }, +// ----------------------------------------------------------------------------- +// Tokens |Cookies |First Run |Gaia calls|Tokens aft.|Cookies aft | +// ----------------------------------------------------------------------------- +{ "ABC", "ABC", IsFirstReconcile::kBoth, "" , "ABC", "ABC" }, +{ "ABC", "", IsFirstReconcile::kBoth, "UABC", "ABC", "ABC" }, +{ "", "ABC", IsFirstReconcile::kBoth, "X", "", "" }, +{ "", "", IsFirstReconcile::kBoth, "", "", "" }, // Order of Gaia accounts can be different from chrome accounts. -{ "ABC", "CBA", IsFirstReconcile::kBoth, "" , "ABC", "CBA" }, -{ "ABC", "CB", IsFirstReconcile::kBoth, "U", "ABC", "CBA" }, +{ "ABC", "CBA", IsFirstReconcile::kBoth, "" , "ABC", "CBA" }, +{ "ABC", "CB", IsFirstReconcile::kBoth, "UCBA", "ABC", "CBA" }, // Gaia accounts which are not present in chrome accounts should be removed. In // this case Gaia accounts are going to be in the same order as chrome accounts. -// this case Gaia accounts are going to be in thcousame order as chromcnts. -{ "A", "AB", IsFirstReconcile::kBoth, "U", "A", "A" }, -{ "AB", "CBA", IsFirstReconcile::kBoth, "U", "AB", "AB" }, -{ "AB", "C", IsFirstReconcile::kBoth, "U", "AB", "AB" }, +{ "A", "AB", IsFirstReconcile::kBoth, "UA", "A", "A" }, +{ "AB", "CBA", IsFirstReconcile::kBoth, "UAB", "AB", "AB" }, +{ "AB", "C", IsFirstReconcile::kBoth, "UAB", "AB", "AB" }, // Cookies can be refreshed in pace, without logout. -{ "AB", "xAxB", IsFirstReconcile::kBoth, "U", "AB", "AB" }, +{ "AB", "xAxB", IsFirstReconcile::kBoth, "UAB", "AB", "AB" }, // Token error on the account - remove it from cookies -{ "AxB", "AB", IsFirstReconcile::kBoth, "U", "AxB", "A" }, -{ "xAxB", "AB", IsFirstReconcile::kBoth, "X", "xAxB", "" }, +{ "AxB", "AB", IsFirstReconcile::kBoth, "UA", "AxB", "A" }, +{ "xAxB", "AB", IsFirstReconcile::kBoth, "X", "xAxB", "" }, +// For idempotency checks. +{ "A", "A", IsFirstReconcile::kBoth, "", "A", "A" }, +{ "AB", "AB", IsFirstReconcile::kBoth, "", "AB", "AB" }, +{ "AxB", "A", IsFirstReconcile::kBoth, "", "AxB", "A" }, +{ "xAxB", "", IsFirstReconcile::kBoth, "", "xAxB", "" }, }; // clang-format on TEST_P(AccountReconcilorTestActiveDirectory, TableRowTestMultilogin) { - // Setup cookies. - std::vector<Cookie> cookies = ParseCookieString(GetParam().cookies); - ConfigureCookieManagerService(cookies); - - // Call list accounts now so that the next call completes synchronously. - identity_test_env()->identity_manager()->GetAccountsInCookieJar(); - base::RunLoop().RunUntilIdle(); - - // Setup tokens. - std::vector<Token> tokens = ParseTokenString(GetParam().tokens); - SetupTokens(GetParam().tokens); - - testing::InSequence mock_sequence; - DeleteReconcilor(); - MockAccountReconcilor* reconcilor = CreateMockReconcilor( - std::make_unique<signin::ActiveDirectoryAccountReconcilorDelegate>()); - - // Setup expectations. - bool should_logout; - if (GetParam().gaia_api_calls[0] != '\0') { - if (GetParam().gaia_api_calls[0] == 'X') { - should_logout = true; - EXPECT_CALL(*reconcilor, PerformLogoutAllAccountsAction()).Times(1); - EXPECT_CALL(*reconcilor, PerformSetCookiesAction(_)).Times(0); - cookies.clear(); - } else if (GetParam().gaia_api_calls[0] == 'U') { - should_logout = false; - std::vector<CoreAccountId> accounts_to_send; - for (int i = 0; GetParam().cookies_after_reconcile[i] != '\0'; ++i) { - char cookie = GetParam().cookies_after_reconcile[i]; - std::string account_to_send = GaiaIdForAccountKey(cookie); - accounts_to_send.push_back(PickAccountIdForAccount( - account_to_send, - accounts_[GetParam().cookies_after_reconcile[i]].email)); - } - const signin::MultiloginParameters params( - gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER, - accounts_to_send); - EXPECT_CALL(*reconcilor, PerformSetCookiesAction(params)).Times(1); - EXPECT_CALL(*reconcilor, PerformLogoutAllAccountsAction()).Times(0); - } - } - - // Reconcile. - ASSERT_TRUE(reconcilor); - ASSERT_TRUE(reconcilor->first_execution_); - reconcilor->first_execution_ = - GetParam().is_first_reconcile == IsFirstReconcile::kFirst ? true : false; - reconcilor->StartReconcile(AccountReconcilor::Trigger::kCookieChange); - if (GetParam().gaia_api_calls[0] != '\0') { - if (should_logout) { - SimulateLogOutFromCookieCompleted( - reconcilor, GoogleServiceAuthError::AuthErrorNone()); - } else { - SimulateSetAccountsInCookieCompleted( - reconcilor, signin::SetAccountsInCookieResult::kSuccess); - } - } - - ASSERT_FALSE(reconcilor->is_reconcile_started_); - ASSERT_EQ(signin_metrics::ACCOUNT_RECONCILOR_OK, reconcilor->GetState()); - VerifyCurrentTokens(ParseTokenString(GetParam().tokens_after_reconcile)); - - testing::Mock::VerifyAndClearExpectations(reconcilor); - - // Another reconcile is sometimes triggered if Chrome accounts have - // changed. Allow it to finish. - EXPECT_CALL(*reconcilor, PerformSetCookiesAction(testing::_)) - .WillRepeatedly(testing::Return()); - EXPECT_CALL(*reconcilor, PerformLogoutAllAccountsAction()) - .WillRepeatedly(testing::Return()); - ConfigureCookieManagerService({}); - base::RunLoop().RunUntilIdle(); + CheckReconcileIdempotent(kActiveDirectoryParams, GetParam()); + RunRowTest(GetParam()); } INSTANTIATE_TEST_SUITE_P( @@ -1551,7 +1452,7 @@ INSTANTIATE_TEST_SUITE_P( // automatically started when tokens are loaded. TEST_F(AccountReconcilorMirrorTest, TokensNotLoaded) { const CoreAccountId account_id = - ConnectProfileToAccount("user@gmail.com").account_id; + ConnectProfileToAccount(kFakeEmail).account_id; signin::SetListAccountsResponseNoAccounts(&test_url_loader_factory_); identity_test_env()->ResetToAccountsNotYetLoadedFromDiskState(); @@ -1581,7 +1482,7 @@ TEST_F(AccountReconcilorMirrorTest, TokensNotLoaded) { } TEST_F(AccountReconcilorMirrorTest, GetAccountsFromCookieSuccess) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); const CoreAccountId account_id = account_info.account_id; signin::SetListAccountsResponseOneAccountWithParams( {account_info.email, account_info.gaia, false /* valid */, @@ -1615,7 +1516,7 @@ TEST_F(AccountReconcilorMirrorTest, GetAccountsFromCookieSuccess) { // Checks that calling EnableReconcile() while the reconcilor is already running // doesn't have any effect. Regression test for https://crbug.com/1043651 TEST_F(AccountReconcilorMirrorTest, EnableReconcileWhileAlreadyRunning) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); const CoreAccountId account_id = account_info.account_id; signin::SetListAccountsResponseOneAccountWithParams( {account_info.email, account_info.gaia, false /* valid */, @@ -1649,7 +1550,7 @@ TEST_F(AccountReconcilorMirrorTest, EnableReconcileWhileAlreadyRunning) { } TEST_F(AccountReconcilorMirrorTest, GetAccountsFromCookieFailure) { - ConnectProfileToAccount("user@gmail.com"); + ConnectProfileToAccount(kFakeEmail); signin::SetListAccountsResponseWithUnexpectedServiceResponse( &test_url_loader_factory_); @@ -1675,7 +1576,7 @@ TEST_F(AccountReconcilorMirrorTest, GetAccountsFromCookieFailure) { // Regression test for https://crbug.com/923716 TEST_F(AccountReconcilorMirrorTest, ExtraCookieChangeNotification) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); const CoreAccountId account_id = account_info.account_id; signin::CookieParams cookie_params = { account_info.email, account_info.gaia, false /* valid */, @@ -1716,7 +1617,7 @@ TEST_F(AccountReconcilorMirrorTest, ExtraCookieChangeNotification) { } TEST_F(AccountReconcilorMirrorTest, StartReconcileNoop) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); AccountReconcilor* reconcilor = GetMockReconcilor(); ASSERT_TRUE(reconcilor); @@ -1740,7 +1641,7 @@ TEST_F(AccountReconcilorMirrorTest, StartReconcileNoop) { TEST_F(AccountReconcilorMirrorTest, StartReconcileCookiesDisabled) { const CoreAccountId account_id = - ConnectProfileToAccount("user@gmail.com").account_id; + ConnectProfileToAccount(kFakeEmail).account_id; identity_test_env()->SetRefreshTokenForAccount(account_id); test_signin_client()->set_are_signin_cookies_allowed(false); @@ -1761,7 +1662,7 @@ TEST_F(AccountReconcilorMirrorTest, StartReconcileCookiesDisabled) { TEST_F(AccountReconcilorMirrorTest, StartReconcileContentSettings) { const CoreAccountId account_id = - ConnectProfileToAccount("user@gmail.com").account_id; + ConnectProfileToAccount(kFakeEmail).account_id; identity_test_env()->SetRefreshTokenForAccount(account_id); AccountReconcilor* reconcilor = GetMockReconcilor(); @@ -1782,7 +1683,7 @@ TEST_F(AccountReconcilorMirrorTest, StartReconcileContentSettings) { TEST_F(AccountReconcilorMirrorTest, StartReconcileContentSettingsGaiaUrl) { const CoreAccountId account_id = - ConnectProfileToAccount("user@gmail.com").account_id; + ConnectProfileToAccount(kFakeEmail).account_id; identity_test_env()->SetRefreshTokenForAccount(account_id); AccountReconcilor* reconcilor = GetMockReconcilor(); @@ -1796,7 +1697,7 @@ TEST_F(AccountReconcilorMirrorTest, StartReconcileContentSettingsGaiaUrl) { TEST_F(AccountReconcilorMirrorTest, StartReconcileContentSettingsNonGaiaUrl) { const CoreAccountId account_id = - ConnectProfileToAccount("user@gmail.com").account_id; + ConnectProfileToAccount(kFakeEmail).account_id; identity_test_env()->SetRefreshTokenForAccount(account_id); AccountReconcilor* reconcilor = GetMockReconcilor(); @@ -1811,7 +1712,7 @@ TEST_F(AccountReconcilorMirrorTest, StartReconcileContentSettingsNonGaiaUrl) { TEST_F(AccountReconcilorMirrorTest, StartReconcileContentSettingsWildcardPattern) { const CoreAccountId account_id = - ConnectProfileToAccount("user@gmail.com").account_id; + ConnectProfileToAccount(kFakeEmail).account_id; identity_test_env()->SetRefreshTokenForAccount(account_id); AccountReconcilor* reconcilor = GetMockReconcilor(); @@ -1849,9 +1750,9 @@ TEST_F(AccountReconcilorMirrorTest, StartReconcileNoopWithDots) { #endif TEST_F(AccountReconcilorMirrorTest, StartReconcileNoopMultiple) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); AccountInfo account_info_2 = - identity_test_env()->MakeAccountAvailable("other@gmail.com"); + identity_test_env()->MakeAccountAvailable(kFakeEmail2); signin::SetListAccountsResponseTwoAccounts( account_info.email, account_info.gaia, account_info_2.email, account_info_2.gaia, &test_url_loader_factory_); @@ -1865,20 +1766,20 @@ TEST_F(AccountReconcilorMirrorTest, StartReconcileNoopMultiple) { } TEST_F(AccountReconcilorMirrorTest, StartReconcileAddToCookie) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); const CoreAccountId account_id = account_info.account_id; identity_test_env()->SetRefreshTokenForAccount(account_id); signin::SetListAccountsResponseOneAccount( account_info.email, account_info.gaia, &test_url_loader_factory_); const CoreAccountId account_id2 = - identity_test_env()->MakeAccountAvailable("other@gmail.com").account_id; + identity_test_env()->MakeAccountAvailable(kFakeEmail2).account_id; - std::vector<CoreAccountId> accounts_to_send = {account_id, account_id2}; - const signin::MultiloginParameters params( - gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER, - accounts_to_send); - EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)); + std::vector<CoreAccountId> accounts_to_send = {account_id, account_id2}; + const signin::MultiloginParameters params( + gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER, + accounts_to_send); + EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)); AccountReconcilor* reconcilor = GetMockReconcilor(); reconcilor->StartReconcile(AccountReconcilor::Trigger::kCookieChange); @@ -1934,7 +1835,7 @@ TEST_F(AccountReconcilorTest, AuthErrorTriggersListAccount) { #endif // Add one account to Chrome and instantiate the reconcilor. - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); const CoreAccountId account_id = account_info.account_id; identity_test_env()->SetRefreshTokenForAccount(account_id); TestGaiaCookieObserver observer; @@ -1947,7 +1848,7 @@ TEST_F(AccountReconcilorTest, AuthErrorTriggersListAccount) { bool expect_logout = #if BUILDFLAG(IS_CHROMEOS_LACROS) - base::FeatureList::IsEnabled(switches::kLacrosNonSyncingProfiles); + true; #else account_consistency == signin::AccountConsistencyMethod::kDice; #endif @@ -1977,20 +1878,20 @@ TEST_F(AccountReconcilorTest, AuthErrorTriggersListAccount) { // which is not a flow that exists on ChromeOS. TEST_F(AccountReconcilorMirrorTest, SignoutAfterErrorDoesNotRecordUma) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); const CoreAccountId account_id = account_info.account_id; identity_test_env()->SetRefreshTokenForAccount(account_id); signin::SetListAccountsResponseOneAccount( account_info.email, account_info.gaia, &test_url_loader_factory_); const CoreAccountId account_id2 = - identity_test_env()->MakeAccountAvailable("other@gmail.com").account_id; + identity_test_env()->MakeAccountAvailable(kFakeEmail2).account_id; - std::vector<CoreAccountId> accounts_to_send = {account_id, account_id2}; - const signin::MultiloginParameters params( - gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER, - accounts_to_send); - EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)); + std::vector<CoreAccountId> accounts_to_send = {account_id, account_id2}; + const signin::MultiloginParameters params( + gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER, + accounts_to_send); + EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)); AccountReconcilor* reconcilor = GetMockReconcilor(); reconcilor->StartReconcile(AccountReconcilor::Trigger::kCookieChange); @@ -2013,18 +1914,18 @@ TEST_F(AccountReconcilorMirrorTest, SignoutAfterErrorDoesNotRecordUma) { #endif // !BUILDFLAG(IS_CHROMEOS_ASH) TEST_F(AccountReconcilorMirrorTest, StartReconcileRemoveFromCookie) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); const CoreAccountId account_id = account_info.account_id; identity_test_env()->SetRefreshTokenForAccount(account_id); signin::SetListAccountsResponseTwoAccounts( - account_info.email, account_info.gaia, "other@gmail.com", "12345", + account_info.email, account_info.gaia, kFakeEmail2, kFakeGaiaId, &test_url_loader_factory_); - std::vector<CoreAccountId> accounts_to_send = {account_id}; - const signin::MultiloginParameters params( - gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER, - accounts_to_send); - EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)); + std::vector<CoreAccountId> accounts_to_send = {account_id}; + const signin::MultiloginParameters params( + gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER, + accounts_to_send); + EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)); AccountReconcilor* reconcilor = GetMockReconcilor(); reconcilor->StartReconcile(AccountReconcilor::Trigger::kCookieChange); @@ -2041,39 +1942,35 @@ TEST_F(AccountReconcilorMirrorTest, StartReconcileRemoveFromCookie) { // Check that token error on primary account results in a logout to all accounts // on Lacros. For other mirror platforms, reconcile is aborted. TEST_F(AccountReconcilorMirrorTest, TokenErrorOnPrimary) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); signin::UpdatePersistentErrorOfRefreshTokenForAccount( identity_test_env()->identity_manager(), account_info.account_id, GoogleServiceAuthError(GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS)); #if BUILDFLAG(IS_CHROMEOS_LACROS) - if (base::FeatureList::IsEnabled(switches::kLacrosNonSyncingProfiles)) { - EXPECT_CALL(*GetMockReconcilor(), PerformLogoutAllAccountsAction()); - } + EXPECT_CALL(*GetMockReconcilor(), PerformLogoutAllAccountsAction()); #endif AccountReconcilor* reconcilor = GetMockReconcilor(); signin::SetListAccountsResponseTwoAccounts( - account_info.email, account_info.gaia, "other@gmail.com", "67890", + account_info.email, account_info.gaia, kFakeEmail2, "67890", &test_url_loader_factory_); reconcilor->StartReconcile(AccountReconcilor::Trigger::kCookieChange); base::RunLoop().RunUntilIdle(); #if BUILDFLAG(IS_CHROMEOS_LACROS) - if (base::FeatureList::IsEnabled(switches::kLacrosNonSyncingProfiles)) { - ASSERT_TRUE(reconcilor->is_reconcile_started_); - SimulateLogOutFromCookieCompleted(reconcilor, - GoogleServiceAuthError::AuthErrorNone()); - testing::Mock::VerifyAndClearExpectations(GetMockReconcilor()); - base::RunLoop().RunUntilIdle(); - } + ASSERT_TRUE(reconcilor->is_reconcile_started_); + SimulateLogOutFromCookieCompleted(reconcilor, + GoogleServiceAuthError::AuthErrorNone()); + testing::Mock::VerifyAndClearExpectations(GetMockReconcilor()); + base::RunLoop().RunUntilIdle(); #endif ASSERT_FALSE(reconcilor->is_reconcile_started_); } TEST_F(AccountReconcilorMirrorTest, StartReconcileAddToCookieTwice) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); const CoreAccountId account_id = account_info.account_id; AccountInfo account_info2 = - identity_test_env()->MakeAccountAvailable("other@gmail.com"); + identity_test_env()->MakeAccountAvailable(kFakeEmail2); const CoreAccountId account_id2 = account_info2.account_id; const std::string email3 = "third@gmail.com"; @@ -2126,11 +2023,11 @@ TEST_F(AccountReconcilorMirrorTest, StartReconcileAddToCookieTwice) { } TEST_F(AccountReconcilorMirrorTest, StartReconcileBadPrimary) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); const CoreAccountId account_id = account_info.account_id; AccountInfo account_info2 = - identity_test_env()->MakeAccountAvailable("other@gmail.com"); + identity_test_env()->MakeAccountAvailable(kFakeEmail2); const CoreAccountId account_id2 = account_info2.account_id; signin::SetListAccountsResponseTwoAccounts( account_info2.email, account_info2.gaia, account_info.email, @@ -2155,7 +2052,7 @@ TEST_F(AccountReconcilorMirrorTest, StartReconcileBadPrimary) { } TEST_F(AccountReconcilorMirrorTest, StartReconcileOnlyOnce) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); signin::SetListAccountsResponseOneAccount( account_info.email, account_info.gaia, &test_url_loader_factory_); @@ -2171,7 +2068,7 @@ TEST_F(AccountReconcilorMirrorTest, StartReconcileOnlyOnce) { } TEST_F(AccountReconcilorMirrorTest, Lock) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); signin::SetListAccountsResponseOneAccount( account_info.email, account_info.gaia, &test_url_loader_factory_); @@ -2246,16 +2143,134 @@ TEST_F(AccountReconcilorMirrorTest, Lock) { EXPECT_FALSE(reconcilor->is_reconcile_started_); } +#if BUILDFLAG(ENABLE_MIRROR) +TEST_F(AccountReconcilorTest, ForceReconcileEarlyExitsForInactiveReconcilor) { + AccountReconcilor* reconcilor = GetMockReconcilor(); + ASSERT_TRUE(reconcilor); + ASSERT_EQ(signin_metrics::ACCOUNT_RECONCILOR_INACTIVE, + reconcilor->GetState()); + + reconcilor->ForceReconcile(); + EXPECT_EQ(signin_metrics::ACCOUNT_RECONCILOR_INACTIVE, + reconcilor->GetState()); + EXPECT_FALSE(reconcilor->is_reconcile_started_); +} + +TEST_F(AccountReconcilorMirrorTest, + ForceReconcileImmediatelyStartsForIdleReconcilor) { + // Get the reconcilor to an OK (signin_metrics::ACCOUNT_RECONCILOR_OK) state. + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); + AccountReconcilor* reconcilor = GetMockReconcilor(); + ASSERT_TRUE(reconcilor); + signin::SetListAccountsResponseOneAccount( + account_info.email, account_info.gaia, &test_url_loader_factory_); + std::vector<CoreAccountId> accounts_to_send = {account_info.account_id}; + const signin::MultiloginParameters params( + gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER, + accounts_to_send); + EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)); + reconcilor->SetState(signin_metrics::ACCOUNT_RECONCILOR_OK); + ASSERT_FALSE(reconcilor->is_reconcile_started_); + + // Now try to force a reconcile. + reconcilor->ForceReconcile(); + base::RunLoop().RunUntilIdle(); + EXPECT_EQ(signin_metrics::ACCOUNT_RECONCILOR_RUNNING, reconcilor->GetState()); + EXPECT_TRUE(reconcilor->is_reconcile_started_); +} + +TEST_F(AccountReconcilorMirrorTest, + ForceReconcileImmediatelyStartsForErroredOutReconcilor) { + // Get the reconcilor to an error state. + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); + AccountReconcilor* reconcilor = GetMockReconcilor(); + ASSERT_TRUE(reconcilor); + signin::SetListAccountsResponseOneAccount( + account_info.email, account_info.gaia, &test_url_loader_factory_); + std::vector<CoreAccountId> accounts_to_send = {account_info.account_id}; + const signin::MultiloginParameters params( + gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER, + accounts_to_send); + EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)); + reconcilor->SetState(signin_metrics::ACCOUNT_RECONCILOR_ERROR); + ASSERT_FALSE(reconcilor->is_reconcile_started_); + + // Now try to force a reconcile. + reconcilor->ForceReconcile(); + base::RunLoop().RunUntilIdle(); + EXPECT_EQ(signin_metrics::ACCOUNT_RECONCILOR_RUNNING, reconcilor->GetState()); + EXPECT_TRUE(reconcilor->is_reconcile_started_); +} + +TEST_F(AccountReconcilorMirrorTest, + ForceReconcileSchedulesReconciliationIfReconcilorIsAlreadyRunning) { + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); + identity_test_env()->WaitForRefreshTokensLoaded(); + const CoreAccountId account_id = account_info.account_id; + + // Do NOT set a ListAccounts response. We do not want reconciliation to finish + // immediately. + std::vector<CoreAccountId> accounts_to_send = {account_id}; + const signin::MultiloginParameters params( + gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER, + accounts_to_send); + EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)); + + AccountReconcilor* reconcilor = GetMockReconcilor(); + ASSERT_TRUE(reconcilor); + + // Schedule a regular reconciliation cycle. This will eventually end up in a + // noop because the accounts in cookie match the Primary Account in Chrome. + reconcilor->StartReconcile(AccountReconcilor::Trigger::kInitialized); + ASSERT_EQ(signin_metrics::ACCOUNT_RECONCILOR_RUNNING, reconcilor->GetState()); + ASSERT_TRUE(reconcilor->is_reconcile_started_); + + // Immediately force a reconciliation. This should cause a forced + // reconciliation to be tried later in + // `kForcedReconciliationWaitTimeInSeconds` seconds. + reconcilor->ForceReconcile(); + + // Now set the account in cookie as the Primary Account in Chrome. This will + // unblock the regular (`AccountReconcilor::Trigger::kInitialized`) + // reconciliation cycle. + signin::SetListAccountsResponseOneAccount( + /*email=*/account_info.email, /*gaia_id=*/account_info.gaia, + /*test_url_loader_factory=*/&test_url_loader_factory_); + // This forced reconciliation attempt should also be blocked since + // test_url_loader_factory_ will itself post a task to wake up pending + // requests. + task_environment()->FastForwardBy( + base::Seconds(kForcedReconciliationWaitTimeInSeconds)); + base::RunLoop().RunUntilIdle(); + + // Give the queued forced reconciliation cycle a chance to actually run. + task_environment()->FastForwardBy( + base::Seconds(kForcedReconciliationWaitTimeInSeconds)); + base::RunLoop().RunUntilIdle(); + + // Indirectly test through histograms that the forced reconciliation cycle was + // actually run. + histogram_tester()->ExpectBucketCount( + AccountReconcilor::kOperationHistogramName, + AccountReconcilor::Operation::kMultilogin, 1); + histogram_tester()->ExpectUniqueSample( + AccountReconcilor::kTriggerMultiloginHistogramName, + AccountReconcilor::Trigger::kForcedReconcile, 1); + histogram_tester()->ExpectTotalCount( + AccountReconcilor::kTriggerMultiloginHistogramName, 1); +} +#endif // BUILDFLAG(ENABLE_MIRROR) + // Checks that an "invalid" Gaia account can be refreshed in place, without // performing a full logout. TEST_P(AccountReconcilorMethodParamTest, StartReconcileWithSessionInfoExpiredDefault) { signin::AccountConsistencyMethod account_consistency = GetParam(); SetAccountConsistency(account_consistency); - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); const CoreAccountId account_id = account_info.account_id; AccountInfo account_info2 = - identity_test_env()->MakeAccountAvailable("other@gmail.com"); + identity_test_env()->MakeAccountAvailable(kFakeEmail2); const CoreAccountId account_id2 = account_info2.account_id; signin::SetListAccountsResponseWithParams( {{account_info.email, account_info.gaia, false /* valid */, @@ -2299,7 +2314,7 @@ TEST_P(AccountReconcilorMethodParamTest, TEST_F(AccountReconcilorMirrorTest, AddAccountToCookieCompletedWithBogusAccount) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); const CoreAccountId account_id = account_info.account_id; signin::SetListAccountsResponseOneAccountWithParams( {account_info.email, account_info.gaia, false /* valid */, @@ -2332,10 +2347,10 @@ TEST_F(AccountReconcilorMirrorTest, TEST_F(AccountReconcilorMirrorTest, NoLoopWithBadPrimary) { // Connect profile to a primary account and then add a secondary account. - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); const CoreAccountId account_id1 = account_info.account_id; AccountInfo account_info2 = - identity_test_env()->MakeAccountAvailable("other@gmail.com"); + identity_test_env()->MakeAccountAvailable(kFakeEmail2); const CoreAccountId account_id2 = account_info2.account_id; std::vector<CoreAccountId> accounts_to_send = {account_id1, account_id2}; @@ -2384,9 +2399,9 @@ TEST_F(AccountReconcilorMirrorTest, NoLoopWithBadPrimary) { TEST_F(AccountReconcilorMirrorTest, WontMergeAccountsWithError) { // Connect profile to a primary account and then add a secondary account. const CoreAccountId account_id1 = - ConnectProfileToAccount("user@gmail.com").account_id; + ConnectProfileToAccount(kFakeEmail).account_id; const CoreAccountId account_id2 = - identity_test_env()->MakeAccountAvailable("other@gmail.com").account_id; + identity_test_env()->MakeAccountAvailable(kFakeEmail2).account_id; // Mark the secondary account in auth error state. signin::UpdatePersistentErrorOfRefreshTokenForAccount( @@ -2423,7 +2438,7 @@ TEST_F(AccountReconcilorMirrorTest, WontMergeAccountsWithError) { // Test that delegate timeout is called when the delegate offers a valid // timeout. TEST_F(AccountReconcilorTest, DelegateTimeoutIsCalled) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); auto spy_delegate0 = std::make_unique<SpyReconcilorDelegate>(); SpyReconcilorDelegate* spy_delegate = spy_delegate0.get(); AccountReconcilor* reconcilor = @@ -2447,7 +2462,7 @@ TEST_F(AccountReconcilorTest, DelegateTimeoutIsCalled) { // Test that delegate timeout is not called when the delegate does not offer a // valid timeout. TEST_F(AccountReconcilorMirrorTest, DelegateTimeoutIsNotCalled) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); signin::SetListAccountsResponseOneAccount( account_info.email, account_info.gaia, &test_url_loader_factory_); AccountReconcilor* reconcilor = GetMockReconcilor(); @@ -2462,7 +2477,7 @@ TEST_F(AccountReconcilorMirrorTest, DelegateTimeoutIsNotCalled) { } TEST_F(AccountReconcilorTest, DelegateTimeoutIsNotCalledIfTimeoutIsNotReached) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); signin::SetListAccountsResponseOneAccount( account_info.email, account_info.gaia, &test_url_loader_factory_); auto spy_delegate0 = std::make_unique<SpyReconcilorDelegate>(); @@ -2485,6 +2500,90 @@ TEST_F(AccountReconcilorTest, DelegateTimeoutIsNotCalledIfTimeoutIsNotReached) { EXPECT_FALSE(reconcilor->is_reconcile_started_); } +TEST_F(AccountReconcilorTest, ForcedReconcileTriggerShouldNotCallListAccounts) { +#if BUILDFLAG(ENABLE_DICE_SUPPORT) + signin::AccountConsistencyMethod account_consistency = + signin::AccountConsistencyMethod::kDice; + SetAccountConsistency(account_consistency); + gaia::MultiloginMode multilogin_mode = + gaia::MultiloginMode::MULTILOGIN_PRESERVE_COOKIE_ACCOUNTS_ORDER; +#else + signin::AccountConsistencyMethod account_consistency = + signin::AccountConsistencyMethod::kMirror; + SetAccountConsistency(account_consistency); + gaia::MultiloginMode multilogin_mode = + gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER; +#endif + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); + identity_test_env()->WaitForRefreshTokensLoaded(); + const CoreAccountId account_id = account_info.account_id; + + // Do not set a ListAccounts response, but still expect multilogin to be + // called. + std::vector<CoreAccountId> accounts_to_send = {account_id}; + const signin::MultiloginParameters params(multilogin_mode, accounts_to_send); + EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)); + + AccountReconcilor* reconcilor = GetMockReconcilor(); + ASSERT_TRUE(reconcilor); + + reconcilor->StartReconcile(AccountReconcilor::Trigger::kForcedReconcile); + ASSERT_EQ(signin_metrics::ACCOUNT_RECONCILOR_RUNNING, reconcilor->GetState()); + base::RunLoop().RunUntilIdle(); +} + +// Forced account reconciliation +// (`AccountReconcilor::Trigger::kForcedReconcile`) should not result in a noop +// - even if ListAccounts claims to have the same set of accounts as Chrome. +TEST_F(AccountReconcilorTest, ForcedReconcileTriggerShouldNotResultInNoop) { +#if BUILDFLAG(ENABLE_DICE_SUPPORT) + signin::AccountConsistencyMethod account_consistency = + signin::AccountConsistencyMethod::kDice; + SetAccountConsistency(account_consistency); + gaia::MultiloginMode multilogin_mode = + gaia::MultiloginMode::MULTILOGIN_PRESERVE_COOKIE_ACCOUNTS_ORDER; +#else + signin::AccountConsistencyMethod account_consistency = + signin::AccountConsistencyMethod::kMirror; + SetAccountConsistency(account_consistency); + gaia::MultiloginMode multilogin_mode = + gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER; +#endif + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); + identity_test_env()->WaitForRefreshTokensLoaded(); + const CoreAccountId account_id = account_info.account_id; + + // Set a ListAccounts response to match the Primary Account in Chrome. + signin::SetListAccountsResponseOneAccount( + /*email=*/account_info.email, /*gaia_id=*/account_info.gaia, + /*test_url_loader_factory=*/&test_url_loader_factory_); + std::vector<CoreAccountId> accounts_to_send = {account_id}; + const signin::MultiloginParameters params(multilogin_mode, accounts_to_send); + // `PerformSetCookiesAction()` should be called, despite the cookie jar having + // the same account(s) as Chrome. + EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(params)); + + AccountReconcilor* reconcilor = GetMockReconcilor(); + ASSERT_TRUE(reconcilor); + + reconcilor->StartReconcile(AccountReconcilor::Trigger::kForcedReconcile); + ASSERT_EQ(signin_metrics::ACCOUNT_RECONCILOR_RUNNING, reconcilor->GetState()); + base::RunLoop().RunUntilIdle(); + + // Check the reported histograms. Noop bucket should not have a sample. + // Multilogin bucket should have a sample. + histogram_tester()->ExpectUniqueSample( + AccountReconcilor::kOperationHistogramName, + AccountReconcilor::Operation::kMultilogin, 1); + histogram_tester()->ExpectUniqueSample( + AccountReconcilor::kTriggerMultiloginHistogramName, + AccountReconcilor::Trigger::kForcedReconcile, 1); + histogram_tester()->ExpectTotalCount( + AccountReconcilor::kTriggerNoopHistogramName, 0); + histogram_tester()->ExpectTotalCount( + AccountReconcilor::kTriggerMultiloginHistogramName, 1); +} + TEST_F(AccountReconcilorTest, ScopedSyncedDataDeletionDestructionOrder) { AccountReconcilor* reconcilor = GetMockReconcilor(); std::unique_ptr<AccountReconcilor::ScopedSyncedDataDeletion> data_deletion = @@ -2530,7 +2629,7 @@ TEST_F(AccountReconcilorTest, MultiloginLogout) { MockAccountReconcilor* reconcilor = CreateMockReconcilor(std::make_unique<MultiloginLogoutDelegate>()); - signin::SetListAccountsResponseOneAccount("user@gmail.com", "123456", + signin::SetListAccountsResponseOneAccount(kFakeEmail, "123456", &test_url_loader_factory_); // Logout call to Gaia. @@ -2650,10 +2749,10 @@ class AccountReconcilorThrottlerTest : public AccountReconcilorTest { }; TEST_F(AccountReconcilorThrottlerTest, RefillOneRequest) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); const CoreAccountId account_id = account_info.account_id; signin::SetListAccountsResponseOneAccount( - "other@gmail.com", signin::GetTestGaiaIdForEmail("other@gmail.com"), + kFakeEmail2, signin::GetTestGaiaIdForEmail(kFakeEmail2), &test_url_loader_factory_); signin::MultiloginParameters params( @@ -2706,10 +2805,10 @@ TEST_F(AccountReconcilorThrottlerTest, RefillOneRequest) { } TEST_F(AccountReconcilorThrottlerTest, RefillFiveRequests) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); const CoreAccountId account_id = account_info.account_id; signin::SetListAccountsResponseOneAccount( - "other@gmail.com", signin::GetTestGaiaIdForEmail("other@gmail.com"), + kFakeEmail2, signin::GetTestGaiaIdForEmail(kFakeEmail2), &test_url_loader_factory_); signin::MultiloginParameters params( @@ -2741,10 +2840,10 @@ TEST_F(AccountReconcilorThrottlerTest, RefillFiveRequests) { } TEST_F(AccountReconcilorThrottlerTest, NewRequestParamsPasses) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); const CoreAccountId account_id = account_info.account_id; signin::SetListAccountsResponseOneAccount( - "other@gmail.com", signin::GetTestGaiaIdForEmail("other@gmail.com"), + kFakeEmail2, signin::GetTestGaiaIdForEmail(kFakeEmail2), &test_url_loader_factory_); signin::MultiloginParameters params( @@ -2761,7 +2860,7 @@ TEST_F(AccountReconcilorThrottlerTest, NewRequestParamsPasses) { // Trigger different params. AccountReconcilor* reconcilor = GetMockReconcilor(); EXPECT_CALL(*GetMockReconcilor(), PerformSetCookiesAction(testing::_)); - identity_test_env()->MakeAccountAvailable("other@gmail.com"); + identity_test_env()->MakeAccountAvailable(kFakeEmail2); base::RunLoop().RunUntilIdle(); ASSERT_TRUE(reconcilor->is_reconcile_started_); SimulateSetAccountsInCookieCompleted( @@ -2773,10 +2872,10 @@ TEST_F(AccountReconcilorThrottlerTest, NewRequestParamsPasses) { } TEST_F(AccountReconcilorThrottlerTest, BlockFiveRequests) { - AccountInfo account_info = ConnectProfileToAccount("user@gmail.com"); + AccountInfo account_info = ConnectProfileToAccount(kFakeEmail); const CoreAccountId account_id = account_info.account_id; signin::SetListAccountsResponseOneAccount( - "other@gmail.com", signin::GetTestGaiaIdForEmail("other@gmail.com"), + kFakeEmail2, signin::GetTestGaiaIdForEmail(kFakeEmail2), &test_url_loader_factory_); signin::MultiloginParameters params( diff --git a/chromium/components/signin/core/browser/active_directory_account_reconcilor_delegate.cc b/chromium/components/signin/core/browser/active_directory_account_reconcilor_delegate.cc index 54629b0767c..f2687e27e4a 100644 --- a/chromium/components/signin/core/browser/active_directory_account_reconcilor_delegate.cc +++ b/chromium/components/signin/core/browser/active_directory_account_reconcilor_delegate.cc @@ -4,8 +4,8 @@ #include "components/signin/core/browser/active_directory_account_reconcilor_delegate.h" -#include "ash/components/tpm/install_attributes.h" #include "base/containers/contains.h" +#include "chromeos/ash/components/install_attributes/install_attributes.h" #include "google_apis/gaia/core_account_id.h" namespace signin { diff --git a/chromium/components/signin/core/browser/chrome_connected_header_helper.cc b/chromium/components/signin/core/browser/chrome_connected_header_helper.cc index 15fa8016f24..6dce9e7e1b3 100644 --- a/chromium/components/signin/core/browser/chrome_connected_header_helper.cc +++ b/chromium/components/signin/core/browser/chrome_connected_header_helper.cc @@ -63,6 +63,13 @@ GAIAServiceType GetGAIAServiceTypeFromHeader(const std::string& header_value) { return GAIA_SERVICE_TYPE_NONE; } +bool NewRequestHeaderCheckOrder() { + // The result is computed once and cached because the code is on the hot path. + static bool new_order = + base::FeatureList::IsEnabled(switches::kNewSigninRequestHeaderCheckOrder); + return new_order; +} + } // namespace const char kChromeConnectedCookieName[] = "CHROME_CONNECTED"; @@ -125,12 +132,22 @@ ManageAccountsParams ChromeConnectedHeaderHelper::BuildManageAccountsParams( bool ChromeConnectedHeaderHelper::ShouldBuildRequestHeader( const GURL& url, const content_settings::CookieSettings* cookie_settings) { + // The 'new order' refers to the order of the two checks performed in this + // function. In the new order the less expensive URL-based check is performed + // first in the most common case (non-Google URLs), and the cookie-based + // check is performed second. + bool new_order = NewRequestHeaderCheckOrder(); + + // Check if url is eligible for the header. New order. + if (new_order && !IsUrlEligibleForRequestHeader(url)) + return false; + // If signin cookies are not allowed, don't add the header. if (!SettingsAllowSigninCookies(cookie_settings)) return false; - // Check if url is eligible for the header. - if (!IsUrlEligibleForRequestHeader(url)) + // Check if url is eligible for the header. Old order. + if (!new_order && !IsUrlEligibleForRequestHeader(url)) return false; return true; @@ -248,19 +265,14 @@ std::string ChromeConnectedHeaderHelper::BuildRequestHeader( break; } + parts.push_back(base::StringPrintf("%s=%s", + kConsistencyEnabledByDefaultAttrName, #if BUILDFLAG(IS_CHROMEOS_LACROS) - std::string consistency_enabled_by_default = - base::FeatureList::IsEnabled(switches::kLacrosNonSyncingProfiles) - ? "true" - : "false"; + "true")); #else - std::string consistency_enabled_by_default = "false"; + "false")); #endif - parts.push_back(base::StringPrintf("%s=%s", - kConsistencyEnabledByDefaultAttrName, - consistency_enabled_by_default.c_str())); - return base::JoinString(parts, is_header_request ? "," : ":"); } diff --git a/chromium/components/signin/core/browser/consistency_cookie_manager.cc b/chromium/components/signin/core/browser/consistency_cookie_manager.cc index e655db405bb..df261f8755f 100644 --- a/chromium/components/signin/core/browser/consistency_cookie_manager.cc +++ b/chromium/components/signin/core/browser/consistency_cookie_manager.cc @@ -73,7 +73,7 @@ ConsistencyCookieManager::ConsistencyCookieManager( account_reconcilor_state_(account_reconcilor_->GetState()) { DCHECK(signin_client_); DCHECK(account_reconcilor_); - account_reconcilor_observation_.Observe(account_reconcilor_); + account_reconcilor_observation_.Observe(account_reconcilor_.get()); UpdateCookieIfNeeded(/*force_creation=*/false); } diff --git a/chromium/components/signin/core/browser/consistency_cookie_manager.h b/chromium/components/signin/core/browser/consistency_cookie_manager.h index 068adb797fd..bcf4f06e9cf 100644 --- a/chromium/components/signin/core/browser/consistency_cookie_manager.h +++ b/chromium/components/signin/core/browser/consistency_cookie_manager.h @@ -6,6 +6,7 @@ #define COMPONENTS_SIGNIN_CORE_BROWSER_CONSISTENCY_COOKIE_MANAGER_H_ #include "base/gtest_prod_util.h" +#include "base/memory/raw_ptr.h" #include "base/memory/weak_ptr.h" #include "base/scoped_observation.h" #include "components/signin/core/browser/account_reconcilor.h" @@ -135,8 +136,8 @@ class ConsistencyCookieManager : public AccountReconcilor::Observer { const net::CookieAccessResultList& cookie_list, const net::CookieAccessResultList& /*excluded_cookies*/); - SigninClient* const signin_client_; - AccountReconcilor* const account_reconcilor_; + const raw_ptr<SigninClient> signin_client_; + const raw_ptr<AccountReconcilor> account_reconcilor_; signin_metrics::AccountReconcilorState account_reconcilor_state_ = signin_metrics::ACCOUNT_RECONCILOR_INACTIVE; int scoped_update_count_ = 0; diff --git a/chromium/components/signin/core/browser/consistency_cookie_manager_unittest.cc b/chromium/components/signin/core/browser/consistency_cookie_manager_unittest.cc index ae03f5124c8..d2dcf256052 100644 --- a/chromium/components/signin/core/browser/consistency_cookie_manager_unittest.cc +++ b/chromium/components/signin/core/browser/consistency_cookie_manager_unittest.cc @@ -6,11 +6,10 @@ #include <memory> -#include "base/test/scoped_feature_list.h" +#include "base/memory/raw_ptr.h" #include "components/signin/core/browser/account_reconcilor.h" #include "components/signin/core/browser/account_reconcilor_delegate.h" #include "components/signin/public/base/signin_metrics.h" -#include "components/signin/public/base/signin_switches.h" #include "components/signin/public/base/test_signin_client.h" #include "google_apis/gaia/gaia_urls.h" #include "net/cookies/canonical_cookie.h" @@ -127,14 +126,9 @@ class ConsistencyCookieManagerTest : public testing::Test { MockCookieManager* cookie_manager() { return cookie_manager_; } private: - // The kLacrosNonSyncingProfiles flags bundles several features: non-syncing - // profiles, signed out profiles, and Mirror Landing. The - // `ConsistencyCookieManager` is related to MirrorLanding. - base::test::ScopedFeatureList feature_list_{ - switches::kLacrosNonSyncingProfiles}; - TestSigninClient signin_client_{/*prefs=*/nullptr}; - MockCookieManager* cookie_manager_ = nullptr; // Owned by `signin_client_`. + raw_ptr<MockCookieManager> cookie_manager_ = + nullptr; // Owned by `signin_client_`. std::unique_ptr<AccountReconcilor> reconcilor_; }; diff --git a/chromium/components/signin/core/browser/cookie_settings_util.cc b/chromium/components/signin/core/browser/cookie_settings_util.cc index 2f9cebf4951..58e2182c76f 100644 --- a/chromium/components/signin/core/browser/cookie_settings_util.cc +++ b/chromium/components/signin/core/browser/cookie_settings_util.cc @@ -15,16 +15,19 @@ bool SettingsAllowSigninCookies( GURL gaia_url = GaiaUrls::GetInstance()->gaia_url(); GURL google_url = GaiaUrls::GetInstance()->google_url(); return cookie_settings && - cookie_settings->IsFullCookieAccessAllowed(gaia_url, gaia_url) && - cookie_settings->IsFullCookieAccessAllowed(google_url, google_url); + cookie_settings->IsFullCookieAccessAllowed( + gaia_url, gaia_url, + content_settings::CookieSettings::QueryReason::kCookies) && + cookie_settings->IsFullCookieAccessAllowed( + google_url, google_url, + content_settings::CookieSettings::QueryReason::kCookies); } bool SettingsDeleteSigninCookiesOnExit( const content_settings::CookieSettings* cookie_settings) { GURL gaia_url = GaiaUrls::GetInstance()->gaia_url(); GURL google_url = GaiaUrls::GetInstance()->google_url(); - ContentSettingsForOneType settings; - cookie_settings->GetCookieSettings(&settings); + ContentSettingsForOneType settings = cookie_settings->GetCookieSettings(); return !cookie_settings || cookie_settings->ShouldDeleteCookieOnExit( diff --git a/chromium/components/signin/core/browser/signin_error_controller_unittest.cc b/chromium/components/signin/core/browser/signin_error_controller_unittest.cc index f7885d818cf..100100e23be 100644 --- a/chromium/components/signin/core/browser/signin_error_controller_unittest.cc +++ b/chromium/components/signin/core/browser/signin_error_controller_unittest.cc @@ -157,7 +157,9 @@ TEST(SigninErrorControllerTest, AuthStatusEnumerateAllErrors) { GoogleServiceAuthError::SERVICE_UNAVAILABLE, GoogleServiceAuthError::REQUEST_CANCELED, GoogleServiceAuthError::UNEXPECTED_SERVICE_RESPONSE, - GoogleServiceAuthError::SERVICE_ERROR}; + GoogleServiceAuthError::SERVICE_ERROR, + GoogleServiceAuthError::SCOPE_LIMITED_UNRECOVERABLE_ERROR, + }; static_assert( std::size(table) == GoogleServiceAuthError::NUM_STATES - GoogleServiceAuthError::kDeprecatedStateCount, @@ -166,8 +168,8 @@ TEST(SigninErrorControllerTest, AuthStatusEnumerateAllErrors) { for (GoogleServiceAuthError::State state : table) { GoogleServiceAuthError error(state); - if (error.IsTransientError()) - continue; // Only persistent errors or non-errors are reported. + if (error.IsTransientError() || error.IsScopePersistentError()) + continue; // Only non scope persistent errors or non-errors are reported. identity_test_env.UpdatePersistentErrorOfRefreshTokenForAccount( test_account_id, error); diff --git a/chromium/components/signin/core/browser/signin_header_helper_unittest.cc b/chromium/components/signin/core/browser/signin_header_helper_unittest.cc index 0953cdca7f2..fefe396b93a 100644 --- a/chromium/components/signin/core/browser/signin_header_helper_unittest.cc +++ b/chromium/components/signin/core/browser/signin_header_helper_unittest.cc @@ -10,7 +10,6 @@ #include "base/command_line.h" #include "base/strings/stringprintf.h" #include "base/test/metrics/histogram_tester.h" -#include "base/test/scoped_feature_list.h" #include "base/test/task_environment.h" #include "build/build_config.h" #include "build/chromeos_buildflags.h" @@ -19,7 +18,6 @@ #include "components/signin/core/browser/chrome_connected_header_helper.h" #include "components/signin/public/base/account_consistency_method.h" #include "components/signin/public/base/signin_buildflags.h" -#include "components/signin/public/base/signin_switches.h" #include "components/signin/public/identity_manager/tribool.h" #include "components/sync_preferences/testing_pref_service_syncable.h" #include "google_apis/gaia/gaia_urls.h" @@ -173,8 +171,6 @@ class SigninHeaderHelperTest : public testing::Test { sync_preferences::TestingPrefServiceSyncable prefs_; #if BUILDFLAG(IS_CHROMEOS_LACROS) - base::test::ScopedFeatureList scoped_feature_list_{ - switches::kLacrosNonSyncingProfiles}; std::unique_ptr<chromeos::ScopedLacrosServiceTestHelper> scoped_lacros_test_helper_; #endif // BUILDFLAG(IS_CHROMEOS_LACROS) diff --git a/chromium/components/signin/core/browser/signin_investigator.cc b/chromium/components/signin/core/browser/signin_investigator.cc deleted file mode 100644 index 82fa9cf03fc..00000000000 --- a/chromium/components/signin/core/browser/signin_investigator.cc +++ /dev/null @@ -1,66 +0,0 @@ -// Copyright 2015 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/signin_investigator.h" - -#include "base/check.h" -#include "base/metrics/histogram_macros.h" -#include "components/signin/public/base/signin_metrics.h" -#include "components/signin/public/base/signin_pref_names.h" -#include "google_apis/gaia/gaia_auth_util.h" - -using signin_metrics::AccountEquality; -using signin_metrics::LogAccountEquality; - -SigninInvestigator::SigninInvestigator(const std::string& current_email, - const std::string& current_id, - DependencyProvider* provider) - : current_email_(current_email), - current_id_(current_id), - provider_(provider) { - DCHECK(!current_email_.empty()); - DCHECK(provider); -} - -SigninInvestigator::~SigninInvestigator() {} - -bool SigninInvestigator::AreAccountsEqualWithFallback() { - const std::string last_id = - provider_->GetPrefs()->GetString(prefs::kGoogleServicesLastAccountId); - bool same_email = gaia::AreEmailsSame( - current_email_, - provider_->GetPrefs()->GetString(prefs::kGoogleServicesLastUsername)); - if (!current_id_.empty() && !last_id.empty()) { - bool same_id = current_id_ == last_id; - if (same_email && same_id) { - LogAccountEquality(AccountEquality::BOTH_EQUAL); - } else if (same_email) { - LogAccountEquality(AccountEquality::ONLY_SAME_EMAIL); - } else if (same_id) { - LogAccountEquality(AccountEquality::ONLY_SAME_ID); - } else { - LogAccountEquality(AccountEquality::BOTH_DIFFERENT); - } - return same_id; - } else { - LogAccountEquality(AccountEquality::EMAIL_FALLBACK); - return same_email; - } -} - -InvestigatedScenario SigninInvestigator::Investigate() { - InvestigatedScenario scenario; - if (provider_->GetPrefs() - ->GetString(prefs::kGoogleServicesLastUsername) - .empty()) { - scenario = InvestigatedScenario::kFirstSignIn; - } else if (AreAccountsEqualWithFallback()) { - scenario = InvestigatedScenario::kSameAccount; - } else { - scenario = InvestigatedScenario::kDifferentAccount; - } - - UMA_HISTOGRAM_ENUMERATION("Signin.InvestigatedScenario", scenario); - return scenario; -} diff --git a/chromium/components/signin/core/browser/signin_investigator.h b/chromium/components/signin/core/browser/signin_investigator.h deleted file mode 100644 index 2dcb34862c0..00000000000 --- a/chromium/components/signin/core/browser/signin_investigator.h +++ /dev/null @@ -1,79 +0,0 @@ -// Copyright 2015 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 COMPONENTS_SIGNIN_CORE_BROWSER_SIGNIN_INVESTIGATOR_H_ -#define COMPONENTS_SIGNIN_CORE_BROWSER_SIGNIN_INVESTIGATOR_H_ - -#include <string> - -#include "base/memory/raw_ptr.h" -#include "components/prefs/pref_service.h" - -// These values are persisted to logs. Entries should not be renumbered and -// numeric values should never be reused. -// A Java counterpart will be generated for this enum. -// GENERATED_JAVA_ENUM_PACKAGE: org.chromium.signin -// Broad categorization of signin type from investigation. -enum class InvestigatedScenario : int { - // First signin and should not be warned. As little friction as possible - // should get between the user and signing in. - kFirstSignIn = 0, - // Was never used (see crbug.com/983183, crbug.com/572754). - kDeprecatedUpgradeHighRisk = 1, - // Relogging with the same account. - kSameAccount = 2, - // User is switching accounts, can be very dangerous depending on the amount - // of local syncable data. - kDifferentAccount = 3, - // Always the last enumerated type. - kMaxValue = kDifferentAccount, -}; - -// Performs various checks with the current account being used to login in and -// against the local data. Decides what kind of signin scenario we're in, if -// we should warn the user about sync merge dangerous, and emits metrics. -class SigninInvestigator { - public: - // This interface allows the embedder to only have to worry about retrieving - // dependencies, not any of the logic for using them. - class DependencyProvider { - public: - virtual ~DependencyProvider() {} - // Returns a non owning pointer to the pref service. - virtual PrefService* GetPrefs() = 0; - }; - - SigninInvestigator(const std::string& current_email, - const std::string& current_id, - DependencyProvider* provider); - - SigninInvestigator(const SigninInvestigator&) = delete; - SigninInvestigator& operator=(const SigninInvestigator&) = delete; - - ~SigninInvestigator(); - - // Determines the current scenario, wether it is an upgrade, same account, or - // different. - InvestigatedScenario Investigate(); - - private: - friend class SigninInvestigatorTest; - - // Determines if the current account is the same as the last email/gaia id. - // Because email can change but gaia id cannot, the id is the authoritative - // source of account equality. However, initially we were only storing the - // last username/email that was used to sign in, so for any client that hasn't - // logged in since we added logic to store the gaia id, the last id is blank. - // In this case, we fallback to using last_email. This equality check is - // slightly more strict than the version AccountId defines as == operator. - bool AreAccountsEqualWithFallback(); - - std::string current_email_; - std::string current_id_; - - // Non-owning pointer. - raw_ptr<DependencyProvider> provider_; -}; - -#endif // COMPONENTS_SIGNIN_CORE_BROWSER_SIGNIN_INVESTIGATOR_H_ diff --git a/chromium/components/signin/core/browser/signin_investigator_unittest.cc b/chromium/components/signin/core/browser/signin_investigator_unittest.cc deleted file mode 100644 index d9e30548d08..00000000000 --- a/chromium/components/signin/core/browser/signin_investigator_unittest.cc +++ /dev/null @@ -1,129 +0,0 @@ -// Copyright 2015 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 <string> - -#include "base/test/metrics/histogram_tester.h" -#include "base/test/task_environment.h" -#include "components/prefs/pref_registry_simple.h" -#include "components/signin/core/browser/signin_investigator.h" -#include "components/signin/public/base/signin_metrics.h" -#include "components/signin/public/base/signin_pref_names.h" -#include "components/signin/public/identity_manager/identity_test_environment.h" -#include "components/sync_preferences/testing_pref_service_syncable.h" -#include "testing/gtest/include/gtest/gtest.h" - -using signin_metrics::AccountEquality; - -namespace { -const char kSameEmail[] = "user1@domain.com"; -const char kDifferentEmail[] = "user2@domain.com"; -const char kSameId[] = "1"; -const char kDifferentId[] = "2"; -const char kEmptyId[] = ""; - -class FakeProvider : public SigninInvestigator::DependencyProvider { - public: - FakeProvider(const std::string& last_email, const std::string& last_id) - : identity_test_env_(/*test_url_loader_factory=*/nullptr, &prefs_) { - prefs_.SetString(prefs::kGoogleServicesLastUsername, last_email); - prefs_.SetString(prefs::kGoogleServicesLastAccountId, last_id); - } - PrefService* GetPrefs() override { return &prefs_; } - - private: - base::test::TaskEnvironment task_environment_; - sync_preferences::TestingPrefServiceSyncable prefs_; - signin::IdentityTestEnvironment identity_test_env_; -}; -} // namespace - -class SigninInvestigatorTest : public testing::Test { - protected: - void AssertAccountEquality(const std::string& email, - const std::string& id, - bool equals_expectated, - AccountEquality histogram_expected) { - FakeProvider provider(email, id); - SigninInvestigator investigator(kSameEmail, kSameId, &provider); - AssertAccountEquality(&investigator, equals_expectated, histogram_expected); - } - - void AssertAccountEquality(SigninInvestigator* investigator, - bool equals_expectated, - AccountEquality histogram_expected) { - base::HistogramTester histogram_tester; - bool equals_actual = investigator->AreAccountsEqualWithFallback(); - ASSERT_EQ(equals_expectated, equals_actual); - histogram_tester.ExpectUniqueSample( - "Signin.AccountEquality", static_cast<int>(histogram_expected), 1); - } - - void AssertInvestigatedScenario(const std::string& email, - const std::string& id, - InvestigatedScenario expected) { - base::HistogramTester histogram_tester; - FakeProvider provider(email, id); - SigninInvestigator investigator(kSameEmail, kSameId, &provider); - InvestigatedScenario actual = investigator.Investigate(); - ASSERT_EQ(expected, actual); - histogram_tester.ExpectUniqueSample("Signin.InvestigatedScenario", - static_cast<int>(expected), 1); - } -}; - -TEST_F(SigninInvestigatorTest, EqualitySameEmailsSameIds) { - AssertAccountEquality(kSameEmail, kSameId, true, AccountEquality::BOTH_EQUAL); -} - -TEST_F(SigninInvestigatorTest, EqualitySameEmailsDifferentIds) { - AssertAccountEquality(kSameEmail, kDifferentId, false, - AccountEquality::ONLY_SAME_EMAIL); -} - -TEST_F(SigninInvestigatorTest, EqualityDifferentEmailsSameIds) { - AssertAccountEquality(kDifferentEmail, kSameId, true, - AccountEquality::ONLY_SAME_ID); -} - -TEST_F(SigninInvestigatorTest, EqualityDifferentEmailsDifferentIds) { - AssertAccountEquality(kDifferentEmail, kDifferentId, false, - AccountEquality::BOTH_DIFFERENT); -} - -TEST_F(SigninInvestigatorTest, EqualitySameEmailFallback) { - AssertAccountEquality(kSameEmail, kEmptyId, true, - AccountEquality::EMAIL_FALLBACK); -} - -TEST_F(SigninInvestigatorTest, EqualityDifferentEmailFallback) { - AssertAccountEquality(kDifferentEmail, kEmptyId, false, - AccountEquality::EMAIL_FALLBACK); -} - -TEST_F(SigninInvestigatorTest, EqualitySameEmailFallbackEmptyCurrentId) { - FakeProvider provider(kSameEmail, kDifferentId); - SigninInvestigator investigator(kSameEmail, kEmptyId, &provider); - AssertAccountEquality(&investigator, true, AccountEquality::EMAIL_FALLBACK); -} - -TEST_F(SigninInvestigatorTest, EqualityDifferentEmailFallbackEmptyCurrentId) { - FakeProvider provider(kDifferentId, kDifferentId); - SigninInvestigator investigator(kSameEmail, kEmptyId, &provider); - AssertAccountEquality(&investigator, false, AccountEquality::EMAIL_FALLBACK); -} - -TEST_F(SigninInvestigatorTest, InvestigateSameAccount) { - AssertInvestigatedScenario(kSameEmail, kSameId, - InvestigatedScenario::kSameAccount); -} - -TEST_F(SigninInvestigatorTest, InvestigateFirstSignIn) { - AssertInvestigatedScenario("", "", InvestigatedScenario::kFirstSignIn); -} - -TEST_F(SigninInvestigatorTest, InvestigateDifferentAccount) { - AssertInvestigatedScenario(kDifferentEmail, kDifferentId, - InvestigatedScenario::kDifferentAccount); -} diff --git a/chromium/components/signin/internal/identity_manager/account_capabilities_list.h b/chromium/components/signin/internal/identity_manager/account_capabilities_list.h index ff3f8115535..3b489d28ed0 100644 --- a/chromium/components/signin/internal/identity_manager/account_capabilities_list.h +++ b/chromium/components/signin/internal/identity_manager/account_capabilities_list.h @@ -20,6 +20,10 @@ ACCOUNT_CAPABILITY(kIsSubjectToParentalControlsCapabilityName, IS_SUBJECT_TO_PARENTAL_CONTROLS_CAPABILITY_NAME, "accountcapabilities/guydolldmfya") +ACCOUNT_CAPABILITY(kIsAllowedForMachineLearningCapabilityName, + IS_ALLOWED_FOR_MACHINE_LEARNING_CAPABILITY_NAME, + "accountcapabilities/g42tslldmfya") + ACCOUNT_CAPABILITY(kCanOfferExtendedChromeSyncPromosCapabilityName, CAN_OFFER_EXTENDED_CHROME_SYNC_PROMOS_CAPABILITY_NAME, "accountcapabilities/gi2tklldmfya") diff --git a/chromium/components/signin/internal/identity_manager/account_tracker_service.cc b/chromium/components/signin/internal/identity_manager/account_tracker_service.cc index 36708d97860..ecad90bf008 100644 --- a/chromium/components/signin/internal/identity_manager/account_tracker_service.cc +++ b/chromium/components/signin/internal/identity_manager/account_tracker_service.cc @@ -44,23 +44,23 @@ #endif namespace { -const char kAccountKeyPath[] = "account_id"; -const char kAccountEmailPath[] = "email"; -const char kAccountGaiaPath[] = "gaia"; -const char kAccountHostedDomainPath[] = "hd"; -const char kAccountFullNamePath[] = "full_name"; -const char kAccountGivenNamePath[] = "given_name"; -const char kAccountLocalePath[] = "locale"; -const char kAccountPictureURLPath[] = "picture_url"; -const char kLastDownloadedImageURLWithSizePath[] = +const char kAccountKeyKey[] = "account_id"; +const char kAccountEmailKey[] = "email"; +const char kAccountGaiaKey[] = "gaia"; +const char kAccountHostedDomainKey[] = "hd"; +const char kAccountFullNameKey[] = "full_name"; +const char kAccountGivenNameKey[] = "given_name"; +const char kAccountLocaleKey[] = "locale"; +const char kAccountPictureURLKey[] = "picture_url"; +const char kLastDownloadedImageURLWithSizeKey[] = "last_downloaded_image_url_with_size"; -const char kAccountChildAttributePath[] = "is_supervised_child"; -const char kAdvancedProtectionAccountStatusPath[] = +const char kAccountChildAttributeKey[] = "is_supervised_child"; +const char kAdvancedProtectionAccountStatusKey[] = "is_under_advanced_protection"; // This key is deprecated since 2021/07 and should be removed after migration. -// It was replaced by kAccountChildAttributePath. -const char kDeprecatedChildStatusPath[] = "is_child_account"; +// It was replaced by kAccountChildAttributeKey. +const char kDeprecatedChildStatusKey[] = "is_child_account"; // This key is deprecated since 2022/02 and should be removed after migration. // It was replaced by GetCapabilityPrefPath(capability_name) method that derives @@ -121,11 +121,11 @@ std::string GetCapabilityPrefPath(base::StringPiece capability_name) { return base::StrCat({"accountcapabilities.", capability_name}); } -void SetAccountCapabilityState(base::Value* value, +void SetAccountCapabilityState(base::Value::Dict& value, base::StringPiece capability_name, signin::Tribool state) { - value->SetIntPath(GetCapabilityPrefPath(capability_name), - static_cast<int>(state)); + value.SetByDottedPath(GetCapabilityPrefPath(capability_name), + static_cast<int>(state)); } signin::Tribool ParseTribool(absl::optional<int> int_value) { @@ -144,17 +144,17 @@ signin::Tribool ParseTribool(absl::optional<int> int_value) { } } -signin::Tribool FindAccountCapabilityState(const base::Value& value, +signin::Tribool FindAccountCapabilityState(const base::Value::Dict& dict, base::StringPiece name) { absl::optional<int> capability = - value.FindIntPath(GetCapabilityPrefPath(name)); + dict.FindIntByDottedPath(GetCapabilityPrefPath(name)); return ParseTribool(capability); } -void GetString(const base::Value& dict, +void GetString(const base::Value::Dict& dict, base::StringPiece key, std::string& result) { - if (const std::string* value = dict.FindStringKey(key)) { + if (const std::string* value = dict.FindString(key)) { result = *value; } } @@ -555,15 +555,15 @@ void AccountTrackerService::OnAccountImageUpdated( if (!success || !pref_service_) return; - base::DictionaryValue* dict = nullptr; + base::Value::Dict* dict = nullptr; ListPrefUpdate update(pref_service_, prefs::kAccountInfo); - for (size_t i = 0; i < update->GetListDeprecated().size(); - ++i, dict = nullptr) { - base::Value& dict_value = update->GetListDeprecated()[i]; - if (dict_value.is_dict()) { - dict = static_cast<base::DictionaryValue*>(&dict_value); - const std::string* account_key = dict->FindStringKey(kAccountKeyPath); + base::Value::List& list = update->GetList(); + for (base::Value& value : list) { + base::Value::Dict* maybe_dict = value.GetIfDict(); + if (maybe_dict) { + const std::string* account_key = maybe_dict->FindString(kAccountKeyKey); if (account_key && *account_key == account_id.ToString()) { + dict = maybe_dict; break; } } @@ -572,7 +572,7 @@ void AccountTrackerService::OnAccountImageUpdated( if (!dict) { return; } - dict->SetString(kLastDownloadedImageURLWithSizePath, image_url_with_size); + dict->Set(kLastDownloadedImageURLWithSizeKey, image_url_with_size); } void AccountTrackerService::RemoveAccountImageFromDisk( @@ -584,15 +584,13 @@ void AccountTrackerService::RemoveAccountImageFromDisk( } void AccountTrackerService::LoadFromPrefs() { - const base::Value* list = pref_service_->GetList(prefs::kAccountInfo); + const base::Value::List& list = + pref_service_->GetValueList(prefs::kAccountInfo); std::set<CoreAccountId> to_remove; - for (size_t i = 0; i < list->GetListDeprecated().size(); ++i) { - const base::Value& dict_value = list->GetListDeprecated()[i]; - if (dict_value.is_dict()) { - const base::DictionaryValue& dict = - base::Value::AsDictionaryValue(dict_value); - if (const std::string* account_key = - dict.FindStringKey(kAccountKeyPath)) { + for (size_t i = 0; i < list.size(); ++i) { + const base::Value::Dict* dict = list[i].GetIfDict(); + if (dict) { + if (const std::string* account_key = dict->FindString(kAccountKeyKey)) { // Ignore incorrectly persisted non-canonical account ids. if (account_key->find('@') != std::string::npos && *account_key != gaia::CanonicalizeEmail(*account_key)) { @@ -604,58 +602,55 @@ void AccountTrackerService::LoadFromPrefs() { StartTrackingAccount(account_id); AccountInfo& account_info = accounts_[account_id]; - GetString(dict, kAccountGaiaPath, account_info.gaia); - GetString(dict, kAccountEmailPath, account_info.email); - GetString(dict, kAccountHostedDomainPath, account_info.hosted_domain); - GetString(dict, kAccountFullNamePath, account_info.full_name); - GetString(dict, kAccountGivenNamePath, account_info.given_name); - GetString(dict, kAccountLocalePath, account_info.locale); - GetString(dict, kAccountPictureURLPath, account_info.picture_url); - GetString(dict, kLastDownloadedImageURLWithSizePath, + GetString(*dict, kAccountGaiaKey, account_info.gaia); + GetString(*dict, kAccountEmailKey, account_info.email); + GetString(*dict, kAccountHostedDomainKey, account_info.hosted_domain); + GetString(*dict, kAccountFullNameKey, account_info.full_name); + GetString(*dict, kAccountGivenNameKey, account_info.given_name); + GetString(*dict, kAccountLocaleKey, account_info.locale); + GetString(*dict, kAccountPictureURLKey, account_info.picture_url); + GetString(*dict, kLastDownloadedImageURLWithSizeKey, account_info.last_downloaded_image_url_with_size); if (absl::optional<bool> is_child_status = - dict.FindBoolKey(kDeprecatedChildStatusPath)) { + dict->FindBool(kDeprecatedChildStatusKey)) { account_info.is_child_account = is_child_status.value() ? signin::Tribool::kTrue : signin::Tribool::kFalse; - // Migrate to kAccountChildAttributePath. + // Migrate to kAccountChildAttributeKey. ListPrefUpdate update(pref_service_, prefs::kAccountInfo); - base::Value* update_dict = &update->GetListDeprecated()[i]; - DCHECK(update_dict->is_dict()); - update_dict->SetIntPath( - kAccountChildAttributePath, - static_cast<int>(account_info.is_child_account)); - update_dict->RemoveKey(kDeprecatedChildStatusPath); + base::Value::Dict& update_dict = update->GetList()[i].GetDict(); + update_dict.Set(kAccountChildAttributeKey, + static_cast<int>(account_info.is_child_account)); + update_dict.Remove(kDeprecatedChildStatusKey); } else { account_info.is_child_account = - ParseTribool(dict.FindIntPath(kAccountChildAttributePath)); + ParseTribool(dict->FindInt(kAccountChildAttributeKey)); } absl::optional<bool> is_under_advanced_protection = - dict.FindBoolKey(kAdvancedProtectionAccountStatusPath); + dict->FindBool(kAdvancedProtectionAccountStatusKey); if (is_under_advanced_protection.has_value()) { account_info.is_under_advanced_protection = is_under_advanced_protection.value(); } if (absl::optional<int> can_offer_extended_chrome_sync_promos = - dict.FindIntPath( + dict->FindIntByDottedPath( kDeprecatedCanOfferExtendedChromeSyncPromosPrefPath)) { // Migrate to Capability names based pref paths. ListPrefUpdate update(pref_service_, prefs::kAccountInfo); - base::Value* update_dict = &update->GetListDeprecated()[i]; - DCHECK(update_dict->is_dict()); + base::Value::Dict& update_dict = update->GetList()[i].GetDict(); SetAccountCapabilityState( update_dict, kCanOfferExtendedChromeSyncPromosCapabilityName, ParseTribool(can_offer_extended_chrome_sync_promos)); - update_dict->RemovePath( + update_dict.RemoveByDottedPath( kDeprecatedCanOfferExtendedChromeSyncPromosPrefPath); } for (const std::string& name : AccountCapabilities::GetSupportedAccountCapabilityNames()) { - switch (FindAccountCapabilityState(dict, name)) { + switch (FindAccountCapabilityState(*dict, name)) { case signin::Tribool::kUnknown: account_info.capabilities.capabilities_map_.erase(name); break; @@ -707,47 +702,45 @@ void AccountTrackerService::SaveToPrefs(const AccountInfo& account_info) { if (!pref_service_) return; - base::DictionaryValue* dict = nullptr; + base::Value::Dict* dict = nullptr; ListPrefUpdate update(pref_service_, prefs::kAccountInfo); - for (size_t i = 0; i < update->GetListDeprecated().size(); - ++i, dict = nullptr) { - base::Value& dict_value = update->GetListDeprecated()[i]; - if (dict_value.is_dict()) { - dict = static_cast<base::DictionaryValue*>(&dict_value); - const std::string* account_key = dict->FindStringKey(kAccountKeyPath); + base::Value::List& list = update->GetList(); + for (base::Value& value : list) { + base::Value::Dict* maybe_dict = value.GetIfDict(); + if (maybe_dict) { + const std::string* account_key = maybe_dict->FindString(kAccountKeyKey); if (account_key && *account_key == account_info.account_id.ToString()) { + dict = maybe_dict; break; } } } if (!dict) { - update->Append(base::Value(base::Value::Type::DICTIONARY)); - base::Value& dict_value = update->GetListDeprecated().back(); - DCHECK(dict_value.is_dict()); - dict = static_cast<base::DictionaryValue*>(&dict_value); - dict->SetString(kAccountKeyPath, account_info.account_id.ToString()); - } - - dict->SetString(kAccountEmailPath, account_info.email); - dict->SetString(kAccountGaiaPath, account_info.gaia); - dict->SetString(kAccountHostedDomainPath, account_info.hosted_domain); - dict->SetString(kAccountFullNamePath, account_info.full_name); - dict->SetString(kAccountGivenNamePath, account_info.given_name); - dict->SetString(kAccountLocalePath, account_info.locale); - dict->SetString(kAccountPictureURLPath, account_info.picture_url); - dict->SetIntPath(kAccountChildAttributePath, - static_cast<int>(account_info.is_child_account)); - dict->SetBoolean(kAdvancedProtectionAccountStatusPath, - account_info.is_under_advanced_protection); - // |kLastDownloadedImageURLWithSizePath| should only be set after the GAIA + list.Append(base::Value::Dict()); + dict = &list.back().GetDict(); + dict->Set(kAccountKeyKey, account_info.account_id.ToString()); + } + + dict->Set(kAccountEmailKey, account_info.email); + dict->Set(kAccountGaiaKey, account_info.gaia); + dict->Set(kAccountHostedDomainKey, account_info.hosted_domain); + dict->Set(kAccountFullNameKey, account_info.full_name); + dict->Set(kAccountGivenNameKey, account_info.given_name); + dict->Set(kAccountLocaleKey, account_info.locale); + dict->Set(kAccountPictureURLKey, account_info.picture_url); + dict->Set(kAccountChildAttributeKey, + static_cast<int>(account_info.is_child_account)); + dict->Set(kAdvancedProtectionAccountStatusKey, + account_info.is_under_advanced_protection); + // |kLastDownloadedImageURLWithSizeKey| should only be set after the GAIA // picture is successufly saved to disk. Otherwise, there is no guarantee that - // |kLastDownloadedImageURLWithSizePath| matches the picture on disk. + // |kLastDownloadedImageURLWithSizeKey| matches the picture on disk. for (const std::string& name : AccountCapabilities::GetSupportedAccountCapabilityNames()) { signin::Tribool capability_state = account_info.capabilities.GetCapabilityByName(name); - SetAccountCapabilityState(dict, name, capability_state); + SetAccountCapabilityState(*dict, name, capability_state); } } @@ -757,10 +750,10 @@ void AccountTrackerService::RemoveFromPrefs(const AccountInfo& account_info) { ListPrefUpdate update(pref_service_, prefs::kAccountInfo); const std::string account_id = account_info.account_id.ToString(); - update->EraseListValueIf([&account_id](const base::Value& value) { + update->GetList().EraseIf([&account_id](const base::Value& value) { if (!value.is_dict()) return false; - const std::string* account_key = value.FindStringKey(kAccountKeyPath); + const std::string* account_key = value.GetDict().FindString(kAccountKeyKey); return account_key && *account_key == account_id; }); } diff --git a/chromium/components/signin/internal/identity_manager/account_tracker_service_unittest.cc b/chromium/components/signin/internal/identity_manager/account_tracker_service_unittest.cc index 4a4226c5785..e45958b1c1f 100644 --- a/chromium/components/signin/internal/identity_manager/account_tracker_service_unittest.cc +++ b/chromium/components/signin/internal/identity_manager/account_tracker_service_unittest.cc @@ -249,9 +249,6 @@ class AccountTrackerServiceTest : public testing::Test { // Helpers to fake access token and user info fetching CoreAccountId AccountKeyToAccountId(AccountKey account_key) { - if (force_account_id_to_email_for_legacy_tests_) - return CoreAccountId(AccountKeyToEmail(account_key)); - return CoreAccountId::FromGaiaId(AccountKeyToGaiaId(account_key)); } @@ -357,10 +354,6 @@ class AccountTrackerServiceTest : public testing::Test { return signin_client_.GetTestURLLoaderFactory(); } - bool* force_account_id_to_email_for_legacy_tests_pointer() { - return &force_account_id_to_email_for_legacy_tests_; - } - protected: void ReturnFetchResults(const GURL& url, net::HttpStatusCode response_code, @@ -418,7 +411,6 @@ class AccountTrackerServiceTest : public testing::Test { raw_ptr<FakeAccountCapabilitiesFetcherFactory> fake_account_capabilities_fetcher_factory_ = nullptr; std::vector<TrackingEvent> account_tracker_events_; - bool force_account_id_to_email_for_legacy_tests_ = false; }; void AccountTrackerServiceTest::ReturnFetchResults( @@ -1204,41 +1196,6 @@ TEST_F(AccountTrackerServiceTest, TimerRefresh) { } #if BUILDFLAG(IS_CHROMEOS_ASH) -TEST_F(AccountTrackerServiceTest, LegacyDottedAccountIds) { - // Force legacy of non-normalized email as account_id. - base::AutoReset<bool> force_account_id_to_email_for_legacy_test( - force_account_id_to_email_for_legacy_tests_pointer(), true); - - // Start by creating a tracker and adding an account with a dotted account - // id because of an old bug in token service. The token service would also - // add a correct non-dotted account id for the same account. - ResetAccountTracker(); - - SimulateTokenAvailable(kAccountKeyFooDotBar); - SimulateTokenAvailable(kAccountKeyFooBar); - ReturnAccountInfoFetchSuccess(kAccountKeyFooDotBar); - ReturnAccountInfoFetchSuccess(kAccountKeyFooBar); - - EXPECT_TRUE(account_fetcher()->IsAllUserInfoFetched()); - std::vector<AccountInfo> infos = account_tracker()->GetAccounts(); - ASSERT_EQ(2u, infos.size()); - EXPECT_EQ(AccountKeyToEmail(kAccountKeyFooDotBar), infos[0].email); - EXPECT_EQ(AccountKeyToEmail(kAccountKeyFooBar), infos[1].email); - - // Remove the bad account now from the token service to simulate that it - // has been "fixed". - SimulateTokenRevoked(kAccountKeyFooDotBar); - - // Instantiate a new tracker and validate that it has only one account, and - // it is the correct non dotted one. - ResetAccountTrackerNetworkDisabled(); - - EXPECT_TRUE(account_fetcher()->IsAllUserInfoFetched()); - infos = account_tracker()->GetAccounts(); - ASSERT_EQ(1u, infos.size()); - EXPECT_EQ(AccountKeyToEmail(kAccountKeyFooBar), infos[0].email); -} - TEST_F(AccountTrackerServiceTest, MigrateAccountIdToGaiaId) { base::test::ScopedFeatureList scoped_feature_list; scoped_feature_list.InitAndEnableFeature(switches::kAccountIdMigration); @@ -1249,18 +1206,19 @@ TEST_F(AccountTrackerServiceTest, MigrateAccountIdToGaiaId) { const std::string gaia_beta = AccountKeyToGaiaId(kAccountKeyBeta); ListPrefUpdate update(prefs(), prefs::kAccountInfo); + base::Value::List& update_list = update->GetList(); - base::Value dict(base::Value::Type::DICTIONARY); - dict.SetStringKey("account_id", email_alpha); - dict.SetStringKey("email", email_alpha); - dict.SetStringKey("gaia", gaia_alpha); - update->Append(std::move(dict)); + base::Value::Dict dict; + dict.Set("account_id", email_alpha); + dict.Set("email", email_alpha); + dict.Set("gaia", gaia_alpha); + update_list.Append(std::move(dict)); - dict = base::Value(base::Value::Type::DICTIONARY); - dict.SetStringKey("account_id", email_beta); - dict.SetStringKey("email", email_beta); - dict.SetStringKey("gaia", gaia_beta); - update->Append(std::move(dict)); + dict = base::Value::Dict(); + dict.Set("account_id", email_beta); + dict.Set("email", email_beta); + dict.Set("gaia", gaia_beta); + update_list.Append(std::move(dict)); base::HistogramTester tester; ResetAccountTracker(); @@ -1295,18 +1253,19 @@ TEST_F(AccountTrackerServiceTest, CanNotMigrateAccountIdToGaiaId) { const std::string email_beta = AccountKeyToEmail(kAccountKeyBeta); ListPrefUpdate update(prefs(), prefs::kAccountInfo); + base::Value::List& update_list = update->GetList(); - base::Value dict(base::Value::Type::DICTIONARY); - dict.SetStringKey("account_id", email_alpha); - dict.SetStringKey("email", email_alpha); - dict.SetStringKey("gaia", gaia_alpha); - update->Append(std::move(dict)); + base::Value::Dict dict; + dict.Set("account_id", email_alpha); + dict.Set("email", email_alpha); + dict.Set("gaia", gaia_alpha); + update_list.Append(std::move(dict)); - dict = base::Value(base::Value::Type::DICTIONARY); - dict.SetStringKey("account_id", email_beta); - dict.SetStringKey("email", email_beta); - dict.SetStringKey("gaia", ""); - update->Append(std::move(dict)); + dict = base::Value::Dict(); + dict.Set("account_id", email_beta); + dict.Set("email", email_beta); + dict.Set("gaia", ""); + update_list.Append(std::move(dict)); base::HistogramTester tester; ResetAccountTracker(); @@ -1342,25 +1301,26 @@ TEST_F(AccountTrackerServiceTest, GaiaIdMigrationCrashInTheMiddle) { const std::string gaia_beta = AccountKeyToGaiaId(kAccountKeyBeta); ListPrefUpdate update(prefs(), prefs::kAccountInfo); + base::Value::List& update_list = update->GetList(); - base::Value dict(base::Value::Type::DICTIONARY); - dict.SetStringKey("account_id", email_alpha); - dict.SetStringKey("email", email_alpha); - dict.SetStringKey("gaia", gaia_alpha); - update->Append(std::move(dict)); + base::Value::Dict dict; + dict.Set("account_id", email_alpha); + dict.Set("email", email_alpha); + dict.Set("gaia", gaia_alpha); + update_list.Append(std::move(dict)); - dict = base::Value(base::Value::Type::DICTIONARY); - dict.SetStringKey("account_id", email_beta); - dict.SetStringKey("email", email_beta); - dict.SetStringKey("gaia", gaia_beta); - update->Append(std::move(dict)); + dict = base::Value::Dict(); + dict.Set("account_id", email_beta); + dict.Set("email", email_beta); + dict.Set("gaia", gaia_beta); + update_list.Append(std::move(dict)); // Succeed miggrated account. - dict = base::Value(base::Value::Type::DICTIONARY); - dict.SetStringKey("account_id", gaia_alpha); - dict.SetStringKey("email", email_alpha); - dict.SetStringKey("gaia", gaia_alpha); - update->Append(std::move(dict)); + dict = base::Value::Dict(); + dict.Set("account_id", gaia_alpha); + dict.Set("email", email_alpha); + dict.Set("gaia", gaia_alpha); + update_list.Append(std::move(dict)); base::HistogramTester tester; ResetAccountTracker(); @@ -1696,18 +1656,19 @@ TEST_F(AccountTrackerServiceTest, CountOfLoadedAccounts_TwoAccounts) { const std::string gaia_beta = AccountKeyToGaiaId(kAccountKeyBeta); ListPrefUpdate update(prefs(), prefs::kAccountInfo); + base::Value::List& update_list = update->GetList(); - base::Value dict(base::Value::Type::DICTIONARY); - dict.SetStringKey("account_id", gaia_alpha); - dict.SetStringKey("email", email_alpha); - dict.SetStringKey("gaia", gaia_alpha); - update->Append(std::move(dict)); + base::Value::Dict dict; + dict.Set("account_id", gaia_alpha); + dict.Set("email", email_alpha); + dict.Set("gaia", gaia_alpha); + update_list.Append(std::move(dict)); - dict = base::Value(base::Value::Type::DICTIONARY); - dict.SetStringKey("account_id", gaia_beta); - dict.SetStringKey("email", email_beta); - dict.SetStringKey("gaia", gaia_beta); - update->Append(std::move(dict)); + dict = base::Value::Dict(); + dict.Set("account_id", gaia_beta); + dict.Set("email", email_beta); + dict.Set("gaia", gaia_beta); + update_list.Append(std::move(dict)); base::HistogramTester tester; ResetAccountTracker(); @@ -1725,18 +1686,19 @@ TEST_F(AccountTrackerServiceTest, Migrate_CountOfLoadedAccounts_TwoAccounts) { const std::string gaia_beta = AccountKeyToGaiaId(kAccountKeyBeta); ListPrefUpdate update(prefs(), prefs::kAccountInfo); + base::Value::List& update_list = update->GetList(); - base::Value dict(base::Value::Type::DICTIONARY); - dict.SetStringKey("account_id", email_alpha); - dict.SetStringKey("email", email_alpha); - dict.SetStringKey("gaia", gaia_alpha); - update->Append(std::move(dict)); + base::Value::Dict dict; + dict.Set("account_id", email_alpha); + dict.Set("email", email_alpha); + dict.Set("gaia", gaia_alpha); + update_list.Append(std::move(dict)); - dict = base::Value(base::Value::Type::DICTIONARY); - dict.SetStringKey("account_id", email_beta); - dict.SetStringKey("email", email_beta); - dict.SetStringKey("gaia", gaia_beta); - update->Append(std::move(dict)); + dict = base::Value::Dict(); + dict.Set("account_id", email_beta); + dict.Set("email", email_beta); + dict.Set("gaia", gaia_beta); + update_list.Append(std::move(dict)); base::HistogramTester tester; ResetAccountTracker(); @@ -1754,20 +1716,21 @@ TEST_F(AccountTrackerServiceTest, const std::string gaia_foobar = AccountKeyToGaiaId(kAccountKeyFooDotBar); ListPrefUpdate update(prefs(), prefs::kAccountInfo); + base::Value::List& update_list = update->GetList(); - base::Value dict(base::Value::Type::DICTIONARY); - dict.SetStringKey("account_id", email_alpha); - dict.SetStringKey("email", email_alpha); - dict.SetStringKey("gaia", gaia_alpha); - update->Append(std::move(dict)); + base::Value::Dict dict; + dict.Set("account_id", email_alpha); + dict.Set("email", email_alpha); + dict.Set("gaia", gaia_alpha); + update_list.Append(std::move(dict)); // This account is invalid because the account_id is a non-canonicalized // version of the email. - dict = base::Value(base::Value::Type::DICTIONARY); - dict.SetStringKey("account_id", email_foobar); - dict.SetStringKey("email", email_foobar); - dict.SetStringKey("gaia", gaia_foobar); - update->Append(std::move(dict)); + dict = base::Value::Dict(); + dict.Set("account_id", email_foobar); + dict.Set("email", email_foobar); + dict.Set("gaia", gaia_foobar); + update_list.Append(std::move(dict)); base::HistogramTester tester; ResetAccountTracker(); @@ -1799,20 +1762,21 @@ TEST_F(AccountTrackerServiceTest, CapabilityPrefNameMigration) { ->GetAccountInfo(AccountKeyToAccountId(kAccountKeyAlpha)) .capabilities.can_offer_extended_chrome_sync_promos()); ListPrefUpdate update(prefs(), prefs::kAccountInfo); - ASSERT_FALSE(update->GetListDeprecated().empty()); - base::Value& dict = update->GetListDeprecated()[0]; - ASSERT_TRUE(dict.is_dict()); + base::Value::List& update_list = update->GetList(); + ASSERT_FALSE(update_list.empty()); + base::Value::Dict* dict = update_list[0].GetIfDict(); + ASSERT_TRUE(dict); const char kDeprecatedCapabilityKey[] = "accountcapabilities.can_offer_extended_chrome_sync_promos"; const char kNewCapabilityKey[] = "accountcapabilities.accountcapabilities/gi2tklldmfya"; // The deprecated key is not set. - EXPECT_FALSE(dict.FindIntPath(kDeprecatedCapabilityKey)); - EXPECT_TRUE(dict.FindIntPath(kNewCapabilityKey)); + EXPECT_FALSE(dict->FindIntByDottedPath(kDeprecatedCapabilityKey)); + EXPECT_TRUE(dict->FindIntByDottedPath(kNewCapabilityKey)); // Set the capability using the deprecated key, and reload the account. - dict.SetIntPath(kDeprecatedCapabilityKey, 1); - dict.RemovePath(kNewCapabilityKey); + dict->SetByDottedPath(kDeprecatedCapabilityKey, 1); + dict->RemoveByDottedPath(kNewCapabilityKey); ClearAccountTrackerEvents(); ResetAccountTrackerWithPersistence(scoped_user_data_dir.GetPath()); EXPECT_TRUE(CheckAccountTrackerEvents( @@ -1828,9 +1792,9 @@ TEST_F(AccountTrackerServiceTest, CapabilityPrefNameMigration) { EXPECT_EQ(signin::Tribool::kTrue, infos[0].capabilities.can_offer_extended_chrome_sync_promos()); // The deprecated key has been removed. - EXPECT_FALSE(dict.FindIntPath(kDeprecatedCapabilityKey)); + EXPECT_FALSE(dict->FindIntByDottedPath(kDeprecatedCapabilityKey)); // The new key has been written. - absl::optional<int> new_key = dict.FindIntPath(kNewCapabilityKey); + absl::optional<int> new_key = dict->FindIntByDottedPath(kNewCapabilityKey); ASSERT_TRUE(new_key.has_value()); EXPECT_EQ(static_cast<int>(signin::Tribool::kTrue), new_key.value()); } diff --git a/chromium/components/signin/internal/identity_manager/fake_profile_oauth2_token_service_delegate.cc b/chromium/components/signin/internal/identity_manager/fake_profile_oauth2_token_service_delegate.cc index c54f007bd63..54756a6d4eb 100644 --- a/chromium/components/signin/internal/identity_manager/fake_profile_oauth2_token_service_delegate.cc +++ b/chromium/components/signin/internal/identity_manager/fake_profile_oauth2_token_service_delegate.cc @@ -13,34 +13,11 @@ #include "google_apis/gaia/gaia_access_token_fetcher.h" #include "google_apis/gaia/gaia_constants.h" -namespace { -// Values used from |MutableProfileOAuth2TokenServiceDelegate|. -const net::BackoffEntry::Policy kBackoffPolicy = { - 0 /* int num_errors_to_ignore */, - - 1000 /* int initial_delay_ms */, - - 2.0 /* double multiply_factor */, - - 0.2 /* double jitter_factor */, - - 15 * 60 * 1000 /* int64_t maximum_backoff_ms */, - - -1 /* int64_t entry_lifetime_ms */, - - false /* bool always_use_initial_delay */, -}; -} // namespace - -FakeProfileOAuth2TokenServiceDelegate::AccountInfo::AccountInfo( - const std::string& refresh_token) - : refresh_token(refresh_token), error(GoogleServiceAuthError::NONE) {} - FakeProfileOAuth2TokenServiceDelegate::FakeProfileOAuth2TokenServiceDelegate() - : shared_factory_( + : ProfileOAuth2TokenServiceDelegate(/*use_backoff=*/true), + shared_factory_( base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>( - &test_url_loader_factory_)), - backoff_entry_(&kBackoffPolicy) {} + &test_url_loader_factory_)) {} FakeProfileOAuth2TokenServiceDelegate:: ~FakeProfileOAuth2TokenServiceDelegate() = default; @@ -54,7 +31,7 @@ FakeProfileOAuth2TokenServiceDelegate::CreateAccessTokenFetcher( DCHECK(it != refresh_tokens_.end()); return GaiaAccessTokenFetcher:: CreateExchangeRefreshTokenForAccessTokenInstance( - consumer, url_loader_factory, it->second->refresh_token); + consumer, url_loader_factory, it->second); } bool FakeProfileOAuth2TokenServiceDelegate::RefreshTokenIsAvailable( @@ -62,26 +39,14 @@ bool FakeProfileOAuth2TokenServiceDelegate::RefreshTokenIsAvailable( return !GetRefreshToken(account_id).empty(); } -GoogleServiceAuthError FakeProfileOAuth2TokenServiceDelegate::GetAuthError( - const CoreAccountId& account_id) const { - auto it = refresh_tokens_.find(account_id); - return (it == refresh_tokens_.end()) ? GoogleServiceAuthError::AuthErrorNone() - : it->second->error; -} - std::string FakeProfileOAuth2TokenServiceDelegate::GetRefreshToken( const CoreAccountId& account_id) const { auto it = refresh_tokens_.find(account_id); if (it != refresh_tokens_.end()) - return it->second->refresh_token; + return it->second; return std::string(); } -const net::BackoffEntry* FakeProfileOAuth2TokenServiceDelegate::BackoffEntry() - const { - return &backoff_entry_; -} - std::vector<CoreAccountId> FakeProfileOAuth2TokenServiceDelegate::GetAccounts() const { std::vector<CoreAccountId> account_ids; @@ -123,23 +88,27 @@ void FakeProfileOAuth2TokenServiceDelegate::IssueRefreshTokenForUser( if (token.empty()) { base::Erase(account_ids_, account_id); refresh_tokens_.erase(account_id); + ClearAuthError(account_id); FireRefreshTokenRevoked(account_id); } else { // Look for the account ID in the list, and if it is not present append it. if (base::ranges::find(account_ids_, account_id) == account_ids_.end()) { account_ids_.push_back(account_id); } - refresh_tokens_[account_id] = std::make_unique<AccountInfo>(token); + refresh_tokens_[account_id] = token; // If the token is a special "invalid" value, then that means the token was // rejected by the client and is thus not valid. So set the appropriate // error in that case. This logic is essentially duplicated from // MutableProfileOAuth2TokenServiceDelegate. - if (token == GaiaConstants::kInvalidRefreshToken) { - refresh_tokens_[account_id]->error = - GoogleServiceAuthError::FromInvalidGaiaCredentialsReason( - GoogleServiceAuthError::InvalidGaiaCredentialsReason:: - CREDENTIALS_REJECTED_BY_CLIENT); - } + GoogleServiceAuthError error = + token == GaiaConstants::kInvalidRefreshToken + ? GoogleServiceAuthError::FromInvalidGaiaCredentialsReason( + GoogleServiceAuthError::InvalidGaiaCredentialsReason:: + CREDENTIALS_REJECTED_BY_CLIENT) + : GoogleServiceAuthError(GoogleServiceAuthError::NONE); + UpdateAuthError(account_id, error, + /*fire_auth_error_changed=*/false); + FireRefreshTokenAvailable(account_id); } } @@ -154,8 +123,7 @@ void FakeProfileOAuth2TokenServiceDelegate::ExtractCredentials( const CoreAccountId& account_id) { auto it = refresh_tokens_.find(account_id); DCHECK(it != refresh_tokens_.end()); - to_service->GetDelegate()->UpdateCredentials(account_id, - it->second->refresh_token); + to_service->GetDelegate()->UpdateCredentials(account_id, it->second); RevokeCredentials(account_id); } @@ -168,25 +136,6 @@ bool FakeProfileOAuth2TokenServiceDelegate::FixRequestErrorIfPossible() { return fix_request_if_possible_; } -void FakeProfileOAuth2TokenServiceDelegate::UpdateAuthError( - const CoreAccountId& account_id, - const GoogleServiceAuthError& error) { - backoff_entry_.InformOfRequest(!error.IsTransientError()); - // Drop transient errors to match ProfileOAuth2TokenService's stated contract - // for GetAuthError() and to allow clients to test proper behavior in the case - // of transient errors. - if (error.IsTransientError()) - return; - - if (GetAuthError(account_id) == error) - return; - - auto it = refresh_tokens_.find(account_id); - DCHECK(it != refresh_tokens_.end()); - it->second->error = error; - FireAuthErrorChanged(account_id, error); -} - #if BUILDFLAG(IS_ANDROID) base::android::ScopedJavaLocalRef<jobject> FakeProfileOAuth2TokenServiceDelegate::GetJavaObject() { diff --git a/chromium/components/signin/internal/identity_manager/fake_profile_oauth2_token_service_delegate.h b/chromium/components/signin/internal/identity_manager/fake_profile_oauth2_token_service_delegate.h index 8aa89396dcb..8bf8cc938e5 100644 --- a/chromium/components/signin/internal/identity_manager/fake_profile_oauth2_token_service_delegate.h +++ b/chromium/components/signin/internal/identity_manager/fake_profile_oauth2_token_service_delegate.h @@ -39,10 +39,6 @@ class FakeProfileOAuth2TokenServiceDelegate // Overriden to make sure it works on Android. bool RefreshTokenIsAvailable(const CoreAccountId& account_id) const override; - GoogleServiceAuthError GetAuthError( - const CoreAccountId& account_id) const override; - void UpdateAuthError(const CoreAccountId& account_id, - const GoogleServiceAuthError& error) override; std::vector<CoreAccountId> GetAccounts() const override; void RevokeAllCredentials() override; void LoadCredentials(const CoreAccountId& primary_account_id, @@ -68,16 +64,7 @@ class FakeProfileOAuth2TokenServiceDelegate fix_request_if_possible_ = value; } - const net::BackoffEntry* BackoffEntry() const override; - private: - struct AccountInfo { - AccountInfo(const std::string& refresh_token); - - const std::string refresh_token; - GoogleServiceAuthError error; - }; - void IssueRefreshTokenForUser(const CoreAccountId& account_id, const std::string& token); @@ -89,13 +76,11 @@ class FakeProfileOAuth2TokenServiceDelegate // A given account ID appears at most once in this list. std::list<CoreAccountId> account_ids_; - // Maps account ids to info. - std::map<CoreAccountId, std::unique_ptr<AccountInfo>> refresh_tokens_; + // Maps account ids to tokens. + std::map<CoreAccountId, std::string> refresh_tokens_; network::TestURLLoaderFactory test_url_loader_factory_; scoped_refptr<network::SharedURLLoaderFactory> shared_factory_; bool fix_request_if_possible_ = false; - - net::BackoffEntry backoff_entry_; }; #endif // COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_FAKE_PROFILE_OAUTH2_TOKEN_SERVICE_DELEGATE_H_ diff --git a/chromium/components/signin/internal/identity_manager/mutable_profile_oauth2_token_service_delegate.cc b/chromium/components/signin/internal/identity_manager/mutable_profile_oauth2_token_service_delegate.cc index 0e2e638171b..60d7e017270 100644 --- a/chromium/components/signin/internal/identity_manager/mutable_profile_oauth2_token_service_delegate.cc +++ b/chromium/components/signin/internal/identity_manager/mutable_profile_oauth2_token_service_delegate.cc @@ -197,9 +197,8 @@ MutableProfileOAuth2TokenServiceDelegate:: signin::AccountConsistencyMethod account_consistency, bool revoke_all_tokens_on_load, FixRequestErrorCallback fix_request_error_callback) - : web_data_service_request_(0), - backoff_entry_(&backoff_policy_), - backoff_error_(GoogleServiceAuthError::NONE), + : ProfileOAuth2TokenServiceDelegate(/*use_backoff=*/true), + web_data_service_request_(0), client_(client), account_tracker_service_(account_tracker_service), network_connection_tracker_(network_connection_tracker), @@ -212,14 +211,6 @@ MutableProfileOAuth2TokenServiceDelegate:: DCHECK(account_tracker_service_); DCHECK(network_connection_tracker_); DCHECK_NE(signin::AccountConsistencyMethod::kMirror, account_consistency_); - // It's okay to fill the backoff policy after being used in construction. - backoff_policy_.num_errors_to_ignore = 0; - backoff_policy_.initial_delay_ms = 1000; - backoff_policy_.multiply_factor = 2.0; - backoff_policy_.jitter_factor = 0.2; - backoff_policy_.maximum_backoff_ms = 15 * 60 * 1000; - backoff_policy_.entry_lifetime_ms = -1; - backoff_policy_.always_use_initial_delay = false; network_connection_tracker_->AddNetworkConnectionObserver(this); } @@ -237,17 +228,18 @@ MutableProfileOAuth2TokenServiceDelegate::CreateAccessTokenFetcher( OAuth2AccessTokenConsumer* consumer) { ValidateAccountId(account_id); // check whether the account has persistent error. - if (refresh_tokens_[account_id].last_auth_error.IsPersistentError()) { + GoogleServiceAuthError auth_error = GetAuthError(account_id); + if (auth_error.IsPersistentError()) { VLOG(1) << "Request for token has been rejected due to persistent error #" - << refresh_tokens_[account_id].last_auth_error.state(); - return std::make_unique<OAuth2AccessTokenFetcherImmediateError>( - consumer, refresh_tokens_[account_id].last_auth_error); + << auth_error.state(); + return std::make_unique<OAuth2AccessTokenFetcherImmediateError>(consumer, + auth_error); } - if (backoff_entry_.ShouldRejectRequest()) { + if (BackoffEntry()->ShouldRejectRequest()) { VLOG(1) << "Request for token has been rejected due to backoff rules from" - << " previous error #" << backoff_error_.state(); + << " previous error #" << BackOffError().state(); return std::make_unique<OAuth2AccessTokenFetcherImmediateError>( - consumer, backoff_error_); + consumer, BackOffError()); } std::string refresh_token = GetRefreshToken(account_id); DCHECK(!refresh_token.empty()); @@ -256,53 +248,14 @@ MutableProfileOAuth2TokenServiceDelegate::CreateAccessTokenFetcher( consumer, url_loader_factory, refresh_token); } -GoogleServiceAuthError MutableProfileOAuth2TokenServiceDelegate::GetAuthError( - const CoreAccountId& account_id) const { - auto it = refresh_tokens_.find(account_id); - return (it == refresh_tokens_.end()) ? GoogleServiceAuthError::AuthErrorNone() - : it->second.last_auth_error; -} - -void MutableProfileOAuth2TokenServiceDelegate::UpdateAuthError( - const CoreAccountId& account_id, - const GoogleServiceAuthError& error) { - VLOG(1) << "MutablePO2TS::UpdateAuthError. Error: " << error.state() - << " account_id=" << account_id; - backoff_entry_.InformOfRequest(!error.IsTransientError()); - ValidateAccountId(account_id); - - // Do not report connection errors as these are not actually auth errors. - // We also want to avoid masking a "real" auth error just because we - // subsequently get a transient network error. We do keep it around though - // to report for future requests being denied for "backoff" reasons. - if (error.IsTransientError()) { - backoff_error_ = error; - return; - } - - if (refresh_tokens_.count(account_id) == 0) { - // This could happen if the preferences have been corrupted (see - // http://crbug.com/321370). In a Debug build that would be a bug, but in a - // Release build we want to deal with it gracefully. - NOTREACHED(); - return; - } - - AccountStatus* status = &refresh_tokens_[account_id]; - if (error != status->last_auth_error) { - status->last_auth_error = error; - FireAuthErrorChanged(account_id, error); - } -} - std::string MutableProfileOAuth2TokenServiceDelegate::GetTokenForMultilogin( const CoreAccountId& account_id) const { auto iter = refresh_tokens_.find(account_id); if (iter == refresh_tokens_.end() || - iter->second.last_auth_error != GoogleServiceAuthError::AuthErrorNone()) { + GetAuthError(account_id) != GoogleServiceAuthError::AuthErrorNone()) { return std::string(); } - const std::string& refresh_token = iter->second.refresh_token; + const std::string& refresh_token = iter->second; DCHECK(!refresh_token.empty()); return refresh_token; } @@ -317,7 +270,7 @@ std::string MutableProfileOAuth2TokenServiceDelegate::GetRefreshToken( const CoreAccountId& account_id) const { auto iter = refresh_tokens_.find(account_id); if (iter != refresh_tokens_.end()) { - const std::string refresh_token = iter->second.refresh_token; + const std::string refresh_token = iter->second; DCHECK(!refresh_token.empty()); return refresh_token; } @@ -368,6 +321,7 @@ void MutableProfileOAuth2TokenServiceDelegate::LoadCredentials( DCHECK_EQ(0, web_data_service_request_); refresh_tokens_.clear(); + ClearAuthError(absl::nullopt); if (!token_web_data_) { // This case only exists in unit tests that do not care about loading @@ -531,7 +485,7 @@ void MutableProfileOAuth2TokenServiceDelegate::UpdateCredentialsInMemory( // If token present, and different from the new one, cancel its requests, // and clear the entries in cache related to that account. if (refresh_token_present) { - DCHECK_NE(refresh_token, refresh_tokens_[account_id].refresh_token); + DCHECK_NE(refresh_token, refresh_tokens_[account_id]); VLOG(1) << "MutablePO2TS::UpdateCredentials; Refresh Token was present. " << "account_id=" << account_id; @@ -547,9 +501,9 @@ void MutableProfileOAuth2TokenServiceDelegate::UpdateCredentialsInMemory( // would also be invalidated server-side). // See http://crbug.com/865189 for more information about this regression. if (is_refresh_token_invalidated) - RevokeCredentialsOnServer(refresh_tokens_[account_id].refresh_token); + RevokeCredentialsOnServer(refresh_tokens_[account_id]); - refresh_tokens_[account_id].refresh_token = refresh_token; + refresh_tokens_[account_id] = refresh_token; UpdateAuthError(account_id, error); } else { VLOG(1) << "MutablePO2TS::UpdateCredentials; Refresh Token was absent. " @@ -658,12 +612,7 @@ void MutableProfileOAuth2TokenServiceDelegate::OnConnectionChanged( network::mojom::ConnectionType type) { // If our network has changed, reset the backoff timer so that errors caused // by a previous lack of network connectivity don't prevent new requests. - backoff_entry_.Reset(); -} - -const net::BackoffEntry* -MutableProfileOAuth2TokenServiceDelegate::BackoffEntry() const { - return &backoff_entry_; + ResetBackOffEntry(); } bool MutableProfileOAuth2TokenServiceDelegate::FixRequestErrorIfPossible() { @@ -677,7 +626,8 @@ void MutableProfileOAuth2TokenServiceDelegate::AddAccountStatus( const std::string& refresh_token, const GoogleServiceAuthError& error) { DCHECK_EQ(0u, refresh_tokens_.count(account_id)); - refresh_tokens_[account_id] = AccountStatus{refresh_token, error}; + refresh_tokens_[account_id] = refresh_token; + UpdateAuthError(account_id, error, /*fire_auth_error_changed=*/false); FireAuthErrorChanged(account_id, error); } @@ -694,10 +644,10 @@ void MutableProfileOAuth2TokenServiceDelegate::RevokeCredentialsImpl( if (refresh_tokens_.count(account_id) > 0) { VLOG(1) << "MutablePO2TS::RevokeCredentials for account_id=" << account_id; ScopedBatchChange batch(this); - const std::string& token = refresh_tokens_[account_id].refresh_token; if (revoke_on_server) - RevokeCredentialsOnServer(token); + RevokeCredentialsOnServer(refresh_tokens_[account_id]); refresh_tokens_.erase(account_id); + ClearAuthError(account_id); ClearPersistedCredentials(account_id); FireRefreshTokenRevoked(account_id); } diff --git a/chromium/components/signin/internal/identity_manager/mutable_profile_oauth2_token_service_delegate.h b/chromium/components/signin/internal/identity_manager/mutable_profile_oauth2_token_service_delegate.h index 82a113efdd0..4fa67d16f6c 100644 --- a/chromium/components/signin/internal/identity_manager/mutable_profile_oauth2_token_service_delegate.h +++ b/chromium/components/signin/internal/identity_manager/mutable_profile_oauth2_token_service_delegate.h @@ -55,16 +55,9 @@ class MutableProfileOAuth2TokenServiceDelegate scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, OAuth2AccessTokenConsumer* consumer) override; - // Updates the internal cache of the result from the most-recently-completed - // auth request (used for reporting errors to the user). - void UpdateAuthError(const CoreAccountId& account_id, - const GoogleServiceAuthError& error) override; - std::string GetTokenForMultilogin( const CoreAccountId& account_id) const override; bool RefreshTokenIsAvailable(const CoreAccountId& account_id) const override; - GoogleServiceAuthError GetAuthError( - const CoreAccountId& account_id) const override; std::vector<CoreAccountId> GetAccounts() const override; scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory() const override; @@ -81,9 +74,6 @@ class MutableProfileOAuth2TokenServiceDelegate // Overridden from NetworkConnectionTracker::NetworkConnectionObserver. void OnConnectionChanged(network::mojom::ConnectionType type) override; - // Overridden from ProfileOAuth2TokenServiceDelegate. - const net::BackoffEntry* BackoffEntry() const override; - bool FixRequestErrorIfPossible() override; // Returns the account's refresh token used for testing purposes. @@ -94,11 +84,6 @@ class MutableProfileOAuth2TokenServiceDelegate class RevokeServerRefreshToken; - struct AccountStatus { - std::string refresh_token; - GoogleServiceAuthError last_auth_error; - }; - FRIEND_TEST_ALL_PREFIXES(MutableProfileOAuth2TokenServiceDelegateTest, PersistenceDBUpgrade); FRIEND_TEST_ALL_PREFIXES(MutableProfileOAuth2TokenServiceDelegateTest, @@ -132,8 +117,6 @@ class MutableProfileOAuth2TokenServiceDelegate FRIEND_TEST_ALL_PREFIXES(MutableProfileOAuth2TokenServiceDelegateTest, GetAccounts); FRIEND_TEST_ALL_PREFIXES(MutableProfileOAuth2TokenServiceDelegateTest, - RetryBackoff); - FRIEND_TEST_ALL_PREFIXES(MutableProfileOAuth2TokenServiceDelegateTest, CanonicalizeAccountId); FRIEND_TEST_ALL_PREFIXES(MutableProfileOAuth2TokenServiceDelegateTest, CanonAndNonCanonAccountId); @@ -196,11 +179,8 @@ class MutableProfileOAuth2TokenServiceDelegate void RevokeCredentialsImpl(const CoreAccountId& account_id, bool revoke_on_server); - // Maps the |account_id| of accounts known to ProfileOAuth2TokenService - // to information about the account. - typedef std::map<CoreAccountId, AccountStatus> AccountStatusMap; // In memory refresh token store mapping account_id to refresh_token. - AccountStatusMap refresh_tokens_; + std::map<CoreAccountId, std::string> refresh_tokens_; // Handle to the request reading tokens from database. WebDataServiceBase::Handle web_data_service_request_; @@ -215,11 +195,6 @@ class MutableProfileOAuth2TokenServiceDelegate // this instance was created. THREAD_CHECKER(thread_checker_); - // Used to rate-limit network token requests so as to not overload the server. - net::BackoffEntry::Policy backoff_policy_; - net::BackoffEntry backoff_entry_; - GoogleServiceAuthError backoff_error_; - raw_ptr<SigninClient> client_; raw_ptr<AccountTrackerService> account_tracker_service_; raw_ptr<network::NetworkConnectionTracker> network_connection_tracker_; diff --git a/chromium/components/signin/internal/identity_manager/mutable_profile_oauth2_token_service_delegate_unittest.cc b/chromium/components/signin/internal/identity_manager/mutable_profile_oauth2_token_service_delegate_unittest.cc index 97c523d76e3..498242e4700 100644 --- a/chromium/components/signin/internal/identity_manager/mutable_profile_oauth2_token_service_delegate_unittest.cc +++ b/chromium/components/signin/internal/identity_manager/mutable_profile_oauth2_token_service_delegate_unittest.cc @@ -264,8 +264,7 @@ TEST_F(MutableProfileOAuth2TokenServiceDelegateTest, PersistenceDBUpgrade) { EXPECT_TRUE( oauth2_service_delegate_->RefreshTokenIsAvailable(primary_account_id)); EXPECT_EQ(GaiaConstants::kInvalidRefreshToken, - oauth2_service_delegate_->refresh_tokens_[primary_account_id] - .refresh_token); + oauth2_service_delegate_->refresh_tokens_[primary_account_id]); } TEST_F(MutableProfileOAuth2TokenServiceDelegateTest, @@ -406,13 +405,13 @@ TEST_F(MutableProfileOAuth2TokenServiceDelegateTest, // LoadCredentials() guarantees that the account given to it as argument // is in the refresh_token map. EXPECT_EQ(1U, oauth2_service_delegate_->refresh_tokens_.size()); - EXPECT_EQ( - GaiaConstants::kInvalidRefreshToken, - oauth2_service_delegate_->refresh_tokens_[account_id].refresh_token); + EXPECT_EQ(GaiaConstants::kInvalidRefreshToken, + oauth2_service_delegate_->refresh_tokens_[account_id]); // Setup a DB with tokens that don't require upgrade and clear memory. oauth2_service_delegate_->UpdateCredentials(account_id, "refresh_token"); oauth2_service_delegate_->UpdateCredentials(account_id2, "refresh_token2"); oauth2_service_delegate_->refresh_tokens_.clear(); + oauth2_service_delegate_->ClearAuthError(absl::nullopt); EXPECT_EQ(2, end_batch_changes_); EXPECT_EQ(2, auth_error_changed_count_); ResetObserverCounts(); @@ -477,6 +476,7 @@ TEST_F(MutableProfileOAuth2TokenServiceDelegateTest, oauth2_service_delegate_->UpdateCredentials(account_id, "refresh_token"); oauth2_service_delegate_->UpdateCredentials(account_id2, "refresh_token2"); oauth2_service_delegate_->refresh_tokens_.clear(); + oauth2_service_delegate_->ClearAuthError(absl::nullopt); EXPECT_EQ(2, end_batch_changes_); EXPECT_EQ(2, auth_error_changed_count_); ResetObserverCounts(); @@ -862,11 +862,11 @@ TEST_F(MutableProfileOAuth2TokenServiceDelegateTest, RetryBackoff) { EXPECT_EQ(0, access_token_success_count_); EXPECT_EQ(1, access_token_failure_count_); // Expect a positive backoff time. - EXPECT_GT(oauth2_service_delegate_->backoff_entry_.GetTimeUntilRelease(), + EXPECT_GT(oauth2_service_delegate_->BackoffEntry()->GetTimeUntilRelease(), base::TimeDelta()); // Pretend that backoff has expired and try again. - oauth2_service_delegate_->backoff_entry_.SetCustomReleaseTime( + oauth2_service_delegate_->backoff_entry_->SetCustomReleaseTime( base::TimeTicks()); std::unique_ptr<OAuth2AccessTokenFetcher> fetcher2 = oauth2_service_delegate_->CreateAccessTokenFetcher( diff --git a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate.cc b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate.cc index 925866b9d79..be42bdf8826 100644 --- a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate.cc +++ b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate.cc @@ -9,6 +9,26 @@ #include "google_apis/gaia/oauth2_access_token_consumer.h" #include "services/network/public/cpp/shared_url_loader_factory.h" +namespace { + +const net::BackoffEntry::Policy kBackoffPolicy = { + 0 /* int num_errors_to_ignore */, + + 1000 /* int initial_delay_ms */, + + 2.0 /* double multiply_factor */, + + 0.2 /* double jitter_factor */, + + 15 * 60 * 1000 /* int64_t maximum_backoff_ms (15 minutes) */, + + -1 /* int64_t entry_lifetime_ms */, + + false /* bool always_use_initial_delay */, +}; + +} // namespace + ProfileOAuth2TokenServiceDelegate::ScopedBatchChange::ScopedBatchChange( ProfileOAuth2TokenServiceDelegate* delegate) : delegate_(delegate) { @@ -20,8 +40,12 @@ ProfileOAuth2TokenServiceDelegate::ScopedBatchChange::~ScopedBatchChange() { delegate_->EndBatchChanges(); } -ProfileOAuth2TokenServiceDelegate::ProfileOAuth2TokenServiceDelegate() - : batch_change_depth_(0) {} +ProfileOAuth2TokenServiceDelegate::ProfileOAuth2TokenServiceDelegate( + bool use_backoff) + : batch_change_depth_(0) { + if (use_backoff) + backoff_entry_ = std::make_unique<net::BackoffEntry>(&kBackoffPolicy); +} ProfileOAuth2TokenServiceDelegate::~ProfileOAuth2TokenServiceDelegate() = default; @@ -107,11 +131,6 @@ ProfileOAuth2TokenServiceDelegate::GetURLLoaderFactory() const { return nullptr; } -GoogleServiceAuthError ProfileOAuth2TokenServiceDelegate::GetAuthError( - const CoreAccountId& account_id) const { - return GoogleServiceAuthError::AuthErrorNone(); -} - std::vector<CoreAccountId> ProfileOAuth2TokenServiceDelegate::GetAccounts() const { return std::vector<CoreAccountId>(); @@ -119,7 +138,7 @@ std::vector<CoreAccountId> ProfileOAuth2TokenServiceDelegate::GetAccounts() const net::BackoffEntry* ProfileOAuth2TokenServiceDelegate::BackoffEntry() const { - return nullptr; + return backoff_entry_.get(); } void ProfileOAuth2TokenServiceDelegate::LoadCredentials( @@ -140,3 +159,83 @@ void ProfileOAuth2TokenServiceDelegate::ExtractCredentials( bool ProfileOAuth2TokenServiceDelegate::FixRequestErrorIfPossible() { return false; } + +GoogleServiceAuthError ProfileOAuth2TokenServiceDelegate::GetAuthError( + const CoreAccountId& account_id) const { + auto it = errors_.find(account_id); + return (it == errors_.end()) ? GoogleServiceAuthError::AuthErrorNone() + : it->second; +} + +void ProfileOAuth2TokenServiceDelegate::UpdateAuthError( + const CoreAccountId& account_id, + const GoogleServiceAuthError& error, + bool fire_auth_error_changed) { + DVLOG(1) << "ProfileOAuth2TokenServiceDelegate::UpdateAuthError" + << " account=" << account_id << " error=" << error.ToString(); + + if (!RefreshTokenIsAvailable(account_id)) { + DLOG(ERROR) << "Update auth error failed because account=" + << account_id.ToString() << "has no refresh token"; + DCHECK_EQ(GetAuthError(account_id), + GoogleServiceAuthError::AuthErrorNone()); + return; + } + + if (backoff_entry_) { + backoff_entry_->InformOfRequest(!error.IsTransientError()); + backoff_error_ = error; + } + ValidateAccountId(account_id); + + // Do not report connection errors as these are not actually auth errors. + // We also want to avoid masking a "real" auth error just because we + // subsequently get a transient network error. We do keep it around though + // to report for future requests being denied for "backoff" reasons. + if (error.IsTransientError()) + return; + + // Scope errors are only relevant to the scope set of the request and it does + // not imply that the account is in an error state. + if (error.IsScopePersistentError()) + return; + + auto it = errors_.find(account_id); + if (error.state() == GoogleServiceAuthError::NONE) { + if (it == errors_.end()) + return; + errors_.erase(it); + } else { + if (it != errors_.end() && it->second == error) + return; + errors_[account_id] = error; + } + + if (fire_auth_error_changed) + FireAuthErrorChanged(account_id, error); +} + +void ProfileOAuth2TokenServiceDelegate::ClearAuthError( + const absl::optional<CoreAccountId>& account_id) { + if (!account_id.has_value()) { + errors_.clear(); + return; + } + + auto it = errors_.find(account_id.value()); + if (it != errors_.end()) + errors_.erase(it); +} + +GoogleServiceAuthError ProfileOAuth2TokenServiceDelegate::BackOffError() const { + return backoff_error_; +} + +void ProfileOAuth2TokenServiceDelegate::ResetBackOffEntry() { + if (!backoff_entry_) { + NOTREACHED() << "Should be called only if `use_backoff` was true in the " + "constructor."; + return; + } + backoff_entry_->Reset(); +} diff --git a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate.h b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate.h index fb6cfe1dcc9..216d89cdeb8 100644 --- a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate.h +++ b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate.h @@ -39,7 +39,7 @@ class ProfileOAuth2TokenService; // CreateAccessTokenFetcher properly. class ProfileOAuth2TokenServiceDelegate { public: - ProfileOAuth2TokenServiceDelegate(); + explicit ProfileOAuth2TokenServiceDelegate(bool use_backoff); ProfileOAuth2TokenServiceDelegate(const ProfileOAuth2TokenServiceDelegate&) = delete; @@ -61,10 +61,12 @@ class ProfileOAuth2TokenServiceDelegate { // returned by |GetAccounts|. virtual bool RefreshTokenIsAvailable( const CoreAccountId& account_id) const = 0; + virtual GoogleServiceAuthError GetAuthError( const CoreAccountId& account_id) const; virtual void UpdateAuthError(const CoreAccountId& account_id, - const GoogleServiceAuthError& error) {} + const GoogleServiceAuthError& error, + bool fire_auth_error_changed = true); // Returns a list of accounts for which a refresh token is maintained by // |this| instance, in the order the refresh tokens were added. @@ -104,8 +106,8 @@ class ProfileOAuth2TokenServiceDelegate { void AddObserver(ProfileOAuth2TokenServiceObserver* observer); void RemoveObserver(ProfileOAuth2TokenServiceObserver* observer); - // Returns a pointer to its instance of net::BackoffEntry if it has one, or - // a nullptr otherwise. + // Returns a pointer to its instance of net::BackoffEntry if it has one + // (`use_backoff` was true in the constructor), or a nullptr otherwise. virtual const net::BackoffEntry* BackoffEntry() const; // ----------------------------------------------------------------------- @@ -161,6 +163,11 @@ class ProfileOAuth2TokenServiceDelegate { load_credentials_state_ = state; } + virtual void ClearAuthError(const absl::optional<CoreAccountId>& account_id); + virtual GoogleServiceAuthError BackOffError() const; + // Can be called only if `use_backoff` was true in the constructor. + virtual void ResetBackOffEntry(); + // Called by subclasses to notify observers. void FireEndBatchChanges(); void FireRefreshTokenAvailable(const CoreAccountId& account_id); @@ -186,6 +193,13 @@ class ProfileOAuth2TokenServiceDelegate { }; private: + FRIEND_TEST_ALL_PREFIXES(MutableProfileOAuth2TokenServiceDelegateTest, + RetryBackoff); + FRIEND_TEST_ALL_PREFIXES(ProfileOAuth2TokenServiceDelegateChromeOSTest, + BackOffIsTriggerredForTransientErrors); + FRIEND_TEST_ALL_PREFIXES(ProfileOAuth2TokenServiceDelegateTest, + UpdateAuthError_TransientErrors); + // List of observers to notify when refresh token availability changes. // Makes sure list is empty on destruction. base::ObserverList<ProfileOAuth2TokenServiceObserver, true>::Unchecked @@ -200,6 +214,15 @@ class ProfileOAuth2TokenServiceDelegate { // The depth of batch changes. int batch_change_depth_; + + // If the error is transient, back off is used on some platforms to rate-limit + // network token requests so as to not overload the server. |backoff_entry_| + // is initialized only if `use_backoff` was true in the constructor. + std::unique_ptr<net::BackoffEntry> backoff_entry_; + GoogleServiceAuthError backoff_error_; + + // A map from account id to the last seen error for that account. + std::map<CoreAccountId, GoogleServiceAuthError> errors_; }; #endif // COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_PROFILE_OAUTH2_TOKEN_SERVICE_DELEGATE_H_ diff --git a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_android.cc b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_android.cc index 010b083f5a6..3f8266850dc 100644 --- a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_android.cc +++ b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_android.cc @@ -149,7 +149,8 @@ std::string AndroidAccessTokenFetcher::CombineScopes( ProfileOAuth2TokenServiceDelegateAndroid:: ProfileOAuth2TokenServiceDelegateAndroid( AccountTrackerService* account_tracker_service) - : account_tracker_service_(account_tracker_service), + : ProfileOAuth2TokenServiceDelegate(/*use_backoff=*/false), + account_tracker_service_(account_tracker_service), fire_refresh_token_loaded_(RT_LOAD_NOT_START) { DVLOG(1) << "ProfileOAuth2TokenServiceDelegateAndroid::ctor"; DCHECK(account_tracker_service_); @@ -193,35 +194,6 @@ bool ProfileOAuth2TokenServiceDelegateAndroid::RefreshTokenIsAvailable( return refresh_token_is_available == JNI_TRUE; } -GoogleServiceAuthError ProfileOAuth2TokenServiceDelegateAndroid::GetAuthError( - const CoreAccountId& account_id) const { - auto it = errors_.find(account_id); - return (it == errors_.end()) ? GoogleServiceAuthError::AuthErrorNone() - : it->second; -} - -void ProfileOAuth2TokenServiceDelegateAndroid::UpdateAuthError( - const CoreAccountId& account_id, - const GoogleServiceAuthError& error) { - DVLOG(1) << "ProfileOAuth2TokenServiceDelegateAndroid::UpdateAuthError" - << " account=" << account_id << " error=" << error.ToString(); - - if (error.IsTransientError()) - return; - - auto it = errors_.find(account_id); - if (error.state() == GoogleServiceAuthError::NONE) { - if (it == errors_.end()) - return; - errors_.erase(it); - } else { - if (it != errors_.end() && it->second == error) - return; - errors_[account_id] = error; - } - FireAuthErrorChanged(account_id, error); -} - std::vector<CoreAccountId> ProfileOAuth2TokenServiceDelegateAndroid::GetAccounts() const { return accounts_; @@ -320,7 +292,7 @@ void ProfileOAuth2TokenServiceDelegateAndroid::UpdateAccountList( << " prev_ids=" << prev_ids.size() << " curr_ids=" << curr_ids.size(); // Clear any auth errors so that client can retry to get access tokens. - errors_.clear(); + ClearAuthError(absl::nullopt); std::vector<CoreAccountId> refreshed_ids; std::vector<CoreAccountId> revoked_ids; diff --git a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_android.h b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_android.h index e452292110c..7be0aafec3f 100644 --- a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_android.h +++ b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_android.h @@ -44,10 +44,7 @@ class ProfileOAuth2TokenServiceDelegateAndroid // ProfileOAuth2TokenServiceDelegate overrides: bool RefreshTokenIsAvailable(const CoreAccountId& account_id) const override; - GoogleServiceAuthError GetAuthError( - const CoreAccountId& account_id) const override; - void UpdateAuthError(const CoreAccountId& account_id, - const GoogleServiceAuthError& error) override; + std::vector<CoreAccountId> GetAccounts() const override; // Overridden from ProfileOAuth2TokenService to complete signout of all diff --git a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos.cc b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos.cc index 9b5045e9779..65cfa247b1c 100644 --- a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos.cc +++ b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos.cc @@ -23,23 +23,6 @@ namespace signin { namespace { -// Values used from |MutableProfileOAuth2TokenServiceDelegate|. -const net::BackoffEntry::Policy kBackoffPolicy = { - 0 /* int num_errors_to_ignore */, - - 1000 /* int initial_delay_ms */, - - 2.0 /* double multiply_factor */, - - 0.2 /* double jitter_factor */, - - 15 * 60 * 1000 /* int64_t maximum_backoff_ms */, - - -1 /* int64_t entry_lifetime_ms */, - - false /* bool always_use_initial_delay */, -}; - // Maps crOS Account Manager |account_keys| to the account id representation // used by the OAuth token service chain. |account_keys| can safely contain Gaia // and non-Gaia accounts. Non-Gaia accounts will be filtered out. @@ -147,12 +130,11 @@ ProfileOAuth2TokenServiceDelegateChromeOS:: bool delete_signin_cookies_on_exit, #endif // BUILDFLAG(IS_CHROMEOS_LACROS) bool is_regular_profile) - : signin_client_(signin_client), + : ProfileOAuth2TokenServiceDelegate(/*use_backoff=*/true), + signin_client_(signin_client), account_tracker_service_(account_tracker_service), network_connection_tracker_(network_connection_tracker), account_manager_facade_(account_manager_facade), - backoff_entry_(&kBackoffPolicy), - backoff_error_(GoogleServiceAuthError::NONE), #if BUILDFLAG(IS_CHROMEOS_LACROS) delete_signin_cookies_on_exit_(delete_signin_cookies_on_exit), #endif @@ -182,28 +164,28 @@ ProfileOAuth2TokenServiceDelegateChromeOS::CreateAccessTokenFetcher( // Check if we need to reject the request. // We will reject the request if we are facing a persistent error for this // account. - auto it = errors_.find(account_id); - if (it != errors_.end() && it->second.last_auth_error.IsPersistentError()) { + GoogleServiceAuthError error = GetAuthError(account_id); + if (error.IsPersistentError()) { VLOG(1) << "Request for token has been rejected due to persistent error #" - << it->second.last_auth_error.state(); + << error.state(); // |ProfileOAuth2TokenService| will manage the lifetime of this pointer. - return std::make_unique<OAuth2AccessTokenFetcherImmediateError>( - consumer, it->second.last_auth_error); + return std::make_unique<OAuth2AccessTokenFetcherImmediateError>(consumer, + error); } // Or when we need to backoff. - if (backoff_entry_.ShouldRejectRequest()) { + if (BackoffEntry()->ShouldRejectRequest()) { VLOG(1) << "Request for token has been rejected due to backoff rules from" - << " previous error #" << backoff_error_.state(); + << " previous error #" << BackOffError().state(); // |ProfileOAuth2TokenService| will manage the lifetime of this pointer. return std::make_unique<OAuth2AccessTokenFetcherImmediateError>( - consumer, backoff_error_); + consumer, BackOffError()); } return account_manager_facade_->CreateAccessTokenFetcher( account_manager::AccountKey{ account_tracker_service_->GetAccountInfo(account_id).gaia, account_manager::AccountType::kGaia} /* account_key */, - consumer->GetConsumerName(), consumer); + consumer); } // Note: This method should use the same logic for filtering accounts as @@ -224,56 +206,6 @@ bool ProfileOAuth2TokenServiceDelegateChromeOS::RefreshTokenIsAvailable( account_id); } -void ProfileOAuth2TokenServiceDelegateChromeOS::UpdateAuthError( - const CoreAccountId& account_id, - const GoogleServiceAuthError& error) { - UpdateAuthErrorInternal(account_id, error, /*fire_auth_error_changed=*/true); -} - -void ProfileOAuth2TokenServiceDelegateChromeOS::UpdateAuthErrorInternal( - const CoreAccountId& account_id, - const GoogleServiceAuthError& error, - bool fire_auth_error_changed) { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); - - backoff_entry_.InformOfRequest(!error.IsTransientError()); - ValidateAccountId(account_id); - if (error.IsTransientError()) { - backoff_error_ = error; - return; - } - - auto it = errors_.find(account_id); - if (it != errors_.end()) { - if (error == it->second.last_auth_error) - return; - // Update the existing error. - if (error.state() == GoogleServiceAuthError::NONE) - errors_.erase(it); - else - it->second.last_auth_error = error; - if (fire_auth_error_changed) { - FireAuthErrorChanged(account_id, error); - } - } else if (error.state() != GoogleServiceAuthError::NONE) { - // Add a new error. - errors_.emplace(account_id, AccountErrorStatus{error}); - if (fire_auth_error_changed) { - FireAuthErrorChanged(account_id, error); - } - } -} - -GoogleServiceAuthError ProfileOAuth2TokenServiceDelegateChromeOS::GetAuthError( - const CoreAccountId& account_id) const { - auto it = errors_.find(account_id); - if (it != errors_.end()) { - return it->second.last_auth_error; - } - - return GoogleServiceAuthError::AuthErrorNone(); -} - // Note: This method should use the same logic for filtering accounts as // |RefreshTokenIsAvailable|. See crbug.com/919793 for details. At the time of // writing, both |GetAccounts| and |RefreshTokenIsAvailable| use @@ -458,8 +390,8 @@ void ProfileOAuth2TokenServiceDelegateChromeOS::FinishAddingPendingAccount( // Don't call |FireAuthErrorChanged|, since we call it at the end of this // function. - UpdateAuthErrorInternal(account_id, error, - /*fire_auth_error_changed=*/false); + UpdateAuthError(account_id, error, + /*fire_auth_error_changed=*/false); ScopedBatchChange batch(this); FireRefreshTokenAvailable(account_id); @@ -524,8 +456,7 @@ void ProfileOAuth2TokenServiceDelegateChromeOS::OnAccountRemoved( ->FindAccountInfoByGaiaId(account.key.id() /* gaia_id */) .account_id; DCHECK(!account_id.empty()); - UpdateAuthErrorInternal(account_id, GoogleServiceAuthError::AuthErrorNone(), - /*fire_auth_error_changed=*/false); + ClearAuthError(account_id); ScopedBatchChange batch(this); @@ -560,14 +491,9 @@ void ProfileOAuth2TokenServiceDelegateChromeOS::RevokeAllCredentials() { #endif } -const net::BackoffEntry* -ProfileOAuth2TokenServiceDelegateChromeOS::BackoffEntry() const { - return &backoff_entry_; -} - void ProfileOAuth2TokenServiceDelegateChromeOS::OnConnectionChanged( network::mojom::ConnectionType type) { - backoff_entry_.Reset(); + ResetBackOffEntry(); } } // namespace signin diff --git a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos.h b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos.h index eb6487ec1de..9e30846c902 100644 --- a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos.h +++ b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos.h @@ -12,6 +12,7 @@ #include <vector> #include "base/gtest_prod_util.h" +#include "base/memory/raw_ptr.h" #include "base/memory/weak_ptr.h" #include "base/sequence_checker.h" #include "components/account_manager_core/account.h" @@ -56,13 +57,6 @@ class ProfileOAuth2TokenServiceDelegateChromeOS scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, OAuth2AccessTokenConsumer* consumer) override; bool RefreshTokenIsAvailable(const CoreAccountId& account_id) const override; - void UpdateAuthError(const CoreAccountId& account_id, - const GoogleServiceAuthError& error) override; - void UpdateAuthErrorInternal(const CoreAccountId& account_id, - const GoogleServiceAuthError& error, - bool fire_auth_error_changed = true); - GoogleServiceAuthError GetAuthError( - const CoreAccountId& account_id) const override; std::vector<CoreAccountId> GetAccounts() const override; void LoadCredentials(const CoreAccountId& primary_account_id, bool is_syncing) override; @@ -72,7 +66,6 @@ class ProfileOAuth2TokenServiceDelegateChromeOS const override; void RevokeCredentials(const CoreAccountId& account_id) override; void RevokeAllCredentials() override; - const net::BackoffEntry* BackoffEntry() const override; // `account_manager::AccountManagerFacade::Observer` overrides. void OnAccountUpserted(const account_manager::Account& account) override; @@ -82,17 +75,7 @@ class ProfileOAuth2TokenServiceDelegateChromeOS void OnConnectionChanged(network::mojom::ConnectionType type) override; private: - FRIEND_TEST_ALL_PREFIXES(ProfileOAuth2TokenServiceDelegateChromeOSTest, - BackOffIsTriggerredForTransientErrors); - FRIEND_TEST_ALL_PREFIXES(ProfileOAuth2TokenServiceDelegateChromeOSTest, - BackOffIsResetOnNetworkChange); - - // A utility class to keep track of |GoogleServiceAuthError|s for an account. - struct AccountErrorStatus { - // The last auth error seen. - GoogleServiceAuthError last_auth_error; - }; - + friend class TestProfileOAuth2TokenServiceDelegateChromeOS; // Callback handler for `account_manager::AccountManagerFacade::GetAccounts`. void OnGetAccounts(const std::vector<account_manager::Account>& accounts); @@ -106,10 +89,10 @@ class ProfileOAuth2TokenServiceDelegateChromeOS const GoogleServiceAuthError& error); // Non-owning pointers. - SigninClient* const signin_client_; - AccountTrackerService* const account_tracker_service_; - network::NetworkConnectionTracker* const network_connection_tracker_; - account_manager::AccountManagerFacade* const account_manager_facade_; + const raw_ptr<SigninClient> signin_client_; + const raw_ptr<AccountTrackerService> account_tracker_service_; + const raw_ptr<network::NetworkConnectionTracker> network_connection_tracker_; + const raw_ptr<account_manager::AccountManagerFacade> account_manager_facade_; // When the delegate receives an account from either `GetAccounts` or // `OnAccountUpserted`, this account is first added to pending accounts, until @@ -122,13 +105,6 @@ class ProfileOAuth2TokenServiceDelegateChromeOS // A cache of AccountKeys. std::set<account_manager::AccountKey> account_keys_; - // A map from account id to the last seen error for that account. - std::map<CoreAccountId, AccountErrorStatus> errors_; - - // Used to rate-limit token fetch requests so as to not overload the server. - net::BackoffEntry backoff_entry_; - GoogleServiceAuthError backoff_error_; - #if BUILDFLAG(IS_CHROMEOS_LACROS) const bool delete_signin_cookies_on_exit_; #endif // BUILDFLAG(IS_CHROMEOS_LACROS) diff --git a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos_legacy.h b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos_legacy.h deleted file mode 100644 index f957ad595ed..00000000000 --- a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos_legacy.h +++ /dev/null @@ -1,124 +0,0 @@ -// 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. - -#ifndef COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_PROFILE_OAUTH2_TOKEN_SERVICE_DELEGATE_CHROMEOS_LEGACY_H_ -#define COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_PROFILE_OAUTH2_TOKEN_SERVICE_DELEGATE_CHROMEOS_LEGACY_H_ - -#include <map> -#include <memory> -#include <set> -#include <string> -#include <vector> - -#include "base/gtest_prod_util.h" -#include "base/memory/weak_ptr.h" -#include "base/sequence_checker.h" -#include "components/account_manager_core/account.h" -#include "components/account_manager_core/chromeos/account_manager.h" -#include "components/signin/internal/identity_manager/profile_oauth2_token_service_delegate.h" -#include "services/network/public/cpp/network_connection_tracker.h" - -class AccountTrackerService; - -namespace signin { -// This is a fallback version of ProfileOAuth2TokenServiceDelegateChromeOS that -// doesn't use AccountManagerFacade. This version is used if -// `UseAccountManagerFacade` feature is disabled. -// TODO(https://crbug.com/1188696): Remove this. -class ProfileOAuth2TokenServiceDelegateChromeOSLegacy - : public ProfileOAuth2TokenServiceDelegate, - public account_manager::AccountManager::Observer, - public network::NetworkConnectionTracker::NetworkConnectionObserver { - public: - // Accepts non-owning pointers to |AccountTrackerService|, - // |NetworkConnectorTracker|, and |account_manager::AccountManager|. These - // objects must all outlive |this| delegate. - ProfileOAuth2TokenServiceDelegateChromeOSLegacy( - AccountTrackerService* account_tracker_service, - network::NetworkConnectionTracker* network_connection_tracker, - account_manager::AccountManager* account_manager, - bool is_regular_profile); - - ProfileOAuth2TokenServiceDelegateChromeOSLegacy( - const ProfileOAuth2TokenServiceDelegateChromeOSLegacy&) = delete; - ProfileOAuth2TokenServiceDelegateChromeOSLegacy& operator=( - const ProfileOAuth2TokenServiceDelegateChromeOSLegacy&) = delete; - - ~ProfileOAuth2TokenServiceDelegateChromeOSLegacy() override; - - // ProfileOAuth2TokenServiceDelegate overrides. - std::unique_ptr<OAuth2AccessTokenFetcher> CreateAccessTokenFetcher( - const CoreAccountId& account_id, - scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, - OAuth2AccessTokenConsumer* consumer) override; - bool RefreshTokenIsAvailable(const CoreAccountId& account_id) const override; - void UpdateAuthError(const CoreAccountId& account_id, - const GoogleServiceAuthError& error) override; - void UpdateAuthErrorInternal(const CoreAccountId& account_id, - const GoogleServiceAuthError& error, - bool fire_auth_error_changed = true); - GoogleServiceAuthError GetAuthError( - const CoreAccountId& account_id) const override; - std::vector<CoreAccountId> GetAccounts() const override; - void LoadCredentials(const CoreAccountId& primary_account_id) override; - void UpdateCredentials(const CoreAccountId& account_id, - const std::string& refresh_token) override; - scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory() - const override; - void RevokeCredentials(const CoreAccountId& account_id) override; - void RevokeAllCredentials() override; - const net::BackoffEntry* BackoffEntry() const override; - - // |account_manager::AccountManager::Observer| overrides. - void OnTokenUpserted(const account_manager::Account& account) override; - void OnAccountRemoved(const account_manager::Account& account) override; - - // |NetworkConnectionTracker::NetworkConnectionObserver| overrides. - void OnConnectionChanged(network::mojom::ConnectionType type) override; - - private: - FRIEND_TEST_ALL_PREFIXES(ProfileOAuth2TokenServiceDelegateChromeOSTest, - BackOffIsTriggerredForTransientErrors); - FRIEND_TEST_ALL_PREFIXES(ProfileOAuth2TokenServiceDelegateChromeOSTest, - BackOffIsResetOnNetworkChange); - - // A utility class to keep track of |GoogleServiceAuthError|s for an account. - struct AccountErrorStatus { - // The last auth error seen. - GoogleServiceAuthError last_auth_error; - }; - - // Callback handler for |account_manager::AccountManager::GetAccounts|. - void OnGetAccounts(const std::vector<account_manager::Account>& accounts); - - // Callback handler for |account_manager::AccountManager::HasDummyGaiaToken|. - void ContinueTokenUpsertProcessing(const CoreAccountId& account_id, - bool has_dummy_token); - - // Non-owning pointers. - AccountTrackerService* const account_tracker_service_; - network::NetworkConnectionTracker* const network_connection_tracker_; - account_manager::AccountManager* const account_manager_; - - // A cache of AccountKeys. - std::set<account_manager::AccountKey> account_keys_; - - // A map from account id to the last seen error for that account. - std::map<CoreAccountId, AccountErrorStatus> errors_; - - // Used to rate-limit token fetch requests so as to not overload the server. - net::BackoffEntry backoff_entry_; - GoogleServiceAuthError backoff_error_; - - // Is |this| attached to a regular (non-Signin && non-LockScreen) Profile. - const bool is_regular_profile_; - - SEQUENCE_CHECKER(sequence_checker_); - base::WeakPtrFactory<ProfileOAuth2TokenServiceDelegateChromeOSLegacy> - weak_factory_; -}; - -} // namespace signin - -#endif // COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_PROFILE_OAUTH2_TOKEN_SERVICE_DELEGATE_CHROMEOS_LEGACY_H_ diff --git a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos_unittest.cc b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos_unittest.cc index e4c68b45b08..54da9c07a3a 100644 --- a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos_unittest.cc +++ b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos_unittest.cc @@ -13,6 +13,7 @@ #include "base/bind.h" #include "base/containers/contains.h" #include "base/files/scoped_temp_dir.h" +#include "base/memory/raw_ptr.h" #include "base/memory/scoped_refptr.h" #include "base/test/gmock_callback_support.h" #include "base/test/task_environment.h" @@ -36,8 +37,6 @@ #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" -namespace signin { - namespace { using ::account_manager::AccountManager; @@ -153,7 +152,7 @@ class TestOAuth2TokenServiceObserver std::vector<std::vector<CoreAccountId>> batch_change_records_; // Non-owning pointer. - ProfileOAuth2TokenServiceDelegate* const delegate_; + const raw_ptr<ProfileOAuth2TokenServiceDelegate> delegate_; }; class MockProfileOAuth2TokenServiceObserver @@ -182,7 +181,7 @@ class MockProfileOAuth2TokenServiceObserver MOCK_METHOD(void, OnRefreshTokenRevoked, (const CoreAccountId&), (override)); private: - ProfileOAuth2TokenServiceDelegate* const delegate_; + const raw_ptr<ProfileOAuth2TokenServiceDelegate> delegate_; }; } // namespace @@ -243,14 +242,15 @@ class ProfileOAuth2TokenServiceDelegateChromeOSTest : public testing::Test { bool delete_signin_cookies_on_exit, bool is_syncing) { delegate_.reset(); - delegate_ = std::make_unique<ProfileOAuth2TokenServiceDelegateChromeOS>( - client_.get(), &account_tracker_service_, - network::TestNetworkConnectionTracker::GetInstance(), - account_manager_facade_.get(), + delegate_ = + std::make_unique<signin::ProfileOAuth2TokenServiceDelegateChromeOS>( + client_.get(), &account_tracker_service_, + network::TestNetworkConnectionTracker::GetInstance(), + account_manager_facade_.get(), #if BUILDFLAG(IS_CHROMEOS_LACROS) - delete_signin_cookies_on_exit, + delete_signin_cookies_on_exit, #endif // BUILDFLAG(IS_CHROMEOS_LACROS) - /*is_regular_profile=*/true); + /*is_regular_profile=*/true); LoadCredentialsAndWaitForCompletion( /*primary_account_id=*/account_info_.account_id, is_syncing); @@ -386,7 +386,7 @@ class ProfileOAuth2TokenServiceDelegateChromeOSTest : public testing::Test { account_manager_mojo_service_; std::unique_ptr<account_manager::AccountManagerFacade> account_manager_facade_; - std::unique_ptr<ProfileOAuth2TokenServiceDelegateChromeOS> delegate_; + std::unique_ptr<signin::ProfileOAuth2TokenServiceDelegateChromeOS> delegate_; AccountManager::DelayNetworkCallRunner immediate_callback_runner_ = base::BindRepeating( [](base::OnceClosure closure) -> void { std::move(closure).Run(); }); @@ -455,14 +455,15 @@ TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, // behaviour. AccountManager account_manager; - auto delegate = std::make_unique<ProfileOAuth2TokenServiceDelegateChromeOS>( - client_.get(), &account_tracker_service_, - network::TestNetworkConnectionTracker::GetInstance(), - account_manager_facade_.get(), + auto delegate = + std::make_unique<signin::ProfileOAuth2TokenServiceDelegateChromeOS>( + client_.get(), &account_tracker_service_, + network::TestNetworkConnectionTracker::GetInstance(), + account_manager_facade_.get(), #if BUILDFLAG(IS_CHROMEOS_LACROS) - /*delete_signin_cookies_on_exit=*/false, + /*delete_signin_cookies_on_exit=*/false, #endif // BUILDFLAG(IS_CHROMEOS_LACROS) - /*is_regular_profile=*/false); + /*is_regular_profile=*/false); TestOAuth2TokenServiceObserver observer(delegate.get()); // Test that LoadCredentials works as expected. @@ -470,14 +471,16 @@ TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, delegate->LoadCredentials(CoreAccountId() /* primary_account_id */, /*is_syncing=*/false); EXPECT_TRUE(observer.refresh_tokens_loaded_); - EXPECT_EQ(LoadCredentialsState::LOAD_CREDENTIALS_FINISHED_WITH_SUCCESS, - delegate->load_credentials_state()); + EXPECT_EQ( + signin::LoadCredentialsState::LOAD_CREDENTIALS_FINISHED_WITH_SUCCESS, + delegate->load_credentials_state()); } TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, RefreshTokenIsAvailableReturnsTrueForValidGaiaTokens) { - EXPECT_EQ(LoadCredentialsState::LOAD_CREDENTIALS_FINISHED_WITH_SUCCESS, - delegate_->load_credentials_state()); + EXPECT_EQ( + signin::LoadCredentialsState::LOAD_CREDENTIALS_FINISHED_WITH_SUCCESS, + delegate_->load_credentials_state()); EXPECT_FALSE(delegate_->RefreshTokenIsAvailable(account_info_.account_id)); EXPECT_FALSE( @@ -492,8 +495,9 @@ TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, RefreshTokenIsAvailableReturnsTrueForInvalidGaiaTokens) { - EXPECT_EQ(LoadCredentialsState::LOAD_CREDENTIALS_FINISHED_WITH_SUCCESS, - delegate_->load_credentials_state()); + EXPECT_EQ( + signin::LoadCredentialsState::LOAD_CREDENTIALS_FINISHED_WITH_SUCCESS, + delegate_->load_credentials_state()); EXPECT_FALSE(delegate_->RefreshTokenIsAvailable(account_info_.account_id)); EXPECT_FALSE( @@ -509,6 +513,7 @@ TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, ObserversAreNotifiedOnAuthErrorChange) { + UpsertAccountAndWaitForCompletion(gaia_account_key(), kUserEmail, kGaiaToken); TestOAuth2TokenServiceObserver observer(delegate_.get()); auto error = GoogleServiceAuthError(GoogleServiceAuthError::State::SERVICE_ERROR); @@ -521,18 +526,21 @@ TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, ObserversAreNotNotifiedIfErrorDidntChange) { + UpsertAccountAndWaitForCompletion(gaia_account_key(), kUserEmail, kGaiaToken); TestOAuth2TokenServiceObserver observer(delegate_.get()); auto error = GoogleServiceAuthError(GoogleServiceAuthError::State::SERVICE_ERROR); delegate_->UpdateAuthError(account_info_.account_id, error); EXPECT_EQ(1, observer.on_auth_error_changed_calls); + EXPECT_EQ(error, delegate_->GetAuthError(account_info_.account_id)); delegate_->UpdateAuthError(account_info_.account_id, error); EXPECT_EQ(1, observer.on_auth_error_changed_calls); } TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, ObserversAreNotifiedIfErrorDidChange) { + UpsertAccountAndWaitForCompletion(gaia_account_key(), kUserEmail, kGaiaToken); TestOAuth2TokenServiceObserver observer(delegate_.get()); delegate_->UpdateAuthError( account_info_.account_id, @@ -581,6 +589,7 @@ TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, kGaiaToken); // Deliberately add an error. delegate_->UpdateAuthError(account_info_.account_id, error); + EXPECT_EQ(error, delegate_->GetAuthError(account_info_.account_id)); RemoveAccountAndWaitForCompletion(gaia_account_key()); EXPECT_EQ(GoogleServiceAuthError::AuthErrorNone(), delegate_->GetAuthError(account_info_.account_id)); @@ -683,14 +692,15 @@ TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, CreateAccountManagerFacade(account_manager_mojo_service.get()); // Register callbacks before AccountManager has been fully initialized. - auto delegate = std::make_unique<ProfileOAuth2TokenServiceDelegateChromeOS>( - client_.get(), &account_tracker_service_, - network::TestNetworkConnectionTracker::GetInstance(), - account_manager_facade.get(), + auto delegate = + std::make_unique<signin::ProfileOAuth2TokenServiceDelegateChromeOS>( + client_.get(), &account_tracker_service_, + network::TestNetworkConnectionTracker::GetInstance(), + account_manager_facade.get(), #if BUILDFLAG(IS_CHROMEOS_LACROS) - /*delete_signin_cookies_on_exit=*/false, + /*delete_signin_cookies_on_exit=*/false, #endif // BUILDFLAG(IS_CHROMEOS_LACROS - /*is_regular_profile=*/true); + /*is_regular_profile=*/true); delegate->LoadCredentials(account1.account_id /* primary_account_id */, /*is_syncing=*/false); TestOAuth2TokenServiceObserver observer(delegate.get()); @@ -749,8 +759,9 @@ TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, RefreshTokenMustBeAvailableForAllAccountsReturnedByGetAccounts) { - EXPECT_EQ(LoadCredentialsState::LOAD_CREDENTIALS_FINISHED_WITH_SUCCESS, - delegate_->load_credentials_state()); + EXPECT_EQ( + signin::LoadCredentialsState::LOAD_CREDENTIALS_FINISHED_WITH_SUCCESS, + delegate_->load_credentials_state()); EXPECT_TRUE(delegate_->GetAccounts().empty()); const std::string kUserEmail2 = "random-email2@example.com"; const std::string kUserEmail3 = "random-email3@example.com"; @@ -850,6 +861,7 @@ TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, SigninErrorObserversAreNotifiedOnAuthErrorChange) { + UpsertAccountAndWaitForCompletion(gaia_account_key(), kUserEmail, kGaiaToken); auto error = GoogleServiceAuthError(GoogleServiceAuthError::State::SERVICE_ERROR); @@ -860,6 +872,7 @@ TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, TransientErrorsAreNotShown) { + UpsertAccountAndWaitForCompletion(gaia_account_key(), kUserEmail, kGaiaToken); auto transient_error = GoogleServiceAuthError( GoogleServiceAuthError::State::SERVICE_UNAVAILABLE); EXPECT_EQ(GoogleServiceAuthError::AuthErrorNone(), @@ -897,10 +910,11 @@ TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, EXPECT_EQ(0, access_token_consumer.num_access_token_fetch_success_); EXPECT_EQ(1, access_token_consumer.num_access_token_fetch_failure_); // Expect a positive backoff time. - EXPECT_GT(delegate_->backoff_entry_.GetTimeUntilRelease(), base::TimeDelta()); + EXPECT_GT(delegate_->BackoffEntry()->GetTimeUntilRelease(), + base::TimeDelta()); // Pretend that backoff has expired and try again. - delegate_->backoff_entry_.SetCustomReleaseTime(base::TimeTicks()); + delegate_->backoff_entry_->SetCustomReleaseTime(base::TimeTicks()); fetcher = delegate_->CreateAccessTokenFetcher( account_info_.account_id, delegate_->GetURLLoaderFactory(), &access_token_consumer); @@ -936,7 +950,8 @@ TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, EXPECT_EQ(0, access_token_consumer.num_access_token_fetch_success_); EXPECT_EQ(1, access_token_consumer.num_access_token_fetch_failure_); // Expect a positive backoff time. - EXPECT_GT(delegate_->backoff_entry_.GetTimeUntilRelease(), base::TimeDelta()); + EXPECT_GT(delegate_->BackoffEntry()->GetTimeUntilRelease(), + base::TimeDelta()); // Notify of network change and ensure that request now runs. delegate_->OnConnectionChanged( @@ -949,5 +964,3 @@ TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, EXPECT_EQ(1, access_token_consumer.num_access_token_fetch_success_); EXPECT_EQ(1, access_token_consumer.num_access_token_fetch_failure_); } - -} // namespace signin diff --git a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_ios.h b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_ios.h index 1750fde31e2..53ab5f2e5cf 100644 --- a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_ios.h +++ b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_ios.h @@ -41,10 +41,6 @@ class ProfileOAuth2TokenServiceIOSDelegate void Shutdown() override; bool RefreshTokenIsAvailable(const CoreAccountId& account_id) const override; - GoogleServiceAuthError GetAuthError( - const CoreAccountId& account_id) const override; - void UpdateAuthError(const CoreAccountId& account_id, - const GoogleServiceAuthError& error) override; void LoadCredentials(const CoreAccountId& primary_account_id, bool is_syncing) override; @@ -76,21 +72,13 @@ class ProfileOAuth2TokenServiceIOSDelegate private: friend class ProfileOAuth2TokenServiceIOSDelegateTest; - struct AccountStatus { - GoogleServiceAuthError last_auth_error; - }; - - // Maps the |account_id| of accounts known to ProfileOAuth2TokenService - // to information about the account. - typedef std::map<CoreAccountId, AccountStatus> AccountStatusMap; - // Reloads accounts from the provider. Fires |OnRefreshTokenAvailable| for // each new account. Fires |OnRefreshTokenRevoked| for each account that was // removed. void ReloadCredentials(const CoreAccountId& primary_account_id); // Info about the existing accounts. - AccountStatusMap accounts_; + std::set<CoreAccountId> accounts_; // Calls to this class are expected to be made from the browser UI thread. // The purpose of this checker is to detect access to diff --git a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_ios.mm b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_ios.mm index 3b4d9bf18bb..72b006af570 100644 --- a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_ios.mm +++ b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_ios.mm @@ -160,7 +160,8 @@ ProfileOAuth2TokenServiceIOSDelegate::ProfileOAuth2TokenServiceIOSDelegate( SigninClient* client, std::unique_ptr<DeviceAccountsProvider> provider, AccountTrackerService* account_tracker_service) - : client_(client), + : ProfileOAuth2TokenServiceDelegate(/*use_backoff=*/false), + client_(client), provider_(std::move(provider)), account_tracker_service_(account_tracker_service) { DCHECK(client_); @@ -175,6 +176,7 @@ ProfileOAuth2TokenServiceIOSDelegate::~ProfileOAuth2TokenServiceIOSDelegate() { void ProfileOAuth2TokenServiceIOSDelegate::Shutdown() { DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); accounts_.clear(); + ClearAuthError(absl::nullopt); } void ProfileOAuth2TokenServiceIOSDelegate::LoadCredentials( @@ -209,12 +211,11 @@ void ProfileOAuth2TokenServiceIOSDelegate::LoadCredentials( // For whatever reason, we failed to load the device account for the primary // account. There must always be an account for the primary account - accounts_[primary_account_id].last_auth_error = - GoogleServiceAuthError::FromInvalidGaiaCredentialsReason( - GoogleServiceAuthError::InvalidGaiaCredentialsReason:: - CREDENTIALS_MISSING); - FireAuthErrorChanged(primary_account_id, - accounts_[primary_account_id].last_auth_error); + accounts_.insert(primary_account_id); + UpdateAuthError(primary_account_id, + GoogleServiceAuthError::FromInvalidGaiaCredentialsReason( + GoogleServiceAuthError::InvalidGaiaCredentialsReason:: + CREDENTIALS_MISSING)); FireRefreshTokenAvailable(primary_account_id); set_load_credentials_state( signin::LoadCredentialsState:: @@ -287,9 +288,9 @@ void ProfileOAuth2TokenServiceIOSDelegate::RevokeAllCredentials() { DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); ScopedBatchChange batch(this); - AccountStatusMap toRemove = accounts_; - for (auto& accountStatus : toRemove) - RemoveAccount(accountStatus.first); + std::set<CoreAccountId> toRemove = accounts_; + for (auto& account_id : toRemove) + RemoveAccount(account_id); DCHECK_EQ(0u, accounts_.size()); } @@ -319,10 +320,7 @@ ProfileOAuth2TokenServiceIOSDelegate::CreateAccessTokenFetcher( std::vector<CoreAccountId> ProfileOAuth2TokenServiceIOSDelegate::GetAccounts() const { DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); - std::vector<CoreAccountId> account_ids; - for (const auto& account : accounts_) - account_ids.push_back(account.first); - return account_ids; + return std::vector<CoreAccountId>(accounts_.begin(), accounts_.end()); } bool ProfileOAuth2TokenServiceIOSDelegate::RefreshTokenIsAvailable( @@ -332,36 +330,6 @@ bool ProfileOAuth2TokenServiceIOSDelegate::RefreshTokenIsAvailable( return accounts_.count(account_id) > 0; } -GoogleServiceAuthError ProfileOAuth2TokenServiceIOSDelegate::GetAuthError( - const CoreAccountId& account_id) const { - auto it = accounts_.find(account_id); - return (it == accounts_.end()) ? GoogleServiceAuthError::AuthErrorNone() - : it->second.last_auth_error; -} - -void ProfileOAuth2TokenServiceIOSDelegate::UpdateAuthError( - const CoreAccountId& account_id, - const GoogleServiceAuthError& error) { - DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); - - // Do not report connection errors as these are not actually auth errors. - // We also want to avoid masking a "real" auth error just because we - // subsequently get a transient network error. - if (error.IsTransientError()) - return; - - if (accounts_.count(account_id) == 0) { - // Nothing to update as the account has already been removed. - return; - } - - AccountStatus* status = &accounts_[account_id]; - if (error.state() != status->last_auth_error.state()) { - status->last_auth_error = error; - FireAuthErrorChanged(account_id, error); - } -} - // Clear the authentication error state and notify all observers that a new // refresh token is available so that they request new access tokens. void ProfileOAuth2TokenServiceIOSDelegate::AddOrUpdateAccount( @@ -373,15 +341,16 @@ void ProfileOAuth2TokenServiceIOSDelegate::AddOrUpdateAccount( DCHECK(!account_tracker_service_->GetAccountInfo(account_id).email.empty()); bool account_present = accounts_.count(account_id) > 0; - if (account_present && accounts_[account_id].last_auth_error.state() == - GoogleServiceAuthError::NONE) { + if (account_present && + GetAuthError(account_id) == GoogleServiceAuthError::AuthErrorNone()) { // No need to update the account if it is already a known account and if // there is no auth error. return; } - accounts_[account_id].last_auth_error = - GoogleServiceAuthError::AuthErrorNone(); + accounts_.insert(account_id); + UpdateAuthError(account_id, GoogleServiceAuthError::AuthErrorNone(), + /*fire_auth_error_changed=*/false); FireAuthErrorChanged(account_id, GoogleServiceAuthError::AuthErrorNone()); FireRefreshTokenAvailable(account_id); } @@ -393,6 +362,7 @@ void ProfileOAuth2TokenServiceIOSDelegate::RemoveAccount( if (accounts_.count(account_id) > 0) { accounts_.erase(account_id); + ClearAuthError(account_id); FireRefreshTokenRevoked(account_id); } } diff --git a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_unittest.cc b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_unittest.cc index 4b0432ec741..6ad152d9f42 100644 --- a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_unittest.cc +++ b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_unittest.cc @@ -15,26 +15,36 @@ #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" +class MockOAuth2TokenServiceObserver + : public ProfileOAuth2TokenServiceObserver { + public: + MOCK_METHOD2(OnAuthErrorChanged, + void(const CoreAccountId&, const GoogleServiceAuthError&)); +}; + +class ProfileOAuth2TokenServiceDelegateTest : public testing::Test { + public: + ProfileOAuth2TokenServiceDelegateTest() { delegate_.AddObserver(&observer_); } + + ~ProfileOAuth2TokenServiceDelegateTest() override { + delegate_.RemoveObserver(&observer_); + } + + // Note: Tests in this suite tests the default base implementation of + // 'ProfileOAuth2TokenServiceDelegate'. + FakeProfileOAuth2TokenServiceDelegate delegate_; + MockOAuth2TokenServiceObserver observer_; +}; + // The default implementation of // ProfileOAuth2TokenServiceDelegate::InvalidateTokensForMultilogin is used on // mobile where refresh tokens are not accessible. This test checks that refresh // tokens are not affected on INVALID_TOKENS Multilogin error. -TEST(ProfileOAuth2TokenServiceDelegateTest, InvalidateTokensForMultilogin) { - class TestOAuth2TokenServiceObserver - : public ProfileOAuth2TokenServiceObserver { - public: - MOCK_METHOD2(OnAuthErrorChanged, - void(const CoreAccountId&, const GoogleServiceAuthError&)); - }; - - FakeProfileOAuth2TokenServiceDelegate delegate; - - TestOAuth2TokenServiceObserver observer; - delegate.AddObserver(&observer); +TEST_F(ProfileOAuth2TokenServiceDelegateTest, InvalidateTokensForMultilogin) { // Check that OnAuthErrorChanged is not fired from // InvalidateTokensForMultilogin and refresh tokens are not set in error. EXPECT_CALL( - observer, + observer_, OnAuthErrorChanged(::testing::_, GoogleServiceAuthError( GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS))) @@ -43,15 +53,146 @@ TEST(ProfileOAuth2TokenServiceDelegateTest, InvalidateTokensForMultilogin) { const CoreAccountId account_id1("account_id1"); const CoreAccountId account_id2("account_id2"); - delegate.UpdateCredentials(account_id1, "refresh_token1"); - delegate.UpdateCredentials(account_id2, "refresh_token2"); + delegate_.UpdateCredentials(account_id1, "refresh_token1"); + delegate_.UpdateCredentials(account_id2, "refresh_token2"); - delegate.InvalidateTokenForMultilogin(account_id1); + delegate_.InvalidateTokenForMultilogin(account_id1); - EXPECT_EQ(delegate.GetAuthError(account_id1).state(), + EXPECT_EQ(delegate_.GetAuthError(account_id1).state(), GoogleServiceAuthError::NONE); - EXPECT_EQ(delegate.GetAuthError(account_id2).state(), + EXPECT_EQ(delegate_.GetAuthError(account_id2).state(), GoogleServiceAuthError::NONE); +} + +const GoogleServiceAuthError::State table[] = { + GoogleServiceAuthError::NONE, + GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS, + GoogleServiceAuthError::USER_NOT_SIGNED_UP, + GoogleServiceAuthError::CONNECTION_FAILED, + GoogleServiceAuthError::SERVICE_UNAVAILABLE, + GoogleServiceAuthError::REQUEST_CANCELED, + GoogleServiceAuthError::UNEXPECTED_SERVICE_RESPONSE, + GoogleServiceAuthError::SERVICE_ERROR, + GoogleServiceAuthError::SCOPE_LIMITED_UNRECOVERABLE_ERROR, +}; + +TEST_F(ProfileOAuth2TokenServiceDelegateTest, UpdateAuthError_PersistenErrors) { + const CoreAccountId account_id("account_id"); + delegate_.UpdateCredentials(account_id, "refresh_token"); + + static_assert( + std::size(table) == GoogleServiceAuthError::NUM_STATES - + GoogleServiceAuthError::kDeprecatedStateCount, + "table size should match number of auth error types"); + + for (GoogleServiceAuthError::State state : table) { + GoogleServiceAuthError error(state); + if (!error.IsPersistentError() || error.IsScopePersistentError()) + continue; + + EXPECT_CALL(observer_, OnAuthErrorChanged(account_id, error)).Times(1); + + delegate_.UpdateAuthError(account_id, error); + EXPECT_EQ(delegate_.GetAuthError(account_id), error); + // Backoff only used for transient errors. + EXPECT_EQ(delegate_.BackoffEntry()->failure_count(), 0); + testing::Mock::VerifyAndClearExpectations(&observer_); + } +} + +TEST_F(ProfileOAuth2TokenServiceDelegateTest, UpdateAuthError_TransientErrors) { + const CoreAccountId account_id("account_id"); + delegate_.UpdateCredentials(account_id, "refresh_token"); + + static_assert( + std::size(table) == GoogleServiceAuthError::NUM_STATES - + GoogleServiceAuthError::kDeprecatedStateCount, + "table size should match number of auth error types"); + + EXPECT_TRUE(delegate_.BackoffEntry()); + int failure_count = 0; + for (GoogleServiceAuthError::State state : table) { + GoogleServiceAuthError error(state); + if (!error.IsTransientError()) + continue; + + EXPECT_CALL(observer_, OnAuthErrorChanged(account_id, ::testing::_)) + .Times(0); + + failure_count++; + delegate_.UpdateAuthError(account_id, error); + EXPECT_EQ(delegate_.GetAuthError(account_id), + GoogleServiceAuthError::AuthErrorNone()); + EXPECT_GT(delegate_.BackoffEntry()->GetTimeUntilRelease(), + base::TimeDelta()); + EXPECT_EQ(delegate_.BackoffEntry()->failure_count(), failure_count); + EXPECT_EQ(delegate_.BackOffError(), error); + testing::Mock::VerifyAndClearExpectations(&observer_); + } +} + +TEST_F(ProfileOAuth2TokenServiceDelegateTest, + UpdateAuthError_ScopePersistenErrors) { + const CoreAccountId account_id("account_id"); + delegate_.UpdateCredentials(account_id, "refresh_token"); + GoogleServiceAuthError error( + GoogleServiceAuthError::SCOPE_LIMITED_UNRECOVERABLE_ERROR); + + // Scope persistent errors are not persisted or notified as it does not imply + // that the account is in an error state but the error is only relevant to + // the scope set requested in the access token request. + EXPECT_CALL(observer_, OnAuthErrorChanged(account_id, error)).Times(0); + + delegate_.UpdateAuthError(account_id, error); + EXPECT_EQ(delegate_.GetAuthError(account_id), + GoogleServiceAuthError::AuthErrorNone()); + // Backoff only used for transient errors. + EXPECT_EQ(delegate_.BackoffEntry()->failure_count(), 0); + testing::Mock::VerifyAndClearExpectations(&observer_); +} + +TEST_F(ProfileOAuth2TokenServiceDelegateTest, + UpdateAuthError_RefreshTokenNotAvailable) { + const CoreAccountId account_id("account_id"); + EXPECT_FALSE(delegate_.RefreshTokenIsAvailable(account_id)); + EXPECT_CALL(observer_, OnAuthErrorChanged(::testing::_, ::testing::_)) + .Times(0); + delegate_.UpdateAuthError( + account_id, + GoogleServiceAuthError(GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS)); + EXPECT_EQ(delegate_.GetAuthError(account_id), + GoogleServiceAuthError::AuthErrorNone()); + testing::Mock::VerifyAndClearExpectations(&observer_); + // Backoff only used for transient errors. + EXPECT_EQ(delegate_.BackoffEntry()->failure_count(), 0); +} + +TEST_F(ProfileOAuth2TokenServiceDelegateTest, AuthErrorChanged) { + const CoreAccountId account_id("account_id"); + delegate_.UpdateCredentials(account_id, "refresh_token"); + + GoogleServiceAuthError error( + GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS); + EXPECT_CALL(observer_, OnAuthErrorChanged(account_id, error)).Times(1); + delegate_.UpdateAuthError(account_id, error); + EXPECT_EQ(delegate_.GetAuthError(account_id), error); + + // Update the error but without changing it should not fire a notification. + delegate_.UpdateAuthError(account_id, error); + testing::Mock::VerifyAndClearExpectations(&observer_); + + // Change the error. + EXPECT_CALL( + observer_, + OnAuthErrorChanged(account_id, GoogleServiceAuthError::AuthErrorNone())) + .Times(1); + delegate_.UpdateAuthError(account_id, + GoogleServiceAuthError::AuthErrorNone()); + EXPECT_EQ(delegate_.GetAuthError(account_id), + GoogleServiceAuthError::AuthErrorNone()); - delegate.RemoveObserver(&observer); + // Update to none again should not fire any notification. + delegate_.UpdateAuthError(account_id, + GoogleServiceAuthError::AuthErrorNone()); + testing::Mock::VerifyAndClearExpectations(&observer_); } diff --git a/chromium/components/signin/internal/identity_manager/test_profile_oauth2_token_service_delegate_chromeos.cc b/chromium/components/signin/internal/identity_manager/test_profile_oauth2_token_service_delegate_chromeos.cc index 2742c8f5217..41eb3731ab8 100644 --- a/chromium/components/signin/internal/identity_manager/test_profile_oauth2_token_service_delegate_chromeos.cc +++ b/chromium/components/signin/internal/identity_manager/test_profile_oauth2_token_service_delegate_chromeos.cc @@ -18,7 +18,8 @@ TestProfileOAuth2TokenServiceDelegateChromeOS:: SigninClient* client, AccountTrackerService* account_tracker_service, crosapi::AccountManagerMojoService* account_manager_mojo_service, - bool is_regular_profile) { + bool is_regular_profile) + : ProfileOAuth2TokenServiceDelegate(/*use_backoff=*/true) { if (!network::TestNetworkConnectionTracker::HasInstance()) { owned_tracker_ = network::TestNetworkConnectionTracker::CreateInstance(); } @@ -60,8 +61,9 @@ bool TestProfileOAuth2TokenServiceDelegateChromeOS::RefreshTokenIsAvailable( void TestProfileOAuth2TokenServiceDelegateChromeOS::UpdateAuthError( const CoreAccountId& account_id, - const GoogleServiceAuthError& error) { - delegate_->UpdateAuthError(account_id, error); + const GoogleServiceAuthError& error, + bool fire_auth_error_changed) { + delegate_->UpdateAuthError(account_id, error, fire_auth_error_changed); } GoogleServiceAuthError @@ -75,6 +77,20 @@ TestProfileOAuth2TokenServiceDelegateChromeOS::GetAccounts() const { return delegate_->GetAccounts(); } +void TestProfileOAuth2TokenServiceDelegateChromeOS::ClearAuthError( + const absl::optional<CoreAccountId>& account_id) { + delegate_->ClearAuthError(account_id); +} + +GoogleServiceAuthError +TestProfileOAuth2TokenServiceDelegateChromeOS::BackOffError() const { + return delegate_->BackOffError(); +} + +void TestProfileOAuth2TokenServiceDelegateChromeOS::ResetBackOffEntry() { + delegate_->ResetBackOffEntry(); +} + void TestProfileOAuth2TokenServiceDelegateChromeOS::LoadCredentials( const CoreAccountId& primary_account_id, bool is_syncing) { diff --git a/chromium/components/signin/internal/identity_manager/test_profile_oauth2_token_service_delegate_chromeos.h b/chromium/components/signin/internal/identity_manager/test_profile_oauth2_token_service_delegate_chromeos.h index a37f65f6162..34d0305d124 100644 --- a/chromium/components/signin/internal/identity_manager/test_profile_oauth2_token_service_delegate_chromeos.h +++ b/chromium/components/signin/internal/identity_manager/test_profile_oauth2_token_service_delegate_chromeos.h @@ -46,7 +46,8 @@ class TestProfileOAuth2TokenServiceDelegateChromeOS OAuth2AccessTokenConsumer* consumer) override; bool RefreshTokenIsAvailable(const CoreAccountId& account_id) const override; void UpdateAuthError(const CoreAccountId& account_id, - const GoogleServiceAuthError& error) override; + const GoogleServiceAuthError& error, + bool fire_auth_error_changed) override; GoogleServiceAuthError GetAuthError( const CoreAccountId& account_id) const override; std::vector<CoreAccountId> GetAccounts() const override; @@ -59,6 +60,9 @@ class TestProfileOAuth2TokenServiceDelegateChromeOS void RevokeCredentials(const CoreAccountId& account_id) override; void RevokeAllCredentials() override; const net::BackoffEntry* BackoffEntry() const override; + void ClearAuthError(const absl::optional<CoreAccountId>& account_id) override; + GoogleServiceAuthError BackOffError() const override; + void ResetBackOffEntry() override; // |ProfileOAuth2TokenServiceObserver| implementation. void OnRefreshTokenAvailable(const CoreAccountId& account_id) override; diff --git a/chromium/components/signin/public/android/BUILD.gn b/chromium/components/signin/public/android/BUILD.gn index 17bfae08775..32b09337e08 100644 --- a/chromium/components/signin/public/android/BUILD.gn +++ b/chromium/components/signin/public/android/BUILD.gn @@ -127,7 +127,7 @@ android_resources("signin_test_resources") { sources = [ "javatests/res/drawable/test_profile_picture.xml" ] } -android_library("javatests") { +android_library("unit_device_javatests") { testonly = true deps = [ ":java", @@ -148,9 +148,7 @@ android_library("javatests") { ] } -java_library("junit") { - bypass_platform_checks = true - testonly = true +robolectric_library("junit") { sources = [ "junit/src/org/chromium/components/signin/AccountManagerFacadeImplTest.java", "junit/src/org/chromium/components/signin/AccountRenameCheckerTest.java", @@ -171,7 +169,6 @@ java_library("junit") { "//components/externalauth/android:java", "//testing/android/junit:junit_test_support", "//third_party/android_deps:guava_android_java", - "//third_party/android_deps:robolectric_all_java", "//third_party/androidx:androidx_annotation_annotation_java", "//third_party/androidx:androidx_test_rules_java", "//third_party/junit", diff --git a/chromium/components/signin/public/android/OWNERS b/chromium/components/signin/public/android/OWNERS index 6fee66a0f74..e69de29bb2d 100644 --- a/chromium/components/signin/public/android/OWNERS +++ b/chromium/components/signin/public/android/OWNERS @@ -1 +0,0 @@ -aliceywang@chromium.org diff --git a/chromium/components/signin/public/android/junit/src/org/chromium/components/signin/AccountManagerFacadeImplTest.java b/chromium/components/signin/public/android/junit/src/org/chromium/components/signin/AccountManagerFacadeImplTest.java index 8472a2770b7..afe4d7e4dcd 100644 --- a/chromium/components/signin/public/android/junit/src/org/chromium/components/signin/AccountManagerFacadeImplTest.java +++ b/chromium/components/signin/public/android/junit/src/org/chromium/components/signin/AccountManagerFacadeImplTest.java @@ -279,17 +279,6 @@ public class AccountManagerFacadeImplTest { } @Test - public void testGetAccessToken() throws AuthException { - final Account account = AccountUtils.createAccountFromName("test@gmail.com"); - final AccessTokenData originalToken = - mFacadeWithSystemDelegate.getAccessToken(account, TEST_TOKEN_SCOPE); - - Assert.assertEquals("The same token should be returned.", - mFacadeWithSystemDelegate.getAccessToken(account, TEST_TOKEN_SCOPE).getToken(), - originalToken.getToken()); - } - - @Test public void testGetAndInvalidateAccessToken() throws AuthException { final Account account = addTestAccount("test@gmail.com"); final AccessTokenData originalToken = mFacade.getAccessToken(account, TEST_TOKEN_SCOPE); diff --git a/chromium/components/signin/public/android/junit/src/org/chromium/components/signin/base/AccountCapabilitiesTest.java b/chromium/components/signin/public/android/junit/src/org/chromium/components/signin/base/AccountCapabilitiesTest.java index adb4f3006e4..30b589018ee 100644 --- a/chromium/components/signin/public/android/junit/src/org/chromium/components/signin/base/AccountCapabilitiesTest.java +++ b/chromium/components/signin/public/android/junit/src/org/chromium/components/signin/base/AccountCapabilitiesTest.java @@ -50,6 +50,8 @@ public final class AccountCapabilitiesTest { return capabilities.canRunChromePrivacySandboxTrials(); case AccountCapabilitiesConstants.IS_SUBJECT_TO_PARENTAL_CONTROLS_CAPABILITY_NAME: return capabilities.isSubjectToParentalControls(); + case AccountCapabilitiesConstants.IS_ALLOWED_FOR_MACHINE_LEARNING_CAPABILITY_NAME: + return capabilities.isAllowedForMachineLearning(); case AccountCapabilitiesConstants.CAN_STOP_PARENTAL_SUPERVISION_CAPABILITY_NAME: return capabilities.canStopParentalSupervision(); case AccountCapabilitiesConstants.CAN_TOGGLE_AUTO_UPDATES_NAME: @@ -93,7 +95,11 @@ public final class AccountCapabilitiesTest { .CAN_OFFER_EXTENDED_CHROME_SYNC_PROMOS_CAPABILITY_NAME), new ParameterSet() .name("CanToggleAutoUpdates") - .value(AccountCapabilitiesConstants.CAN_TOGGLE_AUTO_UPDATES_NAME)); + .value(AccountCapabilitiesConstants.CAN_TOGGLE_AUTO_UPDATES_NAME), + new ParameterSet() + .name("IsAllowedForMachineLearning") + .value(AccountCapabilitiesConstants + .IS_ALLOWED_FOR_MACHINE_LEARNING_CAPABILITY_NAME)); // Returns String value added from Capabilities ParameterSet. static String getCapabilityName(ParameterSet parameterSet) { diff --git a/chromium/components/signin/public/base/signin_metrics.cc b/chromium/components/signin/public/base/signin_metrics.cc index a0831b455a9..c230126a075 100644 --- a/chromium/components/signin/public/base/signin_metrics.cc +++ b/chromium/components/signin/public/base/signin_metrics.cc @@ -16,477 +16,6 @@ namespace signin_metrics { -namespace { -void RecordSigninUserActionForAccessPoint(AccessPoint access_point) { - switch (access_point) { - case AccessPoint::ACCESS_POINT_START_PAGE: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromStartPage")); - break; - case AccessPoint::ACCESS_POINT_NTP_LINK: - base::RecordAction(base::UserMetricsAction("Signin_Signin_FromNTP")); - break; - case AccessPoint::ACCESS_POINT_MENU: - base::RecordAction(base::UserMetricsAction("Signin_Signin_FromMenu")); - break; - case AccessPoint::ACCESS_POINT_SETTINGS: - base::RecordAction(base::UserMetricsAction("Signin_Signin_FromSettings")); - break; - case AccessPoint::ACCESS_POINT_SUPERVISED_USER: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromSupervisedUser")); - break; - case AccessPoint::ACCESS_POINT_EXTENSION_INSTALL_BUBBLE: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromExtensionInstallBubble")); - break; - case AccessPoint::ACCESS_POINT_EXTENSIONS: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromExtensions")); - break; - case AccessPoint::ACCESS_POINT_BOOKMARK_BUBBLE: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromBookmarkBubble")); - break; - case AccessPoint::ACCESS_POINT_BOOKMARK_MANAGER: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromBookmarkManager")); - break; - case AccessPoint::ACCESS_POINT_AVATAR_BUBBLE_SIGN_IN: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromAvatarBubbleSignin")); - break; - case AccessPoint::ACCESS_POINT_USER_MANAGER: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromUserManager")); - break; - case AccessPoint::ACCESS_POINT_DEVICES_PAGE: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromDevicesPage")); - break; - case AccessPoint::ACCESS_POINT_CLOUD_PRINT: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromCloudPrint")); - break; - case AccessPoint::ACCESS_POINT_CONTENT_AREA: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromContentArea")); - break; - case AccessPoint::ACCESS_POINT_SIGNIN_PROMO: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromSigninPromo")); - break; - case AccessPoint::ACCESS_POINT_RECENT_TABS: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromRecentTabs")); - break; - case AccessPoint::ACCESS_POINT_UNKNOWN: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromUnknownAccessPoint")); - break; - case AccessPoint::ACCESS_POINT_PASSWORD_BUBBLE: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromPasswordBubble")); - break; - case AccessPoint::ACCESS_POINT_AUTOFILL_DROPDOWN: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromAutofillDropdown")); - break; - case AccessPoint::ACCESS_POINT_NTP_CONTENT_SUGGESTIONS: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromNTPContentSuggestions")); - break; - case AccessPoint::ACCESS_POINT_RESIGNIN_INFOBAR: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromReSigninInfobar")); - break; - case AccessPoint::ACCESS_POINT_TAB_SWITCHER: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromTabSwitcher")); - break; - case AccessPoint::ACCESS_POINT_MACHINE_LOGON: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromMachineLogon")); - break; - case AccessPoint::ACCESS_POINT_GOOGLE_SERVICES_SETTINGS: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromGoogleServicesSettings")); - break; - case AccessPoint::ACCESS_POINT_ENTERPRISE_SIGNOUT_COORDINATOR: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromEnterpriseSignoutSheet")); - break; - case AccessPoint::ACCESS_POINT_SIGNIN_INTERCEPT_FIRST_RUN_EXPERIENCE: - base::RecordAction(base::UserMetricsAction( - "Signin_Signin_FromSigninInterceptFirstRunExperience")); - break; - case AccessPoint::ACCESS_POINT_NTP_FEED_TOP_PROMO: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromNTPFeedTopPromo")); - break; - case AccessPoint::ACCESS_POINT_KALEIDOSCOPE: - NOTREACHED() << "Access point " << static_cast<int>(access_point) - << " is only used to trigger non-sync sign-in and this" - << " action should only be triggered for sync-enabled" - << " sign-ins."; - break; - case AccessPoint::ACCESS_POINT_SYNC_ERROR_CARD: - case AccessPoint::ACCESS_POINT_FORCED_SIGNIN: - case AccessPoint::ACCESS_POINT_ACCOUNT_RENAMED: - case AccessPoint::ACCESS_POINT_WEB_SIGNIN: - case AccessPoint::ACCESS_POINT_SETTINGS_SYNC_OFF_ROW: - NOTREACHED() << "Access point " << static_cast<int>(access_point) - << " is not supposed to log signin user actions."; - break; - case AccessPoint::ACCESS_POINT_SAFETY_CHECK: - VLOG(1) << "Signin_Signin_From* user action is not recorded " - << "for access point " << static_cast<int>(access_point); - break; - case AccessPoint::ACCESS_POINT_SEND_TAB_TO_SELF_PROMO: - base::RecordAction( - base::UserMetricsAction("Signin_Signin_FromSendTabToSelfPromo")); - break; - case AccessPoint::ACCESS_POINT_MAX: - NOTREACHED(); - break; - } -} - -void RecordSigninWithDefaultUserActionForAccessPoint( - signin_metrics::AccessPoint access_point) { - switch (access_point) { - case signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS: - base::RecordAction( - base::UserMetricsAction("Signin_SigninWithDefault_FromSettings")); - break; - case AccessPoint::ACCESS_POINT_EXTENSION_INSTALL_BUBBLE: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninWithDefault_FromExtensionInstallBubble")); - break; - case AccessPoint::ACCESS_POINT_BOOKMARK_BUBBLE: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninWithDefault_FromBookmarkBubble")); - break; - case signin_metrics::AccessPoint::ACCESS_POINT_BOOKMARK_MANAGER: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninWithDefault_FromBookmarkManager")); - break; - case AccessPoint::ACCESS_POINT_AVATAR_BUBBLE_SIGN_IN: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninWithDefault_FromAvatarBubbleSignin")); - break; - case signin_metrics::AccessPoint::ACCESS_POINT_RECENT_TABS: - base::RecordAction( - base::UserMetricsAction("Signin_SigninWithDefault_FromRecentTabs")); - break; - case AccessPoint::ACCESS_POINT_PASSWORD_BUBBLE: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninWithDefault_FromPasswordBubble")); - break; - case signin_metrics::AccessPoint::ACCESS_POINT_TAB_SWITCHER: - base::RecordAction( - base::UserMetricsAction("Signin_SigninWithDefault_FromTabSwitcher")); - break; - case AccessPoint::ACCESS_POINT_NTP_CONTENT_SUGGESTIONS: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninWithDefault_FromNTPContentSuggestions")); - break; - case AccessPoint::ACCESS_POINT_NTP_FEED_TOP_PROMO: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninWithDefault_FromNTPFeedTopPromo")); - break; - case AccessPoint::ACCESS_POINT_ENTERPRISE_SIGNOUT_COORDINATOR: - case AccessPoint::ACCESS_POINT_START_PAGE: - case AccessPoint::ACCESS_POINT_NTP_LINK: - case AccessPoint::ACCESS_POINT_MENU: - case AccessPoint::ACCESS_POINT_SUPERVISED_USER: - case AccessPoint::ACCESS_POINT_EXTENSIONS: - case AccessPoint::ACCESS_POINT_USER_MANAGER: - case AccessPoint::ACCESS_POINT_DEVICES_PAGE: - case AccessPoint::ACCESS_POINT_CLOUD_PRINT: - case AccessPoint::ACCESS_POINT_CONTENT_AREA: - case AccessPoint::ACCESS_POINT_SIGNIN_PROMO: - case AccessPoint::ACCESS_POINT_AUTOFILL_DROPDOWN: - case AccessPoint::ACCESS_POINT_RESIGNIN_INFOBAR: - case AccessPoint::ACCESS_POINT_UNKNOWN: - case AccessPoint::ACCESS_POINT_MACHINE_LOGON: - case AccessPoint::ACCESS_POINT_GOOGLE_SERVICES_SETTINGS: - case AccessPoint::ACCESS_POINT_SYNC_ERROR_CARD: - case AccessPoint::ACCESS_POINT_FORCED_SIGNIN: - case AccessPoint::ACCESS_POINT_ACCOUNT_RENAMED: - case AccessPoint::ACCESS_POINT_WEB_SIGNIN: - case AccessPoint::ACCESS_POINT_SAFETY_CHECK: - case AccessPoint::ACCESS_POINT_KALEIDOSCOPE: - case AccessPoint::ACCESS_POINT_SIGNIN_INTERCEPT_FIRST_RUN_EXPERIENCE: - case AccessPoint::ACCESS_POINT_SEND_TAB_TO_SELF_PROMO: - case signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS_SYNC_OFF_ROW: - NOTREACHED() << "Signin_SigninWithDefault_From* user actions" - << " are not recorded for access_point " - << static_cast<int>(access_point) - << " as it does not support a personalized sign-in promo."; - break; - case AccessPoint::ACCESS_POINT_MAX: - NOTREACHED(); - break; - } -} - -void RecordSigninNotDefaultUserActionForAccessPoint( - signin_metrics::AccessPoint access_point) { - switch (access_point) { - case signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS: - base::RecordAction( - base::UserMetricsAction("Signin_SigninNotDefault_FromSettings")); - break; - case AccessPoint::ACCESS_POINT_EXTENSION_INSTALL_BUBBLE: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNotDefault_FromExtensionInstallBubble")); - break; - case AccessPoint::ACCESS_POINT_BOOKMARK_BUBBLE: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNotDefault_FromBookmarkBubble")); - break; - case signin_metrics::AccessPoint::ACCESS_POINT_BOOKMARK_MANAGER: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNotDefault_FromBookmarkManager")); - break; - case AccessPoint::ACCESS_POINT_AVATAR_BUBBLE_SIGN_IN: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNotDefault_FromAvatarBubbleSignin")); - break; - case signin_metrics::AccessPoint::ACCESS_POINT_RECENT_TABS: - base::RecordAction( - base::UserMetricsAction("Signin_SigninNotDefault_FromRecentTabs")); - break; - case AccessPoint::ACCESS_POINT_PASSWORD_BUBBLE: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNotDefault_FromPasswordBubble")); - break; - case signin_metrics::AccessPoint::ACCESS_POINT_TAB_SWITCHER: - base::RecordAction( - base::UserMetricsAction("Signin_SigninNotDefault_FromTabSwitcher")); - break; - case AccessPoint::ACCESS_POINT_NTP_CONTENT_SUGGESTIONS: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNotDefault_FromNTPContentSuggestions")); - break; - case AccessPoint::ACCESS_POINT_NTP_FEED_TOP_PROMO: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNotDefault_FromNTPFeedTopPromo")); - break; - case AccessPoint::ACCESS_POINT_ENTERPRISE_SIGNOUT_COORDINATOR: - case AccessPoint::ACCESS_POINT_START_PAGE: - case AccessPoint::ACCESS_POINT_NTP_LINK: - case AccessPoint::ACCESS_POINT_MENU: - case AccessPoint::ACCESS_POINT_SUPERVISED_USER: - case AccessPoint::ACCESS_POINT_EXTENSIONS: - case AccessPoint::ACCESS_POINT_USER_MANAGER: - case AccessPoint::ACCESS_POINT_DEVICES_PAGE: - case AccessPoint::ACCESS_POINT_CLOUD_PRINT: - case AccessPoint::ACCESS_POINT_CONTENT_AREA: - case AccessPoint::ACCESS_POINT_SIGNIN_PROMO: - case AccessPoint::ACCESS_POINT_AUTOFILL_DROPDOWN: - case AccessPoint::ACCESS_POINT_RESIGNIN_INFOBAR: - case AccessPoint::ACCESS_POINT_UNKNOWN: - case AccessPoint::ACCESS_POINT_MACHINE_LOGON: - case AccessPoint::ACCESS_POINT_GOOGLE_SERVICES_SETTINGS: - case AccessPoint::ACCESS_POINT_SYNC_ERROR_CARD: - case AccessPoint::ACCESS_POINT_FORCED_SIGNIN: - case AccessPoint::ACCESS_POINT_ACCOUNT_RENAMED: - case AccessPoint::ACCESS_POINT_WEB_SIGNIN: - case AccessPoint::ACCESS_POINT_SAFETY_CHECK: - case AccessPoint::ACCESS_POINT_KALEIDOSCOPE: - case AccessPoint::ACCESS_POINT_SIGNIN_INTERCEPT_FIRST_RUN_EXPERIENCE: - case AccessPoint::ACCESS_POINT_SEND_TAB_TO_SELF_PROMO: - case signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS_SYNC_OFF_ROW: - NOTREACHED() << "Signin_SigninNotDefault_From* user actions" - << " are not recorded for access point " - << static_cast<int>(access_point) - << " as it does not support a personalized sign-in promo."; - break; - case AccessPoint::ACCESS_POINT_MAX: - NOTREACHED(); - break; - } -} - -void RecordSigninNewAccountNoExistingAccountUserActionForAccessPoint( - signin_metrics::AccessPoint access_point) { - switch (access_point) { - case signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountNoExistingAccount_FromSettings")); - break; - case AccessPoint::ACCESS_POINT_EXTENSION_INSTALL_BUBBLE: - // clang-format off - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountNoExistingAccount_FromExtensionInstallBubble")); // NOLINT(whitespace/line_length) - // clang-format on - break; - case AccessPoint::ACCESS_POINT_BOOKMARK_BUBBLE: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountNoExistingAccount_FromBookmarkBubble")); - break; - case signin_metrics::AccessPoint::ACCESS_POINT_BOOKMARK_MANAGER: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountNoExistingAccount_FromBookmarkManager")); - break; - case AccessPoint::ACCESS_POINT_AVATAR_BUBBLE_SIGN_IN: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountNoExistingAccount_FromAvatarBubbleSignin")); - break; - case signin_metrics::AccessPoint::ACCESS_POINT_RECENT_TABS: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountNoExistingAccount_FromRecentTabs")); - break; - case AccessPoint::ACCESS_POINT_PASSWORD_BUBBLE: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountNoExistingAccount_FromPasswordBubble")); - break; - case signin_metrics::AccessPoint::ACCESS_POINT_TAB_SWITCHER: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountNoExistingAccount_FromTabSwitcher")); - break; - case AccessPoint::ACCESS_POINT_NTP_CONTENT_SUGGESTIONS: - // clang-format off - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountNoExistingAccount_FromNTPContentSuggestions")); // NOLINT(whitespace/line_length) - // clang-format on - break; - case AccessPoint::ACCESS_POINT_NTP_FEED_TOP_PROMO: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountNoExistingAccount_FromNTPFeedTopPromo")); - break; - case AccessPoint::ACCESS_POINT_ENTERPRISE_SIGNOUT_COORDINATOR: - case AccessPoint::ACCESS_POINT_START_PAGE: - case AccessPoint::ACCESS_POINT_NTP_LINK: - case AccessPoint::ACCESS_POINT_MENU: - case AccessPoint::ACCESS_POINT_SUPERVISED_USER: - case AccessPoint::ACCESS_POINT_EXTENSIONS: - case AccessPoint::ACCESS_POINT_USER_MANAGER: - case AccessPoint::ACCESS_POINT_DEVICES_PAGE: - case AccessPoint::ACCESS_POINT_CLOUD_PRINT: - case AccessPoint::ACCESS_POINT_CONTENT_AREA: - case AccessPoint::ACCESS_POINT_SIGNIN_PROMO: - case AccessPoint::ACCESS_POINT_AUTOFILL_DROPDOWN: - case AccessPoint::ACCESS_POINT_RESIGNIN_INFOBAR: - case AccessPoint::ACCESS_POINT_UNKNOWN: - case AccessPoint::ACCESS_POINT_MACHINE_LOGON: - case AccessPoint::ACCESS_POINT_GOOGLE_SERVICES_SETTINGS: - case AccessPoint::ACCESS_POINT_SYNC_ERROR_CARD: - case AccessPoint::ACCESS_POINT_FORCED_SIGNIN: - case AccessPoint::ACCESS_POINT_ACCOUNT_RENAMED: - case AccessPoint::ACCESS_POINT_WEB_SIGNIN: - case AccessPoint::ACCESS_POINT_SAFETY_CHECK: - case AccessPoint::ACCESS_POINT_KALEIDOSCOPE: - case AccessPoint::ACCESS_POINT_SIGNIN_INTERCEPT_FIRST_RUN_EXPERIENCE: - case AccessPoint::ACCESS_POINT_SEND_TAB_TO_SELF_PROMO: - case signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS_SYNC_OFF_ROW: - // These access points do not support personalized sign-in promos, so - // |Signin_SigninNewAccountNoExistingAccount_From*| user actions should - // not be recorded for them. Note: To avoid bloating the sign-in APIs, the - // sign-in metrics simply ignore if the caller passes - // |PROMO_ACTION_NEW_ACCOUNT_NO_EXISTING_ACCOUNT| when a sign-in flow is - // started from any access point instead of treating it as an error like - // in the other cases (|WithDefault| and |NotDefault|). - VLOG(1) << "Signin_SigninNewAccountNoExistingAccount_From* user actions" - << " are not recorded for access point " - << static_cast<int>(access_point) - << " as it does not support a personalized sign-in promo."; - break; - case AccessPoint::ACCESS_POINT_MAX: - NOTREACHED(); - break; - } -} - -void RecordSigninNewAccountExistingAccountUserActionForAccessPoint( - signin_metrics::AccessPoint access_point) { - switch (access_point) { - case signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountExistingAccount_FromSettings")); - break; - case AccessPoint::ACCESS_POINT_EXTENSION_INSTALL_BUBBLE: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountExistingAccount_FromExtensionInstallBubble")); - break; - case AccessPoint::ACCESS_POINT_BOOKMARK_BUBBLE: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountExistingAccount_FromBookmarkBubble")); - break; - case signin_metrics::AccessPoint::ACCESS_POINT_BOOKMARK_MANAGER: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountExistingAccount_FromBookmarkManager")); - break; - case AccessPoint::ACCESS_POINT_AVATAR_BUBBLE_SIGN_IN: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountExistingAccount_FromAvatarBubbleSignin")); - break; - case signin_metrics::AccessPoint::ACCESS_POINT_RECENT_TABS: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountExistingAccount_FromRecentTabs")); - break; - case AccessPoint::ACCESS_POINT_PASSWORD_BUBBLE: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountExistingAccount_FromPasswordBubble")); - break; - case signin_metrics::AccessPoint::ACCESS_POINT_TAB_SWITCHER: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountExistingAccount_FromTabSwitcher")); - break; - case AccessPoint::ACCESS_POINT_NTP_CONTENT_SUGGESTIONS: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountExistingAccount_FromNTPContentSuggestions")); - break; - case AccessPoint::ACCESS_POINT_NTP_FEED_TOP_PROMO: - base::RecordAction(base::UserMetricsAction( - "Signin_SigninNewAccountExistingAccount_FromNTPFeedTopPromo")); - break; - case AccessPoint::ACCESS_POINT_ENTERPRISE_SIGNOUT_COORDINATOR: - case AccessPoint::ACCESS_POINT_START_PAGE: - case AccessPoint::ACCESS_POINT_NTP_LINK: - case AccessPoint::ACCESS_POINT_MENU: - case AccessPoint::ACCESS_POINT_SUPERVISED_USER: - case AccessPoint::ACCESS_POINT_EXTENSIONS: - case AccessPoint::ACCESS_POINT_USER_MANAGER: - case AccessPoint::ACCESS_POINT_DEVICES_PAGE: - case AccessPoint::ACCESS_POINT_CLOUD_PRINT: - case AccessPoint::ACCESS_POINT_CONTENT_AREA: - case AccessPoint::ACCESS_POINT_SIGNIN_PROMO: - case AccessPoint::ACCESS_POINT_AUTOFILL_DROPDOWN: - case AccessPoint::ACCESS_POINT_RESIGNIN_INFOBAR: - case AccessPoint::ACCESS_POINT_UNKNOWN: - case AccessPoint::ACCESS_POINT_MACHINE_LOGON: - case AccessPoint::ACCESS_POINT_GOOGLE_SERVICES_SETTINGS: - case AccessPoint::ACCESS_POINT_SYNC_ERROR_CARD: - case AccessPoint::ACCESS_POINT_FORCED_SIGNIN: - case AccessPoint::ACCESS_POINT_ACCOUNT_RENAMED: - case AccessPoint::ACCESS_POINT_WEB_SIGNIN: - case AccessPoint::ACCESS_POINT_SAFETY_CHECK: - case AccessPoint::ACCESS_POINT_KALEIDOSCOPE: - case AccessPoint::ACCESS_POINT_SIGNIN_INTERCEPT_FIRST_RUN_EXPERIENCE: - case AccessPoint::ACCESS_POINT_SEND_TAB_TO_SELF_PROMO: - case AccessPoint::ACCESS_POINT_SETTINGS_SYNC_OFF_ROW: - // These access points do not support personalized sign-in promos, so - // |Signin_SigninNewAccountExistingAccount_From*| user actions should not - // be recorded for them. Note: To avoid bloating the sign-in APIs, the - // sign-in metrics simply ignore if the caller passes - // |PROMO_ACTION_NEW_ACCOUNT_EXISTING_ACCOUNT| when a sign-in flow is - // started from any access point instead of treating it as an error like - // in the other cases (|WithDefault| and |NotDefault|). - VLOG(1) << "Signin_SigninNewAccountExistingAccount_From* user actions" - << " are not recorded for access point " - << static_cast<int>(access_point) - << " as it does not support a personalized sign-in promo."; - break; - case AccessPoint::ACCESS_POINT_MAX: - NOTREACHED(); - break; - } -} -} // namespace - // These intermediate macros are necessary when we may emit to different // histograms from the same logical place in the code. The base histogram macros // expand in a way that can only work for a single histogram name, so these @@ -656,12 +185,6 @@ void LogAccountReconcilorStateOnGaiaResponse(AccountReconcilorState state) { state); } -void LogAccountEquality(AccountEquality equality) { - UMA_HISTOGRAM_ENUMERATION("Signin.AccountEquality", - static_cast<int>(equality), - static_cast<int>(AccountEquality::HISTOGRAM_COUNT)); -} - void LogCookieJarStableAge(const base::TimeDelta stable_age, const ReportingType type) { INVESTIGATOR_HISTOGRAM_CUSTOM_COUNTS( @@ -759,25 +282,137 @@ void RecordSigninAccountType(signin::ConsentLevel consent_level, // User actions // -------------------------------------------------------------- -void RecordSigninUserActionForAccessPoint(AccessPoint access_point, - PromoAction promo_action) { - RecordSigninUserActionForAccessPoint(access_point); - switch (promo_action) { - case PromoAction::PROMO_ACTION_NO_SIGNIN_PROMO: +void RecordSigninUserActionForAccessPoint(AccessPoint access_point) { + switch (access_point) { + case AccessPoint::ACCESS_POINT_START_PAGE: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromStartPage")); break; - case PromoAction::PROMO_ACTION_WITH_DEFAULT: - RecordSigninWithDefaultUserActionForAccessPoint(access_point); + case AccessPoint::ACCESS_POINT_NTP_LINK: + base::RecordAction(base::UserMetricsAction("Signin_Signin_FromNTP")); break; - case PromoAction::PROMO_ACTION_NOT_DEFAULT: - RecordSigninNotDefaultUserActionForAccessPoint(access_point); + case AccessPoint::ACCESS_POINT_MENU: + base::RecordAction(base::UserMetricsAction("Signin_Signin_FromMenu")); break; - case PromoAction::PROMO_ACTION_NEW_ACCOUNT_NO_EXISTING_ACCOUNT: - RecordSigninNewAccountNoExistingAccountUserActionForAccessPoint( - access_point); + case AccessPoint::ACCESS_POINT_SETTINGS: + base::RecordAction(base::UserMetricsAction("Signin_Signin_FromSettings")); break; - case PromoAction::PROMO_ACTION_NEW_ACCOUNT_EXISTING_ACCOUNT: - RecordSigninNewAccountExistingAccountUserActionForAccessPoint( - access_point); + case AccessPoint::ACCESS_POINT_SUPERVISED_USER: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromSupervisedUser")); + break; + case AccessPoint::ACCESS_POINT_EXTENSION_INSTALL_BUBBLE: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromExtensionInstallBubble")); + break; + case AccessPoint::ACCESS_POINT_EXTENSIONS: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromExtensions")); + break; + case AccessPoint::ACCESS_POINT_BOOKMARK_BUBBLE: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromBookmarkBubble")); + break; + case AccessPoint::ACCESS_POINT_BOOKMARK_MANAGER: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromBookmarkManager")); + break; + case AccessPoint::ACCESS_POINT_AVATAR_BUBBLE_SIGN_IN: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromAvatarBubbleSignin")); + break; + case AccessPoint::ACCESS_POINT_USER_MANAGER: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromUserManager")); + break; + case AccessPoint::ACCESS_POINT_DEVICES_PAGE: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromDevicesPage")); + break; + case AccessPoint::ACCESS_POINT_CLOUD_PRINT: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromCloudPrint")); + break; + case AccessPoint::ACCESS_POINT_CONTENT_AREA: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromContentArea")); + break; + case AccessPoint::ACCESS_POINT_SIGNIN_PROMO: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromSigninPromo")); + break; + case AccessPoint::ACCESS_POINT_RECENT_TABS: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromRecentTabs")); + break; + case AccessPoint::ACCESS_POINT_UNKNOWN: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromUnknownAccessPoint")); + break; + case AccessPoint::ACCESS_POINT_PASSWORD_BUBBLE: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromPasswordBubble")); + break; + case AccessPoint::ACCESS_POINT_AUTOFILL_DROPDOWN: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromAutofillDropdown")); + break; + case AccessPoint::ACCESS_POINT_NTP_CONTENT_SUGGESTIONS: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromNTPContentSuggestions")); + break; + case AccessPoint::ACCESS_POINT_RESIGNIN_INFOBAR: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromReSigninInfobar")); + break; + case AccessPoint::ACCESS_POINT_TAB_SWITCHER: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromTabSwitcher")); + break; + case AccessPoint::ACCESS_POINT_MACHINE_LOGON: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromMachineLogon")); + break; + case AccessPoint::ACCESS_POINT_GOOGLE_SERVICES_SETTINGS: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromGoogleServicesSettings")); + break; + case AccessPoint::ACCESS_POINT_ENTERPRISE_SIGNOUT_COORDINATOR: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromEnterpriseSignoutSheet")); + break; + case AccessPoint::ACCESS_POINT_SIGNIN_INTERCEPT_FIRST_RUN_EXPERIENCE: + base::RecordAction(base::UserMetricsAction( + "Signin_Signin_FromSigninInterceptFirstRunExperience")); + break; + case AccessPoint::ACCESS_POINT_NTP_FEED_TOP_PROMO: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromNTPFeedTopPromo")); + break; + case AccessPoint::ACCESS_POINT_KALEIDOSCOPE: + NOTREACHED() << "Access point " << static_cast<int>(access_point) + << " is only used to trigger non-sync sign-in and this" + << " action should only be triggered for sync-enabled" + << " sign-ins."; + break; + case AccessPoint::ACCESS_POINT_SYNC_ERROR_CARD: + case AccessPoint::ACCESS_POINT_FORCED_SIGNIN: + case AccessPoint::ACCESS_POINT_ACCOUNT_RENAMED: + case AccessPoint::ACCESS_POINT_WEB_SIGNIN: + case AccessPoint::ACCESS_POINT_SETTINGS_SYNC_OFF_ROW: + NOTREACHED() << "Access point " << static_cast<int>(access_point) + << " is not supposed to log signin user actions."; + break; + case AccessPoint::ACCESS_POINT_SAFETY_CHECK: + VLOG(1) << "Signin_Signin_From* user action is not recorded " + << "for access point " << static_cast<int>(access_point); + break; + case AccessPoint::ACCESS_POINT_SEND_TAB_TO_SELF_PROMO: + base::RecordAction( + base::UserMetricsAction("Signin_Signin_FromSendTabToSelfPromo")); + break; + case AccessPoint::ACCESS_POINT_MAX: + NOTREACHED(); break; } } diff --git a/chromium/components/signin/public/base/signin_metrics.h b/chromium/components/signin/public/base/signin_metrics.h index 7bcc60e3423..80404ebb7f5 100644 --- a/chromium/components/signin/public/base/signin_metrics.h +++ b/chromium/components/signin/public/base/signin_metrics.h @@ -335,26 +335,6 @@ enum AccountReconcilorState { kMaxValue = ACCOUNT_RECONCILOR_SCHEDULED, }; -// Values of histogram comparing account id and email. -enum class AccountEquality : int { - // Expected case when the user is not switching accounts. - BOTH_EQUAL = 0, - // Expected case when the user is switching accounts. - BOTH_DIFFERENT, - // The user has changed at least two email account names. This is actually - // a different account, even though the email matches. - ONLY_SAME_EMAIL, - // The user has changed the email of their account, but the account is - // actually the same. - ONLY_SAME_ID, - // The last account id was not present, email equality was used. This should - // happen once to all old clients. Does not differentiate between same and - // different accounts. - EMAIL_FALLBACK, - // Always the last enumerated type. - HISTOGRAM_COUNT, -}; - // Values of Signin.AccountType histogram. This histogram records if the user // uses a gmail account or a managed account when signing in. enum class SigninAccountType : int { @@ -510,10 +490,6 @@ void LogAuthError(const GoogleServiceAuthError& auth_error); // be shown a different set of accounts in the content-area and the settings UI. void LogAccountReconcilorStateOnGaiaResponse(AccountReconcilorState state); -// Records the AccountEquality metric when an investigator compares the current -// and previous id/emails during a signin. -void LogAccountEquality(AccountEquality equality); - // Records the amount of time since the cookie jar was last changed. void LogCookieJarStableAge(const base::TimeDelta stable_age, const ReportingType type); @@ -557,8 +533,7 @@ void RecordSigninAccountType(signin::ConsentLevel consent_level, // ----------------------------------------------------------------------------- // Records corresponding sign in user action for an access point. -void RecordSigninUserActionForAccessPoint(AccessPoint access_point, - PromoAction promo_action); +void RecordSigninUserActionForAccessPoint(AccessPoint access_point); // Records |Signin_Impression_From*| user action. void RecordSigninImpressionUserActionForAccessPoint(AccessPoint access_point); diff --git a/chromium/components/signin/public/base/signin_metrics_unittest.cc b/chromium/components/signin/public/base/signin_metrics_unittest.cc index dac68c1350f..bc5f3580a13 100644 --- a/chromium/components/signin/public/base/signin_metrics_unittest.cc +++ b/chromium/components/signin/public/base/signin_metrics_unittest.cc @@ -164,73 +164,12 @@ class SigninMetricsTest : public ::testing::Test { TEST_F(SigninMetricsTest, RecordSigninUserActionForAccessPoint) { for (const AccessPoint& ap : kAccessPointsThatSupportUserAction) { base::UserActionTester user_action_tester; - RecordSigninUserActionForAccessPoint( - ap, signin_metrics::PromoAction::PROMO_ACTION_NO_SIGNIN_PROMO); + RecordSigninUserActionForAccessPoint(ap); EXPECT_EQ(1, user_action_tester.GetActionCount( "Signin_Signin_From" + GetAccessPointDescription(ap))); } } -TEST_F(SigninMetricsTest, RecordSigninUserActionWithPromoAction) { - for (const AccessPoint& ap : kAccessPointsThatSupportPersonalizedPromos) { - { - // PROMO_ACTION_WITH_DEFAULT promo action - base::UserActionTester user_action_tester; - RecordSigninUserActionForAccessPoint( - ap, signin_metrics::PromoAction::PROMO_ACTION_WITH_DEFAULT); - EXPECT_EQ( - 1, user_action_tester.GetActionCount("Signin_SigninWithDefault_From" + - GetAccessPointDescription(ap))); - } - { - // PROMO_ACTION_NOT_DEFAULT promo action - base::UserActionTester user_action_tester; - RecordSigninUserActionForAccessPoint( - ap, signin_metrics::PromoAction::PROMO_ACTION_NOT_DEFAULT); - EXPECT_EQ( - 1, user_action_tester.GetActionCount("Signin_SigninNotDefault_From" + - GetAccessPointDescription(ap))); - } - } -} - -TEST_F(SigninMetricsTest, RecordSigninUserActionWithNewNoExistingPromoAction) { - for (const AccessPoint& ap : kAccessPointsThatSupportUserAction) { - base::UserActionTester user_action_tester; - RecordSigninUserActionForAccessPoint( - ap, signin_metrics::PromoAction:: - PROMO_ACTION_NEW_ACCOUNT_NO_EXISTING_ACCOUNT); - if (AccessPointSupportsPersonalizedPromo(ap)) { - EXPECT_EQ(1, user_action_tester.GetActionCount( - "Signin_SigninNewAccountNoExistingAccount_From" + - GetAccessPointDescription(ap))); - } else { - EXPECT_EQ(0, user_action_tester.GetActionCount( - "Signin_SigninNewAccountNoExistingAccount_From" + - GetAccessPointDescription(ap))); - } - } -} - -TEST_F(SigninMetricsTest, - RecordSigninUserActionWithNewWithExistingPromoAction) { - for (const AccessPoint& ap : kAccessPointsThatSupportUserAction) { - base::UserActionTester user_action_tester; - RecordSigninUserActionForAccessPoint( - ap, - signin_metrics::PromoAction::PROMO_ACTION_NEW_ACCOUNT_EXISTING_ACCOUNT); - if (AccessPointSupportsPersonalizedPromo(ap)) { - EXPECT_EQ(1, user_action_tester.GetActionCount( - "Signin_SigninNewAccountExistingAccount_From" + - GetAccessPointDescription(ap))); - } else { - EXPECT_EQ(0, user_action_tester.GetActionCount( - "Signin_SigninNewAccountExistingAccount_From" + - GetAccessPointDescription(ap))); - } - } -} - TEST_F(SigninMetricsTest, RecordSigninImpressionUserAction) { for (const AccessPoint& ap : kAccessPointsThatSupportImpression) { base::UserActionTester user_action_tester; diff --git a/chromium/components/signin/public/base/signin_switches.cc b/chromium/components/signin/public/base/signin_switches.cc index 24139003d84..054988bca4f 100644 --- a/chromium/components/signin/public/base/signin_switches.cc +++ b/chromium/components/signin/public/base/signin_switches.cc @@ -20,6 +20,12 @@ const base::Feature kAllowSyncOffForChildAccounts{ "AllowSyncOffForChildAccounts", base::FEATURE_DISABLED_BY_DEFAULT}; #endif +// If enabled, performs the URL-based check first when proving that the +// X-Chrome-Connected header is not needed in request headers on HTTP +// redirects. The hypothesis is that this order of checks is faster to perform. +const base::Feature kNewSigninRequestHeaderCheckOrder{ + "NewSigninRequestHeaderCheckOrder", base::FEATURE_DISABLED_BY_DEFAULT}; + // Clears the token service before using it. This allows simulating the // expiration of credentials during testing. const char kClearTokenService[] = "clear-token-service"; @@ -59,10 +65,4 @@ const base::Feature kTangibleSync{"TangibleSync", base::FEATURE_DISABLED_BY_DEFAULT}; #endif -#if BUILDFLAG(IS_CHROMEOS_LACROS) -// Allows local (not signed-in) profiles on lacros. -const base::Feature kLacrosNonSyncingProfiles{"LacrosNonSyncingProfiles", - base::FEATURE_ENABLED_BY_DEFAULT}; -#endif - } // namespace switches diff --git a/chromium/components/signin/public/base/signin_switches.h b/chromium/components/signin/public/base/signin_switches.h index 37c8689f95c..dac8fc4719f 100644 --- a/chromium/components/signin/public/base/signin_switches.h +++ b/chromium/components/signin/public/base/signin_switches.h @@ -27,6 +27,8 @@ extern const base::Feature kAccountIdMigration; extern const base::Feature kAllowSyncOffForChildAccounts; #endif +extern const base::Feature kNewSigninRequestHeaderCheckOrder; + extern const char kClearTokenService[]; extern const char kDisableSigninScopedDeviceId[]; @@ -41,10 +43,6 @@ extern const base::Feature kForceStartupSigninPromo; extern const base::Feature kTangibleSync; #endif -#if BUILDFLAG(IS_CHROMEOS_LACROS) -extern const base::Feature kLacrosNonSyncingProfiles; -#endif - } // namespace switches #endif // COMPONENTS_SIGNIN_PUBLIC_BASE_SIGNIN_SWITCHES_H_ diff --git a/chromium/components/signin/public/identity_manager/BUILD.gn b/chromium/components/signin/public/identity_manager/BUILD.gn index fac2ebe4be2..01e76302e53 100644 --- a/chromium/components/signin/public/identity_manager/BUILD.gn +++ b/chromium/components/signin/public/identity_manager/BUILD.gn @@ -139,7 +139,7 @@ source_set("unit_tests") { } if (is_chromeos_ash) { - deps += [ "//ash/components/account_manager" ] + deps += [ "//chromeos/ash/components/account_manager" ] } } @@ -175,7 +175,7 @@ source_set("test_support") { ] if (is_chromeos_ash) { - deps += [ "//ash/components/account_manager" ] + deps += [ "//chromeos/ash/components/account_manager" ] } if (is_chromeos_lacros) { diff --git a/chromium/components/signin/public/identity_manager/access_token_constants.cc b/chromium/components/signin/public/identity_manager/access_token_constants.cc index c7bb097e40e..d50e889537d 100644 --- a/chromium/components/signin/public/identity_manager/access_token_constants.cc +++ b/chromium/components/signin/public/identity_manager/access_token_constants.cc @@ -64,6 +64,11 @@ const std::set<std::string> GetUnconsentedOAuth2Scopes() { // Required to fetch the ManagedAccounsSigninRestriction policy. GaiaConstants::kSecureConnectOAuth2Scope, + // Required for requesting Discover feed with personalization without + // sync consent. Sync consent isn't required for personalization but can + // improve suggestions. + GaiaConstants::kFeedOAuth2Scope, + // Required by ChromeOS only. #if BUILDFLAG(IS_CHROMEOS_ASH) GaiaConstants::kAccountsReauthOAuth2Scope, diff --git a/chromium/components/signin/public/identity_manager/account_capabilities.cc b/chromium/components/signin/public/identity_manager/account_capabilities.cc index b6d109499d1..a42da3ff254 100644 --- a/chromium/components/signin/public/identity_manager/account_capabilities.cc +++ b/chromium/components/signin/public/identity_manager/account_capabilities.cc @@ -77,6 +77,10 @@ signin::Tribool AccountCapabilities::is_subject_to_parental_controls() const { return GetCapabilityByName(kIsSubjectToParentalControlsCapabilityName); } +signin::Tribool AccountCapabilities::is_allowed_for_machine_learning() const { + return GetCapabilityByName(kIsAllowedForMachineLearningCapabilityName); +} + signin::Tribool AccountCapabilities::can_toggle_auto_updates() const { return GetCapabilityByName(kCanToggleAutoUpdatesName); } diff --git a/chromium/components/signin/public/identity_manager/account_capabilities.h b/chromium/components/signin/public/identity_manager/account_capabilities.h index 3e0b7dd3cc3..844cfbec2b6 100644 --- a/chromium/components/signin/public/identity_manager/account_capabilities.h +++ b/chromium/components/signin/public/identity_manager/account_capabilities.h @@ -55,6 +55,10 @@ class AccountCapabilities { // Chrome applies parental controls to accounts with this capability. signin::Tribool is_subject_to_parental_controls() const; + // Chrome can send user data to Google servers for machine learning purposes + // with this capability. + signin::Tribool is_allowed_for_machine_learning() const; + // Chrome can toggle auto updates with this capability. signin::Tribool can_toggle_auto_updates() const; diff --git a/chromium/components/signin/public/identity_manager/account_capabilities_test_mutator.cc b/chromium/components/signin/public/identity_manager/account_capabilities_test_mutator.cc index 956131f604d..f077f57ca37 100644 --- a/chromium/components/signin/public/identity_manager/account_capabilities_test_mutator.cc +++ b/chromium/components/signin/public/identity_manager/account_capabilities_test_mutator.cc @@ -42,6 +42,12 @@ void AccountCapabilitiesTestMutator::set_is_subject_to_parental_controls( value; } +void AccountCapabilitiesTestMutator::set_is_allowed_for_machine_learning( + bool value) { + capabilities_->capabilities_map_[kIsAllowedForMachineLearningCapabilityName] = + value; +} + void AccountCapabilitiesTestMutator::set_can_toggle_auto_updates(bool value) { capabilities_->capabilities_map_[kCanToggleAutoUpdatesName] = value; } diff --git a/chromium/components/signin/public/identity_manager/account_capabilities_test_mutator.h b/chromium/components/signin/public/identity_manager/account_capabilities_test_mutator.h index 08a2ac41a35..f8edf5e9fa4 100644 --- a/chromium/components/signin/public/identity_manager/account_capabilities_test_mutator.h +++ b/chromium/components/signin/public/identity_manager/account_capabilities_test_mutator.h @@ -22,6 +22,7 @@ class AccountCapabilitiesTestMutator { void set_can_run_chrome_privacy_sandbox_trials(bool value); void set_can_stop_parental_supervision(bool value); void set_is_subject_to_parental_controls(bool value); + void set_is_allowed_for_machine_learning(bool value); void set_can_toggle_auto_updates(bool value); // Modifies all supported capabilities at once. diff --git a/chromium/components/signin/public/identity_manager/account_capabilities_unittest.cc b/chromium/components/signin/public/identity_manager/account_capabilities_unittest.cc index 31698927e83..b3cceb63772 100644 --- a/chromium/components/signin/public/identity_manager/account_capabilities_unittest.cc +++ b/chromium/components/signin/public/identity_manager/account_capabilities_unittest.cc @@ -73,6 +73,21 @@ TEST_F(AccountCapabilitiesTest, IsSubjectToParentalControls) { signin::Tribool::kFalse); } +TEST_F(AccountCapabilitiesTest, IsAllowedForMachineLearning) { + AccountCapabilities capabilities; + EXPECT_EQ(capabilities.is_allowed_for_machine_learning(), + signin::Tribool::kUnknown); + + AccountCapabilitiesTestMutator mutator(&capabilities); + mutator.set_is_allowed_for_machine_learning(true); + EXPECT_EQ(capabilities.is_allowed_for_machine_learning(), + signin::Tribool::kTrue); + + mutator.set_is_allowed_for_machine_learning(false); + EXPECT_EQ(capabilities.is_allowed_for_machine_learning(), + signin::Tribool::kFalse); +} + TEST_F(AccountCapabilitiesTest, CanToggleAutoUpdates) { AccountCapabilities capabilities; EXPECT_EQ(capabilities.can_toggle_auto_updates(), signin::Tribool::kUnknown); diff --git a/chromium/components/signin/public/identity_manager/identity_manager.cc b/chromium/components/signin/public/identity_manager/identity_manager.cc index 223b74bf7dd..9de8ca32099 100644 --- a/chromium/components/signin/public/identity_manager/identity_manager.cc +++ b/chromium/components/signin/public/identity_manager/identity_manager.cc @@ -7,7 +7,6 @@ #include <string> #include "base/bind.h" -#include "base/feature_list.h" #include "base/observer_list.h" #include "build/build_config.h" #include "build/chromeos_buildflags.h" @@ -41,7 +40,6 @@ #if BUILDFLAG(IS_CHROMEOS_LACROS) #include "components/account_manager_core/account.h" -#include "components/signin/public/base/signin_switches.h" #include "components/signin/public/identity_manager/tribool.h" #include "third_party/abseil-cpp/absl/types/optional.h" #endif @@ -174,16 +172,12 @@ IdentityManager::IdentityManager(IdentityManager::InitParameters&& parameters) const absl::optional<bool>& initial_account_is_child = signin_client_->IsInitialPrimaryAccountChild(); CHECK(initial_account_is_child.has_value()); - SetPrimaryAccount( - this, account_tracker_service_.get(), signin_client_, - initial_account.value(), - initial_account_is_child.value() ? signin::Tribool::kTrue - : signin::Tribool::kFalse, - base::FeatureList::IsEnabled(switches::kLacrosNonSyncingProfiles) - ? ConsentLevel::kSignin - : ConsentLevel::kSync - - ); + SetPrimaryAccount(this, account_tracker_service_.get(), signin_client_, + initial_account.value(), + initial_account_is_child.value() + ? signin::Tribool::kTrue + : signin::Tribool::kFalse, + ConsentLevel::kSignin); } #endif } diff --git a/chromium/components/signin/public/identity_manager/identity_manager.h b/chromium/components/signin/public/identity_manager/identity_manager.h index 912c2b7c453..8a14328feda 100644 --- a/chromium/components/signin/public/identity_manager/identity_manager.h +++ b/chromium/components/signin/public/identity_manager/identity_manager.h @@ -9,6 +9,7 @@ #include <string> #include "base/gtest_prod_util.h" +#include "base/memory/raw_ptr.h" #include "base/observer_list.h" #include "base/scoped_observation.h" #include "build/build_config.h" @@ -369,10 +370,11 @@ class IdentityManager : public KeyedService, AccountConsistencyMethod account_consistency = AccountConsistencyMethod::kDisabled; #if BUILDFLAG(IS_CHROMEOS_LACROS) - SigninClient* signin_client = nullptr; + raw_ptr<SigninClient> signin_client = nullptr; #endif #if BUILDFLAG(IS_CHROMEOS) - account_manager::AccountManagerFacade* account_manager_facade = nullptr; + raw_ptr<account_manager::AccountManagerFacade> account_manager_facade = + nullptr; #endif InitParameters(); @@ -686,10 +688,10 @@ class IdentityManager : public KeyedService, std::unique_ptr<PrimaryAccountManager> primary_account_manager_; std::unique_ptr<AccountFetcherService> account_fetcher_service_; #if BUILDFLAG(IS_CHROMEOS_LACROS) - SigninClient* const signin_client_; + const raw_ptr<SigninClient> signin_client_; #endif #if BUILDFLAG(IS_CHROMEOS) - account_manager::AccountManagerFacade* const account_manager_facade_; + const raw_ptr<account_manager::AccountManagerFacade> account_manager_facade_; #endif IdentityMutator identity_mutator_; diff --git a/chromium/components/signin/public/identity_manager/identity_manager_builder.h b/chromium/components/signin/public/identity_manager/identity_manager_builder.h index 0cdab6f70ff..8c9f894acb3 100644 --- a/chromium/components/signin/public/identity_manager/identity_manager_builder.h +++ b/chromium/components/signin/public/identity_manager/identity_manager_builder.h @@ -71,7 +71,8 @@ struct IdentityManagerBuildParams { #endif #if BUILDFLAG(IS_CHROMEOS) - account_manager::AccountManagerFacade* account_manager_facade = nullptr; + raw_ptr<account_manager::AccountManagerFacade> account_manager_facade = + nullptr; bool is_regular_profile = false; #endif diff --git a/chromium/components/signin/public/identity_manager/identity_manager_unittest.cc b/chromium/components/signin/public/identity_manager/identity_manager_unittest.cc index a7599181e47..db6e0581952 100644 --- a/chromium/components/signin/public/identity_manager/identity_manager_unittest.cc +++ b/chromium/components/signin/public/identity_manager/identity_manager_unittest.cc @@ -65,7 +65,7 @@ #endif #if BUILDFLAG(IS_CHROMEOS_ASH) -#include "ash/components/account_manager/account_manager_factory.h" +#include "chromeos/ash/components/account_manager/account_manager_factory.h" #include "components/account_manager_core/account.h" #include "components/account_manager_core/account_manager_facade_impl.h" #include "components/account_manager_core/chromeos/account_manager.h" @@ -2370,14 +2370,12 @@ TEST_F(IdentityManagerTest, SetPrimaryAccount) { RecreateIdentityManager(AccountConsistencyMethod::kDisabled, PrimaryAccountManagerSetup::kNoAuthenticatedAccount); - // We should have a Primary Account set up automatically. - ConsentLevel consent_level = - base::FeatureList::IsEnabled(switches::kLacrosNonSyncingProfiles) - ? ConsentLevel::kSignin - : ConsentLevel::kSync; - ASSERT_TRUE(identity_manager()->HasPrimaryAccount(consent_level)); - EXPECT_EQ(kTestGaiaId, - identity_manager()->GetPrimaryAccountInfo(consent_level).gaia); + // We should have a non-syncing Primary Account set up automatically. + ASSERT_TRUE(identity_manager()->HasPrimaryAccount(ConsentLevel::kSignin)); + ASSERT_FALSE(identity_manager()->HasPrimaryAccount(ConsentLevel::kSync)); + EXPECT_EQ( + kTestGaiaId, + identity_manager()->GetPrimaryAccountInfo(ConsentLevel::kSignin).gaia); } // TODO(https://crbug.com/1223364): Remove this when all the users are migrated. @@ -2395,14 +2393,11 @@ TEST_F(IdentityManagerTest, SetPrimaryAccountClearsExistingPrimaryAccount) { // provided by the SigninClient. RecreateIdentityManager(AccountConsistencyMethod::kDisabled, PrimaryAccountManagerSetup::kWithAuthenticatedAccout); - - ConsentLevel consent_level = - base::FeatureList::IsEnabled(switches::kLacrosNonSyncingProfiles) - ? ConsentLevel::kSignin - : ConsentLevel::kSync; - ASSERT_TRUE(identity_manager()->HasPrimaryAccount(consent_level)); - EXPECT_EQ(kTestGaiaId2, - identity_manager()->GetPrimaryAccountInfo(consent_level).gaia); + ASSERT_TRUE(identity_manager()->HasPrimaryAccount(ConsentLevel::kSignin)); + ASSERT_FALSE(identity_manager()->HasPrimaryAccount(ConsentLevel::kSync)); + EXPECT_EQ( + kTestGaiaId2, + identity_manager()->GetPrimaryAccountInfo(ConsentLevel::kSignin).gaia); } #endif diff --git a/chromium/components/signin/public/identity_manager/identity_test_environment.cc b/chromium/components/signin/public/identity_manager/identity_test_environment.cc index b0947a422a2..7c4d1725aa2 100644 --- a/chromium/components/signin/public/identity_manager/identity_test_environment.cc +++ b/chromium/components/signin/public/identity_manager/identity_test_environment.cc @@ -40,7 +40,7 @@ #include "services/network/test/test_url_loader_factory.h" #if BUILDFLAG(IS_CHROMEOS_ASH) -#include "ash/components/account_manager/account_manager_factory.h" +#include "chromeos/ash/components/account_manager/account_manager_factory.h" #include "components/account_manager_core/account_manager_facade_impl.h" #include "components/account_manager_core/chromeos/account_manager.h" #include "components/account_manager_core/chromeos/account_manager_mojo_service.h" diff --git a/chromium/components/signin/public/identity_manager/identity_test_utils.cc b/chromium/components/signin/public/identity_manager/identity_test_utils.cc index fdaddfbcbb4..e61e80fdd08 100644 --- a/chromium/components/signin/public/identity_manager/identity_test_utils.cc +++ b/chromium/components/signin/public/identity_manager/identity_test_utils.cc @@ -243,12 +243,6 @@ void ClearPrimaryAccount(IdentityManager* identity_manager) { // synchronously with IdentityManager. NOTREACHED(); #else - -#if BUILDFLAG(IS_CHROMEOS_LACROS) - DCHECK_NE(identity_manager->GetAccountConsistency(), - AccountConsistencyMethod::kMirror); -#endif // BUILDFLAG(IS_CHROMEOS_LACROS) - if (!identity_manager->HasPrimaryAccount(ConsentLevel::kSignin)) return; |