diff options
author | Allan Sandfeld Jensen <allan.jensen@qt.io> | 2022-05-17 17:24:03 +0200 |
---|---|---|
committer | Allan Sandfeld Jensen <allan.jensen@qt.io> | 2022-06-22 07:51:41 +0000 |
commit | 774f54339e5db91f785733232d3950366db65d07 (patch) | |
tree | 068e1b47bd1af94d77094ed12b604a6b83d9c22a /chromium/components/signin | |
parent | f7eaed5286974984ba5f9e3189d8f49d03e99f81 (diff) | |
download | qtwebengine-chromium-774f54339e5db91f785733232d3950366db65d07.tar.gz |
BASELINE: Update Chromium to 102.0.5005.57
Change-Id: I885f714bb40ee724c28f94ca6bd8dbdb39915158
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
Diffstat (limited to 'chromium/components/signin')
105 files changed, 3157 insertions, 1006 deletions
diff --git a/chromium/components/signin/core/browser/BUILD.gn b/chromium/components/signin/core/browser/BUILD.gn index 64c2d2bf6f1..efe9ed3b30f 100644 --- a/chromium/components/signin/core/browser/BUILD.gn +++ b/chromium/components/signin/core/browser/BUILD.gn @@ -84,8 +84,8 @@ static_library("browser") { "active_directory_account_reconcilor_delegate.h", ] deps += [ + "//ash/components/tpm", "//chromeos/crosapi/mojom", - "//chromeos/tpm:tpm", ] sources -= [ "signin_status_metrics_provider.cc", @@ -153,8 +153,8 @@ source_set("unit_tests") { if (is_chromeos_ash) { deps += [ + "//ash/components/tpm:test_support", "//chromeos/crosapi/mojom", - "//chromeos/tpm:test_support", ] sources -= [ "account_investigator_unittest.cc", diff --git a/chromium/components/signin/core/browser/DEPS b/chromium/components/signin/core/browser/DEPS index 18afe921c4e..b166d8a8fea 100644 --- a/chromium/components/signin/core/browser/DEPS +++ b/chromium/components/signin/core/browser/DEPS @@ -6,11 +6,10 @@ include_rules = [ specific_include_rules = { "active_directory_account_reconcilor_delegate.cc": [ - "+chromeos/tpm/install_attributes.h" + "+ash/components/tpm/install_attributes.h" ], "account_reconcilor_unittest.cc": [ - "+chromeos/tpm/install_attributes.h", - "+chromeos/tpm/stub_install_attributes.h" + "+ash/components/tpm/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 4d42cd71a11..7b14cdcc988 100644 --- a/chromium/components/signin/core/browser/about_signin_internals.cc +++ b/chromium/components/signin/core/browser/about_signin_internals.cc @@ -12,6 +12,7 @@ #include "base/command_line.h" #include "base/hash/hash.h" #include "base/logging.h" +#include "base/observer_list.h" #include "base/strings/stringprintf.h" #include "base/time/time_to_iso8601.h" #include "base/trace_event/trace_event.h" diff --git a/chromium/components/signin/core/browser/about_signin_internals.h b/chromium/components/signin/core/browser/about_signin_internals.h index 7bfaed3aad1..a8460812666 100644 --- a/chromium/components/signin/core/browser/about_signin_internals.h +++ b/chromium/components/signin/core/browser/about_signin_internals.h @@ -14,6 +14,7 @@ #include "base/memory/raw_ptr.h" #include "base/observer_list.h" +#include "base/time/time.h" #include "base/values.h" #include "components/content_settings/core/browser/content_settings_observer.h" #include "components/keyed_service/core/keyed_service.h" diff --git a/chromium/components/signin/core/browser/account_investigator.h b/chromium/components/signin/core/browser/account_investigator.h index a204b96ff12..32b10470d91 100644 --- a/chromium/components/signin/core/browser/account_investigator.h +++ b/chromium/components/signin/core/browser/account_investigator.h @@ -9,6 +9,7 @@ #include <vector> #include "base/memory/raw_ptr.h" +#include "base/time/time.h" #include "base/timer/timer.h" #include "components/keyed_service/core/keyed_service.h" #include "components/signin/public/identity_manager/identity_manager.h" diff --git a/chromium/components/signin/core/browser/account_investigator_unittest.cc b/chromium/components/signin/core/browser/account_investigator_unittest.cc index a43cd77de5e..931618218ee 100644 --- a/chromium/components/signin/core/browser/account_investigator_unittest.cc +++ b/chromium/components/signin/core/browser/account_investigator_unittest.cc @@ -9,6 +9,7 @@ #include "base/run_loop.h" #include "base/test/metrics/histogram_tester.h" #include "base/test/task_environment.h" +#include "base/time/time.h" #include "build/build_config.h" #include "components/prefs/pref_registry_simple.h" #include "components/signin/public/base/signin_metrics.h" diff --git a/chromium/components/signin/core/browser/account_reconcilor.cc b/chromium/components/signin/core/browser/account_reconcilor.cc index cf5b71c5079..c93bf597888 100644 --- a/chromium/components/signin/core/browser/account_reconcilor.cc +++ b/chromium/components/signin/core/browser/account_reconcilor.cc @@ -14,11 +14,13 @@ #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" #include "base/metrics/histogram_functions.h" #include "base/metrics/histogram_macros.h" +#include "base/observer_list.h" #include "base/strings/string_util.h" #include "base/task/single_thread_task_runner.h" #include "base/threading/thread_task_runner_handle.h" @@ -27,6 +29,7 @@ #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" @@ -35,6 +38,10 @@ #include "google_apis/gaia/gaia_urls.h" #include "google_apis/gaia/google_service_auth_error.h" +#if BUILDFLAG(IS_CHROMEOS_LACROS) +#include "components/signin/core/browser/consistency_cookie_manager.h" +#endif + using signin::AccountReconcilorDelegate; using signin::ConsentLevel; using signin_metrics::AccountReconcilorState; @@ -183,6 +190,13 @@ AccountReconcilor::AccountReconcilor( DCHECK(delegate_); delegate_->set_reconcilor(this); timeout_ = delegate_->GetReconcileTimeout(); + +#if BUILDFLAG(IS_CHROMEOS_LACROS) + if (base::FeatureList::IsEnabled(switches::kLacrosNonSyncingProfiles)) { + consistency_cookie_manager_ = + std::make_unique<signin::ConsistencyCookieManager>(client_, this); + } +#endif } AccountReconcilor::~AccountReconcilor() { @@ -223,11 +237,12 @@ void AccountReconcilor::EnableReconcile() { } void AccountReconcilor::DisableReconcile(bool logout_all_accounts) { + const bool log_out_in_progress = log_out_in_progress_; AbortReconcile(); SetState(AccountReconcilorState::ACCOUNT_RECONCILOR_INACTIVE); UnregisterWithAllDependencies(); - if (logout_all_accounts) + if (logout_all_accounts && !log_out_in_progress) PerformLogoutAllAccountsAction(); } @@ -589,7 +604,8 @@ void AccountReconcilor::OnAccountsInCookieUpdated( std::vector<CoreAccountId> chrome_accounts = LoadValidAccountsFromTokenService(); - if (delegate_->ShouldAbortReconcileIfPrimaryHasError() && + if (!primary_account.empty() && + delegate_->ShouldAbortReconcileIfPrimaryHasError() && !base::Contains(chrome_accounts, primary_account)) { VLOG(1) << "Primary account has error, abort."; DCHECK(is_reconcile_started_); @@ -796,6 +812,13 @@ bool AccountReconcilor::IsReconcileBlocked() const { return account_reconcilor_lock_count_ > 0; } +#if BUILDFLAG(IS_CHROMEOS_LACROS) +signin::ConsistencyCookieManager* +AccountReconcilor::GetConsistencyCookieManager() { + return consistency_cookie_manager_.get(); +} +#endif + void AccountReconcilor::BlockReconcile() { DCHECK(IsReconcileBlocked()); VLOG(1) << "AccountReconcilor::BlockReconcile."; diff --git a/chromium/components/signin/core/browser/account_reconcilor.h b/chromium/components/signin/core/browser/account_reconcilor.h index ac10b0d4c74..754c29d9b97 100644 --- a/chromium/components/signin/core/browser/account_reconcilor.h +++ b/chromium/components/signin/core/browser/account_reconcilor.h @@ -34,9 +34,10 @@ class AccountReconcilorDelegate; enum class SetAccountsInCookieResult; #if BUILDFLAG(IS_CHROMEOS_LACROS) +class ConsistencyCookieManager; class ConsistencyCookieManagerTest; #endif -} +} // namespace signin class SigninClient; @@ -141,6 +142,12 @@ class AccountReconcilor : public KeyedService, // Returns true if reconcilor is blocked. bool IsReconcileBlocked() const; +#if BUILDFLAG(IS_CHROMEOS_LACROS) + // Gets the ConsistencyCookieManager, which updates the + // "CHROME_ID_CONSISTENCY_STATE" cookie. + signin::ConsistencyCookieManager* GetConsistencyCookieManager(); +#endif + protected: void OnSetAccountsInCookieCompleted(signin::SetAccountsInCookieResult result); void OnLogOutFromCookieCompleted(const GoogleServiceAuthError& error); @@ -442,6 +449,10 @@ class AccountReconcilor : public KeyedService, // Set to true when Shutdown() is called. bool was_shut_down_ = false; +#if BUILDFLAG(IS_CHROMEOS_LACROS) + std::unique_ptr<signin::ConsistencyCookieManager> consistency_cookie_manager_; +#endif + base::WeakPtrFactory<AccountReconcilor> weak_factory_{this}; }; diff --git a/chromium/components/signin/core/browser/account_reconcilor_unittest.cc b/chromium/components/signin/core/browser/account_reconcilor_unittest.cc index 6faa4028aa4..81299e3dd4e 100644 --- a/chromium/components/signin/core/browser/account_reconcilor_unittest.cc +++ b/chromium/components/signin/core/browser/account_reconcilor_unittest.cc @@ -48,7 +48,7 @@ #endif #if BUILDFLAG(IS_CHROMEOS_ASH) -#include "chromeos/tpm/stub_install_attributes.h" +#include "ash/components/tpm/stub_install_attributes.h" #include "components/signin/core/browser/active_directory_account_reconcilor_delegate.h" #endif @@ -1428,10 +1428,9 @@ class AccountReconcilorTestActiveDirectory : public AccountReconcilorTestTable { } private: - chromeos::ScopedStubInstallAttributes install_attributes_{ - chromeos::StubInstallAttributes::CreateActiveDirectoryManaged( - "realm.com", - "device_id")}; + ash::ScopedStubInstallAttributes install_attributes_{ + ash::StubInstallAttributes::CreateActiveDirectoryManaged("realm.com", + "device_id")}; }; // clang-format off 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 637d53e67a4..1e720841a60 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,15 +4,15 @@ #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/tpm/install_attributes.h" #include "google_apis/gaia/core_account_id.h" namespace signin { ActiveDirectoryAccountReconcilorDelegate:: ActiveDirectoryAccountReconcilorDelegate() { - DCHECK(chromeos::InstallAttributes::Get()->IsActiveDirectoryManaged()); + DCHECK(ash::InstallAttributes::Get()->IsActiveDirectoryManaged()); } ActiveDirectoryAccountReconcilorDelegate:: diff --git a/chromium/components/signin/core/browser/consistency_cookie_manager.cc b/chromium/components/signin/core/browser/consistency_cookie_manager.cc index 29a36d582e8..e655db405bb 100644 --- a/chromium/components/signin/core/browser/consistency_cookie_manager.cc +++ b/chromium/components/signin/core/browser/consistency_cookie_manager.cc @@ -5,8 +5,12 @@ #include "components/signin/core/browser/consistency_cookie_manager.h" #include "base/check.h" +#include "base/containers/contains.h" +#include "base/containers/cxx20_erase.h" +#include "base/strings/string_util.h" #include "base/time/time.h" #include "components/signin/public/base/signin_client.h" +#include "components/signin/public/base/signin_metrics.h" #include "google_apis/gaia/gaia_urls.h" #include "net/cookies/canonical_cookie.h" #include "net/cookies/cookie_options.h" @@ -24,20 +28,118 @@ const char ConsistencyCookieManager::kCookieValueStringInconsistent[] = "Inconsistent"; const char ConsistencyCookieManager::kCookieValueStringUpdating[] = "Updating"; +ConsistencyCookieManager::ScopedAccountUpdate::~ScopedAccountUpdate() { + if (!consistency_cookie_manager_) + return; + + DCHECK_GT(consistency_cookie_manager_->scoped_update_count_, 0); + --consistency_cookie_manager_->scoped_update_count_; + consistency_cookie_manager_->UpdateCookieIfNeeded(/*force_creation=*/false); +} + +ConsistencyCookieManager::ScopedAccountUpdate::ScopedAccountUpdate( + ScopedAccountUpdate&& other) { + consistency_cookie_manager_ = std::move(other.consistency_cookie_manager_); + // Explicitly reset the weak pointer, in case the move operation did not. + other.consistency_cookie_manager_.reset(); +} + +ConsistencyCookieManager::ScopedAccountUpdate& +ConsistencyCookieManager::ScopedAccountUpdate::operator=( + ScopedAccountUpdate&& other) { + if (this != &other) { + consistency_cookie_manager_ = std::move(other.consistency_cookie_manager_); + // Explicitly reset the weak pointer, in case the move operation did not. + other.consistency_cookie_manager_.reset(); + } + return *this; +} + +ConsistencyCookieManager::ScopedAccountUpdate::ScopedAccountUpdate( + base::WeakPtr<ConsistencyCookieManager> manager) + : consistency_cookie_manager_(manager) { + DCHECK(consistency_cookie_manager_); + ++consistency_cookie_manager_->scoped_update_count_; + DCHECK_GT(consistency_cookie_manager_->scoped_update_count_, 0); + bool force_creation = consistency_cookie_manager_->scoped_update_count_ == 1; + consistency_cookie_manager_->UpdateCookieIfNeeded(force_creation); +} + ConsistencyCookieManager::ConsistencyCookieManager( SigninClient* signin_client, AccountReconcilor* reconcilor) - : signin_client_(signin_client), account_reconcilor_(reconcilor) { + : signin_client_(signin_client), + account_reconcilor_(reconcilor), + account_reconcilor_state_(account_reconcilor_->GetState()) { DCHECK(signin_client_); DCHECK(account_reconcilor_); account_reconcilor_observation_.Observe(account_reconcilor_); - UpdateCookieIfNeeded(); + UpdateCookieIfNeeded(/*force_creation=*/false); +} + +ConsistencyCookieManager::~ConsistencyCookieManager() { + DCHECK(extra_cookie_managers_.empty()); } -ConsistencyCookieManager::~ConsistencyCookieManager() = default; +ConsistencyCookieManager::ScopedAccountUpdate +ConsistencyCookieManager::CreateScopedAccountUpdate() { + return ScopedAccountUpdate(weak_factory_.GetWeakPtr()); +} + +void ConsistencyCookieManager::AddExtraCookieManager( + network::mojom::CookieManager* manager) { + DCHECK(manager); + DCHECK(!base::Contains(extra_cookie_managers_, manager)); + extra_cookie_managers_.push_back(manager); + if (cookie_value_ && cookie_value_ != CookieValue::kInvalid) + SetCookieValue(manager, cookie_value_.value()); +} + +void ConsistencyCookieManager::RemoveExtraCookieManager( + network::mojom::CookieManager* manager) { + DCHECK(manager); + DCHECK(base::Contains(extra_cookie_managers_, manager)); + base::Erase(extra_cookie_managers_, manager); +} + +// static +std::unique_ptr<net::CanonicalCookie> +ConsistencyCookieManager::CreateConsistencyCookie(const std::string& value) { + DCHECK(!value.empty()); + base::Time now = base::Time::Now(); + base::Time expiry = now + base::Days(2 * 365); // Two years. + return net::CanonicalCookie::CreateSanitizedCookie( + GaiaUrls::GetInstance()->gaia_url(), kCookieName, value, + GaiaUrls::GetInstance()->gaia_url().host(), + /*path=*/"/", /*creation=*/now, /*expiration=*/expiry, + /*last_access=*/now, /*secure=*/true, /*httponly=*/false, + net::CookieSameSite::STRICT_MODE, net::COOKIE_PRIORITY_DEFAULT, + /*same_party=*/false, /*partition_key=*/absl::nullopt); +} + +// static +bool ConsistencyCookieManager::IsConsistencyCookie( + const net::CanonicalCookie& cookie) { + return cookie.IsSecure() && cookie.Path() == "/" && + cookie.DomainWithoutDot() == + GaiaUrls::GetInstance()->gaia_url().host() && + cookie.Name() == kCookieName; +} // static -void ConsistencyCookieManager::UpdateCookie( +ConsistencyCookieManager::CookieValue +ConsistencyCookieManager::ParseCookieValue(const std::string& value) { + if (base::EqualsCaseInsensitiveASCII(value, kCookieValueStringConsistent)) + return CookieValue::kConsistent; + if (base::EqualsCaseInsensitiveASCII(value, kCookieValueStringInconsistent)) + return CookieValue::kInconsistent; + if (base::EqualsCaseInsensitiveASCII(value, kCookieValueStringUpdating)) + return CookieValue::kUpdating; + return CookieValue::kInvalid; +} + +// static +void ConsistencyCookieManager::SetCookieValue( network::mojom::CookieManager* cookie_manager, CookieValue value) { std::string cookie_value_string; @@ -51,20 +153,15 @@ void ConsistencyCookieManager::UpdateCookie( case CookieValue::kUpdating: cookie_value_string = kCookieValueStringUpdating; break; + case CookieValue::kInvalid: + NOTREACHED(); + break; } DCHECK(!cookie_value_string.empty()); // Update the cookie with the new value. - base::Time now = base::Time::Now(); - base::Time expiry = now + base::Days(2 * 365); // Two years. std::unique_ptr<net::CanonicalCookie> cookie = - net::CanonicalCookie::CreateSanitizedCookie( - GaiaUrls::GetInstance()->gaia_url(), kCookieName, cookie_value_string, - GaiaUrls::GetInstance()->gaia_url().host(), - /*path=*/"/", /*creation=*/now, /*expiration=*/expiry, - /*last_access=*/now, /*secure=*/true, /*httponly=*/false, - net::CookieSameSite::LAX_MODE, net::COOKIE_PRIORITY_DEFAULT, - /*same_party=*/false, /*partition_key=*/absl::nullopt); + CreateConsistencyCookie(cookie_value_string); net::CookieOptions cookie_options; // Permit to set SameSite cookies. cookie_options.set_same_site_cookie_context( @@ -77,12 +174,30 @@ void ConsistencyCookieManager::UpdateCookie( void ConsistencyCookieManager::OnStateChanged( signin_metrics::AccountReconcilorState state) { - UpdateCookieIfNeeded(); + DCHECK_NE(state, account_reconcilor_state_); + // If a `ScopedAccountUpdate` was created while the reconcilor was inactive, + // it was ignored and creation was not forced. Force the creation when the + // reconcilor becomes active. + bool force_creation = + scoped_update_count_ > 0 && + account_reconcilor_state_ == signin_metrics::ACCOUNT_RECONCILOR_INACTIVE; + account_reconcilor_state_ = state; + UpdateCookieIfNeeded(force_creation); } absl::optional<ConsistencyCookieManager::CookieValue> ConsistencyCookieManager::CalculateCookieValue() const { - switch (account_reconcilor_->GetState()) { + // Only update the cookie when the reconcilor is active. + if (account_reconcilor_state_ == signin_metrics::ACCOUNT_RECONCILOR_INACTIVE) + return absl::nullopt; + + // If there is a live `ScopedAccountUpdate`, return `kStateUpdating`. + DCHECK_GE(scoped_update_count_, 0); + if (scoped_update_count_ > 0) + return CookieValue::kUpdating; + + // Otherwise compute the cookie based on the reconcilor state. + switch (account_reconcilor_state_) { case signin_metrics::ACCOUNT_RECONCILOR_OK: return CookieValue::kConsistent; case signin_metrics::ACCOUNT_RECONCILOR_RUNNING: @@ -91,16 +206,85 @@ ConsistencyCookieManager::CalculateCookieValue() const { case signin_metrics::ACCOUNT_RECONCILOR_ERROR: return CookieValue::kInconsistent; case signin_metrics::ACCOUNT_RECONCILOR_INACTIVE: + // This case is already handled at the top of the function. + NOTREACHED(); return absl::nullopt; } } -void ConsistencyCookieManager::UpdateCookieIfNeeded() { +void ConsistencyCookieManager::UpdateCookieIfNeeded(bool force_creation) { absl::optional<CookieValue> cookie_value = CalculateCookieValue(); - if (!cookie_value.has_value() || cookie_value == cookie_value_) + if (!cookie_value.has_value()) + return; + + DCHECK_NE(cookie_value.value(), CookieValue::kInvalid); + if (force_creation) { + cookie_value_ = cookie_value; + // Cancel any ongoing operation and set the cookie immediately. + pending_cookie_update_ = absl::nullopt; + SetCookieValue(signin_client_->GetCookieManager(), cookie_value_.value()); + for (auto* extra_manager : extra_cookie_managers_) + SetCookieValue(extra_manager, cookie_value_.value()); + return; + } + + // When creation is not forced, skip the update if the cookie already has the + // desired value or if it is missing, based on the last-known value. This is + // an optimisation to avoid querying the cookie repeatedly. + if (!cookie_value_ || cookie_value_ == cookie_value) { + pending_cookie_update_ = absl::nullopt; return; - cookie_value_ = cookie_value; - UpdateCookie(signin_client_->GetCookieManager(), cookie_value_.value()); + } + + // Query the cookie, and set it only if it exists and has a different value. + // Override the pending value if there is already a query in progress, + // otherwise start a new query. + bool has_pending_update = pending_cookie_update_.has_value(); + pending_cookie_update_ = cookie_value; + + if (has_pending_update) + return; + + net::CookieOptions options = net::CookieOptions::MakeAllInclusive(); + options.set_exclude_httponly(); + signin_client_->GetCookieManager()->GetCookieList( + GaiaUrls::GetInstance()->gaia_url(), options, + net::CookiePartitionKeyCollection(), + base::BindOnce(&ConsistencyCookieManager::UpdateCookieIfExists, + weak_factory_.GetWeakPtr())); +} + +void ConsistencyCookieManager::UpdateCookieIfExists( + const net::CookieAccessResultList& cookie_list, + const net::CookieAccessResultList& /*excluded_cookies*/) { + // If the current operation was canceled, return immediately. `cookie_value_` + // must not be updated now, as there is a possible race condition between + // `GetCookieList()` and `SetCanonicalCookie()`. + if (!pending_cookie_update_) + return; + + // Compute the current value of the cookie. + auto it = std::find_if(cookie_list.cbegin(), cookie_list.cend(), + [](const net::CookieWithAccessResult& result) { + return IsConsistencyCookie(result.cookie); + }); + absl::optional<CookieValue> current_value = + (it == cookie_list.cend()) + ? absl::nullopt + : absl::make_optional(ParseCookieValue(it->cookie.Value())); + + CookieValue target_value = pending_cookie_update_.value(); + DCHECK_NE(target_value, CookieValue::kInvalid); + pending_cookie_update_ = absl::nullopt; + if (!current_value || current_value.value() == target_value) { + // The cookie does not exist or already matches. + cookie_value_ = current_value; + return; + } + cookie_value_ = target_value; + SetCookieValue(signin_client_->GetCookieManager(), target_value); + for (auto* extra_manager : extra_cookie_managers_) + SetCookieValue(extra_manager, target_value); } } // namespace signin diff --git a/chromium/components/signin/core/browser/consistency_cookie_manager.h b/chromium/components/signin/core/browser/consistency_cookie_manager.h index 45255ee8791..068adb797fd 100644 --- a/chromium/components/signin/core/browser/consistency_cookie_manager.h +++ b/chromium/components/signin/core/browser/consistency_cookie_manager.h @@ -6,8 +6,10 @@ #define COMPONENTS_SIGNIN_CORE_BROWSER_CONSISTENCY_COOKIE_MANAGER_H_ #include "base/gtest_prod_util.h" +#include "base/memory/weak_ptr.h" #include "base/scoped_observation.h" #include "components/signin/core/browser/account_reconcilor.h" +#include "net/cookies/canonical_cookie.h" #include "third_party/abseil-cpp/absl/types/optional.h" namespace network::mojom { @@ -25,12 +27,32 @@ namespace signin { // adding an account to the OS account manager. // The value of the cookie depends on the state of the `AccountReconcilor` and // whether there is a native account addition flow in progress. -// -// TODO(https://crbug.com/1260291): The `ConsistencyCookieManager` only -// listens to the `AccountReconcilor` for now, it is not updated by UI flows -// yet. +// The cookie is updated only if it already exists. The cookie creation is only +// forced when a `ScopedAccountUpdate` is created, which indicates an explicit +// navigation to Gaia. class ConsistencyCookieManager : public AccountReconcilor::Observer { public: + // Sets the cookie state to "Updating" while it's alive. + // Instances are vended by `CreateScopedAccountUpdate()` and are allowed to + // outlive the `ConsistencyCookieManager`. + class ScopedAccountUpdate final { + public: + ~ScopedAccountUpdate(); + + // Move operations. + ScopedAccountUpdate(ScopedAccountUpdate&& other); + ScopedAccountUpdate& operator=(ScopedAccountUpdate&& other); + + // `ScopedAccountUpdate` is not copyable. + ScopedAccountUpdate(const ScopedAccountUpdate&) = delete; + ScopedAccountUpdate& operator=(const ScopedAccountUpdate&) = delete; + + private: + friend ConsistencyCookieManager; + ScopedAccountUpdate(base::WeakPtr<ConsistencyCookieManager> manager); + base::WeakPtr<ConsistencyCookieManager> consistency_cookie_manager_; + }; + explicit ConsistencyCookieManager(SigninClient* signin_client, AccountReconcilor* reconcilor); ~ConsistencyCookieManager() override; @@ -38,11 +60,45 @@ class ConsistencyCookieManager : public AccountReconcilor::Observer { ConsistencyCookieManager& operator=(const ConsistencyCookieManager&) = delete; ConsistencyCookieManager(const ConsistencyCookieManager&) = delete; + // Web-signin UI flows should guarantee that at least a scoped update is alive + // for the whole flow. This starts from the user interaction and finishes when + // the account has been added to the `IdentityManager`. + ScopedAccountUpdate CreateScopedAccountUpdate(); + + // Adds or removes an extra cookie manager where the cookie updates are + // duplicated. It is expected that an extra cookie manager is only set + // temporarily (e.g. for the duration of a single signin flow), with the + // intent of importing the accounts from the main cookie manager. + void AddExtraCookieManager(network::mojom::CookieManager* manager); + void RemoveExtraCookieManager(network::mojom::CookieManager* manager); + + // Creates the `CanonicalCookie` corresponding to the consistency cookie. + static std::unique_ptr<net::CanonicalCookie> CreateConsistencyCookie( + const std::string& value); + private: friend class ConsistencyCookieManagerTest; + FRIEND_TEST_ALL_PREFIXES(ConsistencyCookieManagerTest, MoveOperations); FRIEND_TEST_ALL_PREFIXES(ConsistencyCookieManagerTest, ReconcilorState); - - enum class CookieValue { kConsistent, kInconsistent, kUpdating }; + FRIEND_TEST_ALL_PREFIXES(ConsistencyCookieManagerTest, ScopedAccountUpdate); + FRIEND_TEST_ALL_PREFIXES(ConsistencyCookieManagerTest, + ScopedAccountUpdate_Inactive); + FRIEND_TEST_ALL_PREFIXES(ConsistencyCookieManagerTest, + UpdateAfterDestruction); + FRIEND_TEST_ALL_PREFIXES(ConsistencyCookieManagerTest, FirstCookieUpdate); + FRIEND_TEST_ALL_PREFIXES(ConsistencyCookieManagerTest, CookieDeleted); + FRIEND_TEST_ALL_PREFIXES(ConsistencyCookieManagerTest, CookieInvalid); + FRIEND_TEST_ALL_PREFIXES(ConsistencyCookieManagerTest, CookieAlreadySet); + FRIEND_TEST_ALL_PREFIXES(ConsistencyCookieManagerTest, CoalesceCookieQueries); + FRIEND_TEST_ALL_PREFIXES(ConsistencyCookieManagerTest, CancelPendingQuery); + FRIEND_TEST_ALL_PREFIXES(ConsistencyCookieManagerTest, ExtraCookieManager); + + enum class CookieValue { + kConsistent, // Value is "Consistent". + kInconsistent, // Value is "Inconsistent". + kUpdating, // Value is "Updating". + kInvalid // Any other value. + }; // Cookie name and values. static const char kCookieName[]; @@ -50,26 +106,60 @@ class ConsistencyCookieManager : public AccountReconcilor::Observer { static const char kCookieValueStringInconsistent[]; static const char kCookieValueStringUpdating[]; + // Returns whether `cookie` is the consistency cookie. + static bool IsConsistencyCookie(const net::CanonicalCookie& cookie); + + // Parses the cookie value from its string representation. + static CookieValue ParseCookieValue(const std::string& value); + // Sets the cookie to match `value`. - static void UpdateCookie(network::mojom::CookieManager* cookie_manager, - CookieValue value); + static void SetCookieValue(network::mojom::CookieManager* cookie_manager, + CookieValue value); // AccountReconcilor::Observer: void OnStateChanged(signin_metrics::AccountReconcilorState state) override; - // Calculates the cookie value based on the reconcilor state. + // Calculates the cookie value based on the reconcilor state and the count of + // live `ScopedAccountUpdate` instances. Returns `absl::nullopt` if the value + // cannot be computed (e.g. if the reconcilor is not started). absl::optional<CookieValue> CalculateCookieValue() const; // Gets the new value using `CalculateCookieValue()` and sets the cookie if it - // changed. - void UpdateCookieIfNeeded(); + // changed. If `force_creation` is false, triggers a cookie query, as it + // should only be updated if it already exists. + void UpdateCookieIfNeeded(bool force_creation); + + // Callback for `CookieManager::GetCookieList()`. Updates the cached value of + // the cookie, and updates the cookie only if the cookie already exists. + void UpdateCookieIfExists( + const net::CookieAccessResultList& cookie_list, + const net::CookieAccessResultList& /*excluded_cookies*/); SigninClient* const signin_client_; AccountReconcilor* const account_reconcilor_; - absl::optional<CookieValue> cookie_value_ = absl::nullopt; + signin_metrics::AccountReconcilorState account_reconcilor_state_ = + signin_metrics::ACCOUNT_RECONCILOR_INACTIVE; + int scoped_update_count_ = 0; + + // Cached value of the cookie, equal to the last value that was either set or + // queried. `absl::nullopt` when the cookie is missing. Initialized as + // `CookieValue::kInvalid` so that the first cookie update is always tried, + // but should never be set to `CookieValue::kInvalid` after that. + absl::optional<CookieValue> cookie_value_ = CookieValue::kInvalid; + + // Pending cookie update, applied after querying the cookie value. The pending + // update is only applied if the cookie already exists. + absl::optional<CookieValue> pending_cookie_update_; + + // Extra cookie managers where the cookie is also written. These are never + // read, and if they go out of sync with the main cookie manager, they may + // not be updated correctly. + std::vector<network::mojom::CookieManager*> extra_cookie_managers_; base::ScopedObservation<AccountReconcilor, AccountReconcilor::Observer> account_reconcilor_observation_{this}; + + base::WeakPtrFactory<ConsistencyCookieManager> weak_factory_{this}; }; } // namespace signin 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 2bfef6e0f8d..ae03f5124c8 100644 --- a/chromium/components/signin/core/browser/consistency_cookie_manager_unittest.cc +++ b/chromium/components/signin/core/browser/consistency_cookie_manager_unittest.cc @@ -4,9 +4,13 @@ #include "components/signin/core/browser/consistency_cookie_manager.h" +#include <memory> + +#include "base/test/scoped_feature_list.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" @@ -27,6 +31,13 @@ class MockCookieManager const GURL& source_url, const net::CookieOptions& cookie_options, SetCanonicalCookieCallback callback)); + + MOCK_METHOD4(GetCookieList, + void(const GURL& url, + const net::CookieOptions& cookie_options, + const net::CookiePartitionKeyCollection& + cookie_partition_key_collection, + GetCookieListCallback callback)); }; } // namespace @@ -38,15 +49,36 @@ class ConsistencyCookieManagerTest : public testing::Test { std::make_unique<MockCookieManager>(); cookie_manager_ = mock_cookie_manager.get(); signin_client_.set_cookie_manager(std::move(mock_cookie_manager)); + reconcilor_ = std::make_unique<AccountReconcilor>( + /*identity_manager=*/nullptr, &signin_client_, + std::make_unique<AccountReconcilorDelegate>()); } - ~ConsistencyCookieManagerTest() override { reconcilor_.Shutdown(); } + ~ConsistencyCookieManagerTest() override { DeleteConsistencyCookieManager(); } + + ConsistencyCookieManager* GetConsistencyCookieManager() { + return reconcilor_->GetConsistencyCookieManager(); + } + + void DeleteConsistencyCookieManager() { + if (!reconcilor_) + return; + // `AccountReconcilor` shutdown should not trigger a cookie update. + reconcilor_->Shutdown(); + reconcilor_.reset(); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + } void ExpectCookieSet(const std::string& value) { + ExpectCookieSetInManager(cookie_manager_, value); + } + + void ExpectCookieSetInManager(MockCookieManager* manager, + const std::string& value) { const std::string expected_domain = std::string(".") + GaiaUrls::GetInstance()->gaia_url().host(); EXPECT_CALL( - *cookie_manager_, + *manager, SetCanonicalCookie( testing::AllOf( testing::Property(&net::CanonicalCookie::Name, @@ -56,33 +88,69 @@ class ConsistencyCookieManagerTest : public testing::Test { expected_domain), testing::Property(&net::CanonicalCookie::Path, "/"), testing::Property(&net::CanonicalCookie::IsSecure, true), - testing::Property(&net::CanonicalCookie::IsHttpOnly, false)), + testing::Property(&net::CanonicalCookie::IsHttpOnly, false), + testing::Property(&net::CanonicalCookie::SameSite, + net::CookieSameSite::STRICT_MODE)), GaiaUrls::GetInstance()->gaia_url(), testing::_, testing::_)); } + // Configures the default behavior of `GetCookieList()`. Passing an empty + // value simulates a missing cookie. + void SetCookieInManager(const std::string& value) { + net::CookieAccessResultList cookie_list = {}; + std::unique_ptr<net::CanonicalCookie> cookie; + if (!value.empty()) { + cookie = ConsistencyCookieManager::CreateConsistencyCookie(value); + cookie_list.push_back({*cookie, net::CookieAccessResult()}); + } + + ON_CALL(*cookie_manager_, GetCookieList(GaiaUrls::GetInstance()->gaia_url(), + testing::_, testing::_, testing::_)) + .WillByDefault(testing::WithArg<3>(testing::Invoke( + [cookie_list]( + network::mojom::CookieManager::GetCookieListCallback callback) { + std::move(callback).Run(cookie_list, {}); + }))); + } + + void ExpectGetCookie() { + EXPECT_CALL(*cookie_manager_, + GetCookieList(GaiaUrls::GetInstance()->gaia_url(), testing::_, + testing::_, testing::_)); + } + void SetReconcilorState(signin_metrics::AccountReconcilorState state) { account_reconcilor()->SetState(state); } - AccountReconcilor* account_reconcilor() { return &reconcilor_; } - SigninClient* signin_client() { return &signin_client_; } + AccountReconcilor* account_reconcilor() { return reconcilor_.get(); } 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_`. - AccountReconcilor reconcilor_{/*identity_manager=*/nullptr, &signin_client_, - std::make_unique<AccountReconcilorDelegate>()}; + std::unique_ptr<AccountReconcilor> reconcilor_; }; +// Tests that the cookie is updated when the state of the `AccountReconcilor` +// changes. TEST_F(ConsistencyCookieManagerTest, ReconcilorState) { - // Initial state. + // Ensure the cookie manager was created. + ConsistencyCookieManager* consistency_cookie_manager = + GetConsistencyCookieManager(); + ASSERT_TRUE(consistency_cookie_manager); EXPECT_EQ(account_reconcilor()->GetState(), signin_metrics::ACCOUNT_RECONCILOR_INACTIVE); - ConsistencyCookieManager cookie_consistency_manager(signin_client(), - account_reconcilor()); // Cookie has not been set. testing::Mock::VerifyAndClearExpectations(cookie_manager()); + // Set some initial value for the cookie. + SetCookieInManager(ConsistencyCookieManager::kCookieValueStringConsistent); struct AccountReconcilorStateTestCase { signin_metrics::AccountReconcilorState state; @@ -108,10 +176,15 @@ TEST_F(ConsistencyCookieManagerTest, ReconcilorState) { }; for (const AccountReconcilorStateTestCase& test_case : cases) { - if (test_case.cookie_value.has_value()) + if (test_case.cookie_value.has_value()) { + ExpectGetCookie(); ExpectCookieSet(test_case.cookie_value.value()); + } SetReconcilorState(test_case.state); testing::Mock::VerifyAndClearExpectations(cookie_manager()); + // Update the internal state of the mock cookie manager. + if (test_case.cookie_value.has_value()) + SetCookieInManager(test_case.cookie_value.value()); } // Check that the cookie is not updated needlessly. @@ -130,4 +203,356 @@ TEST_F(ConsistencyCookieManagerTest, ReconcilorState) { testing::Mock::VerifyAndClearExpectations(cookie_manager()); } +// Checks that the `ScopedAccountUpdate` updates the reconcilor state and can be +// nested. +TEST_F(ConsistencyCookieManagerTest, ScopedAccountUpdate) { + ConsistencyCookieManager* consistency_cookie_manager = + GetConsistencyCookieManager(); + // Start the reconcilor, with no cookie. + SetCookieInManager(std::string()); + ExpectGetCookie(); + SetReconcilorState(signin_metrics::ACCOUNT_RECONCILOR_OK); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 0); + + { + // Create a scoped update, this sets the cookie to "Updating". + ExpectCookieSet(ConsistencyCookieManager::kCookieValueStringUpdating); + ConsistencyCookieManager::ScopedAccountUpdate update_1 = + consistency_cookie_manager->CreateScopedAccountUpdate(); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 1); + + { + // Create a second update, this does nothing, but increments the internal + // counter. Counter is decremented when it goes out of scope. + ConsistencyCookieManager::ScopedAccountUpdate update_2 = + consistency_cookie_manager->CreateScopedAccountUpdate(); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 2); + } + + // `update_2` was destroyed. Cookie value did not change as `update_1` is + // still alive. The cookie is not queried, only the cached value is used. + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 1); + + // Destroy `update_1`. All updates are destroyed, cookie should go back to + // "Consistent". + SetCookieInManager(ConsistencyCookieManager::kCookieValueStringUpdating); + ExpectGetCookie(); + ExpectCookieSet(ConsistencyCookieManager::kCookieValueStringConsistent); + } + + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 0); +} + +// Tests the behavior of `ScopedAccountUpdate` when the reconcilor is inactive. +TEST_F(ConsistencyCookieManagerTest, ScopedAccountUpdate_Inactive) { + ConsistencyCookieManager* consistency_cookie_manager = + GetConsistencyCookieManager(); + EXPECT_EQ(account_reconcilor()->GetState(), + signin_metrics::ACCOUNT_RECONCILOR_INACTIVE); + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 0); + + { + // Create a scoped update, this does not change the cookie because the + // reconcilor is inactive. + ConsistencyCookieManager::ScopedAccountUpdate update = + consistency_cookie_manager->CreateScopedAccountUpdate(); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 1); + + // Destroy `update`. Cookie is not updated, because the reconcilor is + // inactive. + } + + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + + { + // Create a scoped update, this does not change the cookie because the + // reconcilor is inactive. + ConsistencyCookieManager::ScopedAccountUpdate update = + consistency_cookie_manager->CreateScopedAccountUpdate(); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 1); + + // Start the reconcilor. The state is "Updating" because there is a live + // update, even though it was created when the reconcilor was inactive. + SetCookieInManager( + ConsistencyCookieManager::kCookieValueStringInconsistent); + ExpectCookieSet(ConsistencyCookieManager::kCookieValueStringUpdating); + SetReconcilorState(signin_metrics::ACCOUNT_RECONCILOR_OK); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + + // Destroy `update`. This resets the state to "Consistent". + SetCookieInManager(ConsistencyCookieManager::kCookieValueStringUpdating); + ExpectGetCookie(); + ExpectCookieSet(ConsistencyCookieManager::kCookieValueStringConsistent); + } + + testing::Mock::VerifyAndClearExpectations(cookie_manager()); +} + +// Tests the move operator and constructor of `ScopedAccountUpdate`. +TEST_F(ConsistencyCookieManagerTest, MoveOperations) { + ConsistencyCookieManager* consistency_cookie_manager = + GetConsistencyCookieManager(); + // Start the reconcilor, with no cookie. + SetCookieInManager(std::string()); + ExpectGetCookie(); + SetReconcilorState(signin_metrics::ACCOUNT_RECONCILOR_OK); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 0); + + std::unique_ptr<ConsistencyCookieManager::ScopedAccountUpdate> update_ptr; + + { + // Create a scoped update, this sets the cookie to "Updating". + ExpectCookieSet(ConsistencyCookieManager::kCookieValueStringUpdating); + ConsistencyCookieManager::ScopedAccountUpdate update_1 = + consistency_cookie_manager->CreateScopedAccountUpdate(); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 1); + + // Move the update on itself, this does nothing. + // Use a `dummy` pointer as an indirection, as the compiler does not allow + // `update_1 = std::move(update_1);` + ConsistencyCookieManager::ScopedAccountUpdate* dummy = &update_1; + update_1 = std::move(*dummy); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 1); + + // Move the update to another instance, this does nothing. + ConsistencyCookieManager::ScopedAccountUpdate update_2 = + std::move(update_1); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 1); + + // Move constructor works the same. + update_ptr = + std::make_unique<ConsistencyCookieManager::ScopedAccountUpdate>( + std::move(update_2)); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 1); + + // Now delete all the updates that were moved, it does nothing. + } + + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 1); + + // Delete the remaining update, the cookie goes back to consistent. + SetCookieInManager(ConsistencyCookieManager::kCookieValueStringUpdating); + ExpectGetCookie(); + ExpectCookieSet(ConsistencyCookieManager::kCookieValueStringConsistent); + update_ptr.reset(); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 0); +} + +// `ScopedAccountUpdate` can safely outlive the `AccountReconcilor`. +TEST_F(ConsistencyCookieManagerTest, UpdateAfterDestruction) { + ConsistencyCookieManager* consistency_cookie_manager = + GetConsistencyCookieManager(); + // Start the reconcilor, with no cookie. + SetCookieInManager(std::string()); + ExpectGetCookie(); + SetReconcilorState(signin_metrics::ACCOUNT_RECONCILOR_OK); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 0); + + { + // Create a scoped update, this sets the cookie to "Updating". + ExpectCookieSet(ConsistencyCookieManager::kCookieValueStringUpdating); + ConsistencyCookieManager::ScopedAccountUpdate update_1 = + consistency_cookie_manager->CreateScopedAccountUpdate(); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + EXPECT_EQ(consistency_cookie_manager->scoped_update_count_, 1); + + // Delete the `ConsistencyCookieManager`, but not the update. + DeleteConsistencyCookieManager(); + // The cookie is not updated. + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + + // Delete the update, after the `ConsistencyCookieManager` was + // destroyed. + } + + // The cookie is not updated, and there is no crash. + testing::Mock::VerifyAndClearExpectations(cookie_manager()); +} + +// Tests that the deleted cookie is only re-created when a new +// `ScopedAccountUpdate` is created. +TEST_F(ConsistencyCookieManagerTest, CookieDeleted) { + ConsistencyCookieManager* consistency_cookie_manager = + GetConsistencyCookieManager(); + // No cookie. + SetCookieInManager(std::string()); + // Start the reconcilor, the cookie is not created. + ExpectGetCookie(); + SetReconcilorState(signin_metrics::ACCOUNT_RECONCILOR_OK); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + + // Create a cookie with "Updating" value, cookie creation is forced. + { + ExpectCookieSet(ConsistencyCookieManager::kCookieValueStringUpdating); + ConsistencyCookieManager::ScopedAccountUpdate update = + consistency_cookie_manager->CreateScopedAccountUpdate(); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + // Simulate cookie deletion. + SetCookieInManager(std::string()); + // Destroy the `ScopedAccountUpdate`, this will try to set the cookie to + // "Consistent". However, since the cookie does not exist, the operation is + // aborted and the cookie is not set. + ExpectGetCookie(); + } + + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + // Creating a new `ScopedAccountUpdate` re-creates the cookie. + { + ExpectCookieSet(ConsistencyCookieManager::kCookieValueStringUpdating); + ConsistencyCookieManager::ScopedAccountUpdate update = + consistency_cookie_manager->CreateScopedAccountUpdate(); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + ExpectGetCookie(); + } +} + +// Tests that an invalid value of the cookie is overridden. +TEST_F(ConsistencyCookieManagerTest, CookieInvalid) { + // Set invalid cookie. + SetCookieInManager("invalid_value"); + ExpectGetCookie(); + ExpectCookieSet(ConsistencyCookieManager::kCookieValueStringConsistent); + SetReconcilorState(signin_metrics::ACCOUNT_RECONCILOR_OK); +} + +// Tests that the cookie is not set if it already has the desired value. +TEST_F(ConsistencyCookieManagerTest, CookieAlreadySet) { + // Set cookie to "Consistent". + SetCookieInManager(ConsistencyCookieManager::kCookieValueStringConsistent); + // Start the reconcilor. This queries the cookie, but does not set it again. + ExpectGetCookie(); + SetReconcilorState(signin_metrics::ACCOUNT_RECONCILOR_OK); +} + +// Check that concurrent cookie queries are coalesced and result in only one +// cookie update. +TEST_F(ConsistencyCookieManagerTest, CoalesceCookieQueries) { + // Configure `GetCookieList()` to be hanging. + network::mojom::CookieManager::GetCookieListCallback get_cookie_callback; + EXPECT_CALL(*cookie_manager(), + GetCookieList(GaiaUrls::GetInstance()->gaia_url(), testing::_, + testing::_, testing::_)) + .WillOnce(testing::WithArg<3>(testing::Invoke( + [&get_cookie_callback]( + network::mojom::CookieManager::GetCookieListCallback callback) { + get_cookie_callback = std::move(callback); + }))); + + // Perform multiple account reconcilor changes, while the cookie query is in + // progress. `GetCookieList()` is called only once. + SetReconcilorState(signin_metrics::ACCOUNT_RECONCILOR_OK); + SetReconcilorState(signin_metrics::ACCOUNT_RECONCILOR_RUNNING); + SetReconcilorState(signin_metrics::ACCOUNT_RECONCILOR_ERROR); + // Unblock `GetCookieList()`. `SetCanonicalCookie()` is called only once, with + // the most recent value. + ExpectCookieSet(ConsistencyCookieManager::kCookieValueStringInconsistent); + std::unique_ptr<net::CanonicalCookie> cookie = + ConsistencyCookieManager::CreateConsistencyCookie("dummy"); + net::CookieAccessResultList cookie_list = { + {*cookie, net::CookieAccessResult()}}; + std::move(get_cookie_callback).Run(cookie_list, {}); +} + +// A forced cookie update cancels the pending query. +TEST_F(ConsistencyCookieManagerTest, CancelPendingQuery) { + // Configure `GetCookieList()` to be hanging. + network::mojom::CookieManager::GetCookieListCallback get_cookie_callback; + EXPECT_CALL(*cookie_manager(), + GetCookieList(GaiaUrls::GetInstance()->gaia_url(), testing::_, + testing::_, testing::_)) + .WillOnce(testing::WithArg<3>(testing::Invoke( + [&get_cookie_callback]( + network::mojom::CookieManager::GetCookieListCallback callback) { + get_cookie_callback = std::move(callback); + }))); + // Start a cookie query. + SetReconcilorState(signin_metrics::ACCOUNT_RECONCILOR_OK); + // While the query is in progress, trigger a forced update, the cookie is set + // immediately, and the query is canceled. + { + ConsistencyCookieManager* consistency_cookie_manager = + GetConsistencyCookieManager(); + ExpectCookieSet(ConsistencyCookieManager::kCookieValueStringUpdating); + ConsistencyCookieManager::ScopedAccountUpdate update = + consistency_cookie_manager->CreateScopedAccountUpdate(); + // Let the query complete, nothing happens. The cookie value is ignored, as + // it is most likely outdated. + std::unique_ptr<net::CanonicalCookie> cookie = + ConsistencyCookieManager::CreateConsistencyCookie( + ConsistencyCookieManager::kCookieValueStringConsistent); + net::CookieAccessResultList cookie_list = { + {*cookie, net::CookieAccessResult()}}; + std::move(get_cookie_callback).Run(cookie_list, {}); + testing::Mock::VerifyAndClearExpectations(cookie_manager()); + // Creating a nested `ScopedAccountUpdate` does not trigger anything, even + // though the query returned "Consistent", because that value was not set in + // the cache. + { + ConsistencyCookieManager::ScopedAccountUpdate inner_update = + consistency_cookie_manager->CreateScopedAccountUpdate(); + } + // Destroy the update. This triggers a new query and a new update, even + // though the previous query returned "Consistent". + ExpectGetCookie(); + } +} + +TEST_F(ConsistencyCookieManagerTest, ExtraCookieManager) { + // Start with "Consistent" in the main cookie manager. + SetCookieInManager(ConsistencyCookieManager::kCookieValueStringConsistent); + ExpectGetCookie(); + SetReconcilorState(signin_metrics::ACCOUNT_RECONCILOR_OK); + + // Add an extra cookie manager, the cookie is set immediately. + MockCookieManager extra_cookie_manager; + ExpectCookieSetInManager( + &extra_cookie_manager, + ConsistencyCookieManager::kCookieValueStringConsistent); + GetConsistencyCookieManager()->AddExtraCookieManager(&extra_cookie_manager); + testing::Mock::VerifyAndClearExpectations(&extra_cookie_manager); + + // Cookie changes are applied in the extra manager. + ExpectGetCookie(); + ExpectCookieSet(ConsistencyCookieManager::kCookieValueStringInconsistent); + ExpectCookieSetInManager( + &extra_cookie_manager, + ConsistencyCookieManager::kCookieValueStringInconsistent); + SetReconcilorState(signin_metrics::ACCOUNT_RECONCILOR_ERROR); + testing::Mock::VerifyAndClearExpectations(&extra_cookie_manager); + + // Changes from the `ScopedAccountUpdate` are applied too. + ExpectGetCookie(); + ExpectCookieSet(ConsistencyCookieManager::kCookieValueStringUpdating); + ExpectCookieSetInManager( + &extra_cookie_manager, + ConsistencyCookieManager::kCookieValueStringUpdating); + ConsistencyCookieManager::ScopedAccountUpdate update = + GetConsistencyCookieManager()->CreateScopedAccountUpdate(); + testing::Mock::VerifyAndClearExpectations(&extra_cookie_manager); + + GetConsistencyCookieManager()->RemoveExtraCookieManager( + &extra_cookie_manager); + // Cookie is set back to inconsistent in the main manager, when the + // `ScopedAccountUpdate` is destroyed. + ExpectCookieSet(ConsistencyCookieManager::kCookieValueStringInconsistent); +} + } // namespace signin diff --git a/chromium/components/signin/core/browser/signin_error_controller.cc b/chromium/components/signin/core/browser/signin_error_controller.cc index a43ca2fc652..07c73ebd8b8 100644 --- a/chromium/components/signin/core/browser/signin_error_controller.cc +++ b/chromium/components/signin/core/browser/signin_error_controller.cc @@ -4,6 +4,7 @@ #include "components/signin/core/browser/signin_error_controller.h" +#include "base/observer_list.h" #include "components/signin/public/base/signin_metrics.h" SigninErrorController::SigninErrorController( 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 529fc2022cc..f7885d818cf 100644 --- a/chromium/components/signin/core/browser/signin_error_controller_unittest.cc +++ b/chromium/components/signin/core/browser/signin_error_controller_unittest.cc @@ -9,7 +9,6 @@ #include <functional> #include <memory> -#include "base/cxx17_backports.h" #include "base/scoped_observation.h" #include "base/test/task_environment.h" #include "build/chromeos_buildflags.h" @@ -160,8 +159,8 @@ TEST(SigninErrorControllerTest, AuthStatusEnumerateAllErrors) { GoogleServiceAuthError::UNEXPECTED_SERVICE_RESPONSE, GoogleServiceAuthError::SERVICE_ERROR}; static_assert( - base::size(table) == GoogleServiceAuthError::NUM_STATES - - GoogleServiceAuthError::kDeprecatedStateCount, + std::size(table) == GoogleServiceAuthError::NUM_STATES - + GoogleServiceAuthError::kDeprecatedStateCount, "table array does not match the number of auth error types"); for (GoogleServiceAuthError::State state : table) { diff --git a/chromium/components/signin/features.gni b/chromium/components/signin/features.gni index e253b1e40d7..40a8e598cc8 100644 --- a/chromium/components/signin/features.gni +++ b/chromium/components/signin/features.gni @@ -8,4 +8,4 @@ import("//build/config/chromeos/ui_mode.gni") enable_dice_support = is_linux || is_mac || is_win || is_fuchsia # Mirror is enabled and other account consistency mechanisms are not available. -enable_mirror = is_android || is_chromeos_ash || is_chromeos_lacros || is_ios +enable_mirror = is_android || is_chromeos || is_ios diff --git a/chromium/components/signin/internal/base/BUILD.gn b/chromium/components/signin/internal/base/BUILD.gn deleted file mode 100644 index 557d74a3a09..00000000000 --- a/chromium/components/signin/internal/base/BUILD.gn +++ /dev/null @@ -1,18 +0,0 @@ -# Copyright 2019 The Chromium Authors. All rights reserved. -# Use of this source code is governed by a BSD-style license that can be -# found in the LICENSE file. - -# TODO(crbug.com/986435) Remove this target when IdentityServicesProvider -# will provide AccountManagerFacade. -source_set("base") { - if (is_android) { - sources = [ - "account_manager_facade_android.cc", - "account_manager_facade_android.h", - ] - deps = [ - "//base", - "//components/signin/public/android:jni_headers", - ] - } -} diff --git a/chromium/components/signin/internal/base/DEPS b/chromium/components/signin/internal/base/DEPS deleted file mode 100644 index a2380f6cc1c..00000000000 --- a/chromium/components/signin/internal/base/DEPS +++ /dev/null @@ -1,8 +0,0 @@ -include_rules = [ -] - -specific_include_rules = { - "account_manager_facade_android.cc": [ - "+components/signin/public/android/jni_headers/AccountManagerFacadeProvider_jni.h", - ], -} diff --git a/chromium/components/signin/internal/base/README.md b/chromium/components/signin/internal/base/README.md deleted file mode 100644 index e69de29bb2d..00000000000 --- a/chromium/components/signin/internal/base/README.md +++ /dev/null diff --git a/chromium/components/signin/internal/base/account_manager_facade_android.cc b/chromium/components/signin/internal/base/account_manager_facade_android.cc deleted file mode 100644 index e6af50e9410..00000000000 --- a/chromium/components/signin/internal/base/account_manager_facade_android.cc +++ /dev/null @@ -1,13 +0,0 @@ -// Copyright 2019 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "components/signin/internal/base/account_manager_facade_android.h" - -#include "components/signin/public/android/jni_headers/AccountManagerFacadeProvider_jni.h" - -base::android::ScopedJavaLocalRef<jobject> -AccountManagerFacadeAndroid::GetJavaObject() { - return signin::Java_AccountManagerFacadeProvider_getInstance( - base::android::AttachCurrentThread()); -} diff --git a/chromium/components/signin/internal/base/account_manager_facade_android.h b/chromium/components/signin/internal/base/account_manager_facade_android.h deleted file mode 100644 index 9508a6d11cf..00000000000 --- a/chromium/components/signin/internal/base/account_manager_facade_android.h +++ /dev/null @@ -1,19 +0,0 @@ -// Copyright 2019 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef COMPONENTS_SIGNIN_INTERNAL_BASE_ACCOUNT_MANAGER_FACADE_ANDROID_H_ -#define COMPONENTS_SIGNIN_INTERNAL_BASE_ACCOUNT_MANAGER_FACADE_ANDROID_H_ - -#include "base/android/scoped_java_ref.h" - -// Simple accessor to java's AccountManagerFacade -class AccountManagerFacadeAndroid { - public: - // Returns a reference to the corresponding Java AccountManagerFacade object. - // TODO(crbug.com/986435) Remove direct access to AccountManagerFacade.get - // from C++ when IdentityServicesProvider will handle its instance management. - static base::android::ScopedJavaLocalRef<jobject> GetJavaObject(); -}; - -#endif // COMPONENTS_SIGNIN_INTERNAL_BASE_ACCOUNT_MANAGER_FACADE_ANDROID_H_ diff --git a/chromium/components/signin/internal/identity_manager/BUILD.gn b/chromium/components/signin/internal/identity_manager/BUILD.gn index 08370fd14fe..5a0f4c49dcc 100644 --- a/chromium/components/signin/internal/identity_manager/BUILD.gn +++ b/chromium/components/signin/internal/identity_manager/BUILD.gn @@ -13,6 +13,8 @@ source_set("identity_manager") { "account_capabilities_constants.h", "account_capabilities_fetcher.cc", "account_capabilities_fetcher.h", + "account_capabilities_fetcher_factory.h", + "account_capabilities_list.h", "account_fetcher_service.cc", "account_fetcher_service.h", "account_info_fetcher.cc", @@ -35,7 +37,6 @@ source_set("identity_manager") { "primary_account_manager.h", "primary_account_mutator_impl.cc", "primary_account_mutator_impl.h", - "primary_account_policy_manager.h", "profile_oauth2_token_service.cc", "profile_oauth2_token_service.h", "profile_oauth2_token_service_builder.cc", @@ -71,14 +72,22 @@ source_set("identity_manager") { if (is_android) { sources += [ + "account_capabilities_fetcher_android.cc", + "account_capabilities_fetcher_android.h", + "account_capabilities_fetcher_factory_android.cc", + "account_capabilities_fetcher_factory_android.h", "child_account_info_fetcher_android.cc", "child_account_info_fetcher_android.h", "profile_oauth2_token_service_delegate_android.cc", "profile_oauth2_token_service_delegate_android.h", ] - deps += [ - "//components/signin/internal/base", - "//components/signin/public/android:jni_headers", + deps += [ "//components/signin/public/android:jni_headers" ] + } else { + sources += [ + "account_capabilities_fetcher_factory_gaia.cc", + "account_capabilities_fetcher_factory_gaia.h", + "account_capabilities_fetcher_gaia.cc", + "account_capabilities_fetcher_gaia.h", ] } @@ -89,7 +98,7 @@ source_set("identity_manager") { ] } - if (is_chromeos_ash || is_chromeos_lacros) { + if (is_chromeos) { sources += [ "profile_oauth2_token_service_delegate_chromeos.cc", "profile_oauth2_token_service_delegate_chromeos.h", @@ -100,13 +109,6 @@ source_set("identity_manager") { if (is_chromeos_ash) { deps += [ "//ash/constants" ] public_deps += [ "//ash/components/account_manager" ] - } else { - sources += [ - "primary_account_policy_manager_impl.cc", - "primary_account_policy_manager_impl.h", - ] - - public_deps += [ "//components/prefs" ] } if (!is_android && !is_ios) { @@ -138,6 +140,7 @@ source_set("unit_tests") { testonly = true sources = [ + "account_capabilities_fetcher_unittest.cc", "account_info_util_unittest.cc", "account_tracker_service_unittest.cc", "gaia_cookie_manager_service_unittest.cc", @@ -179,6 +182,11 @@ source_set("unit_tests") { if (is_android) { sources += [ "profile_oauth2_token_service_delegate_android_unittest.cc" ] + + deps += [ + "//components/signin/public/android:signin_java_test_support", + "//components/signin/public/android:test_support_jni_headers", + ] } if (is_chromeos_ash) { @@ -189,8 +197,6 @@ source_set("unit_tests") { "//ash/constants", "//components/account_manager_core:test_support", ] - } else { - sources += [ "primary_account_policy_manager_impl_unittest.cc" ] } if (is_ios) { @@ -211,6 +217,10 @@ static_library("test_support") { testonly = true sources = [ + "fake_account_capabilities_fetcher.cc", + "fake_account_capabilities_fetcher.h", + "fake_account_capabilities_fetcher_factory.cc", + "fake_account_capabilities_fetcher_factory.h", "fake_profile_oauth2_token_service.cc", "fake_profile_oauth2_token_service.h", "fake_profile_oauth2_token_service_delegate.cc", @@ -228,6 +238,7 @@ static_library("test_support") { public_deps = [ ":identity_manager", "//base", + "//components/signin/public/identity_manager", "//google_apis:test_support", ] } diff --git a/chromium/components/signin/internal/identity_manager/DEPS b/chromium/components/signin/internal/identity_manager/DEPS index dff6bbca592..43743789e55 100644 --- a/chromium/components/signin/internal/identity_manager/DEPS +++ b/chromium/components/signin/internal/identity_manager/DEPS @@ -1,9 +1,9 @@ include_rules = [ "+ash/constants", - "+components/signin/internal/base", "+components/signin/public/base", "+components/signin/public/identity_manager", "+components/signin/public/webdata", "+components/signin/public/android/jni_headers", + "+components/signin/public/android/test_support_jni_headers", "+mojo/public", ] diff --git a/chromium/components/signin/internal/identity_manager/account_capabilities_constants.cc b/chromium/components/signin/internal/identity_manager/account_capabilities_constants.cc index 78be14efdb2..47ca9242e36 100644 --- a/chromium/components/signin/internal/identity_manager/account_capabilities_constants.cc +++ b/chromium/components/signin/internal/identity_manager/account_capabilities_constants.cc @@ -4,11 +4,7 @@ #include "components/signin/internal/identity_manager/account_capabilities_constants.h" -const char kIsSubjectToParentalControlsCapabilityName[] = - "accountcapabilities/guydolldmfya"; - -const char kCanOfferExtendedChromeSyncPromosCapabilityName[] = - "accountcapabilities/gi2tklldmfya"; - -const char kCanOfferExtendedChromeSyncPromosCapabilityPrefsPath[] = - "accountcapabilities.can_offer_extended_chrome_sync_promos"; +#define ACCOUNT_CAPABILITY(cpp_label, java_label, name) \ + const char cpp_label[] = name; +#include "components/signin/internal/identity_manager/account_capabilities_list.h" +#undef ACCOUNT_CAPABILITY diff --git a/chromium/components/signin/internal/identity_manager/account_capabilities_constants.h b/chromium/components/signin/internal/identity_manager/account_capabilities_constants.h index 0e638dc6c1f..e94f33a07e5 100644 --- a/chromium/components/signin/internal/identity_manager/account_capabilities_constants.h +++ b/chromium/components/signin/internal/identity_manager/account_capabilities_constants.h @@ -5,8 +5,9 @@ #ifndef COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_CONSTANTS_H_ #define COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_CONSTANTS_H_ -extern const char kCanOfferExtendedChromeSyncPromosCapabilityName[]; -extern const char kCanOfferExtendedChromeSyncPromosCapabilityPrefsPath[]; -extern const char kIsSubjectToParentalControlsCapabilityName[]; +#define ACCOUNT_CAPABILITY(cpp_label, java_label, name) \ + extern const char cpp_label[]; +#include "components/signin/internal/identity_manager/account_capabilities_list.h" +#undef ACCOUNT_CAPABILITY #endif // COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_CONSTANTS_H_ diff --git a/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher.cc b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher.cc index 7c400f0160d..e7c1492be39 100644 --- a/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher.cc +++ b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher.cc @@ -4,138 +4,24 @@ #include "components/signin/internal/identity_manager/account_capabilities_fetcher.h" -#include "base/metrics/histogram_functions.h" -#include "base/time/time.h" -#include "base/trace_event/trace_event.h" -#include "components/signin/internal/identity_manager/account_capabilities_constants.h" -#include "components/signin/internal/identity_manager/account_fetcher_service.h" -#include "components/signin/internal/identity_manager/account_info_util.h" -#include "components/signin/internal/identity_manager/profile_oauth2_token_service.h" -#include "google_apis/gaia/gaia_constants.h" -#include "google_apis/gaia/google_service_auth_error.h" -#include "google_apis/gaia/oauth2_access_token_consumer.h" -#include "services/network/public/cpp/shared_url_loader_factory.h" - AccountCapabilitiesFetcher::AccountCapabilitiesFetcher( - ProfileOAuth2TokenService* token_service, - scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, - AccountFetcherService* service, - const CoreAccountId& account_id) - : OAuth2AccessTokenManager::Consumer("account_capabilities_fetcher"), - token_service_(token_service), - url_loader_factory_(std::move(url_loader_factory)), - service_(service), - account_id_(account_id) { - TRACE_EVENT_NESTABLE_ASYNC_BEGIN1("AccountFetcherService", - "AccountCapabilitiesFetcher", this, - "account_id", account_id.ToString()); + const CoreAccountInfo& account_info, + OnCompleteCallback on_complete_callback) + : account_info_(account_info), + on_complete_callback_(std::move(on_complete_callback)) { + DCHECK(on_complete_callback_); } -AccountCapabilitiesFetcher::~AccountCapabilitiesFetcher() { - TRACE_EVENT_NESTABLE_ASYNC_END0("AccountFetcherService", - "AccountCapabilitiesFetcher", this); - RecordFetchResultAndDuration(FetchResult::kCancelled); -} +AccountCapabilitiesFetcher::~AccountCapabilitiesFetcher() = default; void AccountCapabilitiesFetcher::Start() { - TRACE_EVENT_NESTABLE_ASYNC_BEGIN0("AccountFetcherService", "GetAccessToken", - this); - fetch_start_time_ = base::TimeTicks::Now(); - OAuth2AccessTokenManager::ScopeSet scopes; - scopes.insert(GaiaConstants::kAccountCapabilitiesOAuth2Scope); - login_token_request_ = - token_service_->StartRequest(account_id_, scopes, this); -} - -void AccountCapabilitiesFetcher::OnGetTokenSuccess( - const OAuth2AccessTokenManager::Request* request, - const OAuth2AccessTokenConsumer::TokenResponse& token_response) { - TRACE_EVENT_NESTABLE_ASYNC_END0("AccountFetcherService", "GetAccessToken", - this); - TRACE_EVENT_NESTABLE_ASYNC_BEGIN0("AccountFetcherService", - "GetAccountCapabilities", this); - DCHECK_EQ(request, login_token_request_.get()); - login_token_request_.reset(); - - gaia_oauth_client_ = - std::make_unique<gaia::GaiaOAuthClient>(url_loader_factory_); - const int kMaxRetries = 3; - gaia_oauth_client_->GetAccountCapabilities( - token_response.access_token, - {kCanOfferExtendedChromeSyncPromosCapabilityName}, kMaxRetries, this); -} - -void AccountCapabilitiesFetcher::OnGetTokenFailure( - const OAuth2AccessTokenManager::Request* request, - const GoogleServiceAuthError& error) { - TRACE_EVENT_NESTABLE_ASYNC_END1("AccountFetcherService", "GetAccessToken", - this, "error", error.ToString()); - VLOG(1) << "OnGetTokenFailure: " << error.ToString(); - DCHECK_EQ(request, login_token_request_.get()); - login_token_request_.reset(); - RecordFetchResultAndDuration(FetchResult::kGetTokenFailure); - service_->OnAccountCapabilitiesFetchFailure(account_id_); -} - -void AccountCapabilitiesFetcher::OnGetAccountCapabilitiesResponse( - std::unique_ptr<base::Value> account_capabilities) { - TRACE_EVENT_NESTABLE_ASYNC_END0("AccountFetcherService", - "GetAccountCapabilities", this); - absl::optional<AccountCapabilities> parsed_capabilities = - AccountCapabilitiesFromValue(*account_capabilities); - if (!parsed_capabilities) { - VLOG(1) << "Failed to parse account capabilities for " << account_id_ - << ". Response body: " << account_capabilities->DebugString(); - RecordFetchResultAndDuration(FetchResult::kParseResponseFailure); - service_->OnAccountCapabilitiesFetchFailure(account_id_); - return; - } - - RecordFetchResultAndDuration(FetchResult::kSuccess); - service_->OnAccountCapabilitiesFetchSuccess(account_id_, - parsed_capabilities.value()); -} - -void AccountCapabilitiesFetcher::OnOAuthError() { - TRACE_EVENT_NESTABLE_ASYNC_END1("AccountFetcherService", - "GetAccountCapabilities", this, "error", - "OAuthError"); - VLOG(1) << "OnOAuthError"; - RecordFetchResultAndDuration(FetchResult::kOAuthError); - service_->OnAccountCapabilitiesFetchFailure(account_id_); + DCHECK(!started_); + started_ = true; + StartImpl(); } -void AccountCapabilitiesFetcher::OnNetworkError(int response_code) { - TRACE_EVENT_NESTABLE_ASYNC_END2( - "AccountFetcherService", "GetAccountCapabilities", this, "error", - "NetworkError", "response_code", response_code); - VLOG(1) << "OnNetworkError " << response_code; - RecordFetchResultAndDuration(FetchResult::kNetworkError); - service_->OnAccountCapabilitiesFetchFailure(account_id_); -} - -void AccountCapabilitiesFetcher::RecordFetchResultAndDuration( - FetchResult result) { - if (fetch_histograms_recorded_) { - // Record histograms only once. - return; - } - fetch_histograms_recorded_ = true; - - base::UmaHistogramEnumeration("Signin.AccountCapabilities.FetchResult", - result); - - if (fetch_start_time_.is_null()) { - // Cannot record duration for a fetch that hasn't started. - DCHECK_EQ(result, FetchResult::kCancelled); - return; - } - base::TimeDelta duration = base::TimeTicks::Now() - fetch_start_time_; - if (result == FetchResult::kSuccess) { - base::UmaHistogramMediumTimes( - "Signin.AccountCapabilities.FetchDuration.Success", duration); - } else { - base::UmaHistogramMediumTimes( - "Signin.AccountCapabilities.FetchDuration.Failure", duration); - } +void AccountCapabilitiesFetcher::CompleteFetchAndMaybeDestroySelf( + const absl::optional<AccountCapabilities>& capabilities) { + DCHECK(on_complete_callback_); + std::move(on_complete_callback_).Run(account_info_.account_id, capabilities); } diff --git a/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher.h b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher.h index b03aa32da09..d825d2c239c 100644 --- a/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher.h +++ b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher.h @@ -5,77 +5,49 @@ #ifndef COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_FETCHER_H_ #define COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_FETCHER_H_ -#include "base/memory/raw_ptr.h" -#include "base/memory/scoped_refptr.h" -#include "base/time/time.h" +#include "base/callback.h" +#include "components/signin/public/identity_manager/account_capabilities.h" +#include "components/signin/public/identity_manager/account_info.h" #include "google_apis/gaia/core_account_id.h" -#include "google_apis/gaia/gaia_oauth_client.h" -#include "google_apis/gaia/oauth2_access_token_manager.h" +#include "third_party/abseil-cpp/absl/types/optional.h" -namespace network { -class SharedURLLoaderFactory; -} - -class AccountFetcherService; -class ProfileOAuth2TokenService; -class GoogleServiceAuthError; - -class AccountCapabilitiesFetcher : public OAuth2AccessTokenManager::Consumer, - public gaia::GaiaOAuthClient::Delegate { +class AccountCapabilitiesFetcher { public: - // These values are persisted to logs. Entries should not be renumbered and - // numeric values should never be reused. - enum class FetchResult { - kSuccess = 0, - kGetTokenFailure = 1, - kParseResponseFailure = 2, - kOAuthError = 3, - kNetworkError = 4, - kCancelled = 5, - kMaxValue = kCancelled - }; + using OnCompleteCallback = + base::OnceCallback<void(const CoreAccountId&, + const absl::optional<AccountCapabilities>&)>; - AccountCapabilitiesFetcher( - ProfileOAuth2TokenService* token_service, - scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, - AccountFetcherService* service, - const CoreAccountId& account_id); - ~AccountCapabilitiesFetcher() override; + explicit AccountCapabilitiesFetcher(const CoreAccountInfo& account_info, + OnCompleteCallback on_complete_callback); + virtual ~AccountCapabilitiesFetcher(); AccountCapabilitiesFetcher(const AccountCapabilitiesFetcher&) = delete; AccountCapabilitiesFetcher& operator=(const AccountCapabilitiesFetcher&) = delete; - // Start fetching account capabilities. + // Start fetching account capabilities. Must be called no more than once per + // object lifetime. void Start(); - // OAuth2AccessTokenManager::Consumer: - void OnGetTokenSuccess( - const OAuth2AccessTokenManager::Request* request, - const OAuth2AccessTokenConsumer::TokenResponse& token_response) override; - void OnGetTokenFailure(const OAuth2AccessTokenManager::Request* request, - const GoogleServiceAuthError& error) override; + protected: + const CoreAccountInfo& account_info() { return account_info_; } + const CoreAccountId& account_id() { return account_info_.account_id; } - // gaia::GaiaOAuthClient::Delegate: - void OnGetAccountCapabilitiesResponse( - std::unique_ptr<base::Value> account_capabilities) override; - void OnOAuthError() override; - void OnNetworkError(int response_code) override; + // Implemented by subclasses. + virtual void StartImpl() = 0; - private: - void RecordFetchResultAndDuration(FetchResult result); + // Completes the fetch by calling `on_complete_callback_`. Must be called no + // more than once per object lifetime. + // `this` may be destroyed after calling this function. + void CompleteFetchAndMaybeDestroySelf( + const absl::optional<AccountCapabilities>& capabilities); - raw_ptr<ProfileOAuth2TokenService> token_service_; - scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_; - raw_ptr<AccountFetcherService> service_; - const CoreAccountId account_id_; - - std::unique_ptr<OAuth2AccessTokenManager::Request> login_token_request_; - std::unique_ptr<gaia::GaiaOAuthClient> gaia_oauth_client_; + private: + // Ensures that `Start()` isn't called multiple times. + bool started_ = false; - // Used for metrics: - base::TimeTicks fetch_start_time_; - bool fetch_histograms_recorded_ = false; + const CoreAccountInfo account_info_; + OnCompleteCallback on_complete_callback_; }; #endif // COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_FETCHER_H_ diff --git a/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_android.cc b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_android.cc new file mode 100644 index 00000000000..01c878385a9 --- /dev/null +++ b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_android.cc @@ -0,0 +1,61 @@ +// Copyright 2022 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/internal/identity_manager/account_capabilities_fetcher_android.h" + +#include "base/android/jni_android.h" +#include "base/callback.h" +#include "components/signin/public/android/jni_headers/AccountCapabilitiesFetcher_jni.h" +#include "components/signin/public/identity_manager/account_capabilities.h" +#include "components/signin/public/identity_manager/account_info.h" + +namespace { +using OnAccountCapabilitiesFetchedCallback = + base::OnceCallback<void(const absl::optional<AccountCapabilities>&)>; +} + +AccountCapabilitiesFetcherAndroid::AccountCapabilitiesFetcherAndroid( + const CoreAccountInfo& account_info, + AccountCapabilitiesFetcher::OnCompleteCallback on_complete_callback) + : AccountCapabilitiesFetcher(account_info, + std::move(on_complete_callback)) { + JNIEnv* env = base::android::AttachCurrentThread(); + + // This callback is "owned" by the Java counterpart. There is no explicit way + // of passing object ownership to Java, so allocate the callback on heap and + // pass a raw pointer to Java. The callback object will be collected once + // Java calls back to native in + // `JNI_AccountCapabilitiesFetcher_setAccountCapabilities()`. + auto heap_callback = + std::make_unique<OnAccountCapabilitiesFetchedCallback>(base::BindOnce( + &AccountCapabilitiesFetcherAndroid::CompleteFetchAndMaybeDestroySelf, + weak_ptr_factory_.GetWeakPtr())); + base::android::ScopedJavaLocalRef<jobject> local_java_ref = + signin::Java_AccountCapabilitiesFetcher_Constructor( + env, ConvertToJavaCoreAccountInfo(env, account_info), + reinterpret_cast<intptr_t>(heap_callback.release())); + java_ref_.Reset(env, local_java_ref.obj()); +} + +AccountCapabilitiesFetcherAndroid::~AccountCapabilitiesFetcherAndroid() = + default; + +void AccountCapabilitiesFetcherAndroid::StartImpl() { + JNIEnv* env = base::android::AttachCurrentThread(); + signin::Java_AccountCapabilitiesFetcher_startFetchingAccountCapabilities( + env, java_ref_); +} + +namespace signin { +void JNI_AccountCapabilitiesFetcher_OnCapabilitiesFetchComplete( + JNIEnv* env, + const base::android::JavaParamRef<jobject>& account_capabilities, + jlong native_callback) { + std::unique_ptr<OnAccountCapabilitiesFetchedCallback> heap_callback( + reinterpret_cast<OnAccountCapabilitiesFetchedCallback*>(native_callback)); + std::move(*heap_callback) + .Run(AccountCapabilities::ConvertFromJavaAccountCapabilities( + env, account_capabilities)); +} +} // namespace signin diff --git a/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_android.h b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_android.h new file mode 100644 index 00000000000..95287c84c70 --- /dev/null +++ b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_android.h @@ -0,0 +1,40 @@ +// Copyright 2022 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_ACCOUNT_CAPABILITIES_FETCHER_ANDROID_H_ +#define COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_FETCHER_ANDROID_H_ + +#include <jni.h> + +#include "base/android/scoped_java_ref.h" +#include "base/memory/weak_ptr.h" +#include "components/signin/internal/identity_manager/account_capabilities_fetcher.h" + +// Android implementation of `AccountCapabilitiesFetcher`. It has a Java +// counterpart located in AccountCapabilitiesFetcher.java. +class AccountCapabilitiesFetcherAndroid : public AccountCapabilitiesFetcher { + public: + AccountCapabilitiesFetcherAndroid( + const CoreAccountInfo& account_info, + AccountCapabilitiesFetcher::OnCompleteCallback on_complete_callback); + ~AccountCapabilitiesFetcherAndroid() override; + + AccountCapabilitiesFetcherAndroid(const AccountCapabilitiesFetcherAndroid&) = + delete; + AccountCapabilitiesFetcherAndroid& operator=( + const AccountCapabilitiesFetcherAndroid&) = delete; + + protected: + // AccountCapabilitiesFetcher: + void StartImpl() override; + + private: + // A reference to the Java counterpart of this object. + base::android::ScopedJavaGlobalRef<jobject> java_ref_; + + base::WeakPtrFactory<AccountCapabilitiesFetcherAndroid> weak_ptr_factory_{ + this}; +}; + +#endif // COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_FETCHER_ANDROID_H_ diff --git a/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_factory.h b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_factory.h new file mode 100644 index 00000000000..7e4e958d3fc --- /dev/null +++ b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_factory.h @@ -0,0 +1,26 @@ +// Copyright 2022 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_ACCOUNT_CAPABILITIES_FETCHER_FACTORY_H_ +#define COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_FETCHER_FACTORY_H_ + +#include <memory> + +#include "components/signin/internal/identity_manager/account_capabilities_fetcher.h" + +class AccountCapabilitiesFetcher; +struct CoreAccountInfo; + +// Abstract factory class for creating `AccountCapabilitiesFetcher` objects. +class AccountCapabilitiesFetcherFactory { + public: + virtual ~AccountCapabilitiesFetcherFactory() = default; + + virtual std::unique_ptr<AccountCapabilitiesFetcher> + CreateAccountCapabilitiesFetcher( + const CoreAccountInfo& account_info, + AccountCapabilitiesFetcher::OnCompleteCallback on_complete_callback) = 0; +}; + +#endif // COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_FETCHER_FACTORY_H_ diff --git a/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_factory_android.cc b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_factory_android.cc new file mode 100644 index 00000000000..1ea2bcc85b7 --- /dev/null +++ b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_factory_android.cc @@ -0,0 +1,22 @@ +// Copyright 2022 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/internal/identity_manager/account_capabilities_fetcher_factory_android.h" + +#include "components/signin/internal/identity_manager/account_capabilities_fetcher.h" +#include "components/signin/internal/identity_manager/account_capabilities_fetcher_android.h" +#include "components/signin/public/identity_manager/account_info.h" + +AccountCapabilitiesFetcherFactoryAndroid:: + AccountCapabilitiesFetcherFactoryAndroid() = default; +AccountCapabilitiesFetcherFactoryAndroid:: + ~AccountCapabilitiesFetcherFactoryAndroid() = default; + +std::unique_ptr<AccountCapabilitiesFetcher> +AccountCapabilitiesFetcherFactoryAndroid::CreateAccountCapabilitiesFetcher( + const CoreAccountInfo& account_info, + AccountCapabilitiesFetcher::OnCompleteCallback on_complete_callback) { + return std::make_unique<AccountCapabilitiesFetcherAndroid>( + account_info, std::move(on_complete_callback)); +} diff --git a/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_factory_android.h b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_factory_android.h new file mode 100644 index 00000000000..c14d13b3931 --- /dev/null +++ b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_factory_android.h @@ -0,0 +1,37 @@ +// Copyright 2022 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_ACCOUNT_CAPABILITIES_FETCHER_FACTORY_ANDROID_H_ +#define COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_FETCHER_FACTORY_ANDROID_H_ + +#include "components/signin/internal/identity_manager/account_capabilities_fetcher_factory.h" + +#include <memory> + +class AccountCapabilitiesFetcher; +struct CoreAccountInfo; + +// `AccountCapabilitiesFetcherFactory` implementation that creates +// `AccountCapabilitiesFetcherAndroid` instances. +// `AccountCapabilitiesFetcherAndroid` calls a GMSCore API from Java to obtain +// capabilities values. +class AccountCapabilitiesFetcherFactoryAndroid + : public AccountCapabilitiesFetcherFactory { + public: + AccountCapabilitiesFetcherFactoryAndroid(); + ~AccountCapabilitiesFetcherFactoryAndroid() override; + + AccountCapabilitiesFetcherFactoryAndroid( + const AccountCapabilitiesFetcherFactoryAndroid&) = delete; + AccountCapabilitiesFetcherFactoryAndroid& operator=( + const AccountCapabilitiesFetcherFactoryAndroid&) = delete; + + // AccountCapabilitiesFetcherFactory: + std::unique_ptr<AccountCapabilitiesFetcher> CreateAccountCapabilitiesFetcher( + const CoreAccountInfo& account_info, + AccountCapabilitiesFetcher::OnCompleteCallback on_complete_callback) + override; +}; + +#endif // COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_FETCHER_FACTORY_ANDROID_H_ diff --git a/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_factory_gaia.cc b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_factory_gaia.cc new file mode 100644 index 00000000000..31a7264f70a --- /dev/null +++ b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_factory_gaia.cc @@ -0,0 +1,29 @@ +// Copyright 2022 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/internal/identity_manager/account_capabilities_fetcher_factory_gaia.h" + +#include <memory> + +#include "components/signin/internal/identity_manager/account_capabilities_fetcher_gaia.h" +#include "components/signin/internal/identity_manager/profile_oauth2_token_service.h" +#include "components/signin/public/base/signin_client.h" +#include "services/network/public/cpp/shared_url_loader_factory.h" + +AccountCapabilitiesFetcherFactoryGaia::AccountCapabilitiesFetcherFactoryGaia( + ProfileOAuth2TokenService* token_service, + SigninClient* signin_client) + : token_service_(token_service), signin_client_(signin_client) {} + +AccountCapabilitiesFetcherFactoryGaia:: + ~AccountCapabilitiesFetcherFactoryGaia() = default; + +std::unique_ptr<AccountCapabilitiesFetcher> +AccountCapabilitiesFetcherFactoryGaia::CreateAccountCapabilitiesFetcher( + const CoreAccountInfo& account_info, + AccountCapabilitiesFetcher::OnCompleteCallback on_complete_callback) { + return std::make_unique<AccountCapabilitiesFetcherGaia>( + token_service_, signin_client_->GetURLLoaderFactory(), account_info, + std::move(on_complete_callback)); +} diff --git a/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_factory_gaia.h b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_factory_gaia.h new file mode 100644 index 00000000000..80c02c280e2 --- /dev/null +++ b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_factory_gaia.h @@ -0,0 +1,43 @@ +// Copyright 2022 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_ACCOUNT_CAPABILITIES_FETCHER_FACTORY_GAIA_H_ +#define COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_FETCHER_FACTORY_GAIA_H_ + +#include "base/memory/raw_ptr.h" +#include "base/memory/scoped_refptr.h" +#include "components/signin/internal/identity_manager/account_capabilities_fetcher_factory.h" + +class ProfileOAuth2TokenService; +class SigninClient; + +// `AccountCapabilitiesFetcherFactory` implementation that creates +// `AccountCapabilitiesFetcherGaia` instances. +// `AccountCapabilitiesFetcherGaia` is used on platforms that fetch account +// capabilities directly from the server. +class AccountCapabilitiesFetcherFactoryGaia + : public AccountCapabilitiesFetcherFactory { + public: + AccountCapabilitiesFetcherFactoryGaia( + ProfileOAuth2TokenService* token_service, + SigninClient* signin_client); + ~AccountCapabilitiesFetcherFactoryGaia() override; + + AccountCapabilitiesFetcherFactoryGaia( + const AccountCapabilitiesFetcherFactoryGaia&) = delete; + AccountCapabilitiesFetcherFactoryGaia& operator=( + const AccountCapabilitiesFetcherFactoryGaia&) = delete; + + // AccountCapabilitiesFetcherFactory: + std::unique_ptr<AccountCapabilitiesFetcher> CreateAccountCapabilitiesFetcher( + const CoreAccountInfo& account_info, + AccountCapabilitiesFetcher::OnCompleteCallback on_complete_callback) + override; + + private: + const raw_ptr<ProfileOAuth2TokenService> token_service_; + const raw_ptr<SigninClient> signin_client_; +}; + +#endif // COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_FETCHER_FACTORY_GAIA_H_ diff --git a/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_gaia.cc b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_gaia.cc new file mode 100644 index 00000000000..76381658dc9 --- /dev/null +++ b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_gaia.cc @@ -0,0 +1,140 @@ +// Copyright 2022 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/internal/identity_manager/account_capabilities_fetcher_gaia.h" + +#include "base/metrics/histogram_functions.h" +#include "base/time/time.h" +#include "base/trace_event/trace_event.h" +#include "components/signin/internal/identity_manager/account_capabilities_constants.h" +#include "components/signin/internal/identity_manager/account_info_util.h" +#include "components/signin/internal/identity_manager/profile_oauth2_token_service.h" +#include "components/signin/public/identity_manager/account_capabilities.h" +#include "google_apis/gaia/gaia_constants.h" +#include "google_apis/gaia/google_service_auth_error.h" +#include "google_apis/gaia/oauth2_access_token_consumer.h" +#include "services/network/public/cpp/shared_url_loader_factory.h" +#include "third_party/abseil-cpp/absl/types/optional.h" + +AccountCapabilitiesFetcherGaia::AccountCapabilitiesFetcherGaia( + ProfileOAuth2TokenService* token_service, + scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, + const CoreAccountInfo& account_info, + AccountCapabilitiesFetcher::OnCompleteCallback on_complete_callback) + : AccountCapabilitiesFetcher(account_info, std::move(on_complete_callback)), + OAuth2AccessTokenManager::Consumer("account_capabilities_fetcher"), + token_service_(token_service), + url_loader_factory_(std::move(url_loader_factory)) { + TRACE_EVENT_NESTABLE_ASYNC_BEGIN1("AccountFetcherService", + "AccountCapabilitiesFetcherGaia", this, + "account_id", account_id().ToString()); +} + +AccountCapabilitiesFetcherGaia::~AccountCapabilitiesFetcherGaia() { + TRACE_EVENT_NESTABLE_ASYNC_END0("AccountFetcherService", + "AccountCapabilitiesFetcherGaia", this); + RecordFetchResultAndDuration(FetchResult::kCancelled); +} + +void AccountCapabilitiesFetcherGaia::StartImpl() { + TRACE_EVENT_NESTABLE_ASYNC_BEGIN0("AccountFetcherService", "GetAccessToken", + this); + fetch_start_time_ = base::TimeTicks::Now(); + OAuth2AccessTokenManager::ScopeSet scopes; + scopes.insert(GaiaConstants::kAccountCapabilitiesOAuth2Scope); + login_token_request_ = + token_service_->StartRequest(account_id(), scopes, this); +} + +void AccountCapabilitiesFetcherGaia::OnGetTokenSuccess( + const OAuth2AccessTokenManager::Request* request, + const OAuth2AccessTokenConsumer::TokenResponse& token_response) { + TRACE_EVENT_NESTABLE_ASYNC_END0("AccountFetcherService", "GetAccessToken", + this); + TRACE_EVENT_NESTABLE_ASYNC_BEGIN0("AccountFetcherService", + "GetAccountCapabilities", this); + DCHECK_EQ(request, login_token_request_.get()); + login_token_request_.reset(); + + gaia_oauth_client_ = + std::make_unique<gaia::GaiaOAuthClient>(url_loader_factory_); + const int kMaxRetries = 3; + gaia_oauth_client_->GetAccountCapabilities( + token_response.access_token, + AccountCapabilities::GetSupportedAccountCapabilityNames(), kMaxRetries, + this); +} + +void AccountCapabilitiesFetcherGaia::OnGetTokenFailure( + const OAuth2AccessTokenManager::Request* request, + const GoogleServiceAuthError& error) { + TRACE_EVENT_NESTABLE_ASYNC_END1("AccountFetcherService", "GetAccessToken", + this, "error", error.ToString()); + VLOG(1) << "OnGetTokenFailure: " << error.ToString(); + DCHECK_EQ(request, login_token_request_.get()); + login_token_request_.reset(); + RecordFetchResultAndDuration(FetchResult::kGetTokenFailure); + CompleteFetchAndMaybeDestroySelf(absl::nullopt); +} + +void AccountCapabilitiesFetcherGaia::OnGetAccountCapabilitiesResponse( + std::unique_ptr<base::Value> account_capabilities) { + TRACE_EVENT_NESTABLE_ASYNC_END0("AccountFetcherService", + "GetAccountCapabilities", this); + absl::optional<AccountCapabilities> parsed_capabilities = + AccountCapabilitiesFromValue(*account_capabilities); + FetchResult result = FetchResult::kSuccess; + if (!parsed_capabilities) { + VLOG(1) << "Failed to parse account capabilities for " << account_id() + << ". Response body: " << account_capabilities->DebugString(); + result = FetchResult::kParseResponseFailure; + } + + RecordFetchResultAndDuration(result); + CompleteFetchAndMaybeDestroySelf(parsed_capabilities); +} + +void AccountCapabilitiesFetcherGaia::OnOAuthError() { + TRACE_EVENT_NESTABLE_ASYNC_END1("AccountFetcherService", + "GetAccountCapabilities", this, "error", + "OAuthError"); + VLOG(1) << "OnOAuthError"; + RecordFetchResultAndDuration(FetchResult::kOAuthError); + CompleteFetchAndMaybeDestroySelf(absl::nullopt); +} + +void AccountCapabilitiesFetcherGaia::OnNetworkError(int response_code) { + TRACE_EVENT_NESTABLE_ASYNC_END2( + "AccountFetcherService", "GetAccountCapabilities", this, "error", + "NetworkError", "response_code", response_code); + VLOG(1) << "OnNetworkError " << response_code; + RecordFetchResultAndDuration(FetchResult::kNetworkError); + CompleteFetchAndMaybeDestroySelf(absl::nullopt); +} + +void AccountCapabilitiesFetcherGaia::RecordFetchResultAndDuration( + FetchResult result) { + if (fetch_histograms_recorded_) { + // Record histograms only once. + return; + } + fetch_histograms_recorded_ = true; + + base::UmaHistogramEnumeration("Signin.AccountCapabilities.FetchResult", + result); + + if (fetch_start_time_.is_null()) { + // Cannot record duration for a fetch that hasn't started. + DCHECK_EQ(result, FetchResult::kCancelled); + return; + } + base::TimeDelta duration = base::TimeTicks::Now() - fetch_start_time_; + if (result == FetchResult::kSuccess) { + base::UmaHistogramMediumTimes( + "Signin.AccountCapabilities.FetchDuration.Success", duration); + } else { + base::UmaHistogramMediumTimes( + "Signin.AccountCapabilities.FetchDuration.Failure", duration); + } +} diff --git a/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_gaia.h b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_gaia.h new file mode 100644 index 00000000000..6805a7a477c --- /dev/null +++ b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_gaia.h @@ -0,0 +1,83 @@ +// Copyright 2022 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_ACCOUNT_CAPABILITIES_FETCHER_GAIA_H_ +#define COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_FETCHER_GAIA_H_ + +#include "base/memory/raw_ptr.h" +#include "base/memory/scoped_refptr.h" +#include "base/time/time.h" +#include "components/signin/internal/identity_manager/account_capabilities_fetcher.h" +#include "components/signin/public/identity_manager/account_info.h" +#include "google_apis/gaia/gaia_oauth_client.h" +#include "google_apis/gaia/oauth2_access_token_manager.h" + +namespace network { +class SharedURLLoaderFactory; +} + +class ProfileOAuth2TokenService; +class GoogleServiceAuthError; + +class AccountCapabilitiesFetcherGaia + : public AccountCapabilitiesFetcher, + public OAuth2AccessTokenManager::Consumer, + public gaia::GaiaOAuthClient::Delegate { + public: + // These values are persisted to logs. Entries should not be renumbered and + // numeric values should never be reused. + enum class FetchResult { + kSuccess = 0, + kGetTokenFailure = 1, + kParseResponseFailure = 2, + kOAuthError = 3, + kNetworkError = 4, + kCancelled = 5, + kMaxValue = kCancelled + }; + + AccountCapabilitiesFetcherGaia( + ProfileOAuth2TokenService* token_service, + scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, + const CoreAccountInfo& account_info, + AccountCapabilitiesFetcher::OnCompleteCallback on_complete_callback); + ~AccountCapabilitiesFetcherGaia() override; + + AccountCapabilitiesFetcherGaia(const AccountCapabilitiesFetcherGaia&) = + delete; + AccountCapabilitiesFetcherGaia& operator=( + const AccountCapabilitiesFetcherGaia&) = delete; + + // OAuth2AccessTokenManager::Consumer: + void OnGetTokenSuccess( + const OAuth2AccessTokenManager::Request* request, + const OAuth2AccessTokenConsumer::TokenResponse& token_response) override; + void OnGetTokenFailure(const OAuth2AccessTokenManager::Request* request, + const GoogleServiceAuthError& error) override; + + // gaia::GaiaOAuthClient::Delegate: + void OnGetAccountCapabilitiesResponse( + std::unique_ptr<base::Value> account_capabilities) override; + void OnOAuthError() override; + void OnNetworkError(int response_code) override; + + protected: + // AccountCapabilitiesFetcher: + void StartImpl() override; + + private: + void RecordFetchResultAndDuration(FetchResult result); + + raw_ptr<ProfileOAuth2TokenService> token_service_; + scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_; + + std::unique_ptr<OAuth2AccessTokenManager::Request> login_token_request_; + std::unique_ptr<gaia::GaiaOAuthClient> gaia_oauth_client_; + + // Used for metrics: + base::TimeTicks fetch_start_time_; + bool fetch_histograms_recorded_ = false; +}; + +#endif // COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_FETCHER_GAIA_H_ diff --git a/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_unittest.cc b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_unittest.cc new file mode 100644 index 00000000000..bc37ec0fb29 --- /dev/null +++ b/chromium/components/signin/internal/identity_manager/account_capabilities_fetcher_unittest.cc @@ -0,0 +1,384 @@ +// Copyright 2022 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/internal/identity_manager/account_capabilities_fetcher.h" +#include "base/notreached.h" +#include "base/strings/stringprintf.h" +#include "base/test/metrics/histogram_tester.h" +#include "base/test/mock_callback.h" +#include "base/test/task_environment.h" +#include "components/signin/internal/identity_manager/account_capabilities_constants.h" +#include "components/signin/public/identity_manager/account_capabilities.h" +#include "components/signin/public/identity_manager/account_capabilities_test_mutator.h" +#include "components/signin/public/identity_manager/account_info.h" +#include "components/signin/public/identity_manager/identity_test_utils.h" +#include "google_apis/gaia/core_account_id.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" +#include "third_party/abseil-cpp/absl/types/optional.h" + +#if BUILDFLAG(IS_ANDROID) +#include <jni.h> + +#include "base/android/jni_android.h" +#include "components/signin/internal/identity_manager/account_capabilities_fetcher_android.h" +#include "components/signin/public/android/test_support_jni_headers/AccountCapabilitiesFetcherTestUtil_jni.h" +#else +#include "base/strings/string_piece.h" +#include "base/strings/string_split.h" +#include "base/strings/string_util.h" +#include "components/prefs/testing_pref_service.h" +#include "components/signin/internal/identity_manager/account_capabilities_fetcher_gaia.h" +#include "components/signin/internal/identity_manager/fake_profile_oauth2_token_service.h" +#include "google_apis/gaia/gaia_urls.h" +#include "google_apis/gaia/oauth2_access_token_consumer.h" +#include "net/http/http_status_code.h" +#include "services/network/public/cpp/resource_request.h" +#include "services/network/public/cpp/shared_url_loader_factory.h" +#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h" +#include "services/network/test/test_url_loader_factory.h" +#include "services/network/test/test_utils.h" +#endif // BUILDFLAG(IS_ANDROID) + +namespace { + +using ::testing::_; +using ::testing::Eq; + +CoreAccountInfo GetTestAccountInfoByEmail(const std::string& email) { + CoreAccountInfo result; + result.email = email; + result.gaia = signin::GetTestGaiaIdForEmail(email); + result.account_id = CoreAccountId::FromGaiaId(result.gaia); + return result; +} + +#if BUILDFLAG(IS_ANDROID) +class TestSupportAndroid { + public: + TestSupportAndroid() { + JNIEnv* env = base::android::AttachCurrentThread(); + base::android::ScopedJavaLocalRef<jobject> java_ref = + signin::Java_AccountCapabilitiesFetcherTestUtil_Constructor(env); + java_test_util_ref_.Reset(env, java_ref.obj()); + } + + ~TestSupportAndroid() { + JNIEnv* env = base::android::AttachCurrentThread(); + signin::Java_AccountCapabilitiesFetcherTestUtil_destroy( + env, java_test_util_ref_); + } + + void AddAccount(const CoreAccountInfo& account_info) { + JNIEnv* env = base::android::AttachCurrentThread(); + signin::Java_AccountCapabilitiesFetcherTestUtil_expectAccount( + env, java_test_util_ref_, + ConvertToJavaCoreAccountInfo(env, account_info)); + } + + std::unique_ptr<AccountCapabilitiesFetcher> CreateFetcher( + const CoreAccountInfo& account_info, + AccountCapabilitiesFetcher::OnCompleteCallback callback) { + return std::make_unique<AccountCapabilitiesFetcherAndroid>( + account_info, std::move(callback)); + } + + void ReturnAccountCapabilitiesFetchSuccess( + const CoreAccountInfo& account_info, + bool capability_value) { + AccountCapabilities capabilities; + AccountCapabilitiesTestMutator mutator(&capabilities); + mutator.SetAllSupportedCapabilities(capability_value); + ReturnFetchResults(account_info, capabilities); + } + + void ReturnAccountCapabilitiesFetchFailure( + const CoreAccountInfo& account_info) { + // Return an empty `AccountCapabilities` object. + ReturnFetchResults(account_info, AccountCapabilities()); + } + + void SimulateIssueAccessTokenPersistentError( + const CoreAccountInfo& account_info) { + NOTREACHED(); + } + + private: + void ReturnFetchResults(const CoreAccountInfo& account_info, + const AccountCapabilities& capabilities) { + JNIEnv* env = base::android::AttachCurrentThread(); + signin::Java_AccountCapabilitiesFetcherTestUtil_returnCapabilities( + env, java_test_util_ref_, + ConvertToJavaCoreAccountInfo(env, account_info), + capabilities.ConvertToJavaAccountCapabilities(env)); + } + + base::android::ScopedJavaGlobalRef<jobject> java_test_util_ref_; +}; + +using TestSupport = TestSupportAndroid; +#else +using TokenResponseBuilder = OAuth2AccessTokenConsumer::TokenResponse::Builder; + +const char kAccountCapabilitiesResponseFormat[] = + R"({"accountCapabilities": [%s]})"; + +const char kSingleCapabilitiyResponseFormat[] = + R"({"name": "%s", "booleanValue": %s})"; + +const char kCapabilityParamName[] = "names="; + +std::string GenerateValidAccountCapabilitiesResponse(bool capability_value) { + std::vector<std::string> dict_array; + for (const std::string& name : + AccountCapabilitiesTestMutator::GetSupportedAccountCapabilityNames()) { + dict_array.push_back( + base::StringPrintf(kSingleCapabilitiyResponseFormat, name.c_str(), + capability_value ? "true" : "false")); + } + return base::StringPrintf(kAccountCapabilitiesResponseFormat, + base::JoinString(dict_array, ",").c_str()); +} + +void VerifyAccountCapabilitiesRequest(const network::ResourceRequest& request) { + EXPECT_EQ(request.method, "POST"); + base::StringPiece request_string = request.request_body->elements() + ->at(0) + .As<network::DataElementBytes>() + .AsStringPiece(); + // The request body should look like: + // "names=Name1&names=Name2&names=Name3" + std::vector<std::string> requested_capabilities = base::SplitString( + request_string, "&", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL); + for (auto& name : requested_capabilities) { + EXPECT_TRUE(base::StartsWith(name, kCapabilityParamName)); + name = name.substr(std::strlen(kCapabilityParamName)); + } + EXPECT_THAT(requested_capabilities, + ::testing::ContainerEq(AccountCapabilitiesTestMutator:: + GetSupportedAccountCapabilityNames())); +} +class TestSupportGaia { + public: + TestSupportGaia() : fake_oauth2_token_service_(&pref_service_) { + ProfileOAuth2TokenService::RegisterProfilePrefs(pref_service_.registry()); + } + + void AddAccount(const CoreAccountInfo& account_info) { + const CoreAccountId& account_id = account_info.account_id; + fake_oauth2_token_service_.UpdateCredentials( + account_id, base::StringPrintf("fake-refresh-token-%s", + account_id.ToString().c_str())); + } + + std::unique_ptr<AccountCapabilitiesFetcher> CreateFetcher( + const CoreAccountInfo& account_info, + AccountCapabilitiesFetcher::OnCompleteCallback callback) { + return std::make_unique<AccountCapabilitiesFetcherGaia>( + &fake_oauth2_token_service_, + test_url_loader_factory_.GetSafeWeakWrapper(), account_info, + std::move(callback)); + } + + void ReturnAccountCapabilitiesFetchSuccess( + const CoreAccountInfo& account_info, + bool capability_value) { + IssueAccessToken(account_info.account_id); + ReturnFetchResults( + GaiaUrls::GetInstance()->account_capabilities_url(), net::HTTP_OK, + GenerateValidAccountCapabilitiesResponse(capability_value)); + } + + void ReturnAccountCapabilitiesFetchFailure( + const CoreAccountInfo& account_info) { + IssueAccessToken(account_info.account_id); + ReturnFetchResults(GaiaUrls::GetInstance()->account_capabilities_url(), + net::HTTP_BAD_REQUEST, std::string()); + } + + void SimulateIssueAccessTokenPersistentError( + const CoreAccountInfo& account_info) { + fake_oauth2_token_service_.IssueErrorForAllPendingRequestsForAccount( + account_info.account_id, + GoogleServiceAuthError( + GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS)); + } + + private: + void ReturnFetchResults(const GURL& url, + net::HttpStatusCode response_code, + const std::string& response_string) { + EXPECT_TRUE(test_url_loader_factory_.IsPending(url.spec())); + + // It's possible for multiple requests to be pending. Respond to all of + // them. + while (test_url_loader_factory_.IsPending(url.spec())) { + VerifyAccountCapabilitiesRequest( + test_url_loader_factory_.GetPendingRequest(/*index=*/0)->request); + test_url_loader_factory_.SimulateResponseForPendingRequest( + url, network::URLLoaderCompletionStatus(net::OK), + network::CreateURLResponseHead(response_code), response_string, + network::TestURLLoaderFactory::kMostRecentMatch); + } + } + + void IssueAccessToken(const CoreAccountId& account_id) { + fake_oauth2_token_service_.IssueAllTokensForAccount( + account_id, TokenResponseBuilder() + .WithAccessToken(base::StringPrintf( + "access_token-%s", account_id.ToString().c_str())) + .WithExpirationTime(base::Time::Max()) + .build()); + } + + TestingPrefServiceSimple pref_service_; + FakeProfileOAuth2TokenService fake_oauth2_token_service_; + network::TestURLLoaderFactory test_url_loader_factory_; +}; + +using TestSupport = TestSupportGaia; +#endif + +} // namespace + +class AccountCapabilitiesFetcherTest : public ::testing::Test { + public: + void SetUp() override { test_support_.AddAccount(account_info()); } + + std::unique_ptr<AccountCapabilitiesFetcher> CreateFetcher( + AccountCapabilitiesFetcher::OnCompleteCallback callback) { + return test_support_.CreateFetcher(account_info(), std::move(callback)); + } + + void ReturnAccountCapabilitiesFetchSuccess(bool capability_value) { + test_support_.ReturnAccountCapabilitiesFetchSuccess(account_info(), + capability_value); + } + + void ReturnAccountCapabilitiesFetchFailure() { + test_support_.ReturnAccountCapabilitiesFetchFailure(account_info()); + } + + void SimulateIssueAccessTokenPersistentError() { + test_support_.SimulateIssueAccessTokenPersistentError(account_info()); + } + + const CoreAccountInfo& account_info() { return account_info_; } + const CoreAccountId& account_id() { return account_info_.account_id; } + + private: + base::test::TaskEnvironment task_environment_; + CoreAccountInfo account_info_ = GetTestAccountInfoByEmail("test@gmail.com"); + TestSupport test_support_; +}; + +TEST_F(AccountCapabilitiesFetcherTest, Success_True) { + base::MockCallback<AccountCapabilitiesFetcher::OnCompleteCallback> callback; + std::unique_ptr<AccountCapabilitiesFetcher> fetcher = + CreateFetcher(callback.Get()); + AccountCapabilities expected_capabilities; + AccountCapabilitiesTestMutator mutator(&expected_capabilities); + mutator.SetAllSupportedCapabilities(true); + base::HistogramTester tester; + + fetcher->Start(); + EXPECT_CALL(callback, + Run(account_id(), ::testing::Optional(expected_capabilities))); + ReturnAccountCapabilitiesFetchSuccess(true); + +#if !BUILDFLAG(IS_ANDROID) + tester.ExpectTotalCount("Signin.AccountCapabilities.FetchDuration.Success", + 1); + tester.ExpectTotalCount("Signin.AccountCapabilities.FetchDuration.Failure", + 0); + tester.ExpectUniqueSample( + "Signin.AccountCapabilities.FetchResult", + AccountCapabilitiesFetcherGaia::FetchResult::kSuccess, 1); +#endif // !BUILDFLAG(IS_ANDROID) +} + +TEST_F(AccountCapabilitiesFetcherTest, Success_False) { + base::MockCallback<AccountCapabilitiesFetcher::OnCompleteCallback> callback; + std::unique_ptr<AccountCapabilitiesFetcher> fetcher = + CreateFetcher(callback.Get()); + AccountCapabilities expected_capabilities; + AccountCapabilitiesTestMutator mutator(&expected_capabilities); + mutator.SetAllSupportedCapabilities(false); + + fetcher->Start(); + EXPECT_CALL(callback, + Run(account_id(), ::testing::Optional(expected_capabilities))); + ReturnAccountCapabilitiesFetchSuccess(false); +} + +TEST_F(AccountCapabilitiesFetcherTest, FetchFailure) { + base::MockCallback<AccountCapabilitiesFetcher::OnCompleteCallback> callback; + std::unique_ptr<AccountCapabilitiesFetcher> fetcher = + CreateFetcher(callback.Get()); + base::HistogramTester tester; + + fetcher->Start(); + absl::optional<AccountCapabilities> expected_capabilities; +#if BUILDFLAG(IS_ANDROID) + // Android never returns absl::nullopt even if the fetcher has failed to get + // all capabilities. + expected_capabilities = AccountCapabilities(); +#endif + EXPECT_CALL(callback, Run(account_id(), Eq(expected_capabilities))); + ReturnAccountCapabilitiesFetchFailure(); + +#if !BUILDFLAG(IS_ANDROID) + tester.ExpectTotalCount("Signin.AccountCapabilities.FetchDuration.Success", + 0); + tester.ExpectTotalCount("Signin.AccountCapabilities.FetchDuration.Failure", + 1); + tester.ExpectUniqueSample( + "Signin.AccountCapabilities.FetchResult", + AccountCapabilitiesFetcherGaia::FetchResult::kOAuthError, 1); +#endif // !BUILDFLAG(IS_ANDROID) +} + +// Exclude Android because `AccountCapabilitiesFetcherAndroid` doesn't request +// an access token. +#if !BUILDFLAG(IS_ANDROID) +TEST_F(AccountCapabilitiesFetcherTest, TokenFailure) { + base::MockCallback<AccountCapabilitiesFetcher::OnCompleteCallback> callback; + std::unique_ptr<AccountCapabilitiesFetcher> fetcher = + CreateFetcher(callback.Get()); + base::HistogramTester tester; + + fetcher->Start(); + EXPECT_CALL(callback, Run(account_id(), Eq(absl::nullopt))); + SimulateIssueAccessTokenPersistentError(); + + tester.ExpectTotalCount("Signin.AccountCapabilities.FetchDuration.Success", + 0); + tester.ExpectTotalCount("Signin.AccountCapabilities.FetchDuration.Failure", + 1); + tester.ExpectUniqueSample( + "Signin.AccountCapabilities.FetchResult", + AccountCapabilitiesFetcherGaia::FetchResult::kGetTokenFailure, 1); +} +#endif // !BUILDFLAG(IS_ANDROID) + +TEST_F(AccountCapabilitiesFetcherTest, Cancelled) { + base::MockCallback<AccountCapabilitiesFetcher::OnCompleteCallback> callback; + std::unique_ptr<AccountCapabilitiesFetcher> fetcher = + CreateFetcher(callback.Get()); + base::HistogramTester tester; + + fetcher->Start(); + EXPECT_CALL(callback, Run(_, _)).Times(0); + fetcher.reset(); + +#if !BUILDFLAG(IS_ANDROID) + tester.ExpectTotalCount("Signin.AccountCapabilities.FetchDuration.Success", + 0); + tester.ExpectTotalCount("Signin.AccountCapabilities.FetchDuration.Failure", + 1); + tester.ExpectUniqueSample( + "Signin.AccountCapabilities.FetchResult", + AccountCapabilitiesFetcherGaia::FetchResult::kCancelled, 1); +#endif // !BUILDFLAG(IS_ANDROID) +} diff --git a/chromium/components/signin/internal/identity_manager/account_capabilities_list.h b/chromium/components/signin/internal/identity_manager/account_capabilities_list.h new file mode 100644 index 00000000000..9f243408a2a --- /dev/null +++ b/chromium/components/signin/internal/identity_manager/account_capabilities_list.h @@ -0,0 +1,33 @@ +// Copyright 2022 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. + +// This file intentionally does not have header guards, it's included +// inside a macro to generate a list of constants. The following line silences a +// presubmit and Tricium warning that would otherwise be triggered by this: +// no-include-guard-because-multiply-included +// NOLINT(build/header_guard) + +// This is the list of account capabilities identifiers and their values. For +// the constant declarations, include the file +// "account_capabilities_constants.h". + +// Here we define the values using a macro ACCOUNT_CAPABILITY, so it can be +// expanded differently in some places. The macro has the following signature: +// ACCOUNT_CAPABILITY(cpp_label, java_label, name). + +ACCOUNT_CAPABILITY(kIsSubjectToParentalControlsCapabilityName, + IS_SUBJECT_TO_PARENTAL_CONTROLS_CAPABILITY_NAME, + "accountcapabilities/guydolldmfya") + +ACCOUNT_CAPABILITY(kCanOfferExtendedChromeSyncPromosCapabilityName, + CAN_OFFER_EXTENDED_CHROME_SYNC_PROMOS_CAPABILITY_NAME, + "accountcapabilities/gi2tklldmfya") + +ACCOUNT_CAPABILITY(kCanRunChromePrivacySandboxTrialsCapabilityName, + CAN_RUN_CHROME_PRIVACY_SANDBOX_TRIALS_CAPABILITY_NAME, + "accountcapabilities/gu2dqlldmfya") + +ACCOUNT_CAPABILITY(kCanStopParentalSupervisionCapabilityName, + CAN_STOP_PARENTAL_SUPERVISION_CAPABILITY_NAME, + "accountcapabilities/guzdslldmfya") diff --git a/chromium/components/signin/internal/identity_manager/account_fetcher_service.cc b/chromium/components/signin/internal/identity_manager/account_fetcher_service.cc index 748b82f641a..ada0ccc1526 100644 --- a/chromium/components/signin/internal/identity_manager/account_fetcher_service.cc +++ b/chromium/components/signin/internal/identity_manager/account_fetcher_service.cc @@ -20,6 +20,7 @@ #include "components/prefs/pref_registry_simple.h" #include "components/prefs/pref_service.h" #include "components/signin/internal/identity_manager/account_capabilities_fetcher.h" +#include "components/signin/internal/identity_manager/account_capabilities_fetcher_factory.h" #include "components/signin/internal/identity_manager/account_info_fetcher.h" #include "components/signin/internal/identity_manager/account_tracker_service.h" #include "components/signin/internal/identity_manager/profile_oauth2_token_service.h" @@ -27,6 +28,7 @@ #include "components/signin/public/base/signin_client.h" #include "components/signin/public/base/signin_switches.h" #include "components/signin/public/identity_manager/account_capabilities.h" +#include "components/signin/public/identity_manager/account_info.h" #include "net/http/http_status_code.h" #include "services/network/public/cpp/shared_url_loader_factory.h" @@ -74,7 +76,9 @@ void AccountFetcherService::Initialize( SigninClient* signin_client, ProfileOAuth2TokenService* token_service, AccountTrackerService* account_tracker_service, - std::unique_ptr<image_fetcher::ImageDecoder> image_decoder) { + std::unique_ptr<image_fetcher::ImageDecoder> image_decoder, + std::unique_ptr<AccountCapabilitiesFetcherFactory> + account_capabilities_fetcher_factory) { DCHECK(signin_client); DCHECK(!signin_client_); signin_client_ = signin_client; @@ -88,6 +92,11 @@ void AccountFetcherService::Initialize( DCHECK(image_decoder); DCHECK(!image_decoder_); image_decoder_ = std::move(image_decoder); + DCHECK(!account_capabilities_fetcher_factory_); + DCHECK(account_capabilities_fetcher_factory); + account_capabilities_fetcher_factory_ = + std::move(account_capabilities_fetcher_factory); + repeating_timer_ = std::make_unique<signin::PersistentRepeatingTimer>( signin_client_->GetPrefs(), AccountFetcherService::kLastUpdatePref, kRefreshFromTokenServiceDelay, @@ -238,28 +247,28 @@ void AccountFetcherService::SetIsChildAccount(const CoreAccountId& account_id, } #endif -bool AccountFetcherService::IsAccountCapabilitiesFetcherEnabled() { +bool AccountFetcherService::IsAccountCapabilitiesFetchingEnabled() { if (enable_account_capabilities_fetcher_for_test_) return true; -#if BUILDFLAG(IS_CHROMEOS_ASH) - return true; -#else - return false; -#endif + return base::FeatureList::IsEnabled( + switches::kEnableFetchingAccountCapabilities); } void AccountFetcherService::StartFetchingAccountCapabilities( - const CoreAccountId& account_id) { + const CoreAccountInfo& account_info) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK(network_fetches_enabled_); std::unique_ptr<AccountCapabilitiesFetcher>& request = - account_capabilities_requests_[account_id]; + account_capabilities_requests_[account_info.account_id]; if (!request) { - request = std::make_unique<AccountCapabilitiesFetcher>( - token_service_, signin_client_->GetURLLoaderFactory(), this, - account_id); + request = + account_capabilities_fetcher_factory_->CreateAccountCapabilitiesFetcher( + account_info, + base::BindOnce( + &AccountFetcherService::OnAccountCapabilitiesFetchComplete, + base::Unretained(this))); request->Start(); } } @@ -273,8 +282,8 @@ void AccountFetcherService::RefreshAccountInfo(const CoreAccountId& account_id, if ((!only_fetch_if_invalid || !info.capabilities.AreAllCapabilitiesKnown()) && - IsAccountCapabilitiesFetcherEnabled()) { - StartFetchingAccountCapabilities(account_id); + IsAccountCapabilitiesFetchingEnabled()) { + StartFetchingAccountCapabilities(info); } // |only_fetch_if_invalid| is false when the service is due for a timed @@ -381,17 +390,13 @@ void AccountFetcherService::OnUserInfoFetchFailure( user_info_requests_.erase(account_id); } -void AccountFetcherService::OnAccountCapabilitiesFetchSuccess( +void AccountFetcherService::OnAccountCapabilitiesFetchComplete( const CoreAccountId& account_id, - const AccountCapabilities& account_capabilities) { - account_tracker_service_->SetAccountCapabilities(account_id, - account_capabilities); - account_capabilities_requests_.erase(account_id); -} - -void AccountFetcherService::OnAccountCapabilitiesFetchFailure( - const CoreAccountId& account_id) { - VLOG(1) << "Failed to get AccountCapabilities for " << account_id; + const absl::optional<AccountCapabilities>& account_capabilities) { + if (account_capabilities.has_value()) { + account_tracker_service_->SetAccountCapabilities(account_id, + *account_capabilities); + } // |account_id| is owned by the request. Cannot be used after this line. account_capabilities_requests_.erase(account_id); } diff --git a/chromium/components/signin/internal/identity_manager/account_fetcher_service.h b/chromium/components/signin/internal/identity_manager/account_fetcher_service.h index 6e579a3f287..39835aeefe7 100644 --- a/chromium/components/signin/internal/identity_manager/account_fetcher_service.h +++ b/chromium/components/signin/internal/identity_manager/account_fetcher_service.h @@ -20,14 +20,17 @@ #include "build/build_config.h" #include "components/signin/internal/identity_manager/profile_oauth2_token_service_observer.h" #include "components/signin/public/base/persistent_repeating_timer.h" +#include "third_party/abseil-cpp/absl/types/optional.h" class AccountCapabilities; class AccountCapabilitiesFetcher; +class AccountCapabilitiesFetcherFactory; class AccountInfoFetcher; class AccountTrackerService; class ProfileOAuth2TokenService; class PrefRegistrySimple; class SigninClient; +struct CoreAccountInfo; #if BUILDFLAG(IS_ANDROID) class ChildAccountInfoFetcherAndroid; @@ -70,7 +73,9 @@ class AccountFetcherService : public ProfileOAuth2TokenServiceObserver { void Initialize(SigninClient* signin_client, ProfileOAuth2TokenService* token_service, AccountTrackerService* account_tracker_service, - std::unique_ptr<image_fetcher::ImageDecoder> image_decoder); + std::unique_ptr<image_fetcher::ImageDecoder> image_decoder, + std::unique_ptr<AccountCapabilitiesFetcherFactory> + account_capabilities_fetcher_factory); // Indicates if all user information has been fetched. If the result is false, // there are still unfininshed fetchers. @@ -117,7 +122,6 @@ class AccountFetcherService : public ProfileOAuth2TokenServiceObserver { private: friend class AccountInfoFetcher; - friend class AccountCapabilitiesFetcher; void RefreshAllAccountInfo(bool only_fetch_if_invalid); @@ -141,8 +145,8 @@ class AccountFetcherService : public ProfileOAuth2TokenServiceObserver { void ResetChildInfo(); #endif - bool IsAccountCapabilitiesFetcherEnabled(); - void StartFetchingAccountCapabilities(const CoreAccountId& account_id); + bool IsAccountCapabilitiesFetchingEnabled(); + void StartFetchingAccountCapabilities(const CoreAccountInfo& account_info); // Refreshes the AccountInfo associated with |account_id|. void RefreshAccountInfo(const CoreAccountId& account_id, @@ -154,10 +158,9 @@ class AccountFetcherService : public ProfileOAuth2TokenServiceObserver { void OnUserInfoFetchFailure(const CoreAccountId& account_id); // Called by AccountCapabilitiesFetcher. - void OnAccountCapabilitiesFetchSuccess( + void OnAccountCapabilitiesFetchComplete( const CoreAccountId& account_id, - const AccountCapabilities& account_capabilities); - void OnAccountCapabilitiesFetchFailure(const CoreAccountId& account_id); + const absl::optional<AccountCapabilities>& account_capabilities); image_fetcher::ImageFetcherImpl* GetOrCreateImageFetcher(); @@ -189,6 +192,8 @@ class AccountFetcherService : public ProfileOAuth2TokenServiceObserver { std::unordered_map<CoreAccountId, std::unique_ptr<AccountInfoFetcher>> user_info_requests_; + std::unique_ptr<AccountCapabilitiesFetcherFactory> + account_capabilities_fetcher_factory_; std::map<CoreAccountId, std::unique_ptr<AccountCapabilitiesFetcher>> account_capabilities_requests_; diff --git a/chromium/components/signin/internal/identity_manager/account_info_util.cc b/chromium/components/signin/internal/identity_manager/account_info_util.cc index 07e2c382712..2e9a8e1e406 100644 --- a/chromium/components/signin/internal/identity_manager/account_info_util.cc +++ b/chromium/components/signin/internal/identity_manager/account_info_util.cc @@ -110,10 +110,13 @@ absl::optional<AccountCapabilities> AccountCapabilitiesFromValue( // 2. Fill AccountCapabilities fields based on the mapping. AccountCapabilities capabilities; - auto it = boolean_capabilities.find( - kCanOfferExtendedChromeSyncPromosCapabilityName); - if (it != boolean_capabilities.end()) - capabilities.set_can_offer_extended_chrome_sync_promos(it->second); + for (const std::string& name : + AccountCapabilities::GetSupportedAccountCapabilityNames()) { + auto it = boolean_capabilities.find(name); + if (it != boolean_capabilities.end()) { + capabilities.capabilities_map_[name] = it->second; + } + } return capabilities; } 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 c48334c0389..49a1dbad395 100644 --- a/chromium/components/signin/internal/identity_manager/account_tracker_service.cc +++ b/chromium/components/signin/internal/identity_manager/account_tracker_service.cc @@ -5,6 +5,7 @@ #include "components/signin/internal/identity_manager/account_tracker_service.h" #include <stddef.h> +#include <string> #include "base/bind.h" #include "base/callback.h" @@ -14,11 +15,12 @@ #include "base/files/file_util.h" #include "base/logging.h" #include "base/memory/ptr_util.h" +#include "base/memory/ref_counted_memory.h" #include "base/metrics/histogram_macros.h" +#include "base/strings/strcat.h" #include "base/strings/string_piece.h" #include "base/strings/string_split.h" #include "base/strings/utf_string_conversions.h" -#include "base/task/post_task.h" #include "base/task/task_traits.h" #include "base/task/thread_pool.h" #include "base/threading/scoped_blocking_call.h" @@ -59,6 +61,12 @@ const char kAdvancedProtectionAccountStatusPath[] = // It was replaced by kAccountChildAttributePath. const char kDeprecatedChildStatusPath[] = "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 +// pref name based on the Capabilities service key. +const char kDeprecatedCanOfferExtendedChromeSyncPromosPrefPath[] = + "accountcapabilities.can_offer_extended_chrome_sync_promos"; + // Account folders used for storing account related data at disk. const base::FilePath::CharType kAccountsFolder[] = FILE_PATH_LITERAL("Accounts"); @@ -107,18 +115,22 @@ void RemoveImage(const base::FilePath& image_path) { LOG(ERROR) << "Failed to delete image."; } -void SetAccountCapabilityPath(base::Value* value, - base::StringPiece path, - signin::Tribool state) { - value->SetIntPath(path, static_cast<int>(state)); +// Converts the capability service name into a nested Chrome pref path. +std::string GetCapabilityPrefPath(base::StringPiece capability_name) { + return base::StrCat({"accountcapabilities.", capability_name}); +} + +void SetAccountCapabilityState(base::Value* value, + base::StringPiece capability_name, + signin::Tribool state) { + value->SetIntPath(GetCapabilityPrefPath(capability_name), + static_cast<int>(state)); } -signin::Tribool FindAccountCapabilityPath(const base::Value& value, - base::StringPiece path) { - absl::optional<int> capability = value.FindIntPath(path); - if (!capability.has_value()) +signin::Tribool ParseTribool(absl::optional<int> int_value) { + if (!int_value.has_value()) return signin::Tribool::kUnknown; - switch (capability.value()) { + switch (int_value.value()) { case static_cast<int>(signin::Tribool::kTrue): return signin::Tribool::kTrue; case static_cast<int>(signin::Tribool::kFalse): @@ -126,12 +138,18 @@ signin::Tribool FindAccountCapabilityPath(const base::Value& value, case static_cast<int>(signin::Tribool::kUnknown): return signin::Tribool::kUnknown; default: - LOG(ERROR) << "Unexpected capability value (" << capability.value() - << ") for path: " << path; + LOG(ERROR) << "Unexpected tribool value (" << int_value.value() << ")"; return signin::Tribool::kUnknown; } } +signin::Tribool FindAccountCapabilityState(const base::Value& value, + base::StringPiece name) { + absl::optional<int> capability = + value.FindIntPath(GetCapabilityPrefPath(name)); + return ParseTribool(capability); +} + void GetString(const base::Value& dict, base::StringPiece key, std::string& result) { @@ -598,12 +616,13 @@ void AccountTrackerService::LoadFromPrefs() { ListPrefUpdate update(pref_service_, prefs::kAccountInfo); base::Value* update_dict = &update->GetListDeprecated()[i]; DCHECK(update_dict->is_dict()); - SetAccountCapabilityPath(update_dict, kAccountChildAttributePath, - account_info.is_child_account); + update_dict->SetIntPath( + kAccountChildAttributePath, + static_cast<int>(account_info.is_child_account)); update_dict->RemoveKey(kDeprecatedChildStatusPath); } else { account_info.is_child_account = - FindAccountCapabilityPath(dict, kAccountChildAttributePath); + ParseTribool(dict.FindIntPath(kAccountChildAttributePath)); } absl::optional<bool> is_under_advanced_protection = @@ -613,18 +632,33 @@ void AccountTrackerService::LoadFromPrefs() { is_under_advanced_protection.value(); } - switch (FindAccountCapabilityPath( - dict, kCanOfferExtendedChromeSyncPromosCapabilityPrefsPath)) { - case signin::Tribool::kUnknown: - break; - case signin::Tribool::kTrue: - account_info.capabilities.set_can_offer_extended_chrome_sync_promos( - true); - break; - case signin::Tribool::kFalse: - account_info.capabilities.set_can_offer_extended_chrome_sync_promos( - false); - break; + if (absl::optional<int> can_offer_extended_chrome_sync_promos = + dict.FindIntPath( + 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()); + SetAccountCapabilityState( + update_dict, kCanOfferExtendedChromeSyncPromosCapabilityName, + ParseTribool(can_offer_extended_chrome_sync_promos)); + update_dict->RemovePath( + kDeprecatedCanOfferExtendedChromeSyncPromosPrefPath); + } + + for (const std::string& name : + AccountCapabilities::GetSupportedAccountCapabilityNames()) { + switch (FindAccountCapabilityState(dict, name)) { + case signin::Tribool::kUnknown: + account_info.capabilities.capabilities_map_.erase(name); + break; + case signin::Tribool::kTrue: + account_info.capabilities.capabilities_map_[name] = true; + break; + case signin::Tribool::kFalse: + account_info.capabilities.capabilities_map_[name] = false; + break; + } } if (!account_info.gaia.empty()) @@ -691,16 +725,19 @@ void AccountTrackerService::SaveToPrefs(const AccountInfo& account_info) { dict->SetString(kAccountGivenNamePath, account_info.given_name); dict->SetString(kAccountLocalePath, account_info.locale); dict->SetString(kAccountPictureURLPath, account_info.picture_url); - SetAccountCapabilityPath(dict, kAccountChildAttributePath, - account_info.is_child_account); + 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 // picture is successufly saved to disk. Otherwise, there is no guarantee that // |kLastDownloadedImageURLWithSizePath| matches the picture on disk. - SetAccountCapabilityPath( - dict, kCanOfferExtendedChromeSyncPromosCapabilityPrefsPath, - account_info.capabilities.can_offer_extended_chrome_sync_promos()); + for (const std::string& name : + AccountCapabilities::GetSupportedAccountCapabilityNames()) { + signin::Tribool capability_state = + account_info.capabilities.GetCapabilityByName(name); + SetAccountCapabilityState(dict, name, capability_state); + } } void AccountTrackerService::RemoveFromPrefs(const AccountInfo& account_info) { 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 c1bf49a8a4d..f3e41ad21a6 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 @@ -22,14 +22,17 @@ #include "components/prefs/testing_pref_service.h" #include "components/signin/internal/identity_manager/account_capabilities_constants.h" #include "components/signin/internal/identity_manager/account_capabilities_fetcher.h" +#include "components/signin/internal/identity_manager/account_capabilities_fetcher_gaia.h" #include "components/signin/internal/identity_manager/account_fetcher_service.h" #include "components/signin/internal/identity_manager/account_tracker_service.h" +#include "components/signin/internal/identity_manager/fake_account_capabilities_fetcher_factory.h" #include "components/signin/internal/identity_manager/fake_profile_oauth2_token_service.h" #include "components/signin/public/base/avatar_icon_util.h" #include "components/signin/public/base/signin_pref_names.h" #include "components/signin/public/base/signin_switches.h" #include "components/signin/public/base/test_signin_client.h" #include "components/signin/public/identity_manager/account_capabilities.h" +#include "components/signin/public/identity_manager/account_capabilities_test_mutator.h" #include "components/signin/public/identity_manager/account_info.h" #include "components/signin/public/identity_manager/tribool.h" #include "google_apis/gaia/gaia_oauth_client.h" @@ -92,16 +95,6 @@ const char kTokenInfoIncompleteResponseFormat[] = \"hd\": \"\", \ }"; -const char kAccountCapabilitiesResponseFormat[] = - "{ \ - \"accountCapabilities\": [ \ - { \ - \"name\": \"%s\", \ - \"booleanValue\": %s \ - } \ - ] \ - }"; - enum TrackingEventType { UPDATED, REMOVED, @@ -274,10 +267,11 @@ class AccountTrackerServiceTest : public testing::Test { void CheckAccountCapabilities(AccountKey account_key, const AccountInfo& info) { - EXPECT_EQ(AccountKeyToAccountCapability(account_key) - ? signin::Tribool::kTrue - : signin::Tribool::kFalse, - info.capabilities.can_offer_extended_chrome_sync_promos()); + AccountCapabilities expected_capabilities; + AccountCapabilitiesTestMutator mutator(&expected_capabilities); + mutator.SetAllSupportedCapabilities( + AccountKeyToAccountCapability(account_key)); + EXPECT_EQ(info.capabilities, expected_capabilities); } testing::AssertionResult CheckAccountTrackerEvents( @@ -343,13 +337,6 @@ class AccountTrackerServiceTest : public testing::Test { AccountKeyToEmail(account_key).c_str()); } - std::string GenerateValidAccountCapabilitiesResponse(AccountKey account_key) { - return base::StringPrintf( - kAccountCapabilitiesResponseFormat, - kCanOfferExtendedChromeSyncPromosCapabilityName, - AccountKeyToAccountCapability(account_key) ? "true" : "false"); - } - void ReturnAccountInfoFetchSuccess(AccountKey account_key); void ReturnAccountInfoFetchSuccessIncomplete(AccountKey account_key); void ReturnAccountInfoFetchFailure(AccountKey account_key); @@ -400,9 +387,14 @@ class AccountTrackerServiceTest : public testing::Test { &AccountTrackerServiceTest::OnAccountRemoved, base::Unretained(this))); account_tracker_->Initialize(&pref_service_, std::move(path)); + auto account_capabilities_fetcher_factory = + std::make_unique<FakeAccountCapabilitiesFetcherFactory>(); + fake_account_capabilities_fetcher_factory_ = + account_capabilities_fetcher_factory.get(); account_fetcher_->Initialize( signin_client(), token_service(), account_tracker_.get(), - std::make_unique<image_fetcher::FakeImageDecoder>()); + std::make_unique<image_fetcher::FakeImageDecoder>(), + std::move(account_capabilities_fetcher_factory)); if (network_enabled) { account_fetcher_->EnableNetworkFetchesForTest(); } @@ -421,6 +413,8 @@ class AccountTrackerServiceTest : public testing::Test { FakeProfileOAuth2TokenService fake_oauth2_token_service_; std::unique_ptr<AccountFetcherService> account_fetcher_; std::unique_ptr<AccountTrackerService> account_tracker_; + FakeAccountCapabilitiesFetcherFactory* + fake_account_capabilities_fetcher_factory_ = nullptr; std::vector<TrackingEvent> account_tracker_events_; bool force_account_id_to_email_for_legacy_tests_ = false; }; @@ -477,16 +471,19 @@ void AccountTrackerServiceTest::ReturnAccountImageFetchFailure( void AccountTrackerServiceTest::ReturnAccountCapabilitiesFetchSuccess( AccountKey account_key) { IssueAccessToken(account_key); - ReturnFetchResults(GaiaUrls::GetInstance()->account_capabilities_url(), - net::HTTP_OK, - GenerateValidAccountCapabilitiesResponse(account_key)); + AccountCapabilities capabilities; + AccountCapabilitiesTestMutator mutator(&capabilities); + mutator.SetAllSupportedCapabilities( + AccountKeyToAccountCapability(account_key)); + fake_account_capabilities_fetcher_factory_->CompleteAccountCapabilitiesFetch( + AccountKeyToAccountId(account_key), capabilities); } void AccountTrackerServiceTest::ReturnAccountCapabilitiesFetchFailure( AccountKey account_key) { IssueAccessToken(account_key); - ReturnFetchResults(GaiaUrls::GetInstance()->account_capabilities_url(), - net::HTTP_BAD_REQUEST, std::string()); + fake_account_capabilities_fetcher_factory_->CompleteAccountCapabilitiesFetch( + AccountKeyToAccountId(account_key), absl::nullopt); } TEST_F(AccountTrackerServiceTest, Basic) {} @@ -597,7 +594,6 @@ TEST_F(AccountTrackerServiceTest, TokenAvailable_AccountCapabilitiesSuccess) { SimulateTokenAvailable(kAccountKeyAlpha); EXPECT_FALSE(account_fetcher()->AreAllAccountCapabilitiesFetched()); - base::HistogramTester tester; // AccountUpdated notification requires account's gaia to be known. // Set account's user info first to receive an UPDATED event when capabilities // are fetched. @@ -614,21 +610,12 @@ TEST_F(AccountTrackerServiceTest, TokenAvailable_AccountCapabilitiesSuccess) { AccountInfo account_info = account_tracker()->GetAccountInfo( AccountKeyToAccountId(kAccountKeyAlpha)); CheckAccountCapabilities(kAccountKeyAlpha, account_info); - - tester.ExpectTotalCount("Signin.AccountCapabilities.FetchDuration.Success", - 1); - tester.ExpectTotalCount("Signin.AccountCapabilities.FetchDuration.Failure", - 0); - tester.ExpectUniqueSample("Signin.AccountCapabilities.FetchResult", - AccountCapabilitiesFetcher::FetchResult::kSuccess, - 1); } TEST_F(AccountTrackerServiceTest, TokenAvailable_AccountCapabilitiesFailed) { SimulateTokenAvailable(kAccountKeyAlpha); EXPECT_FALSE(account_fetcher()->AreAllAccountCapabilitiesFetched()); - base::HistogramTester tester; ReturnAccountInfoFetchSuccess(kAccountKeyAlpha); ClearAccountTrackerEvents(); @@ -638,45 +625,12 @@ TEST_F(AccountTrackerServiceTest, TokenAvailable_AccountCapabilitiesFailed) { AccountInfo account_info = account_tracker()->GetAccountInfo( AccountKeyToAccountId(kAccountKeyAlpha)); EXPECT_FALSE(account_info.capabilities.AreAllCapabilitiesKnown()); - - tester.ExpectTotalCount("Signin.AccountCapabilities.FetchDuration.Success", - 0); - tester.ExpectTotalCount("Signin.AccountCapabilities.FetchDuration.Failure", - 1); - tester.ExpectUniqueSample( - "Signin.AccountCapabilities.FetchResult", - AccountCapabilitiesFetcher::FetchResult::kOAuthError, 1); -} - -TEST_F(AccountTrackerServiceTest, - TokenAvailable_AccountCapabilitiesTokenFailure) { - SimulateTokenAvailable(kAccountKeyAlpha); - EXPECT_FALSE(account_fetcher()->AreAllAccountCapabilitiesFetched()); - - base::HistogramTester tester; - - SimulateIssueAccessTokenPersistentError(kAccountKeyAlpha); - EXPECT_TRUE(account_fetcher()->AreAllAccountCapabilitiesFetched()); - EXPECT_TRUE(CheckAccountTrackerEvents({})); - AccountInfo account_info = account_tracker()->GetAccountInfo( - AccountKeyToAccountId(kAccountKeyAlpha)); - EXPECT_FALSE(account_info.capabilities.AreAllCapabilitiesKnown()); - - tester.ExpectTotalCount("Signin.AccountCapabilities.FetchDuration.Success", - 0); - tester.ExpectTotalCount("Signin.AccountCapabilities.FetchDuration.Failure", - 1); - tester.ExpectUniqueSample( - "Signin.AccountCapabilities.FetchResult", - AccountCapabilitiesFetcher::FetchResult::kGetTokenFailure, 1); } TEST_F(AccountTrackerServiceTest, TokenAvailable_AccountCapabilitiesCancelled) { SimulateTokenAvailable(kAccountKeyAlpha); EXPECT_FALSE(account_fetcher()->AreAllAccountCapabilitiesFetched()); - base::HistogramTester tester; - // Issue an access token first to not get the `kGetTokenFailure` error. IssueAccessToken(kAccountKeyAlpha); // Revoking a token will cancel an ongoing request. @@ -686,14 +640,6 @@ TEST_F(AccountTrackerServiceTest, TokenAvailable_AccountCapabilitiesCancelled) { AccountInfo account_info = account_tracker()->GetAccountInfo( AccountKeyToAccountId(kAccountKeyAlpha)); EXPECT_FALSE(account_info.capabilities.AreAllCapabilitiesKnown()); - - tester.ExpectTotalCount("Signin.AccountCapabilities.FetchDuration.Success", - 0); - tester.ExpectTotalCount("Signin.AccountCapabilities.FetchDuration.Failure", - 1); - tester.ExpectUniqueSample("Signin.AccountCapabilities.FetchResult", - AccountCapabilitiesFetcher::FetchResult::kCancelled, - 1); } TEST_F(AccountTrackerServiceTest, @@ -707,10 +653,12 @@ TEST_F(AccountTrackerServiceTest, CheckAccountCapabilities(kAccountKeyAlpha, account_info); } -#if !BUILDFLAG(IS_CHROMEOS_ASH) TEST_F(AccountTrackerServiceTest, TokenAvailable_AccountCapabilitiesFetcherDisabled) { account_fetcher()->EnableAccountCapabilitiesFetcherForTest(false); + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndDisableFeature( + switches::kEnableFetchingAccountCapabilities); SimulateTokenAvailable(kAccountKeyAlpha); EXPECT_TRUE(account_fetcher()->AreAllAccountCapabilitiesFetched()); EXPECT_TRUE(CheckAccountTrackerEvents({})); @@ -718,7 +666,27 @@ TEST_F(AccountTrackerServiceTest, AccountKeyToAccountId(kAccountKeyAlpha)); EXPECT_FALSE(account_info.capabilities.AreAllCapabilitiesKnown()); } -#endif + +// iOS doesn't support the kEnableFetchingAccountCapabilities feature. +// TODO(https://crbug.com/1305191): enable these tests on iOS once the feature +// is supported. +#if !BUILDFLAG(IS_IOS) +TEST_F(AccountTrackerServiceTest, + TokenAvailable_AccountCapabilitiesFetcherEnabled) { + account_fetcher()->EnableAccountCapabilitiesFetcherForTest(false); + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndEnableFeature( + switches::kEnableFetchingAccountCapabilities); + SimulateTokenAvailable(kAccountKeyAlpha); + EXPECT_FALSE(account_fetcher()->AreAllAccountCapabilitiesFetched()); + + ReturnAccountInfoFetchSuccess(kAccountKeyAlpha); + ClearAccountTrackerEvents(); + + ReturnAccountCapabilitiesFetchSuccess(kAccountKeyAlpha); + EXPECT_TRUE(account_fetcher()->AreAllAccountCapabilitiesFetched()); +} +#endif // !BUILDFLAG(IS_IOS) TEST_F(AccountTrackerServiceTest, TokenAvailableTwice_UserInfoOnce) { SimulateTokenAvailable(kAccountKeyAlpha); @@ -1674,6 +1642,22 @@ TEST_F(AccountTrackerServiceTest, RemoveAccountBeforeImageFetchDone) { })); } +TEST_F(AccountTrackerServiceTest, RemoveAccountBeforeCapabilitiesFetched) { + SimulateTokenAvailable(kAccountKeyAlpha); + + ReturnAccountInfoFetchSuccess(kAccountKeyAlpha); + SimulateTokenRevoked(kAccountKeyAlpha); + EXPECT_TRUE(account_fetcher()->AreAllAccountCapabilitiesFetched()); + + // Re-add the same account and verify that capabilities can be fetched + // successfully. + SimulateTokenAvailable(kAccountKeyAlpha); + + ReturnAccountInfoFetchSuccess(kAccountKeyAlpha); + ReturnAccountCapabilitiesFetchSuccess(kAccountKeyAlpha); + EXPECT_TRUE(account_fetcher()->AreAllAccountCapabilitiesFetched()); +} + #if !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_IOS) TEST_F(AccountTrackerServiceTest, AdvancedProtectionAccountBasic) { SimulateTokenAvailable(kAccountKeyAdvancedProtection); @@ -1760,3 +1744,60 @@ TEST_F(AccountTrackerServiceTest, CountOfLoadedAccounts_TwoAccountsOneInvalid) { tester.GetAllSamples("Signin.AccountTracker.CountOfLoadedAccounts"), testing::ElementsAre(base::Bucket(1, 1))); } + +TEST_F(AccountTrackerServiceTest, CapabilityPrefNameMigration) { +#if BUILDFLAG(IS_CHROMEOS_ASH) + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndEnableFeature(switches::kAccountIdMigration); +#endif + base::ScopedTempDir scoped_user_data_dir; + ASSERT_TRUE(scoped_user_data_dir.CreateUniqueTempDir()); + + // Create a tracker and add an account. This should cause the account to be + // saved to persistence. + ResetAccountTrackerWithPersistence(scoped_user_data_dir.GetPath()); + SimulateTokenAvailable(kAccountKeyAlpha); + ReturnAccountInfoFetchSuccess(kAccountKeyAlpha); + + // The capability is unknown, and none of the capability-related keys should + // be set. + EXPECT_EQ(signin::Tribool::kUnknown, + account_tracker() + ->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()); + 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)); + + // Set the capability using the deprecated key, and reload the account. + dict.SetIntPath(kDeprecatedCapabilityKey, 1); + dict.RemovePath(kNewCapabilityKey); + ClearAccountTrackerEvents(); + ResetAccountTrackerWithPersistence(scoped_user_data_dir.GetPath()); + EXPECT_TRUE(CheckAccountTrackerEvents( + {TrackingEvent(UPDATED, AccountKeyToAccountId(kAccountKeyAlpha), + AccountKeyToGaiaId(kAccountKeyAlpha), + AccountKeyToEmail(kAccountKeyAlpha))})); + + // Check that the migration happened. + std::vector<AccountInfo> infos = account_tracker()->GetAccounts(); + ASSERT_EQ(1u, infos.size()); + CheckAccountDetails(kAccountKeyAlpha, infos[0]); + // The deprecated key has been read. + 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)); + // The new key has been written. + absl::optional<int> new_key = dict.FindIntPath(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_account_capabilities_fetcher.cc b/chromium/components/signin/internal/identity_manager/fake_account_capabilities_fetcher.cc new file mode 100644 index 00000000000..263f07cc134 --- /dev/null +++ b/chromium/components/signin/internal/identity_manager/fake_account_capabilities_fetcher.cc @@ -0,0 +1,27 @@ +// Copyright 2022 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/internal/identity_manager/fake_account_capabilities_fetcher.h" + +#include "base/callback.h" +#include "components/signin/public/identity_manager/account_info.h" +#include "third_party/abseil-cpp/absl/types/optional.h" + +FakeAccountCapabilitiesFetcher::FakeAccountCapabilitiesFetcher( + const CoreAccountInfo& account_info, + OnCompleteCallback on_complete_callback, + base::OnceClosure on_destroy_callback) + : AccountCapabilitiesFetcher(account_info, std::move(on_complete_callback)), + on_destroy_callback_(std::move(on_destroy_callback)) {} + +FakeAccountCapabilitiesFetcher::~FakeAccountCapabilitiesFetcher() { + std::move(on_destroy_callback_).Run(); +} + +void FakeAccountCapabilitiesFetcher::StartImpl() {} + +void FakeAccountCapabilitiesFetcher::CompleteFetch( + const absl::optional<AccountCapabilities>& account_capabilities) { + CompleteFetchAndMaybeDestroySelf(account_capabilities); +} diff --git a/chromium/components/signin/internal/identity_manager/fake_account_capabilities_fetcher.h b/chromium/components/signin/internal/identity_manager/fake_account_capabilities_fetcher.h new file mode 100644 index 00000000000..1b73aa31668 --- /dev/null +++ b/chromium/components/signin/internal/identity_manager/fake_account_capabilities_fetcher.h @@ -0,0 +1,39 @@ +// Copyright 2022 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_FAKE_ACCOUNT_CAPABILITIES_FETCHER_H_ +#define COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_FAKE_ACCOUNT_CAPABILITIES_FETCHER_H_ + +#include "base/callback_forward.h" +#include "components/signin/internal/identity_manager/account_capabilities_fetcher.h" + +struct CoreAccountInfo; +class AccountCapabilities; + +// Fake `AccountCapabilitiesFetcher` implementation for tests. +class FakeAccountCapabilitiesFetcher : public AccountCapabilitiesFetcher { + public: + explicit FakeAccountCapabilitiesFetcher( + const CoreAccountInfo& account_info, + OnCompleteCallback on_complete_callback, + base::OnceClosure on_destroy_callback); + ~FakeAccountCapabilitiesFetcher() override; + + FakeAccountCapabilitiesFetcher(const FakeAccountCapabilitiesFetcher&) = + delete; + FakeAccountCapabilitiesFetcher& operator=( + const FakeAccountCapabilitiesFetcher&) = delete; + + void CompleteFetch( + const absl::optional<AccountCapabilities>& account_capabilities); + + protected: + // AccountCapabilitiesFetcher: + void StartImpl() override; + + private: + base::OnceClosure on_destroy_callback_; +}; + +#endif // COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_FAKE_ACCOUNT_CAPABILITIES_FETCHER_H_ diff --git a/chromium/components/signin/internal/identity_manager/fake_account_capabilities_fetcher_factory.cc b/chromium/components/signin/internal/identity_manager/fake_account_capabilities_fetcher_factory.cc new file mode 100644 index 00000000000..64ce835c7dd --- /dev/null +++ b/chromium/components/signin/internal/identity_manager/fake_account_capabilities_fetcher_factory.cc @@ -0,0 +1,41 @@ +// Copyright 2022 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/internal/identity_manager/fake_account_capabilities_fetcher_factory.h" + +#include "base/bind.h" +#include "components/signin/internal/identity_manager/fake_account_capabilities_fetcher.h" +#include "components/signin/public/identity_manager/account_info.h" + +FakeAccountCapabilitiesFetcherFactory::FakeAccountCapabilitiesFetcherFactory() = + default; +FakeAccountCapabilitiesFetcherFactory:: + ~FakeAccountCapabilitiesFetcherFactory() = default; + +std::unique_ptr<AccountCapabilitiesFetcher> +FakeAccountCapabilitiesFetcherFactory::CreateAccountCapabilitiesFetcher( + const CoreAccountInfo& account_info, + AccountCapabilitiesFetcher::OnCompleteCallback on_complete_callback) { + auto fetcher = std::make_unique<FakeAccountCapabilitiesFetcher>( + account_info, std::move(on_complete_callback), + base::BindOnce(&FakeAccountCapabilitiesFetcherFactory::OnFetcherDestroyed, + base::Unretained(this), account_info.account_id)); + DCHECK(!fetchers_.count(account_info.account_id)); + fetchers_[account_info.account_id] = fetcher.get(); + return fetcher; +} + +void FakeAccountCapabilitiesFetcherFactory::CompleteAccountCapabilitiesFetch( + const CoreAccountId& account_id, + const absl::optional<AccountCapabilities> account_capabilities) { + DCHECK(fetchers_.count(account_id)); + // `CompleteFetch` may destroy the fetcher. + fetchers_[account_id]->CompleteFetch(account_capabilities); +} + +void FakeAccountCapabilitiesFetcherFactory::OnFetcherDestroyed( + const CoreAccountId& account_id) { + DCHECK(fetchers_.count(account_id)); + fetchers_.erase(account_id); +} diff --git a/chromium/components/signin/internal/identity_manager/fake_account_capabilities_fetcher_factory.h b/chromium/components/signin/internal/identity_manager/fake_account_capabilities_fetcher_factory.h new file mode 100644 index 00000000000..fd01c95b80f --- /dev/null +++ b/chromium/components/signin/internal/identity_manager/fake_account_capabilities_fetcher_factory.h @@ -0,0 +1,47 @@ +// Copyright 2022 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_FAKE_ACCOUNT_CAPABILITIES_FETCHER_FACTORY_H_ +#define COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_FAKE_ACCOUNT_CAPABILITIES_FETCHER_FACTORY_H_ + +#include <map> +#include <memory> + +#include "components/signin/internal/identity_manager/account_capabilities_fetcher_factory.h" +#include "third_party/abseil-cpp/absl/types/optional.h" + +class FakeAccountCapabilitiesFetcher; +class AccountCapabilities; +struct CoreAccountId; +struct CoreAccountInfo; + +// Fake `AccountCapabilitiesFetcherFactory` implementation for tests. +class FakeAccountCapabilitiesFetcherFactory + : public AccountCapabilitiesFetcherFactory { + public: + FakeAccountCapabilitiesFetcherFactory(); + ~FakeAccountCapabilitiesFetcherFactory() override; + + FakeAccountCapabilitiesFetcherFactory( + const FakeAccountCapabilitiesFetcherFactory&) = delete; + FakeAccountCapabilitiesFetcherFactory& operator=( + const FakeAccountCapabilitiesFetcherFactory&) = delete; + + // AccountCapabilitiesFetcherFactory: + std::unique_ptr<AccountCapabilitiesFetcher> CreateAccountCapabilitiesFetcher( + const CoreAccountInfo& account_info, + AccountCapabilitiesFetcher::OnCompleteCallback on_complete_callback) + override; + + void CompleteAccountCapabilitiesFetch( + const CoreAccountId& account_id, + const absl::optional<AccountCapabilities> account_capabilities); + + private: + void OnFetcherDestroyed(const CoreAccountId& account_id); + + std::map<CoreAccountId, FakeAccountCapabilitiesFetcher*> fetchers_; +}; + +#endif // COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_FAKE_ACCOUNT_CAPABILITIES_FETCHER_FACTORY_H_ 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 255d8e5abd9..0d9458f9db8 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 @@ -92,6 +92,12 @@ std::vector<CoreAccountId> FakeProfileOAuth2TokenServiceDelegate::GetAccounts() void FakeProfileOAuth2TokenServiceDelegate::RevokeAllCredentials() { std::vector<CoreAccountId> account_ids = GetAccounts(); + if (account_ids.empty()) + return; + + // Use `ScopedBatchChange` so that `OnEndBatchOfRefreshTokenStateChanges()` is + // fired only once, like in production. + ScopedBatchChange batch(this); for (const auto& account : account_ids) RevokeCredentials(account); } diff --git a/chromium/components/signin/internal/identity_manager/gaia_cookie_manager_service.h b/chromium/components/signin/internal/identity_manager/gaia_cookie_manager_service.h index 44369e12807..53e240123e2 100644 --- a/chromium/components/signin/internal/identity_manager/gaia_cookie_manager_service.h +++ b/chromium/components/signin/internal/identity_manager/gaia_cookie_manager_service.h @@ -15,6 +15,7 @@ #include "base/containers/circular_deque.h" #include "base/memory/raw_ptr.h" #include "base/memory/weak_ptr.h" +#include "base/time/time.h" #include "base/timer/timer.h" #include "components/prefs/pref_registry_simple.h" #include "components/signin/internal/identity_manager/profile_oauth2_token_service.h" diff --git a/chromium/components/signin/internal/identity_manager/oauth_multilogin_token_fetcher.cc b/chromium/components/signin/internal/identity_manager/oauth_multilogin_token_fetcher.cc index 7ee1cecab12..310f0a3d0de 100644 --- a/chromium/components/signin/internal/identity_manager/oauth_multilogin_token_fetcher.cc +++ b/chromium/components/signin/internal/identity_manager/oauth_multilogin_token_fetcher.cc @@ -103,8 +103,6 @@ void OAuthMultiloginTokenFetcher::OnGetTokenFailure( if (error.IsTransientError() && retried_requests_.find(account_id) == retried_requests_.end()) { retried_requests_.insert(account_id); - UMA_HISTOGRAM_ENUMERATION("Signin.GetAccessTokenRetry", error.state(), - GoogleServiceAuthError::NUM_STATES); EraseRequest(request); // Fetching fresh access tokens requires network. signin_client_->DelayNetworkCall( diff --git a/chromium/components/signin/internal/identity_manager/primary_account_manager.cc b/chromium/components/signin/internal/identity_manager/primary_account_manager.cc index 741a2b973ef..9d0a7c78946 100644 --- a/chromium/components/signin/internal/identity_manager/primary_account_manager.cc +++ b/chromium/components/signin/internal/identity_manager/primary_account_manager.cc @@ -12,12 +12,12 @@ #include "base/command_line.h" #include "base/logging.h" #include "base/metrics/histogram_macros.h" +#include "base/observer_list.h" #include "build/build_config.h" #include "build/chromeos_buildflags.h" #include "components/prefs/pref_registry_simple.h" #include "components/prefs/pref_service.h" #include "components/signin/internal/identity_manager/account_tracker_service.h" -#include "components/signin/internal/identity_manager/primary_account_policy_manager.h" #include "components/signin/internal/identity_manager/profile_oauth2_token_service.h" #include "components/signin/public/base/account_consistency_method.h" #include "components/signin/public/base/signin_client.h" @@ -30,13 +30,10 @@ using signin::PrimaryAccountChangeEvent; PrimaryAccountManager::PrimaryAccountManager( SigninClient* client, ProfileOAuth2TokenService* token_service, - AccountTrackerService* account_tracker_service, - std::unique_ptr<PrimaryAccountPolicyManager> policy_manager) + AccountTrackerService* account_tracker_service) : client_(client), token_service_(token_service), - account_tracker_service_(account_tracker_service), - initialized_(false), - policy_manager_(std::move(policy_manager)) { + account_tracker_service_(account_tracker_service) { DCHECK(client_); DCHECK(account_tracker_service_); } @@ -119,9 +116,6 @@ void PrimaryAccountManager::Initialize(PrefService* local_state) { SetPrimaryAccountInternal(account_info, consented); } - if (policy_manager_) { - policy_manager_->InitializePolicy(local_state, this); - } // It is important to only load credentials after starting to observe the // token service. token_service_->AddObserver(this); diff --git a/chromium/components/signin/internal/identity_manager/primary_account_manager.h b/chromium/components/signin/internal/identity_manager/primary_account_manager.h index 04671a78c3c..dab27fa9031 100644 --- a/chromium/components/signin/internal/identity_manager/primary_account_manager.h +++ b/chromium/components/signin/internal/identity_manager/primary_account_manager.h @@ -18,8 +18,6 @@ #ifndef COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_PRIMARY_ACCOUNT_MANAGER_H_ #define COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_PRIMARY_ACCOUNT_MANAGER_H_ -#include <memory> - #include "base/memory/raw_ptr.h" #include "base/observer_list.h" #include "base/observer_list_types.h" @@ -33,7 +31,6 @@ class AccountTrackerService; class PrefRegistrySimple; class PrefService; -class PrimaryAccountPolicyManager; class ProfileOAuth2TokenService; namespace signin_metrics { @@ -59,11 +56,9 @@ class PrimaryAccountManager : public ProfileOAuth2TokenServiceObserver { kRemoveAllAccounts, }; - PrimaryAccountManager( - SigninClient* client, - ProfileOAuth2TokenService* token_service, - AccountTrackerService* account_tracker_service, - std::unique_ptr<PrimaryAccountPolicyManager> policy_manager); + PrimaryAccountManager(SigninClient* client, + ProfileOAuth2TokenService* token_service, + AccountTrackerService* account_tracker_service); PrimaryAccountManager(const PrimaryAccountManager&) = delete; PrimaryAccountManager& operator=(const PrimaryAccountManager&) = delete; @@ -184,7 +179,6 @@ class PrimaryAccountManager : public ProfileOAuth2TokenServiceObserver { // this field. CoreAccountInfo primary_account_info_; - std::unique_ptr<PrimaryAccountPolicyManager> policy_manager_; base::ObserverList<Observer> observers_; }; diff --git a/chromium/components/signin/internal/identity_manager/primary_account_manager_unittest.cc b/chromium/components/signin/internal/identity_manager/primary_account_manager_unittest.cc index 1ea83d7b7c6..fda15ab8f60 100644 --- a/chromium/components/signin/internal/identity_manager/primary_account_manager_unittest.cc +++ b/chromium/components/signin/internal/identity_manager/primary_account_manager_unittest.cc @@ -22,8 +22,8 @@ #include "components/prefs/testing_pref_service.h" #include "components/signin/internal/identity_manager/account_fetcher_service.h" #include "components/signin/internal/identity_manager/account_tracker_service.h" +#include "components/signin/internal/identity_manager/fake_account_capabilities_fetcher_factory.h" #include "components/signin/internal/identity_manager/fake_profile_oauth2_token_service_delegate.h" -#include "components/signin/internal/identity_manager/primary_account_policy_manager.h" #include "components/signin/internal/identity_manager/profile_oauth2_token_service.h" #include "components/signin/public/base/signin_pref_names.h" #include "components/signin/public/base/signin_switches.h" @@ -31,10 +31,6 @@ #include "components/sync_preferences/testing_pref_service_syncable.h" #include "testing/gtest/include/gtest/gtest.h" -#if !BUILDFLAG(IS_CHROMEOS_ASH) -#include "components/signin/internal/identity_manager/primary_account_policy_manager_impl.h" -#endif - using signin::ConsentLevel; class PrimaryAccountManagerTest : public testing::Test, @@ -53,7 +49,8 @@ class PrimaryAccountManagerTest : public testing::Test, account_tracker_.Initialize(&user_prefs_, base::FilePath()); account_fetcher_.Initialize( &test_signin_client_, &token_service_, &account_tracker_, - std::make_unique<image_fetcher::FakeImageDecoder>()); + std::make_unique<image_fetcher::FakeImageDecoder>(), + std::make_unique<FakeAccountCapabilitiesFetcherFactory>()); } ~PrimaryAccountManagerTest() override { @@ -79,20 +76,8 @@ class PrimaryAccountManagerTest : public testing::Test, void CreatePrimaryAccountManager() { DCHECK(!manager_); - // Supply the primary account manager with a policy manager to reflect - // production usage: null on ChromeOS, a PrimaryAccountPolicyManagerImpl on - // other platforms. - std::unique_ptr<PrimaryAccountPolicyManager> policy_manager; -#if !BUILDFLAG(IS_CHROMEOS_ASH) - policy_manager = - std::make_unique<PrimaryAccountPolicyManagerImpl>(&test_signin_client_); - policy_manager_ = - static_cast<PrimaryAccountPolicyManagerImpl*>(policy_manager.get()); -#endif - manager_ = std::make_unique<PrimaryAccountManager>( - &test_signin_client_, &token_service_, &account_tracker_, - std::move(policy_manager)); + &test_signin_client_, &token_service_, &account_tracker_); manager_->Initialize(&local_state_); manager_->AddObserver(this); } @@ -139,9 +124,6 @@ class PrimaryAccountManagerTest : public testing::Test, ProfileOAuth2TokenService token_service_; AccountTrackerService account_tracker_; AccountFetcherService account_fetcher_; -#if !BUILDFLAG(IS_CHROMEOS_ASH) - raw_ptr<PrimaryAccountPolicyManagerImpl> policy_manager_; -#endif std::unique_ptr<PrimaryAccountManager> manager_; std::vector<std::string> oauth_tokens_fetched_; std::vector<std::string> cookies_; @@ -242,33 +224,6 @@ TEST_F(PrimaryAccountManagerTest, UnconsentedSignOutWhileProhibited) { signin_metrics::SignoutDelete::kIgnoreMetric); EXPECT_FALSE(manager_->HasPrimaryAccount(ConsentLevel::kSignin)); } - -TEST_F(PrimaryAccountManagerTest, ProhibitedAtStartup) { - CoreAccountId account_id = AddToAccountTracker("gaia_id", "user@gmail.com"); - user_prefs_.SetString(prefs::kGoogleServicesAccountId, account_id.ToString()); - local_state_.SetString(prefs::kGoogleServicesUsernamePattern, - ".*@google.com"); - CreatePrimaryAccountManager(); - // Currently signed in user is prohibited by policy, so should be signed out. - EXPECT_EQ("", manager_->GetPrimaryAccountInfo(ConsentLevel::kSync).email); - EXPECT_EQ(CoreAccountId(), - manager_->GetPrimaryAccountId(ConsentLevel::kSync)); -} - -TEST_F(PrimaryAccountManagerTest, ProhibitedAfterStartup) { - CoreAccountId account_id = AddToAccountTracker("gaia_id", "user@gmail.com"); - user_prefs_.SetString(prefs::kGoogleServicesAccountId, account_id.ToString()); - CreatePrimaryAccountManager(); - EXPECT_EQ("user@gmail.com", - manager_->GetPrimaryAccountInfo(ConsentLevel::kSync).email); - EXPECT_EQ(account_id, manager_->GetPrimaryAccountId(ConsentLevel::kSync)); - // Update the profile - user should be signed out. - local_state_.SetString(prefs::kGoogleServicesUsernamePattern, - ".*@google.com"); - EXPECT_EQ("", manager_->GetPrimaryAccountInfo(ConsentLevel::kSync).email); - EXPECT_EQ(CoreAccountId(), - manager_->GetPrimaryAccountId(ConsentLevel::kSync)); -} #endif // Regression test for https://crbug.com/1155519. @@ -338,19 +293,6 @@ TEST_F(PrimaryAccountManagerTest, EXPECT_EQ(account_id, manager_->GetPrimaryAccountId(ConsentLevel::kSync)); } -#if !BUILDFLAG(IS_CHROMEOS_ASH) -TEST_F(PrimaryAccountManagerTest, SigninNotAllowed) { - std::string user("user@google.com"); - CoreAccountId account_id = AddToAccountTracker("gaia_id", user); - user_prefs_.SetString(prefs::kGoogleServicesAccountId, account_id.ToString()); - user_prefs_.SetBoolean(prefs::kSigninAllowed, false); - CreatePrimaryAccountManager(); - // Currently signing in is prohibited by policy, so should be signed out. - EXPECT_EQ("", manager_->GetPrimaryAccountInfo(ConsentLevel::kSync).email); - EXPECT_TRUE(manager_->GetPrimaryAccountId(ConsentLevel::kSync).empty()); -} -#endif - TEST_F(PrimaryAccountManagerTest, GaiaIdMigration) { #if BUILDFLAG(IS_CHROMEOS_ASH) base::test::ScopedFeatureList scoped_feature_list; diff --git a/chromium/components/signin/internal/identity_manager/primary_account_mutator_impl.cc b/chromium/components/signin/internal/identity_manager/primary_account_mutator_impl.cc index e611eba72c1..59ea1009115 100644 --- a/chromium/components/signin/internal/identity_manager/primary_account_mutator_impl.cc +++ b/chromium/components/signin/internal/identity_manager/primary_account_mutator_impl.cc @@ -7,6 +7,7 @@ #include <string> #include "base/check.h" +#include "base/feature_list.h" #include "build/chromeos_buildflags.h" #include "components/prefs/pref_service.h" #include "components/signin/internal/identity_manager/account_tracker_service.h" @@ -15,6 +16,7 @@ #include "components/signin/public/base/signin_buildflags.h" #include "components/signin/public/base/signin_metrics.h" #include "components/signin/public/base/signin_pref_names.h" +#include "components/signin/public/base/signin_switches.h" #include "google_apis/gaia/core_account_id.h" namespace signin { @@ -90,7 +92,8 @@ PrimaryAccountMutatorImpl::SetPrimaryAccount(const CoreAccountId& account_id, } #if !BUILDFLAG(IS_CHROMEOS_ASH) -bool PrimaryAccountMutatorImpl::RevokeConsentShouldClearPrimaryAccount() const { +bool PrimaryAccountMutatorImpl::CanTransitionFromSyncToSigninConsentLevel() + const { switch (account_consistency_) { case AccountConsistencyMethod::kDice: // If DICE is enabled, then adding and removing accounts is handled from @@ -106,7 +109,7 @@ bool PrimaryAccountMutatorImpl::RevokeConsentShouldClearPrimaryAccount() const { // // TODO(msarda): The logic in this function is platform specific and we // should consider moving it to |SigninManager|. - return token_service_->RefreshTokenHasError( + return !token_service_->RefreshTokenHasError( primary_account_manager_->GetPrimaryAccountId(ConsentLevel::kSync)); case AccountConsistencyMethod::kMirror: #if BUILDFLAG(IS_CHROMEOS_LACROS) @@ -114,12 +117,30 @@ bool PrimaryAccountMutatorImpl::RevokeConsentShouldClearPrimaryAccount() const { // main profile and return true, otherwise. This requires implementing // ProfileOAuth2TokenServiceDelegateChromeOS::Revoke* and it's not clear // what these functions should do. - return false; -#else return true; +#elif BUILDFLAG(IS_ANDROID) + // Android supports users being signed in with sync disabled, with the + // exception of child accounts with the kAllowSyncOffForChildAccounts + // flag disabled. + // + // Strictly-speaking we should only look at the value of this flag for + // child accounts, however the child account status is not easily + // available here and it doesn't matter if we clear the primary account + // for non-Child accounts as we don't expose a 'Turn off sync' UI for + // them. As this is a short-lived flag, we leave as-is rather than + // plumb through child status here. + return base::FeatureList::IsEnabled( + switches::kAllowSyncOffForChildAccounts); +#else + // TODO(crbug.com/1165785): once kAllowSyncOffForChildAccounts has been + // rolled out and assuming it has not revealed any issues, make the + // behaviour consistent across all Mirror platforms, by allowing this + // transition on iOS too (i.e. return true with no platform checks for + // kMirror). + return false; #endif case AccountConsistencyMethod::kDisabled: - return true; + return false; } } #endif @@ -130,7 +151,7 @@ void PrimaryAccountMutatorImpl::RevokeSyncConsent( DCHECK(primary_account_manager_->HasPrimaryAccount(ConsentLevel::kSync)); #if !BUILDFLAG(IS_CHROMEOS_ASH) - if (RevokeConsentShouldClearPrimaryAccount()) { + if (!CanTransitionFromSyncToSigninConsentLevel()) { ClearPrimaryAccount(source_metric, delete_metric); return; } diff --git a/chromium/components/signin/internal/identity_manager/primary_account_mutator_impl.h b/chromium/components/signin/internal/identity_manager/primary_account_mutator_impl.h index d20e48813a8..30832589b34 100644 --- a/chromium/components/signin/internal/identity_manager/primary_account_mutator_impl.h +++ b/chromium/components/signin/internal/identity_manager/primary_account_mutator_impl.h @@ -42,9 +42,9 @@ class PrimaryAccountMutatorImpl : public PrimaryAccountMutator { private: #if !BUILDFLAG(IS_CHROMEOS_ASH) - // Returns true if revoking the sync consent should instead clear the primary - // account. - bool RevokeConsentShouldClearPrimaryAccount() const; + // Returns true if transitioning from Sync to Signin consent level is allowed + // for this platform / configuration. + bool CanTransitionFromSyncToSigninConsentLevel() const; #endif // Pointers to the services used by the PrimaryAccountMutatorImpl. They diff --git a/chromium/components/signin/internal/identity_manager/primary_account_policy_manager.h b/chromium/components/signin/internal/identity_manager/primary_account_policy_manager.h deleted file mode 100644 index f5dedd0702d..00000000000 --- a/chromium/components/signin/internal/identity_manager/primary_account_policy_manager.h +++ /dev/null @@ -1,29 +0,0 @@ -// Copyright 2014 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_PRIMARY_ACCOUNT_POLICY_MANAGER_H_ -#define COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_PRIMARY_ACCOUNT_POLICY_MANAGER_H_ - -class PrefService; -class PrimaryAccountManager; - -class PrimaryAccountPolicyManager { - public: - PrimaryAccountPolicyManager() = default; - - PrimaryAccountPolicyManager(const PrimaryAccountPolicyManager&) = delete; - PrimaryAccountPolicyManager& operator=(const PrimaryAccountPolicyManager&) = - delete; - - virtual ~PrimaryAccountPolicyManager() = default; - - // On platforms where PrimaryAccountManager is responsible for dealing with - // invalid username policy updates, we need to check this during - // initialization and sign the user out. - virtual void InitializePolicy( - PrefService* local_state, - PrimaryAccountManager* primary_account_manager) = 0; -}; - -#endif // COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_PRIMARY_ACCOUNT_POLICY_MANAGER_H_ diff --git a/chromium/components/signin/internal/identity_manager/primary_account_policy_manager_impl.cc b/chromium/components/signin/internal/identity_manager/primary_account_policy_manager_impl.cc deleted file mode 100644 index 9116698b912..00000000000 --- a/chromium/components/signin/internal/identity_manager/primary_account_policy_manager_impl.cc +++ /dev/null @@ -1,114 +0,0 @@ -// Copyright 2019 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "components/signin/internal/identity_manager/primary_account_policy_manager_impl.h" - -#include "base/bind.h" -#include "base/logging.h" -#include "build/build_config.h" -#include "components/signin/internal/identity_manager/primary_account_manager.h" -#include "components/signin/public/base/signin_client.h" -#include "components/signin/public/base/signin_metrics.h" -#include "components/signin/public/base/signin_pref_names.h" -#include "components/signin/public/identity_manager/account_info.h" -#include "components/signin/public/identity_manager/identity_utils.h" - -PrimaryAccountPolicyManagerImpl::PrimaryAccountPolicyManagerImpl( - SigninClient* client) - : client_(client) {} - -PrimaryAccountPolicyManagerImpl::~PrimaryAccountPolicyManagerImpl() { - local_state_pref_registrar_.RemoveAll(); -} - -void PrimaryAccountPolicyManagerImpl::InitializePolicy( - PrefService* local_state, - PrimaryAccountManager* primary_account_manager) { - // local_state can be null during unit tests. - if (local_state) { - local_state_pref_registrar_.Init(local_state); - local_state_pref_registrar_.Add( - prefs::kGoogleServicesUsernamePattern, - base::BindRepeating(&PrimaryAccountPolicyManagerImpl:: - OnGoogleServicesUsernamePatternChanged, - weak_pointer_factory_.GetWeakPtr(), - primary_account_manager)); - } - signin_allowed_.Init( - prefs::kSigninAllowed, client_->GetPrefs(), - base::BindRepeating( - &PrimaryAccountPolicyManagerImpl::OnSigninAllowedPrefChanged, - base::Unretained(this), primary_account_manager)); - - CoreAccountInfo account_info = primary_account_manager->GetPrimaryAccountInfo( - signin::ConsentLevel::kSync); - if (!account_info.account_id.empty() && - (!IsAllowedUsername(account_info.email) || !IsSigninAllowed())) { - // User is signed in, but the username is invalid or signin is no longer - // allowed, so the user must be sign out. - // - // This may happen in the following cases: - // a. The user has toggled off signin allowed in settings. - // b. The administrator changed the policy since the last signin. - // - // Note: The token service has not yet loaded its credentials, so accounts - // cannot be revoked here. - // - // On desktop, when PrimaryAccountManager is initializing, the profile was - // not yet marked with sign out allowed. Therefore sign out is not allowed - // and all calls to RevokeSyncConsent() and ClearPrimaryAccount() methods - // are no-op. - // - // TODO(msarda): RevokeSyncConsent() method do not guarantee that the sync - // consent can really be revoked (this depends on whether sign out is - // allowed). Add a check here on desktop to make it clear that - // RevokeSyncConsent() does not do anything. - primary_account_manager->RevokeSyncConsent( - signin_metrics::SIGNIN_PREF_CHANGED_DURING_SIGNIN, - signin_metrics::SignoutDelete::kIgnoreMetric); - } -} - -void PrimaryAccountPolicyManagerImpl::OnGoogleServicesUsernamePatternChanged( - PrimaryAccountManager* primary_account_manager) { - if (primary_account_manager->HasPrimaryAccount(signin::ConsentLevel::kSync) && - !IsAllowedUsername( - primary_account_manager - ->GetPrimaryAccountInfo(signin::ConsentLevel::kSync) - .email)) { - // Signed in user is invalid according to the current policy so sign - // the user out. - primary_account_manager->ClearPrimaryAccount( - signin_metrics::GOOGLE_SERVICE_NAME_PATTERN_CHANGED, - signin_metrics::SignoutDelete::kIgnoreMetric); - } -} - -bool PrimaryAccountPolicyManagerImpl::IsSigninAllowed() const { - return signin_allowed_.GetValue(); -} - -void PrimaryAccountPolicyManagerImpl::OnSigninAllowedPrefChanged( - PrimaryAccountManager* primary_account_manager) { - if (!IsSigninAllowed() && - primary_account_manager->HasPrimaryAccount(signin::ConsentLevel::kSync)) { - VLOG(0) << "IsSigninAllowed() set to false, signing out the user"; - primary_account_manager->ClearPrimaryAccount( - signin_metrics::SIGNOUT_PREF_CHANGED, - signin_metrics::SignoutDelete::kIgnoreMetric); - } -} - -bool PrimaryAccountPolicyManagerImpl::IsAllowedUsername( - const std::string& username) const { - const PrefService* local_state = local_state_pref_registrar_.prefs(); - - // TODO(crbug.com/908121): We need to deal for now with the fact that many - // unit tests have a null |local_state| passed to InitializePolicy(), in which - // case all usernames are considered 'allowed'. - if (!local_state) - return true; - - return signin::IsUsernameAllowedByPatternFromPrefs(local_state, username); -} diff --git a/chromium/components/signin/internal/identity_manager/primary_account_policy_manager_impl.h b/chromium/components/signin/internal/identity_manager/primary_account_policy_manager_impl.h deleted file mode 100644 index fa720ccd199..00000000000 --- a/chromium/components/signin/internal/identity_manager/primary_account_policy_manager_impl.h +++ /dev/null @@ -1,66 +0,0 @@ -// Copyright 2019 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_PRIMARY_ACCOUNT_POLICY_MANAGER_IMPL_H_ -#define COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_PRIMARY_ACCOUNT_POLICY_MANAGER_IMPL_H_ - -#include <string> - -#include "base/gtest_prod_util.h" -#include "base/memory/raw_ptr.h" -#include "base/memory/weak_ptr.h" -#include "components/prefs/pref_change_registrar.h" -#include "components/prefs/pref_member.h" -#include "components/signin/internal/identity_manager/primary_account_policy_manager.h" - -class PrefService; -class PrimaryAccountManager; -class SigninClient; - -class PrimaryAccountPolicyManagerImpl : public PrimaryAccountPolicyManager { - public: - explicit PrimaryAccountPolicyManagerImpl(SigninClient* client); - - PrimaryAccountPolicyManagerImpl(const PrimaryAccountPolicyManagerImpl&) = - delete; - PrimaryAccountPolicyManagerImpl& operator=( - const PrimaryAccountPolicyManagerImpl&) = delete; - - ~PrimaryAccountPolicyManagerImpl() override; - - // PrimaryAccountPolicyManager: - void InitializePolicy( - PrefService* local_state, - PrimaryAccountManager* primary_account_manager) override; - - private: - FRIEND_TEST_ALL_PREFIXES(PrimaryAccountPolicyManagerImplTest, Prohibited); - FRIEND_TEST_ALL_PREFIXES(PrimaryAccountPolicyManagerImplTest, - TestAlternateWildcard); - - // Returns true if a signin to Chrome is allowed (by policy or pref). - bool IsSigninAllowed() const; - - void OnSigninAllowedPrefChanged( - PrimaryAccountManager* primary_account_manager); - void OnGoogleServicesUsernamePatternChanged( - PrimaryAccountManager* primary_account_manager); - - // Returns true if the passed username is allowed by policy. - bool IsAllowedUsername(const std::string& username) const; - - raw_ptr<SigninClient> client_; - - // Helper object to listen for changes to signin preferences stored in non- - // profile-specific local prefs (like kGoogleServicesUsernamePattern). - PrefChangeRegistrar local_state_pref_registrar_; - - // Helper object to listen for changes to the signin allowed preference. - BooleanPrefMember signin_allowed_; - - base::WeakPtrFactory<PrimaryAccountPolicyManagerImpl> weak_pointer_factory_{ - this}; -}; - -#endif // COMPONENTS_SIGNIN_INTERNAL_IDENTITY_MANAGER_PRIMARY_ACCOUNT_POLICY_MANAGER_IMPL_H_ diff --git a/chromium/components/signin/internal/identity_manager/primary_account_policy_manager_impl_unittest.cc b/chromium/components/signin/internal/identity_manager/primary_account_policy_manager_impl_unittest.cc deleted file mode 100644 index 83df3659d90..00000000000 --- a/chromium/components/signin/internal/identity_manager/primary_account_policy_manager_impl_unittest.cc +++ /dev/null @@ -1,74 +0,0 @@ -// Copyright 2019 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "components/signin/internal/identity_manager/primary_account_policy_manager_impl.h" - -#include <memory> -#include <string> - -#include "base/test/task_environment.h" -#include "components/prefs/testing_pref_service.h" -#include "components/signin/internal/identity_manager/account_tracker_service.h" -#include "components/signin/internal/identity_manager/fake_profile_oauth2_token_service_delegate.h" -#include "components/signin/internal/identity_manager/primary_account_manager.h" -#include "components/signin/internal/identity_manager/primary_account_policy_manager_impl.h" -#include "components/signin/internal/identity_manager/profile_oauth2_token_service.h" -#include "components/signin/public/base/account_consistency_method.h" -#include "components/signin/public/base/signin_pref_names.h" -#include "components/signin/public/base/test_signin_client.h" -#include "components/sync_preferences/testing_pref_service_syncable.h" -#include "testing/gtest/include/gtest/gtest.h" - -class PrimaryAccountPolicyManagerImplTest : public testing::Test { - public: - PrimaryAccountPolicyManagerImplTest() - : test_signin_client_(&user_prefs_), - token_service_( - &user_prefs_, - std::make_unique<FakeProfileOAuth2TokenServiceDelegate>()), - primary_account_manager_(&test_signin_client_, - &token_service_, - &account_tracker_, - nullptr /*policy_manager*/), - policy_manager_(&test_signin_client_) { - PrimaryAccountManager::RegisterProfilePrefs(user_prefs_.registry()); - PrimaryAccountManager::RegisterPrefs(local_state_.registry()); - - policy_manager_.InitializePolicy(&local_state_, &primary_account_manager_); - } - - ~PrimaryAccountPolicyManagerImplTest() override { - test_signin_client_.Shutdown(); - } - - base::test::TaskEnvironment task_environment_; - sync_preferences::TestingPrefServiceSyncable user_prefs_; - TestingPrefServiceSimple local_state_; - TestSigninClient test_signin_client_; - ProfileOAuth2TokenService token_service_; - AccountTrackerService account_tracker_; - PrimaryAccountManager primary_account_manager_; - PrimaryAccountPolicyManagerImpl policy_manager_; -}; - -TEST_F(PrimaryAccountPolicyManagerImplTest, Prohibited) { - local_state_.SetString(prefs::kGoogleServicesUsernamePattern, - ".*@google.com"); - EXPECT_TRUE(policy_manager_.IsAllowedUsername("test@google.com")); - EXPECT_TRUE(policy_manager_.IsAllowedUsername("happy@google.com")); - EXPECT_FALSE(policy_manager_.IsAllowedUsername("test@invalid.com")); - EXPECT_FALSE(policy_manager_.IsAllowedUsername("test@notgoogle.com")); - EXPECT_FALSE(policy_manager_.IsAllowedUsername(std::string())); -} - -TEST_F(PrimaryAccountPolicyManagerImplTest, TestAlternateWildcard) { - // Test to make sure we accept "*@google.com" as a pattern (treat it as if - // the admin entered ".*@google.com"). - local_state_.SetString(prefs::kGoogleServicesUsernamePattern, "*@google.com"); - EXPECT_TRUE(policy_manager_.IsAllowedUsername("test@google.com")); - EXPECT_TRUE(policy_manager_.IsAllowedUsername("happy@google.com")); - EXPECT_FALSE(policy_manager_.IsAllowedUsername("test@invalid.com")); - EXPECT_FALSE(policy_manager_.IsAllowedUsername("test@notgoogle.com")); - EXPECT_FALSE(policy_manager_.IsAllowedUsername(std::string())); -} diff --git a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_builder.cc b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_builder.cc index f9018596b17..9d9220a7cdb 100644 --- a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_builder.cc +++ b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_builder.cc @@ -55,12 +55,13 @@ std::unique_ptr<ProfileOAuth2TokenServiceIOSDelegate> CreateIOSOAuthDelegate( } #elif BUILDFLAG(IS_CHROMEOS_ASH) || BUILDFLAG(IS_CHROMEOS_LACROS) std::unique_ptr<ProfileOAuth2TokenServiceDelegate> CreateCrOsOAuthDelegate( + SigninClient* signin_client, AccountTrackerService* account_tracker_service, network::NetworkConnectionTracker* network_connection_tracker, account_manager::AccountManagerFacade* account_manager_facade, bool is_regular_profile) { return std::make_unique<signin::ProfileOAuth2TokenServiceDelegateChromeOS>( - account_tracker_service, network_connection_tracker, + signin_client, account_tracker_service, network_connection_tracker, account_manager_facade, is_regular_profile); } #elif BUILDFLAG(ENABLE_DICE_SUPPORT) @@ -123,7 +124,7 @@ CreateOAuth2TokenServiceDelegate( std::move(device_accounts_provider), account_tracker_service); #elif BUILDFLAG(IS_CHROMEOS_ASH) || BUILDFLAG(IS_CHROMEOS_LACROS) - return CreateCrOsOAuthDelegate(account_tracker_service, + return CreateCrOsOAuthDelegate(signin_client, account_tracker_service, network_connection_tracker, account_manager_facade, is_regular_profile); #elif BUILDFLAG(ENABLE_DICE_SUPPORT) 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 2cd93ecfca3..d73c7e16286 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 @@ -4,6 +4,7 @@ #include "components/signin/internal/identity_manager/profile_oauth2_token_service_delegate.h" +#include "base/observer_list.h" #include "components/signin/internal/identity_manager/profile_oauth2_token_service_observer.h" #include "google_apis/gaia/oauth2_access_token_consumer.h" #include "services/network/public/cpp/shared_url_loader_factory.h" 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 26e10748d74..4718b96e9dc 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 @@ -14,6 +14,7 @@ #include "build/build_config.h" #include "build/chromeos_buildflags.h" #include "components/signin/internal/identity_manager/account_tracker_service.h" +#include "components/signin/public/base/signin_client.h" #include "google_apis/gaia/oauth2_access_token_fetcher_immediate_error.h" #include "net/base/backoff_entry.h" #include "services/network/public/cpp/shared_url_loader_factory.h" @@ -138,11 +139,13 @@ class PersistentErrorsHelper : public base::RefCounted<PersistentErrorsHelper> { ProfileOAuth2TokenServiceDelegateChromeOS:: ProfileOAuth2TokenServiceDelegateChromeOS( + SigninClient* signin_client, AccountTrackerService* account_tracker_service, network::NetworkConnectionTracker* network_connection_tracker, account_manager::AccountManagerFacade* account_manager_facade, bool is_regular_profile) - : account_tracker_service_(account_tracker_service), + : 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), @@ -517,8 +520,14 @@ void ProfileOAuth2TokenServiceDelegateChromeOS::RevokeCredentials( } void ProfileOAuth2TokenServiceDelegateChromeOS::RevokeAllCredentials() { - // Signing out of Chrome is not possible on Chrome OS Ash / Lacros. +#if BUILDFLAG(IS_CHROMEOS_LACROS) + DCHECK(!signin_client_->GetInitialPrimaryAccount().has_value()); + ScopedBatchChange batch(this); + signin_client_->RemoveAllAccounts(); +#else + // Signing out of Chrome is not possible on Chrome OS Ash. NOTREACHED(); +#endif } const net::BackoffEntry* 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 7d4a351b588..76d3e6b9b86 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 @@ -20,6 +20,7 @@ #include "services/network/public/cpp/network_connection_tracker.h" class AccountTrackerService; +class SigninClient; namespace signin { class ProfileOAuth2TokenServiceDelegateChromeOS @@ -31,6 +32,7 @@ class ProfileOAuth2TokenServiceDelegateChromeOS // `NetworkConnectorTracker`, and `account_manager::AccountManagerFacade`. // These objects must all outlive `this` delegate. ProfileOAuth2TokenServiceDelegateChromeOS( + SigninClient* signin_client, AccountTrackerService* account_tracker_service, network::NetworkConnectionTracker* network_connection_tracker, account_manager::AccountManagerFacade* account_manager_facade, @@ -98,6 +100,7 @@ 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_; 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 95f14ccde40..66bfecfb657 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 @@ -221,7 +221,7 @@ class ProfileOAuth2TokenServiceDelegateChromeOSTest : public testing::Test { account_info_ = CreateAccountInfoTestFixture(kGaiaId, kUserEmail); account_tracker_service_.SeedAccountInfo(account_info_); delegate_ = std::make_unique<ProfileOAuth2TokenServiceDelegateChromeOS>( - &account_tracker_service_, + client_.get(), &account_tracker_service_, network::TestNetworkConnectionTracker::GetInstance(), account_manager_facade_.get(), true /* is_regular_profile */); @@ -376,7 +376,7 @@ TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, AccountManager account_manager; auto delegate = std::make_unique<ProfileOAuth2TokenServiceDelegateChromeOS>( - &account_tracker_service_, + client_.get(), &account_tracker_service_, network::TestNetworkConnectionTracker::GetInstance(), account_manager_facade_.get(), false /* is_regular_profile */); TestOAuth2TokenServiceObserver observer(delegate.get()); @@ -599,7 +599,7 @@ TEST_F(ProfileOAuth2TokenServiceDelegateChromeOSTest, // Register callbacks before AccountManager has been fully initialized. auto delegate = std::make_unique<ProfileOAuth2TokenServiceDelegateChromeOS>( - &account_tracker_service_, + client_.get(), &account_tracker_service_, network::TestNetworkConnectionTracker::GetInstance(), account_manager_facade.get(), true /* is_regular_profile */); delegate->LoadCredentials(account1.account_id /* primary_account_id */); diff --git a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_unittest.cc b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_unittest.cc index 1d2898b7465..91cb7d16172 100644 --- a/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_unittest.cc +++ b/chromium/components/signin/internal/identity_manager/profile_oauth2_token_service_unittest.cc @@ -701,8 +701,8 @@ TEST_F(ProfileOAuth2TokenServiceTest, RequestParametersOrderTest) { OAuth2AccessTokenManager::RequestParameters("1", account_id1, set_1), }; - for (size_t i = 0; i < base::size(params); i++) { - for (size_t j = 0; j < base::size(params); j++) { + for (size_t i = 0; i < std::size(params); i++) { + for (size_t j = 0; j < std::size(params); j++) { if (i == j) { EXPECT_FALSE(params[i] < params[j]) << " i=" << i << ", j=" << j; EXPECT_FALSE(params[j] < params[i]) << " i=" << i << ", j=" << j; 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 47aa9d3691d..74dd41c91e9 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 @@ -15,6 +15,7 @@ namespace signin { TestProfileOAuth2TokenServiceDelegateChromeOS:: TestProfileOAuth2TokenServiceDelegateChromeOS( + SigninClient* client, AccountTrackerService* account_tracker_service, crosapi::AccountManagerMojoService* account_manager_mojo_service, bool is_regular_profile) { @@ -32,7 +33,7 @@ TestProfileOAuth2TokenServiceDelegateChromeOS:: /*account_manager_for_tests=*/nullptr); delegate_ = std::make_unique<ProfileOAuth2TokenServiceDelegateChromeOS>( - account_tracker_service, + client, account_tracker_service, network::TestNetworkConnectionTracker::GetInstance(), account_manager_facade_.get(), is_regular_profile); delegate_->AddObserver(this); 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 fb522e27d47..6a990dbaf44 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 @@ -12,6 +12,7 @@ #include "services/network/test/test_network_connection_tracker.h" class AccountTrackerService; +class SigninClient; namespace crosapi { class AccountManagerMojoService; @@ -28,6 +29,7 @@ class TestProfileOAuth2TokenServiceDelegateChromeOS public ProfileOAuth2TokenServiceObserver { public: TestProfileOAuth2TokenServiceDelegateChromeOS( + SigninClient* client, AccountTrackerService* account_tracker_service, crosapi::AccountManagerMojoService* account_manager_mojo_service, bool is_regular_profile); diff --git a/chromium/components/signin/ios/browser/features.cc b/chromium/components/signin/ios/browser/features.cc index c3b4640bb80..a9c40d352c1 100644 --- a/chromium/components/signin/ios/browser/features.cc +++ b/chromium/components/signin/ios/browser/features.cc @@ -12,19 +12,14 @@ bool ForceStartupSigninPromo() { return base::FeatureList::IsEnabled(switches::kForceStartupSigninPromo); } -bool ForceDisableExtendedSyncPromos() { - return base::FeatureList::IsEnabled( - switches::kForceDisableExtendedSyncPromos); -} - const char kDelayThresholdMinutesToUpdateGaiaCookie[] = "minutes-delay-to-restore-gaia-cookies-if-deleted"; const char kWaitThresholdMillisecondsForCapabilitiesApi[] = "wait-threshold-milliseconds-for-capabilities-api"; -const base::Feature kFREMobileIdentityConsistency{ - "FREMobileIdentityConsistency", base::FEATURE_DISABLED_BY_DEFAULT}; +const base::Feature kNewMobileIdentityConsistencyFRE{ + "NewMobileIdentityConsistencyFRE", base::FEATURE_DISABLED_BY_DEFAULT}; const base::Feature kEnableUnicornAccountSupport{ "EnableUnicornAccountSupport", base::FEATURE_DISABLED_BY_DEFAULT}; diff --git a/chromium/components/signin/ios/browser/features.h b/chromium/components/signin/ios/browser/features.h index 3a780500408..cb086e3d1f9 100644 --- a/chromium/components/signin/ios/browser/features.h +++ b/chromium/components/signin/ios/browser/features.h @@ -12,9 +12,6 @@ namespace signin { // Returns true if the startup sign-in promo should be displayed at boot. bool ForceStartupSigninPromo(); -// Returns true if extended sync promos should be disabled unconditionally. -bool ForceDisableExtendedSyncPromos(); - // Name of multi-value switch that controls the delay (in minutes) for polling // for the existence of Gaia cookies for google.com. extern const char kDelayThresholdMinutesToUpdateGaiaCookie[]; @@ -24,7 +21,7 @@ extern const char kDelayThresholdMinutesToUpdateGaiaCookie[]; extern const char kWaitThresholdMillisecondsForCapabilitiesApi[]; // Feature to enable FRE MICe. -extern const base::Feature kFREMobileIdentityConsistency; +extern const base::Feature kNewMobileIdentityConsistencyFRE; // Feature to enable Unicorn account sign-in for iOS. extern const base::Feature kEnableUnicornAccountSupport; diff --git a/chromium/components/signin/public/android/BUILD.gn b/chromium/components/signin/public/android/BUILD.gn index 51428c88de6..f6cd06216cb 100644 --- a/chromium/components/signin/public/android/BUILD.gn +++ b/chromium/components/signin/public/android/BUILD.gn @@ -7,19 +7,21 @@ android_library("java") { "//base:base_java", "//components/externalauth/android:java", "//net/android:net_java", - "//third_party/android_deps:android_support_v4_java", "//third_party/android_deps:chromium_play_services_availability_java", "//third_party/android_deps:guava_android_java", "//third_party/androidx:androidx_annotation_annotation_java", ] srcjar_deps = [ + ":account_capabilities_constants_javagen", + ":tribool_javagen", "//components/signin/public/base:consent_level_enum_javagen", "//components/signin/public/base:signin_metrics_enum_javagen", ] sources = [ "java/src/org/chromium/components/signin/AccessTokenData.java", + "java/src/org/chromium/components/signin/AccountCapabilitiesFetcher.java", "java/src/org/chromium/components/signin/AccountManagerDelegate.java", "java/src/org/chromium/components/signin/AccountManagerFacade.java", "java/src/org/chromium/components/signin/AccountManagerFacadeImpl.java", @@ -33,6 +35,7 @@ android_library("java") { "java/src/org/chromium/components/signin/ConnectionRetry.java", "java/src/org/chromium/components/signin/PatternMatcher.java", "java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java", + "java/src/org/chromium/components/signin/base/AccountCapabilities.java", "java/src/org/chromium/components/signin/base/AccountInfo.java", "java/src/org/chromium/components/signin/base/CoreAccountId.java", "java/src/org/chromium/components/signin/base/CoreAccountInfo.java", @@ -53,8 +56,10 @@ android_library("java") { generate_jni("jni_headers") { namespace = "signin" sources = [ + "java/src/org/chromium/components/signin/AccountCapabilitiesFetcher.java", "java/src/org/chromium/components/signin/AccountManagerFacadeProvider.java", "java/src/org/chromium/components/signin/ChildAccountInfoFetcher.java", + "java/src/org/chromium/components/signin/base/AccountCapabilities.java", "java/src/org/chromium/components/signin/base/AccountInfo.java", "java/src/org/chromium/components/signin/base/CoreAccountId.java", "java/src/org/chromium/components/signin/base/CoreAccountInfo.java", @@ -67,9 +72,23 @@ generate_jni("jni_headers") { ] } +java_cpp_template("account_capabilities_constants_javagen") { + sources = [ "java_templates/AccountCapabilitiesConstants.template" ] + inputs = [ + "//components/signin/internal/identity_manager/account_capabilities_list.h", + ] +} + +java_cpp_enum("tribool_javagen") { + sources = [ "//components/signin/public/identity_manager/tribool.h" ] +} + generate_jni("test_support_jni_headers") { namespace = "signin" - sources = [ "java/src/org/chromium/components/signin/test/util/AccountManagerFacadeUtil.java" ] + sources = [ + "java/src/org/chromium/components/signin/test/util/AccountCapabilitiesFetcherTestUtil.java", + "java/src/org/chromium/components/signin/test/util/AccountManagerFacadeUtil.java", + ] } android_library("signin_java_test_support") { @@ -79,6 +98,7 @@ android_library("signin_java_test_support") { ":signin_test_resources", ":test_support_jni_headers", "//base:base_java", + "//base:base_java_test_support", "//base:jni_java", "//content/public/test/android:content_java_test_support", "//third_party/android_deps:guava_android_java", @@ -86,6 +106,7 @@ android_library("signin_java_test_support") { "//third_party/mockito:mockito_java", ] sources = [ + "java/src/org/chromium/components/signin/test/util/AccountCapabilitiesFetcherTestUtil.java", "java/src/org/chromium/components/signin/test/util/AccountHolder.java", "java/src/org/chromium/components/signin/test/util/AccountManagerFacadeUtil.java", "java/src/org/chromium/components/signin/test/util/FakeAccountInfoService.java", @@ -95,9 +116,13 @@ android_library("signin_java_test_support") { resources_package = "org.chromium.components.signin.test.util" } +android_resources("java_resources") { + sources = [ "java/res/drawable/logo_avatar_anonymous.xml" ] +} + android_resources("signin_test_resources") { testonly = true - sources = [ "java/res/drawable/test_profile_picture.xml" ] + sources = [ "javatests/res/drawable/test_profile_picture.xml" ] } android_library("javatests") { @@ -130,6 +155,7 @@ java_library("junit") { "junit/src/org/chromium/components/signin/AccountRenameCheckerTest.java", "junit/src/org/chromium/components/signin/AccountUtilsTest.java", "junit/src/org/chromium/components/signin/PatternMatcherTest.java", + "junit/src/org/chromium/components/signin/base/AccountCapabilitiesTest.java", "junit/src/org/chromium/components/signin/identitymanager/AccountInfoServiceImplTest.java", "junit/src/org/chromium/components/signin/identitymanager/AccountTrackerServiceTest.java", ] diff --git a/chromium/components/signin/public/android/java_templates/AccountCapabilitiesConstants.template b/chromium/components/signin/public/android/java_templates/AccountCapabilitiesConstants.template new file mode 100644 index 00000000000..3f98018719b --- /dev/null +++ b/chromium/components/signin/public/android/java_templates/AccountCapabilitiesConstants.template @@ -0,0 +1,30 @@ +// Copyright 2022 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. + +// This file is autogenerated by +// //components/signin/public/android/BUILD.gn + +package org.chromium.components.signin; + +import java.util.Set; +import java.util.HashSet; + +/** Account capabilities constants. */ +public abstract class AccountCapabilitiesConstants { + +#define ACCOUNT_CAPABILITY(cpp_label, java_label, name) \ + public static final String java_label = name; +#include "components/signin/internal/identity_manager/account_capabilities_list.h" +#undef ACCOUNT_CAPABILITY + + public static final Set<String> SUPPORTED_ACCOUNT_CAPABILITY_NAMES = new HashSet<String>() { +#define ACCOUNT_CAPABILITY(cpp_label, java_label, name) \ + { add(java_label); } +#include "components/signin/internal/identity_manager/account_capabilities_list.h" +#undef ACCOUNT_CAPABILITY + }; + + // Prevent instantiation. + private AccountCapabilitiesConstants() {} +} diff --git a/chromium/components/signin/public/android/javatests/res/drawable/test_profile_picture.xml b/chromium/components/signin/public/android/javatests/res/drawable/test_profile_picture.xml new file mode 100644 index 00000000000..86a4e2365dd --- /dev/null +++ b/chromium/components/signin/public/android/javatests/res/drawable/test_profile_picture.xml @@ -0,0 +1,25 @@ +<?xml version="1.0" encoding="utf-8"?> +<!-- Copyright 2020 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. +--> +<vector xmlns:android="http://schemas.android.com/apk/res/android" + android:width="40dp" + android:height="40dp" + android:viewportWidth="192" + android:viewportHeight="192"> + + <path + android:fillColor="#1E8E3E" + android:pathData="M96,0C43.01,0,0,43.01,0,96s43.01,96,96,96s96-43.01,96-96S148.99,0,96,0z" /> + <path + android:fillColor="#4C8BF5" + android:pathData="M96,85.09c13.28,0,24-10.72,24-24c0-13.28-10.72-24-24-24s-24,10.72-24,24 +C72,74.37,82.72,85.09,96,85.09z" /> + <path + android:fillColor="#F4C20D" + android:pathData="M96,99.27c-29.33,0-52.36,14.18-52.36,27.27c11.09,17.06,30.51,28.36,52.36,28.36 +s41.27-11.3,52.36-28.36C148.36,113.45,125.33,99.27,96,99.27z" /> + <path + android:pathData="M 0 0 H 192 V 192 H 0 V 0 Z" /> +</vector> 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 715c1f75948..d6190ad2755 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 @@ -49,6 +49,7 @@ import org.chromium.base.test.BaseRobolectricTestRunner; import org.chromium.components.externalauth.ExternalAuthUtils; import org.chromium.components.signin.AccountManagerDelegate.CapabilityResponse; import org.chromium.components.signin.AccountManagerFacade.ChildAccountStatusListener; +import org.chromium.components.signin.base.AccountCapabilities; import org.chromium.components.signin.test.util.AccountHolder; import org.chromium.components.signin.test.util.FakeAccountManagerDelegate; @@ -347,6 +348,57 @@ public class AccountManagerFacadeImplTest { AccountManagerFacadeProvider.getInstance(); } + @Test + public void testGetAccountCapabilitiesResponseYes() { + AccountManagerFacade facade = new AccountManagerFacadeImpl(mDelegate); + final AccountHolder accountHolder = AccountHolder.createFromEmail("test1@gmail.com"); + mDelegate.addAccount(accountHolder); + + doReturn(CapabilityResponse.YES) + .when(mDelegate) + .hasCapability(eq(accountHolder.getAccount()), any()); + + AccountCapabilities capabilities = + facade.getAccountCapabilities(accountHolder.getAccount()).getResult(); + Assert.assertEquals(capabilities.canOfferExtendedSyncPromos(), Tribool.TRUE); + Assert.assertEquals(capabilities.isSubjectToParentalControls(), Tribool.TRUE); + Assert.assertEquals(capabilities.canRunChromePrivacySandboxTrials(), Tribool.TRUE); + } + + @Test + public void testGetAccountCapabilitiesResponseNo() { + AccountManagerFacade facade = new AccountManagerFacadeImpl(mDelegate); + final AccountHolder accountHolder = AccountHolder.createFromEmail("test1@gmail.com"); + mDelegate.addAccount(accountHolder); + + doReturn(CapabilityResponse.NO) + .when(mDelegate) + .hasCapability(eq(accountHolder.getAccount()), any()); + + AccountCapabilities capabilities = + facade.getAccountCapabilities(accountHolder.getAccount()).getResult(); + Assert.assertEquals(capabilities.canOfferExtendedSyncPromos(), Tribool.FALSE); + Assert.assertEquals(capabilities.isSubjectToParentalControls(), Tribool.FALSE); + Assert.assertEquals(capabilities.canRunChromePrivacySandboxTrials(), Tribool.FALSE); + } + + @Test + public void testGetAccountCapabilitiesResponseException() { + AccountManagerFacade facade = new AccountManagerFacadeImpl(mDelegate); + final AccountHolder accountHolder = AccountHolder.createFromEmail("test1@gmail.com"); + mDelegate.addAccount(accountHolder); + + doReturn(CapabilityResponse.EXCEPTION) + .when(mDelegate) + .hasCapability(eq(accountHolder.getAccount()), any()); + + AccountCapabilities capabilities = + facade.getAccountCapabilities(accountHolder.getAccount()).getResult(); + Assert.assertEquals(capabilities.canOfferExtendedSyncPromos(), Tribool.UNKNOWN); + Assert.assertEquals(capabilities.isSubjectToParentalControls(), Tribool.UNKNOWN); + Assert.assertEquals(capabilities.canRunChromePrivacySandboxTrials(), Tribool.UNKNOWN); + } + private Account setFeaturesForAccount(String email, String... features) { final Account account = AccountUtils.createAccountFromName(email); mShadowAccountManager.setFeatures(account, features); 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 new file mode 100644 index 00000000000..00918a621ec --- /dev/null +++ b/chromium/components/signin/public/android/junit/src/org/chromium/components/signin/base/AccountCapabilitiesTest.java @@ -0,0 +1,174 @@ +// Copyright 2022 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. + +package org.chromium.components.signin.base; + +import static org.mockito.Mockito.spy; + +import com.google.common.collect.Lists; + +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.annotation.Config; + +import org.chromium.base.test.params.BlockJUnit4RunnerDelegate; +import org.chromium.base.test.params.ParameterAnnotations; +import org.chromium.base.test.params.ParameterProvider; +import org.chromium.base.test.params.ParameterSet; +import org.chromium.base.test.params.ParameterizedRunner; +import org.chromium.components.signin.AccountCapabilitiesConstants; +import org.chromium.components.signin.AccountManagerDelegate; +import org.chromium.components.signin.Tribool; +import org.chromium.components.signin.test.util.FakeAccountManagerDelegate; + +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; + +/** + * Test class for {@link AccountCapabilities}. + */ +@RunWith(ParameterizedRunner.class) +@ParameterAnnotations.UseRunnerDelegate(BlockJUnit4RunnerDelegate.class) +@Config(manifest = Config.NONE) +public final class AccountCapabilitiesTest { + private FakeAccountManagerDelegate mDelegate; + + /** + * Returns the capability value for the specified capability name + * from the appropriate getter in AccountCapabilities. + */ + public static @Tribool int getCapability( + String capabilityName, AccountCapabilities capabilities) { + switch (capabilityName) { + case AccountCapabilitiesConstants.CAN_OFFER_EXTENDED_CHROME_SYNC_PROMOS_CAPABILITY_NAME: + return capabilities.canOfferExtendedSyncPromos(); + case AccountCapabilitiesConstants.CAN_RUN_CHROME_PRIVACY_SANDBOX_TRIALS_CAPABILITY_NAME: + return capabilities.canRunChromePrivacySandboxTrials(); + case AccountCapabilitiesConstants.IS_SUBJECT_TO_PARENTAL_CONTROLS_CAPABILITY_NAME: + return capabilities.isSubjectToParentalControls(); + case AccountCapabilitiesConstants.CAN_STOP_PARENTAL_SUPERVISION_CAPABILITY_NAME: + return capabilities.canStopParentalSupervision(); + } + assert false : "Capability name is not known."; + return -1; + } + + /** Populates all capabilities with the given response value. */ + public static HashMap<String, Integer> populateCapabilitiesResponse( + @AccountManagerDelegate.CapabilityResponse int value) { + HashMap<String, Integer> response = new HashMap<>(); + for (String capabilityName : + AccountCapabilitiesConstants.SUPPORTED_ACCOUNT_CAPABILITY_NAMES) { + response.put(capabilityName, value); + } + return response; + } + + /** + * List of parameters to run in capability fetching tests. + */ + public static class CapabilitiesTestParams implements ParameterProvider { + private static List<ParameterSet> sCapabilties = Arrays.asList( + new ParameterSet() + .name("CanRunChromePrivacySandboxTrials") + .value(AccountCapabilitiesConstants + .CAN_RUN_CHROME_PRIVACY_SANDBOX_TRIALS_CAPABILITY_NAME), + new ParameterSet() + .name("IsSubjectToParentalControls") + .value(AccountCapabilitiesConstants + .IS_SUBJECT_TO_PARENTAL_CONTROLS_CAPABILITY_NAME), + new ParameterSet() + .name("CanStopParentalSupervision") + .value(AccountCapabilitiesConstants + .CAN_STOP_PARENTAL_SUPERVISION_CAPABILITY_NAME), + new ParameterSet() + .name("CanOfferExtendedChromeSyncPromos") + .value(AccountCapabilitiesConstants + .CAN_OFFER_EXTENDED_CHROME_SYNC_PROMOS_CAPABILITY_NAME)); + + // Returns String value added from Capabilities ParameterSet. + static String getCapabilityName(ParameterSet parameterSet) { + String parameterSetString = parameterSet.toString(); + // Removes the brackets added by Arrays.toString for parameter value. + return parameterSetString.replace("[", "").replace("]", ""); + } + + static { + // Asserts that the list of parameters contains all supported capability names. + assert AccountCapabilitiesConstants.SUPPORTED_ACCOUNT_CAPABILITY_NAMES.containsAll( + Lists.transform(sCapabilties, (paramSet) -> getCapabilityName(paramSet))); + } + + @Override + public Iterable<ParameterSet> getParameters() { + return sCapabilties; + } + } + + @Before + public void setUp() { + mDelegate = spy(new FakeAccountManagerDelegate()); + } + + @Test + @ParameterAnnotations.UseMethodParameter(CapabilitiesTestParams.class) + public void testCapabilityResponseException(String capabilityName) { + AccountCapabilities capabilities = new AccountCapabilities(new HashMap<>()); + Assert.assertEquals(getCapability(capabilityName, capabilities), Tribool.UNKNOWN); + } + + @Test + @ParameterAnnotations.UseMethodParameter(CapabilitiesTestParams.class) + public void testCapabilityResponseYes(String capabilityName) { + AccountCapabilities capabilities = new AccountCapabilities(new HashMap<String, Boolean>() { + { put(capabilityName, true); } + }); + Assert.assertEquals(getCapability(capabilityName, capabilities), Tribool.TRUE); + } + + @Test + @ParameterAnnotations.UseMethodParameter(CapabilitiesTestParams.class) + public void testCapabilityResponseNo(String capabilityName) { + AccountCapabilities capabilities = new AccountCapabilities(new HashMap<String, Boolean>() { + { put(capabilityName, false); } + }); + Assert.assertEquals(getCapability(capabilityName, capabilities), Tribool.FALSE); + } + + @Test + public void testParseFromCapabilitiesResponseWithResponseYes() { + AccountCapabilities capabilities = AccountCapabilities.parseFromCapabilitiesResponse( + populateCapabilitiesResponse(AccountManagerDelegate.CapabilityResponse.YES)); + + for (String capabilityName : + AccountCapabilitiesConstants.SUPPORTED_ACCOUNT_CAPABILITY_NAMES) { + Assert.assertEquals(getCapability(capabilityName, capabilities), Tribool.TRUE); + } + } + + @Test + public void testParseFromCapabilitiesResponseWithResponseNo() { + AccountCapabilities capabilities = AccountCapabilities.parseFromCapabilitiesResponse( + populateCapabilitiesResponse(AccountManagerDelegate.CapabilityResponse.NO)); + + for (String capabilityName : + AccountCapabilitiesConstants.SUPPORTED_ACCOUNT_CAPABILITY_NAMES) { + Assert.assertEquals(getCapability(capabilityName, capabilities), Tribool.FALSE); + } + } + + @Test + public void testParseFromCapabilitiesResponseWithExceptionResponse() { + AccountCapabilities capabilities = AccountCapabilities.parseFromCapabilitiesResponse( + populateCapabilitiesResponse(AccountManagerDelegate.CapabilityResponse.EXCEPTION)); + + for (String capabilityName : + AccountCapabilitiesConstants.SUPPORTED_ACCOUNT_CAPABILITY_NAMES) { + Assert.assertEquals(getCapability(capabilityName, capabilities), Tribool.UNKNOWN); + } + } +} diff --git a/chromium/components/signin/public/android/junit/src/org/chromium/components/signin/identitymanager/AccountInfoServiceImplTest.java b/chromium/components/signin/public/android/junit/src/org/chromium/components/signin/identitymanager/AccountInfoServiceImplTest.java index 8250af21b91..2256f8da6b5 100644 --- a/chromium/components/signin/public/android/junit/src/org/chromium/components/signin/identitymanager/AccountInfoServiceImplTest.java +++ b/chromium/components/signin/public/android/junit/src/org/chromium/components/signin/identitymanager/AccountInfoServiceImplTest.java @@ -22,9 +22,12 @@ import org.mockito.junit.MockitoRule; import org.chromium.base.Promise; import org.chromium.base.test.BaseRobolectricTestRunner; +import org.chromium.components.signin.base.AccountCapabilities; import org.chromium.components.signin.base.AccountInfo; import org.chromium.components.signin.base.CoreAccountId; +import java.util.HashMap; + /** * Unit tests for {@link AccountInfoServiceImpl}. */ @@ -44,9 +47,9 @@ public class AccountInfoServiceImplTest { @Mock private AccountInfoService.Observer mObserverMock; - private final AccountInfo mAccountInfoWithAvatar = - new AccountInfo(new CoreAccountId("gaia-id-test"), ACCOUNT_EMAIL, "gaia-id-test", - "full name", "given name", mock(Bitmap.class)); + private final AccountInfo mAccountInfoWithAvatar = new AccountInfo( + new CoreAccountId("gaia-id-test"), ACCOUNT_EMAIL, "gaia-id-test", "full name", + "given name", mock(Bitmap.class), new AccountCapabilities(new HashMap<>())); private AccountInfoServiceImpl mService; diff --git a/chromium/components/signin/public/base/BUILD.gn b/chromium/components/signin/public/base/BUILD.gn index 3c8f186aab3..75f3787bc26 100644 --- a/chromium/components/signin/public/base/BUILD.gn +++ b/chromium/components/signin/public/base/BUILD.gn @@ -20,7 +20,6 @@ buildflag_header("signin_buildflags") { static_library("base") { sources = [ - "account_consistency_method.cc", "account_consistency_method.h", "avatar_icon_util.cc", "avatar_icon_util.h", diff --git a/chromium/components/signin/public/base/account_consistency_method.cc b/chromium/components/signin/public/base/account_consistency_method.cc deleted file mode 100644 index b1bc6af8743..00000000000 --- a/chromium/components/signin/public/base/account_consistency_method.cc +++ /dev/null @@ -1,16 +0,0 @@ -// Copyright 2014 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/public/base/account_consistency_method.h" - -#include "build/build_config.h" - -namespace signin { - -#if BUILDFLAG(IS_ANDROID) -const base::Feature kMobileIdentityConsistencyPromos{ - "MobileIdentityConsistencyPromos", base::FEATURE_ENABLED_BY_DEFAULT}; -#endif - -} // namespace signin diff --git a/chromium/components/signin/public/base/account_consistency_method.h b/chromium/components/signin/public/base/account_consistency_method.h index 06028904800..41ada0a346e 100644 --- a/chromium/components/signin/public/base/account_consistency_method.h +++ b/chromium/components/signin/public/base/account_consistency_method.h @@ -9,16 +9,8 @@ #ifndef COMPONENTS_SIGNIN_PUBLIC_BASE_ACCOUNT_CONSISTENCY_METHOD_H_ #define COMPONENTS_SIGNIN_PUBLIC_BASE_ACCOUNT_CONSISTENCY_METHOD_H_ -#include "base/feature_list.h" -#include "build/build_config.h" - namespace signin { -#if BUILDFLAG(IS_ANDROID) -// Feature flag for promo-related changes of `kMobileIdentityConsistency`. -extern const base::Feature kMobileIdentityConsistencyPromos; -#endif // BUILDFLAG(IS_ANDROID) - enum class AccountConsistencyMethod : int { // No account consistency. kDisabled, diff --git a/chromium/components/signin/public/base/signin_client.h b/chromium/components/signin/public/base/signin_client.h index 18e7c6d4b06..ebeae99e5dc 100644 --- a/chromium/components/signin/public/base/signin_client.h +++ b/chromium/components/signin/public/base/signin_client.h @@ -96,8 +96,16 @@ class SigninClient : public KeyedService { virtual bool IsNonEnterpriseUser(const std::string& username); #if BUILDFLAG(IS_CHROMEOS_LACROS) + // Returns an account used to sign into Chrome OS session if available. virtual absl::optional<account_manager::Account> GetInitialPrimaryAccount() = 0; + + // Returns whether account used to sign into Chrome OS is a child account. + // Returns nullopt for secondary / non-main profiles in LaCrOS. + virtual absl::optional<bool> IsInitialPrimaryAccountChild() const = 0; + + // Removes all accounts. + virtual void RemoveAllAccounts() = 0; #endif }; diff --git a/chromium/components/signin/public/base/signin_metrics.cc b/chromium/components/signin/public/base/signin_metrics.cc index d7d90415914..e086368f682 100644 --- a/chromium/components/signin/public/base/signin_metrics.cc +++ b/chromium/components/signin/public/base/signin_metrics.cc @@ -124,6 +124,10 @@ void RecordSigninUserActionForAccessPoint(AccessPoint access_point) { 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_KALEIDOSCOPE: NOTREACHED() << "Access point " << static_cast<int>(access_point) << " is only used to trigger non-sync sign-in and this" @@ -134,7 +138,6 @@ void RecordSigninUserActionForAccessPoint(AccessPoint access_point) { case AccessPoint::ACCESS_POINT_FORCED_SIGNIN: case AccessPoint::ACCESS_POINT_ACCOUNT_RENAMED: case AccessPoint::ACCESS_POINT_WEB_SIGNIN: - case AccessPoint::ACCESS_POINT_SIGNIN_INTERCEPT_FIRST_RUN_EXPERIENCE: NOTREACHED() << "Access point " << static_cast<int>(access_point) << " is not supposed to log signin user actions."; break; diff --git a/chromium/components/signin/public/base/signin_metrics.h b/chromium/components/signin/public/base/signin_metrics.h index 2523c63fab2..810bddfa4eb 100644 --- a/chromium/components/signin/public/base/signin_metrics.h +++ b/chromium/components/signin/public/base/signin_metrics.h @@ -60,6 +60,9 @@ enum ProfileSignout : int { // iOS Specific. Sign-out forced because the account was removed from the // device after a device restore. IOS_ACCOUNT_REMOVED_FROM_DEVICE_AFTER_RESTORE = 15, + // User clicked to 'Turn off sync' from the settings page. + // Currently only available for Android Unicorn users. + USER_CLICKED_REVOKE_SYNC_CONSENT_SETTINGS = 16, // Keep this as the last enum. NUM_PROFILE_SIGNOUT_METRICS, }; @@ -267,6 +270,8 @@ enum class AccountConsistencyPromoAction : int { SUPPRESSED_CONSECUTIVE_DISMISSALS = 16, // The timeout erreur was shown to the user. TIMEOUT_ERROR_SHOWN = 17, + // The web sign-in is not shown because the user is already signed in. + SUPPRESSED_ALREADY_SIGNED_IN = 18, MAX = 18, }; #endif // BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_IOS) diff --git a/chromium/components/signin/public/base/signin_switches.cc b/chromium/components/signin/public/base/signin_switches.cc index 7f2ff625e4a..1113d6ad79d 100644 --- a/chromium/components/signin/public/base/signin_switches.cc +++ b/chromium/components/signin/public/base/signin_switches.cc @@ -27,6 +27,19 @@ const char kClearTokenService[] = "clear-token-service"; // Disables sending signin scoped device id to LSO with refresh token request. const char kDisableSigninScopedDeviceId[] = "disable-signin-scoped-device-id"; +// Enables fetching account capabilities and populating AccountInfo with the +// fetch result. +// Disabled on iOS because this platform doesn't have a compatible +// `AccountCapabilitiesFetcher` implementation yet. +// TODO(https://crbug.com/1305191): implement feature on iOS. +#if BUILDFLAG(IS_IOS) +const base::Feature kEnableFetchingAccountCapabilities{ + "EnableFetchingAccountCapabilities", base::FEATURE_DISABLED_BY_DEFAULT}; +#else +const base::Feature kEnableFetchingAccountCapabilities{ + "EnableFetchingAccountCapabilities", base::FEATURE_ENABLED_BY_DEFAULT}; +#endif // BUILDFLAG(IS_IOS) + // This feature disables all extended sync promos. const base::Feature kForceDisableExtendedSyncPromos{ "ForceDisableExtendedSyncPromos", base::FEATURE_DISABLED_BY_DEFAULT}; diff --git a/chromium/components/signin/public/base/signin_switches.h b/chromium/components/signin/public/base/signin_switches.h index e394a215681..e8ccef399a2 100644 --- a/chromium/components/signin/public/base/signin_switches.h +++ b/chromium/components/signin/public/base/signin_switches.h @@ -31,6 +31,8 @@ extern const char kClearTokenService[]; extern const char kDisableSigninScopedDeviceId[]; +extern const base::Feature kEnableFetchingAccountCapabilities; + extern const base::Feature kForceDisableExtendedSyncPromos; #if BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_IOS) diff --git a/chromium/components/signin/public/base/test_signin_client.cc b/chromium/components/signin/public/base/test_signin_client.cc index 9eadc5a06fd..972b6f477c2 100644 --- a/chromium/components/signin/public/base/test_signin_client.cc +++ b/chromium/components/signin/public/base/test_signin_client.cc @@ -122,9 +122,17 @@ TestSigninClient::GetInitialPrimaryAccount() { return initial_primary_account_; } +absl::optional<bool> TestSigninClient::IsInitialPrimaryAccountChild() const { + return is_initial_primary_account_child_; +} + void TestSigninClient::SetInitialPrimaryAccountForTests( - const account_manager::Account& account) { + const account_manager::Account& account, + const absl::optional<bool>& is_child) { initial_primary_account_ = absl::make_optional(account); + is_initial_primary_account_child_ = is_child; } -#endif +void TestSigninClient::RemoveAllAccounts() {} + +#endif // BUILDFLAG(IS_CHROMEOS_LACROS) diff --git a/chromium/components/signin/public/base/test_signin_client.h b/chromium/components/signin/public/base/test_signin_client.h index 04a52683299..bf40e25fcc9 100644 --- a/chromium/components/signin/public/base/test_signin_client.h +++ b/chromium/components/signin/public/base/test_signin_client.h @@ -97,14 +97,15 @@ class TestSigninClient : public SigninClient { GaiaAuthConsumer* consumer, gaia::GaiaSource source) override; bool IsNonEnterpriseUser(const std::string& email) override; + #if BUILDFLAG(IS_CHROMEOS_LACROS) absl::optional<account_manager::Account> GetInitialPrimaryAccount() override; -#endif + absl::optional<bool> IsInitialPrimaryAccountChild() const override; -#if BUILDFLAG(IS_CHROMEOS_LACROS) - void SetInitialPrimaryAccountForTests( - const account_manager::Account& account); -#endif + void SetInitialPrimaryAccountForTests(const account_manager::Account& account, + const absl::optional<bool>& is_child); + void RemoveAllAccounts() override; +#endif // BUILDFLAG(IS_CHROMEOS_LACROS) private: std::unique_ptr<network::TestURLLoaderFactory> @@ -121,7 +122,8 @@ class TestSigninClient : public SigninClient { #if BUILDFLAG(IS_CHROMEOS_LACROS) absl::optional<account_manager::Account> initial_primary_account_; -#endif + absl::optional<bool> is_initial_primary_account_child_; +#endif // BUILDFLAG(IS_CHROMEOS_LACROS) }; #endif // COMPONENTS_SIGNIN_PUBLIC_BASE_TEST_SIGNIN_CLIENT_H_ diff --git a/chromium/components/signin/public/identity_manager/BUILD.gn b/chromium/components/signin/public/identity_manager/BUILD.gn index 4d751f923f6..e726761f763 100644 --- a/chromium/components/signin/public/identity_manager/BUILD.gn +++ b/chromium/components/signin/public/identity_manager/BUILD.gn @@ -147,6 +147,8 @@ source_set("test_support") { testonly = true sources = [ + "account_capabilities_test_mutator.cc", + "account_capabilities_test_mutator.h", "identity_test_environment.cc", "identity_test_environment.h", "identity_test_utils.cc", 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 d18d4d050dc..c7bb097e40e 100644 --- a/chromium/components/signin/public/identity_manager/access_token_constants.cc +++ b/chromium/components/signin/public/identity_manager/access_token_constants.cc @@ -61,6 +61,9 @@ const std::set<std::string> GetUnconsentedOAuth2Scopes() { // devices. Consent is obtained outside Chrome within Family Link flows. GaiaConstants::kKidFamilyReadonlyOAuth2Scope, + // Required to fetch the ManagedAccounsSigninRestriction policy. + GaiaConstants::kSecureConnectOAuth2Scope, + // Required by ChromeOS only. #if BUILDFLAG(IS_CHROMEOS_ASH) GaiaConstants::kAccountsReauthOAuth2Scope, diff --git a/chromium/components/signin/public/identity_manager/access_token_fetcher_unittest.cc b/chromium/components/signin/public/identity_manager/access_token_fetcher_unittest.cc index e9db325aa28..d6229895a85 100644 --- a/chromium/components/signin/public/identity_manager/access_token_fetcher_unittest.cc +++ b/chromium/components/signin/public/identity_manager/access_token_fetcher_unittest.cc @@ -14,7 +14,6 @@ #include "components/prefs/testing_pref_service.h" #include "components/signin/internal/identity_manager/account_tracker_service.h" #include "components/signin/internal/identity_manager/fake_profile_oauth2_token_service.h" -#include "components/signin/internal/identity_manager/primary_account_policy_manager_impl.h" #include "components/signin/internal/identity_manager/profile_oauth2_token_service_delegate.h" #include "components/signin/public/base/signin_pref_names.h" #include "components/signin/public/base/test_signin_client.h" @@ -67,8 +66,7 @@ class AccessTokenFetcherTest account_tracker_(std::make_unique<AccountTrackerService>()), primary_account_manager_(&signin_client_, &token_service_, - account_tracker_.get(), - nullptr /* policy_manager */) { + account_tracker_.get()) { AccountTrackerService::RegisterPrefs(pref_service_.registry()); PrimaryAccountManager::RegisterProfilePrefs(pref_service_.registry()); diff --git a/chromium/components/signin/public/identity_manager/account_capabilities.cc b/chromium/components/signin/public/identity_manager/account_capabilities.cc index 932ba4f6fc3..9a224dff02c 100644 --- a/chromium/components/signin/public/identity_manager/account_capabilities.cc +++ b/chromium/components/signin/public/identity_manager/account_capabilities.cc @@ -2,24 +2,142 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include <map> +#include <string> +#include <vector> + #include "components/signin/public/identity_manager/account_capabilities.h" + +#include "base/no_destructor.h" +#include "components/signin/internal/identity_manager/account_capabilities_constants.h" #include "components/signin/public/identity_manager/tribool.h" +#if BUILDFLAG(IS_ANDROID) +#include "base/android/jni_array.h" +#include "base/android/jni_string.h" +#include "components/signin/public/android/jni_headers/AccountCapabilities_jni.h" +#endif + +AccountCapabilities::AccountCapabilities() = default; +AccountCapabilities::~AccountCapabilities() = default; +AccountCapabilities::AccountCapabilities(const AccountCapabilities& other) = + default; +AccountCapabilities::AccountCapabilities(AccountCapabilities&& other) noexcept = + default; +AccountCapabilities& AccountCapabilities::operator=( + const AccountCapabilities& other) = default; +AccountCapabilities& AccountCapabilities::operator=( + AccountCapabilities&& other) noexcept = default; + +// static +const std::vector<std::string>& +AccountCapabilities::GetSupportedAccountCapabilityNames() { + static base::NoDestructor<std::vector<std::string>> kCapabilityNames{{ +#define ACCOUNT_CAPABILITY(cpp_label, java_label, value) cpp_label, +#include "components/signin/internal/identity_manager/account_capabilities_list.h" +#undef ACCOUNT_CAPABILITY + }}; + return *kCapabilityNames; +} + bool AccountCapabilities::AreAllCapabilitiesKnown() const { - return can_offer_extended_chrome_sync_promos_ != signin::Tribool::kUnknown; + for (const std::string& capability_name : + GetSupportedAccountCapabilityNames()) { + if (GetCapabilityByName(capability_name) == signin::Tribool::kUnknown) { + return false; + } + } + return true; +} + +signin::Tribool AccountCapabilities::GetCapabilityByName( + const std::string& name) const { + const auto iterator = capabilities_map_.find(name); + if (iterator == capabilities_map_.end()) { + return signin::Tribool::kUnknown; + } + return iterator->second ? signin::Tribool::kTrue : signin::Tribool::kFalse; +} + +signin::Tribool AccountCapabilities::can_offer_extended_chrome_sync_promos() + const { + return GetCapabilityByName(kCanOfferExtendedChromeSyncPromosCapabilityName); +} + +signin::Tribool AccountCapabilities::can_run_chrome_privacy_sandbox_trials() + const { + return GetCapabilityByName(kCanRunChromePrivacySandboxTrialsCapabilityName); +} + +signin::Tribool AccountCapabilities::can_stop_parental_supervision() const { + return GetCapabilityByName(kCanStopParentalSupervisionCapabilityName); +} + +signin::Tribool AccountCapabilities::is_subject_to_parental_controls() const { + return GetCapabilityByName(kIsSubjectToParentalControlsCapabilityName); } bool AccountCapabilities::UpdateWith(const AccountCapabilities& other) { bool modified = false; - if (other.can_offer_extended_chrome_sync_promos_ != - signin::Tribool::kUnknown && - other.can_offer_extended_chrome_sync_promos_ != - can_offer_extended_chrome_sync_promos_) { - can_offer_extended_chrome_sync_promos_ = - other.can_offer_extended_chrome_sync_promos_; - modified = true; + for (const std::string& name : GetSupportedAccountCapabilityNames()) { + signin::Tribool other_capability = other.GetCapabilityByName(name); + signin::Tribool current_capability = GetCapabilityByName(name); + if (other_capability != signin::Tribool::kUnknown && + other_capability != current_capability) { + capabilities_map_[name] = other_capability == signin::Tribool::kTrue; + modified = true; + } } return modified; } + +bool AccountCapabilities::operator==(const AccountCapabilities& other) const { + for (const std::string& name : GetSupportedAccountCapabilityNames()) { + if (GetCapabilityByName(name) != other.GetCapabilityByName(name)) + return false; + } + return true; +} + +bool AccountCapabilities::operator!=(const AccountCapabilities& other) const { + return !(*this == other); +} + +#if BUILDFLAG(IS_ANDROID) +// static +AccountCapabilities AccountCapabilities::ConvertFromJavaAccountCapabilities( + JNIEnv* env, + const base::android::JavaRef<jobject>& account_capabilities) { + AccountCapabilities capabilities; + for (const std::string& name : GetSupportedAccountCapabilityNames()) { + signin::Tribool capability_state = static_cast<signin::Tribool>( + signin::Java_AccountCapabilities_getCapabilityByName( + env, account_capabilities, + base::android::ConvertUTF8ToJavaString(env, name))); + if (capability_state != signin::Tribool::kUnknown) { + capabilities.capabilities_map_[name] = + capability_state == signin::Tribool::kTrue; + } + } + return capabilities; +} + +base::android::ScopedJavaLocalRef<jobject> +AccountCapabilities::ConvertToJavaAccountCapabilities(JNIEnv* env) const { + int capabilities_size = capabilities_map_.size(); + std::vector<std::string> capability_names; + auto capability_values = std::make_unique<bool[]>(capabilities_size); + int value_iterator = 0; + for (const auto& kv : capabilities_map_) { + capability_names.push_back(kv.first); + capability_values[value_iterator] = kv.second; + value_iterator++; + } + return signin::Java_AccountCapabilities_Constructor( + env, base::android::ToJavaArrayOfStrings(env, capability_names), + base::android::ToJavaBooleanArray(env, capability_values.get(), + capabilities_size)); +} +#endif diff --git a/chromium/components/signin/public/identity_manager/account_capabilities.h b/chromium/components/signin/public/identity_manager/account_capabilities.h index cf191601031..9bc27f627dd 100644 --- a/chromium/components/signin/public/identity_manager/account_capabilities.h +++ b/chromium/components/signin/public/identity_manager/account_capabilities.h @@ -5,34 +5,81 @@ #ifndef COMPONENTS_SIGNIN_PUBLIC_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_H_ #define COMPONENTS_SIGNIN_PUBLIC_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_H_ +#include <string> +#include <vector> + +#include "base/containers/flat_map.h" +#include "build/build_config.h" #include "components/signin/public/identity_manager/tribool.h" +#include "third_party/abseil-cpp/absl/types/optional.h" + +#if BUILDFLAG(IS_ANDROID) +#include "base/android/scoped_java_ref.h" +#endif + +namespace base { +class Value; +} // Stores the information about account capabilities. Capabilities provide // information about state and features of Gaia accounts. class AccountCapabilities { public: + AccountCapabilities(); + ~AccountCapabilities(); + AccountCapabilities(const AccountCapabilities& other); + AccountCapabilities(AccountCapabilities&& other) noexcept; + AccountCapabilities& operator=(const AccountCapabilities& other); + AccountCapabilities& operator=(AccountCapabilities&& other) noexcept; + +#if BUILDFLAG(IS_ANDROID) + static AccountCapabilities ConvertFromJavaAccountCapabilities( + JNIEnv* env, + const base::android::JavaRef<jobject>& accountCapabilities); + + base::android::ScopedJavaLocalRef<jobject> ConvertToJavaAccountCapabilities( + JNIEnv* env) const; +#endif // Chrome can offer extended promos for turning on Sync to accounts with this // capability. - signin::Tribool can_offer_extended_chrome_sync_promos() const { - return can_offer_extended_chrome_sync_promos_; - } + signin::Tribool can_offer_extended_chrome_sync_promos() const; + + // Chrome can run privacy sandbox trials for accounts with this capability. + signin::Tribool can_run_chrome_privacy_sandbox_trials() const; - void set_can_offer_extended_chrome_sync_promos(bool value) { - can_offer_extended_chrome_sync_promos_ = - value ? signin::Tribool::kTrue : signin::Tribool::kFalse; - } + // Chrome can stop parental supervision if the user chooses to do so with + // this capability. + signin::Tribool can_stop_parental_supervision() const; + + // Chrome applies parental controls to accounts with this capability. + signin::Tribool is_subject_to_parental_controls() const; // Whether none of the capabilities has `signin::Tribool::kUnknown`. bool AreAllCapabilitiesKnown() const; - // Updates the fields of `this` with known fields of `other`. Returns whether - // at least one field was updated. + // Updates the capability state value for keys in `other`. If a value is + // `signin::Tribool::kUnknown` in `other` the corresponding key will not + // be updated in order to avoid overriding known values. bool UpdateWith(const AccountCapabilities& other); + bool operator==(const AccountCapabilities& other) const; + bool operator!=(const AccountCapabilities& other) const; + private: - signin::Tribool can_offer_extended_chrome_sync_promos_ = - signin::Tribool::kUnknown; + friend absl::optional<AccountCapabilities> AccountCapabilitiesFromValue( + const base::Value& account_capabilities); + friend class AccountCapabilitiesFetcherGaia; + friend class AccountCapabilitiesTestMutator; + friend class AccountTrackerService; + + // Returns the capability state using the service name. + signin::Tribool GetCapabilityByName(const std::string& name) const; + + // Returns the list of account capability service names supported in Chrome. + static const std::vector<std::string>& GetSupportedAccountCapabilityNames(); + + base::flat_map<std::string, bool> capabilities_map_; }; #endif // COMPONENTS_SIGNIN_PUBLIC_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_H_ 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 new file mode 100644 index 00000000000..40a4a7db791 --- /dev/null +++ b/chromium/components/signin/public/identity_manager/account_capabilities_test_mutator.cc @@ -0,0 +1,50 @@ +// Copyright 2022 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/public/identity_manager/account_capabilities_test_mutator.h" + +#include "components/signin/internal/identity_manager/account_capabilities_constants.h" + +AccountCapabilitiesTestMutator::AccountCapabilitiesTestMutator( + AccountCapabilities* capabilities) + : capabilities_(capabilities) {} + +// static +const std::vector<std::string>& +AccountCapabilitiesTestMutator::GetSupportedAccountCapabilityNames() { + return AccountCapabilities::GetSupportedAccountCapabilityNames(); +} + +void AccountCapabilitiesTestMutator::set_can_offer_extended_chrome_sync_promos( + bool value) { + capabilities_ + ->capabilities_map_[kCanOfferExtendedChromeSyncPromosCapabilityName] = + value; +} + +void AccountCapabilitiesTestMutator::set_can_run_chrome_privacy_sandbox_trials( + bool value) { + capabilities_ + ->capabilities_map_[kCanRunChromePrivacySandboxTrialsCapabilityName] = + value; +} + +void AccountCapabilitiesTestMutator::set_can_stop_parental_supervision( + bool value) { + capabilities_->capabilities_map_[kCanStopParentalSupervisionCapabilityName] = + value; +} + +void AccountCapabilitiesTestMutator::set_is_subject_to_parental_controls( + bool value) { + capabilities_->capabilities_map_[kIsSubjectToParentalControlsCapabilityName] = + value; +} + +void AccountCapabilitiesTestMutator::SetAllSupportedCapabilities(bool value) { + for (const std::string& name : + AccountCapabilities::GetSupportedAccountCapabilityNames()) { + capabilities_->capabilities_map_[name] = 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 new file mode 100644 index 00000000000..2c40cd011a9 --- /dev/null +++ b/chromium/components/signin/public/identity_manager/account_capabilities_test_mutator.h @@ -0,0 +1,32 @@ +// Copyright 2022 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_PUBLIC_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_TEST_MUTATOR_H_ +#define COMPONENTS_SIGNIN_PUBLIC_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_TEST_MUTATOR_H_ + +#include "components/signin/public/identity_manager/account_capabilities.h" + +// Support class that allows callers to modify internal capability state +// mappings used for tests. +class AccountCapabilitiesTestMutator { + public: + explicit AccountCapabilitiesTestMutator(AccountCapabilities* capabilities); + + // Exposes the full list of supported capabilities for tests. + static const std::vector<std::string>& GetSupportedAccountCapabilityNames(); + + // Exposes setters for the supported capabilities. + void set_can_offer_extended_chrome_sync_promos(bool value); + 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); + + // Modifies all supported capabilities at once. + void SetAllSupportedCapabilities(bool value); + + private: + AccountCapabilities* capabilities_; +}; + +#endif // COMPONENTS_SIGNIN_PUBLIC_IDENTITY_MANAGER_ACCOUNT_CAPABILITIES_TEST_MUTATOR_H_ 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 0cbffa5dd49..bfde01b5409 100644 --- a/chromium/components/signin/public/identity_manager/account_capabilities_unittest.cc +++ b/chromium/components/signin/public/identity_manager/account_capabilities_unittest.cc @@ -3,9 +3,14 @@ // found in the LICENSE file. #include "components/signin/public/identity_manager/account_capabilities.h" +#include "components/signin/public/identity_manager/account_capabilities_test_mutator.h" #include "testing/gtest/include/gtest/gtest.h" +#if BUILDFLAG(IS_ANDROID) +#include "base/android/jni_android.h" +#endif + class AccountCapabilitiesTest : public testing::Test {}; TEST_F(AccountCapabilitiesTest, CanOfferExtendedChromeSyncPromos) { @@ -13,23 +18,79 @@ TEST_F(AccountCapabilitiesTest, CanOfferExtendedChromeSyncPromos) { EXPECT_EQ(capabilities.can_offer_extended_chrome_sync_promos(), signin::Tribool::kUnknown); - capabilities.set_can_offer_extended_chrome_sync_promos(true); + AccountCapabilitiesTestMutator mutator(&capabilities); + mutator.set_can_offer_extended_chrome_sync_promos(true); EXPECT_EQ(capabilities.can_offer_extended_chrome_sync_promos(), signin::Tribool::kTrue); - capabilities.set_can_offer_extended_chrome_sync_promos(false); + mutator.set_can_offer_extended_chrome_sync_promos(false); EXPECT_EQ(capabilities.can_offer_extended_chrome_sync_promos(), signin::Tribool::kFalse); } +TEST_F(AccountCapabilitiesTest, CanRunChromePrivacySandboxTrials) { + AccountCapabilities capabilities; + EXPECT_EQ(capabilities.can_run_chrome_privacy_sandbox_trials(), + signin::Tribool::kUnknown); + + AccountCapabilitiesTestMutator mutator(&capabilities); + mutator.set_can_run_chrome_privacy_sandbox_trials(true); + EXPECT_EQ(capabilities.can_run_chrome_privacy_sandbox_trials(), + signin::Tribool::kTrue); + + mutator.set_can_run_chrome_privacy_sandbox_trials(false); + EXPECT_EQ(capabilities.can_run_chrome_privacy_sandbox_trials(), + signin::Tribool::kFalse); +} + +TEST_F(AccountCapabilitiesTest, CanStopParentalSupervision) { + AccountCapabilities capabilities; + EXPECT_EQ(capabilities.can_stop_parental_supervision(), + signin::Tribool::kUnknown); + + AccountCapabilitiesTestMutator mutator(&capabilities); + mutator.set_can_stop_parental_supervision(true); + EXPECT_EQ(capabilities.can_stop_parental_supervision(), + signin::Tribool::kTrue); + + mutator.set_can_stop_parental_supervision(false); + EXPECT_EQ(capabilities.can_stop_parental_supervision(), + signin::Tribool::kFalse); +} + +TEST_F(AccountCapabilitiesTest, IsSubjectToParentalControls) { + AccountCapabilities capabilities; + EXPECT_EQ(capabilities.is_subject_to_parental_controls(), + signin::Tribool::kUnknown); + + AccountCapabilitiesTestMutator mutator(&capabilities); + mutator.set_is_subject_to_parental_controls(true); + EXPECT_EQ(capabilities.is_subject_to_parental_controls(), + signin::Tribool::kTrue); + + mutator.set_is_subject_to_parental_controls(false); + EXPECT_EQ(capabilities.is_subject_to_parental_controls(), + signin::Tribool::kFalse); +} + TEST_F(AccountCapabilitiesTest, AreAllCapabilitiesKnown_Empty) { AccountCapabilities capabilities; EXPECT_FALSE(capabilities.AreAllCapabilitiesKnown()); } +TEST_F(AccountCapabilitiesTest, AreAllCapabilitiesKnown_PartiallyFilled) { + AccountCapabilities capabilities; + + AccountCapabilitiesTestMutator mutator(&capabilities); + mutator.set_can_offer_extended_chrome_sync_promos(true); + EXPECT_FALSE(capabilities.AreAllCapabilitiesKnown()); +} + TEST_F(AccountCapabilitiesTest, AreAllCapabilitiesKnown_Filled) { AccountCapabilities capabilities; - capabilities.set_can_offer_extended_chrome_sync_promos(true); + + AccountCapabilitiesTestMutator mutator(&capabilities); + mutator.SetAllSupportedCapabilities(true); EXPECT_TRUE(capabilities.AreAllCapabilitiesKnown()); } @@ -37,7 +98,8 @@ TEST_F(AccountCapabilitiesTest, UpdateWith_UnknownToKnown) { AccountCapabilities capabilities; AccountCapabilities other; - other.set_can_offer_extended_chrome_sync_promos(true); + AccountCapabilitiesTestMutator mutator(&other); + mutator.set_can_offer_extended_chrome_sync_promos(true); EXPECT_TRUE(capabilities.UpdateWith(other)); EXPECT_EQ(signin::Tribool::kTrue, @@ -46,7 +108,8 @@ TEST_F(AccountCapabilitiesTest, UpdateWith_UnknownToKnown) { TEST_F(AccountCapabilitiesTest, UpdateWith_KnownToUnknown) { AccountCapabilities capabilities; - capabilities.set_can_offer_extended_chrome_sync_promos(true); + AccountCapabilitiesTestMutator mutator(&capabilities); + mutator.set_can_offer_extended_chrome_sync_promos(true); AccountCapabilities other; @@ -57,12 +120,61 @@ TEST_F(AccountCapabilitiesTest, UpdateWith_KnownToUnknown) { TEST_F(AccountCapabilitiesTest, UpdateWith_OverwriteKnown) { AccountCapabilities capabilities; - capabilities.set_can_offer_extended_chrome_sync_promos(true); + AccountCapabilitiesTestMutator mutator(&capabilities); + mutator.set_can_offer_extended_chrome_sync_promos(true); AccountCapabilities other; - other.set_can_offer_extended_chrome_sync_promos(false); + AccountCapabilitiesTestMutator other_mutator(&other); + other_mutator.set_can_offer_extended_chrome_sync_promos(false); EXPECT_TRUE(capabilities.UpdateWith(other)); EXPECT_EQ(signin::Tribool::kFalse, capabilities.can_offer_extended_chrome_sync_promos()); } + +#if BUILDFLAG(IS_ANDROID) + +TEST_F(AccountCapabilitiesTest, ConversionWithJNI_TriboolTrue) { + AccountCapabilities capabilities; + AccountCapabilitiesTestMutator mutator(&capabilities); + mutator.set_can_offer_extended_chrome_sync_promos(true); + + JNIEnv* env = base::android::AttachCurrentThread(); + base::android::ScopedJavaLocalRef<jobject> java_capabilities = + capabilities.ConvertToJavaAccountCapabilities(env); + AccountCapabilities converted_back = + AccountCapabilities::ConvertFromJavaAccountCapabilities( + env, java_capabilities); + + EXPECT_EQ(capabilities, converted_back); +} + +TEST_F(AccountCapabilitiesTest, ConversionWithJNI_TriboolFalse) { + AccountCapabilities capabilities; + AccountCapabilitiesTestMutator mutator(&capabilities); + mutator.set_can_offer_extended_chrome_sync_promos(false); + + JNIEnv* env = base::android::AttachCurrentThread(); + base::android::ScopedJavaLocalRef<jobject> java_capabilities = + capabilities.ConvertToJavaAccountCapabilities(env); + AccountCapabilities converted_back = + AccountCapabilities::ConvertFromJavaAccountCapabilities( + env, java_capabilities); + + EXPECT_EQ(capabilities, converted_back); +} + +TEST_F(AccountCapabilitiesTest, ConversionWithJNI_TriboolUnknown) { + AccountCapabilities capabilities; + + JNIEnv* env = base::android::AttachCurrentThread(); + base::android::ScopedJavaLocalRef<jobject> java_capabilities = + capabilities.ConvertToJavaAccountCapabilities(env); + AccountCapabilities converted_back = + AccountCapabilities::ConvertFromJavaAccountCapabilities( + env, java_capabilities); + + EXPECT_EQ(capabilities, converted_back); +} + +#endif diff --git a/chromium/components/signin/public/identity_manager/account_info.cc b/chromium/components/signin/public/identity_manager/account_info.cc index 06fe0eb79e8..8b00ad2aa29 100644 --- a/chromium/components/signin/public/identity_manager/account_info.cc +++ b/chromium/components/signin/public/identity_manager/account_info.cc @@ -174,7 +174,8 @@ base::android::ScopedJavaLocalRef<jobject> ConvertToJavaAccountInfo( base::android::ConvertUTF8ToJavaString(env, account_info.given_name), avatar_image.IsEmpty() ? nullptr - : gfx::ConvertToJavaBitmap(*avatar_image.AsImageSkia().bitmap())); + : gfx::ConvertToJavaBitmap(*avatar_image.AsImageSkia().bitmap()), + account_info.capabilities.ConvertToJavaAccountCapabilities(env)); } base::android::ScopedJavaLocalRef<jobject> ConvertToJavaCoreAccountId( diff --git a/chromium/components/signin/public/identity_manager/account_info_unittest.cc b/chromium/components/signin/public/identity_manager/account_info_unittest.cc index 2ab2f7f5856..58f2c981b84 100644 --- a/chromium/components/signin/public/identity_manager/account_info_unittest.cc +++ b/chromium/components/signin/public/identity_manager/account_info_unittest.cc @@ -4,6 +4,7 @@ #include "components/signin/public/identity_manager/account_info.h" #include "components/signin/public/identity_manager/account_capabilities.h" +#include "components/signin/public/identity_manager/account_capabilities_test_mutator.h" #include "testing/gtest/include/gtest/gtest.h" class AccountInfoTest : public testing::Test {}; @@ -95,7 +96,8 @@ TEST_F(AccountInfoTest, UpdateWithSuccessfulUpdate) { other.account_id = CoreAccountId("test_id"); other.full_name = other.given_name = "test_name"; other.is_child_account = signin::Tribool::kTrue; - other.capabilities.set_can_offer_extended_chrome_sync_promos(true); + AccountCapabilitiesTestMutator mutator(&other.capabilities); + mutator.set_can_offer_extended_chrome_sync_promos(true); EXPECT_TRUE(info.UpdateWith(other)); EXPECT_EQ("test_id", info.gaia); diff --git a/chromium/components/signin/public/identity_manager/diagnostics_provider_unittest.cc b/chromium/components/signin/public/identity_manager/diagnostics_provider_unittest.cc index 185370584e5..78aebaa07bb 100644 --- a/chromium/components/signin/public/identity_manager/diagnostics_provider_unittest.cc +++ b/chromium/components/signin/public/identity_manager/diagnostics_provider_unittest.cc @@ -6,6 +6,7 @@ #include "base/callback_helpers.h" #include "base/test/task_environment.h" +#include "base/time/time.h" #include "components/signin/public/identity_manager/accounts_cookie_mutator.h" #include "components/signin/public/identity_manager/identity_test_environment.h" #include "components/signin/public/identity_manager/load_credentials_state.h" diff --git a/chromium/components/signin/public/identity_manager/identity_manager.cc b/chromium/components/signin/public/identity_manager/identity_manager.cc index d1bcdddae0f..2d8d85cd4da 100644 --- a/chromium/components/signin/public/identity_manager/identity_manager.cc +++ b/chromium/components/signin/public/identity_manager/identity_manager.cc @@ -7,14 +7,18 @@ #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" #include "components/signin/internal/identity_manager/account_fetcher_service.h" #include "components/signin/internal/identity_manager/account_tracker_service.h" #include "components/signin/internal/identity_manager/gaia_cookie_manager_service.h" #include "components/signin/internal/identity_manager/ubertoken_fetcher_impl.h" +#include "components/signin/public/base/consent_level.h" #include "components/signin/public/base/signin_buildflags.h" #include "components/signin/public/base/signin_client.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" @@ -38,6 +42,8 @@ #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 namespace signin { @@ -45,10 +51,13 @@ namespace signin { namespace { #if BUILDFLAG(IS_CHROMEOS_LACROS) + void SetPrimaryAccount(IdentityManager* identity_manager, AccountTrackerService* account_tracker_service, SigninClient* signin_client, - const account_manager::Account& device_account) { + const account_manager::Account& device_account, + signin::Tribool device_account_is_child, + ConsentLevel requested_level) { if (device_account.key.account_type() != account_manager::AccountType::kGaia) return; @@ -61,13 +70,17 @@ void SetPrimaryAccount(IdentityManager* identity_manager, account_tracker_service->SeedAccountInfo( /*gaia=*/device_account.key.id(), device_account.raw_email); - // TODO(https://crbug.com/1194983): Figure out how split sync settings will - // work here. For now, we will mimic Ash's behaviour of having sync turned on - // by default. const CoreAccountId primary_account_id = - identity_manager->GetPrimaryAccountId(ConsentLevel::kSync); - if (primary_account_id == device_account_id) + identity_manager->GetPrimaryAccountId(requested_level); + DCHECK(signin_client); + + if (primary_account_id == device_account_id) { + identity_manager->GetAccountsMutator()->UpdateAccountInfo( + device_account_id, device_account_is_child, signin::Tribool::kUnknown); + return; // Already correct primary account set, nothing to do. + } + if (!primary_account_id.empty()) { // Different primary account found, have to clear it first. // TODO(https://crbug.com/1223364): Replace this if with a CHECK after all @@ -76,13 +89,16 @@ void SetPrimaryAccount(IdentityManager* identity_manager, signin_metrics::ACCOUNT_REMOVED_FROM_DEVICE, signin_metrics::SignoutDelete::kIgnoreMetric); } + PrimaryAccountMutator::PrimaryAccountError error = identity_manager->GetPrimaryAccountMutator()->SetPrimaryAccount( - device_account_id, ConsentLevel::kSync); + device_account_id, requested_level); + identity_manager->GetAccountsMutator()->UpdateAccountInfo( + device_account_id, device_account_is_child, signin::Tribool::kUnknown); CHECK_EQ(PrimaryAccountMutator::PrimaryAccountError::kNoError, error) << "SetPrimaryAccount error: " << static_cast<int>(error); - CHECK(identity_manager->HasPrimaryAccount(ConsentLevel::kSync)); - CHECK_EQ(identity_manager->GetPrimaryAccountInfo(ConsentLevel::kSync).gaia, + CHECK(identity_manager->HasPrimaryAccount(requested_level)); + CHECK_EQ(identity_manager->GetPrimaryAccountInfo(requested_level).gaia, device_account.key.id()); } #endif @@ -155,8 +171,19 @@ IdentityManager::IdentityManager(IdentityManager::InitParameters&& parameters) absl::optional<account_manager::Account> initial_account = signin_client_->GetInitialPrimaryAccount(); if (initial_account.has_value()) { - SetPrimaryAccount(this, account_tracker_service_.get(), signin_client_, - initial_account.value()); + 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 + + ); } #endif } diff --git a/chromium/components/signin/public/identity_manager/identity_manager_builder.cc b/chromium/components/signin/public/identity_manager/identity_manager_builder.cc index b3d1243ee17..904912c9ff1 100644 --- a/chromium/components/signin/public/identity_manager/identity_manager_builder.cc +++ b/chromium/components/signin/public/identity_manager/identity_manager_builder.cc @@ -11,6 +11,7 @@ #include "build/chromeos_buildflags.h" #include "components/image_fetcher/core/image_decoder.h" #include "components/prefs/pref_service.h" +#include "components/signin/internal/identity_manager/account_capabilities_fetcher_factory.h" #include "components/signin/internal/identity_manager/account_fetcher_service.h" #include "components/signin/internal/identity_manager/account_tracker_service.h" #include "components/signin/internal/identity_manager/accounts_cookie_mutator_impl.h" @@ -18,7 +19,6 @@ #include "components/signin/internal/identity_manager/gaia_cookie_manager_service.h" #include "components/signin/internal/identity_manager/primary_account_manager.h" #include "components/signin/internal/identity_manager/primary_account_mutator_impl.h" -#include "components/signin/internal/identity_manager/primary_account_policy_manager.h" #include "components/signin/internal/identity_manager/profile_oauth2_token_service.h" #include "components/signin/internal/identity_manager/profile_oauth2_token_service_builder.h" #include "components/signin/public/base/account_consistency_method.h" @@ -26,9 +26,12 @@ #include "components/signin/public/identity_manager/accounts_mutator.h" #include "components/signin/public/identity_manager/device_accounts_synchronizer.h" -#if !BUILDFLAG(IS_ANDROID) +#if BUILDFLAG(IS_ANDROID) +#include "components/signin/internal/identity_manager/account_capabilities_fetcher_factory_android.h" +#else +#include "components/signin/internal/identity_manager/account_capabilities_fetcher_factory_gaia.h" #include "components/signin/public/webdata/token_web_data.h" -#endif +#endif // BUILDFLAG(IS_ANDROID) #if BUILDFLAG(IS_IOS) #include "components/signin/public/identity_manager/ios/device_accounts_provider.h" @@ -42,10 +45,6 @@ #include "components/signin/internal/identity_manager/accounts_mutator_impl.h" #endif -#if !BUILDFLAG(IS_CHROMEOS_ASH) -#include "components/signin/internal/identity_manager/primary_account_policy_manager_impl.h" -#endif - namespace signin { namespace { @@ -64,13 +63,8 @@ std::unique_ptr<PrimaryAccountManager> BuildPrimaryAccountManager( ProfileOAuth2TokenService* token_service, PrefService* local_state) { std::unique_ptr<PrimaryAccountManager> primary_account_manager; - std::unique_ptr<PrimaryAccountPolicyManager> policy_manager; -#if !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_IOS) - policy_manager = std::make_unique<PrimaryAccountPolicyManagerImpl>(client); -#endif primary_account_manager = std::make_unique<PrimaryAccountManager>( - client, token_service, account_tracker_service, - std::move(policy_manager)); + client, token_service, account_tracker_service); primary_account_manager->Initialize(local_state); return primary_account_manager; } @@ -92,11 +86,14 @@ std::unique_ptr<AccountFetcherService> BuildAccountFetcherService( SigninClient* signin_client, ProfileOAuth2TokenService* token_service, AccountTrackerService* account_tracker_service, - std::unique_ptr<image_fetcher::ImageDecoder> image_decoder) { + std::unique_ptr<image_fetcher::ImageDecoder> image_decoder, + std::unique_ptr<AccountCapabilitiesFetcherFactory> + account_capabilities_fetcher_factory) { auto account_fetcher_service = std::make_unique<AccountFetcherService>(); - account_fetcher_service->Initialize(signin_client, token_service, - account_tracker_service, - std::move(image_decoder)); + account_fetcher_service->Initialize( + signin_client, token_service, account_tracker_service, + std::move(image_decoder), + std::move(account_capabilities_fetcher_factory)); return account_fetcher_service; } @@ -157,9 +154,21 @@ IdentityManager::InitParameters BuildIdentityManagerInitParameters( init_params.diagnostics_provider = std::make_unique<DiagnosticsProviderImpl>( token_service.get(), gaia_cookie_manager_service.get()); + std::unique_ptr<AccountCapabilitiesFetcherFactory> + account_capabilities_fetcher_factory; +#if BUILDFLAG(IS_ANDROID) + account_capabilities_fetcher_factory = + std::make_unique<AccountCapabilitiesFetcherFactoryAndroid>(); +#else + account_capabilities_fetcher_factory = + std::make_unique<AccountCapabilitiesFetcherFactoryGaia>( + token_service.get(), params->signin_client); +#endif // BULIDFLAG(IS_ANDROID) + init_params.account_fetcher_service = BuildAccountFetcherService( params->signin_client, token_service.get(), account_tracker_service.get(), - std::move(params->image_decoder)); + std::move(params->image_decoder), + std::move(account_capabilities_fetcher_factory)); #if BUILDFLAG(IS_IOS) || BUILDFLAG(IS_ANDROID) init_params.device_accounts_synchronizer = 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 5fb341a9e2e..a71f5a10746 100644 --- a/chromium/components/signin/public/identity_manager/identity_manager_unittest.cc +++ b/chromium/components/signin/public/identity_manager/identity_manager_unittest.cc @@ -27,11 +27,11 @@ #include "components/signin/internal/identity_manager/accounts_mutator_impl.h" #include "components/signin/internal/identity_manager/device_accounts_synchronizer_impl.h" #include "components/signin/internal/identity_manager/diagnostics_provider_impl.h" +#include "components/signin/internal/identity_manager/fake_account_capabilities_fetcher_factory.h" #include "components/signin/internal/identity_manager/fake_profile_oauth2_token_service.h" #include "components/signin/internal/identity_manager/gaia_cookie_manager_service.h" #include "components/signin/internal/identity_manager/primary_account_manager.h" #include "components/signin/internal/identity_manager/primary_account_mutator_impl.h" -#include "components/signin/internal/identity_manager/primary_account_policy_manager_impl.h" #include "components/signin/internal/identity_manager/profile_oauth2_token_service_delegate.h" #include "components/signin/public/base/account_consistency_method.h" #include "components/signin/public/base/consent_level.h" @@ -60,6 +60,10 @@ #include "components/signin/internal/identity_manager/child_account_info_fetcher_android.h" #endif +#if BUILDFLAG(IS_CHROMEOS_LACROS) +#include "third_party/abseil-cpp/absl/types/optional.h" +#endif + #if BUILDFLAG(IS_CHROMEOS_ASH) #include "ash/components/account_manager/account_manager_factory.h" #include "components/account_manager_core/account.h" @@ -378,7 +382,8 @@ class IdentityManagerTest : public testing::Test { auto token_service = std::make_unique<CustomFakeProfileOAuth2TokenService>( &pref_service_, std::make_unique<TestProfileOAuth2TokenServiceDelegateChromeOS>( - account_tracker_service.get(), ash_account_manager_mojo_service, + &signin_client_, account_tracker_service.get(), + ash_account_manager_mojo_service, /*is_regular_profile=*/true)); #else auto token_service = @@ -392,18 +397,13 @@ class IdentityManagerTest : public testing::Test { auto account_fetcher_service = std::make_unique<AccountFetcherService>(); account_fetcher_service->Initialize( &signin_client_, token_service.get(), account_tracker_service.get(), - std::make_unique<image_fetcher::FakeImageDecoder>()); + std::make_unique<image_fetcher::FakeImageDecoder>(), + std::make_unique<FakeAccountCapabilitiesFetcherFactory>()); DCHECK_EQ(account_consistency, AccountConsistencyMethod::kDisabled) << "AccountConsistency is not used by PrimaryAccountManager"; - std::unique_ptr<PrimaryAccountPolicyManager> policy_manager; -#if !BUILDFLAG(IS_CHROMEOS_ASH) - policy_manager = - std::make_unique<PrimaryAccountPolicyManagerImpl>(&signin_client_); -#endif auto primary_account_manager = std::make_unique<PrimaryAccountManager>( - &signin_client_, token_service.get(), account_tracker_service.get(), - std::move(policy_manager)); + &signin_client_, token_service.get(), account_tracker_service.get()); // Passing this switch ensures that the new PrimaryAccountManager starts // with a clean slate. Otherwise PrimaryAccountManager::Initialize will use @@ -2359,27 +2359,34 @@ TEST_F(IdentityManagerTest, AreRefreshTokensLoaded) { #if BUILDFLAG(IS_CHROMEOS_LACROS) TEST_F(IdentityManagerTest, SetPrimaryAccount) { - signin_client()->SetInitialPrimaryAccountForTests(account_manager::Account{ - account_manager::AccountKey{kTestGaiaId, - account_manager::AccountType::kGaia}, - kTestEmail}); + signin_client()->SetInitialPrimaryAccountForTests( + account_manager::Account{ + account_manager::AccountKey{kTestGaiaId, + account_manager::AccountType::kGaia}, + kTestEmail}, + /*is_child=*/false); // Do not sign into a primary account as part of the test setup. RecreateIdentityManager(AccountConsistencyMethod::kDisabled, PrimaryAccountManagerSetup::kNoAuthenticatedAccount); // We should have a Primary Account set up automatically. - ASSERT_TRUE(identity_manager()->HasPrimaryAccount(ConsentLevel::kSync)); - EXPECT_EQ( - kTestGaiaId, - identity_manager()->GetPrimaryAccountInfo(ConsentLevel::kSync).gaia); + 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); } // TODO(https://crbug.com/1223364): Remove this when all the users are migrated. TEST_F(IdentityManagerTest, SetPrimaryAccountClearsExistingPrimaryAccount) { - signin_client()->SetInitialPrimaryAccountForTests(account_manager::Account{ - account_manager::AccountKey{kTestGaiaId2, - account_manager::AccountType::kGaia}, - kTestEmail2}); + signin_client()->SetInitialPrimaryAccountForTests( + account_manager::Account{ + account_manager::AccountKey{kTestGaiaId2, + account_manager::AccountType::kGaia}, + kTestEmail2}, + /*is_child=*/false); // RecreateIdentityManager will create PrimaryAccountManager with the primary // account set to kTestGaiaId. After that, IdentityManager ctor should clear @@ -2388,10 +2395,13 @@ TEST_F(IdentityManagerTest, SetPrimaryAccountClearsExistingPrimaryAccount) { RecreateIdentityManager(AccountConsistencyMethod::kDisabled, PrimaryAccountManagerSetup::kWithAuthenticatedAccout); - ASSERT_TRUE(identity_manager()->HasPrimaryAccount(ConsentLevel::kSync)); - EXPECT_EQ( - kTestGaiaId2, - identity_manager()->GetPrimaryAccountInfo(ConsentLevel::kSync).gaia); + 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); } #endif diff --git a/chromium/components/signin/public/identity_manager/identity_mutator.cc b/chromium/components/signin/public/identity_manager/identity_mutator.cc index e0d1d8d02c7..a36047e8d77 100644 --- a/chromium/components/signin/public/identity_manager/identity_mutator.cc +++ b/chromium/components/signin/public/identity_manager/identity_mutator.cc @@ -49,6 +49,17 @@ bool JniIdentityMutator::ClearPrimaryAccount(JNIEnv* env, static_cast<signin_metrics::SignoutDelete>(delete_metric)); } +void JniIdentityMutator::RevokeSyncConsent(JNIEnv* env, + jint source_metric, + jint delete_metric) { + PrimaryAccountMutator* primary_account_mutator = + identity_mutator_->GetPrimaryAccountMutator(); + DCHECK(primary_account_mutator); + return primary_account_mutator->RevokeSyncConsent( + static_cast<signin_metrics::ProfileSignout>(source_metric), + static_cast<signin_metrics::SignoutDelete>(delete_metric)); +} + void JniIdentityMutator::ReloadAllAccountsFromSystemWithPrimaryAccount( JNIEnv* env, const base::android::JavaParamRef<jobject>& j_primary_account_id) { diff --git a/chromium/components/signin/public/identity_manager/identity_mutator.h b/chromium/components/signin/public/identity_manager/identity_mutator.h index faca17ba0dd..d22a6e17fb5 100644 --- a/chromium/components/signin/public/identity_manager/identity_mutator.h +++ b/chromium/components/signin/public/identity_manager/identity_mutator.h @@ -53,6 +53,9 @@ class JniIdentityMutator { jint source_metric, jint delete_metric); + // Called by java to revoke sync consent for the primary account. + void RevokeSyncConsent(JNIEnv* env, jint source_metric, jint delete_metric); + // Called by java to reload the accounts in the token service from the system // accounts. void ReloadAllAccountsFromSystemWithPrimaryAccount( 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 6ff97c052de..314ac25ed12 100644 --- a/chromium/components/signin/public/identity_manager/identity_test_environment.cc +++ b/chromium/components/signin/public/identity_manager/identity_test_environment.cc @@ -22,11 +22,11 @@ #include "components/signin/internal/identity_manager/account_tracker_service.h" #include "components/signin/internal/identity_manager/accounts_cookie_mutator_impl.h" #include "components/signin/internal/identity_manager/diagnostics_provider_impl.h" +#include "components/signin/internal/identity_manager/fake_account_capabilities_fetcher_factory.h" #include "components/signin/internal/identity_manager/fake_profile_oauth2_token_service.h" #include "components/signin/internal/identity_manager/gaia_cookie_manager_service.h" #include "components/signin/internal/identity_manager/primary_account_manager.h" #include "components/signin/internal/identity_manager/primary_account_mutator_impl.h" -#include "components/signin/internal/identity_manager/primary_account_policy_manager_impl.h" #include "components/signin/public/base/consent_level.h" #include "components/signin/public/base/test_signin_client.h" #include "components/signin/public/identity_manager/accounts_mutator.h" @@ -277,7 +277,7 @@ IdentityTestEnvironment::BuildIdentityManagerForTests( auto token_service = std::make_unique<FakeProfileOAuth2TokenService>( pref_service, std::make_unique<TestProfileOAuth2TokenServiceDelegateChromeOS>( - account_tracker_service.get(), + signin_client, account_tracker_service.get(), account_manager_factory->GetAccountManagerMojoService( user_data_dir.value()), /*is_regular_profile=*/true)); @@ -335,17 +335,12 @@ IdentityTestEnvironment::FinishBuildIdentityManagerForTests( auto account_fetcher_service = std::make_unique<AccountFetcherService>(); account_fetcher_service->Initialize( signin_client, token_service.get(), account_tracker_service.get(), - std::make_unique<image_fetcher::FakeImageDecoder>()); + std::make_unique<image_fetcher::FakeImageDecoder>(), + std::make_unique<FakeAccountCapabilitiesFetcherFactory>()); - std::unique_ptr<PrimaryAccountPolicyManager> policy_manager; -#if !BUILDFLAG(IS_CHROMEOS_ASH) - policy_manager = - std::make_unique<PrimaryAccountPolicyManagerImpl>(signin_client); -#endif std::unique_ptr<PrimaryAccountManager> primary_account_manager = std::make_unique<PrimaryAccountManager>( - signin_client, token_service.get(), account_tracker_service.get(), - std::move(policy_manager)); + signin_client, token_service.get(), account_tracker_service.get()); primary_account_manager->Initialize(pref_service); std::unique_ptr<GaiaCookieManagerService> gaia_cookie_manager_service = diff --git a/chromium/components/signin/public/identity_manager/primary_account_change_event.cc b/chromium/components/signin/public/identity_manager/primary_account_change_event.cc index a152c963229..31e3b979841 100644 --- a/chromium/components/signin/public/identity_manager/primary_account_change_event.cc +++ b/chromium/components/signin/public/identity_manager/primary_account_change_event.cc @@ -5,6 +5,7 @@ #include "components/signin/public/identity_manager/primary_account_change_event.h" #include "build/build_config.h" +#include "base/check_op.h" #if BUILDFLAG(IS_ANDROID) #include "components/signin/public/android/jni_headers/PrimaryAccountChangeEvent_jni.h" diff --git a/chromium/components/signin/public/identity_manager/primary_account_mutator_unittest.cc b/chromium/components/signin/public/identity_manager/primary_account_mutator_unittest.cc index 24abd003994..aa0962935e2 100644 --- a/chromium/components/signin/public/identity_manager/primary_account_mutator_unittest.cc +++ b/chromium/components/signin/public/identity_manager/primary_account_mutator_unittest.cc @@ -17,6 +17,7 @@ #include "components/signin/public/base/signin_buildflags.h" #include "components/signin/public/base/signin_metrics.h" #include "components/signin/public/base/signin_pref_names.h" +#include "components/signin/public/base/signin_switches.h" #include "components/signin/public/identity_manager/identity_test_environment.h" #include "components/signin/public/identity_manager/identity_test_utils.h" #include "components/sync_preferences/testing_pref_service_syncable.h" @@ -465,8 +466,27 @@ TEST_F(PrimaryAccountMutatorTest, RevokeSyncConsent_DisabledConsistency) { RemoveAccountExpectation::kRemoveAll); } +#if BUILDFLAG(IS_ANDROID) +// Test that revoking the sync consent when account consistency is disabled +// also clears the primary account and removes all accounts, even when sync +// is allowed to be off. +TEST_F(PrimaryAccountMutatorTest, + RevokeSyncConsent_DisabledConsistency_AllowSyncOff) { + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndEnableFeature( + switches::kAllowSyncOffForChildAccounts); + RunRevokeSyncConsentTest(signin::AccountConsistencyMethod::kDisabled, + RemoveAccountExpectation::kRemoveAll); +} +#endif + // Test that revoking sync consent when Mirror account consistency is enabled // clears the primary account (except for lacros). +// +// TODO(crbug.com/1306031): once the kAllowSyncOffForChildAccounts flag is +// cleaned up, we will no longer clear the primary account on Android. Update +// this test case and delete RevokeSyncConsent_MirrorConsistency_AllowSyncOff +// as part of that cleanup. TEST_F(PrimaryAccountMutatorTest, RevokeSyncConsent_MirrorConsistency) { RunRevokeSyncConsentTest(signin::AccountConsistencyMethod::kMirror, #if BUILDFLAG(IS_CHROMEOS_LACROS) @@ -477,6 +497,20 @@ TEST_F(PrimaryAccountMutatorTest, RevokeSyncConsent_MirrorConsistency) { ); } +#if BUILDFLAG(IS_ANDROID) +// Test that revoking sync consent when Mirror account consistency is enabled +// and Android supervised users are allowed to disable sync, results in the +// sync consent being revoked for the primary account. +TEST_F(PrimaryAccountMutatorTest, + RevokeSyncConsent_MirrorConsistency_AllowSyncOff) { + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndEnableFeature( + switches::kAllowSyncOffForChildAccounts); + RunRevokeSyncConsentTest(signin::AccountConsistencyMethod::kMirror, + RemoveAccountExpectation::kKeepAll); +} +#endif + // Test that revoking the sync consent when DICE account consistency is // enabled does not clear the primary account. TEST_F(PrimaryAccountMutatorTest, RevokeSyncConsent_DiceConsistency) { diff --git a/chromium/components/signin/public/identity_manager/tribool.h b/chromium/components/signin/public/identity_manager/tribool.h index 2fb01bac2be..bdef10fadd6 100644 --- a/chromium/components/signin/public/identity_manager/tribool.h +++ b/chromium/components/signin/public/identity_manager/tribool.h @@ -10,6 +10,7 @@ namespace signin { // The values are persisted to disk and must not be changed. +// GENERATED_JAVA_ENUM_PACKAGE: org.chromium.components.signin enum class Tribool { kUnknown = -1, kFalse = 0, kTrue = 1 }; // Returns the string representation of a tribool. |