[Profiles] Implement RevokeAllCredentials for Lacros.

ClearPrimaryAccount removes the primary account and signout all accounts. This is done through ProfileOAuth2TokenServiceDelegateChromeOS::RevokeAllCredentials. This CL implements RevokeAllCredentials. Bug: 1307249 Change-Id: Ib728ab0a0d8be4d28f134a5597ee8d3d8b5fb149 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3545314 Reviewed-by: David Roger <droger@chromium.org> Commit-Queue: Monica Basta <msalama@chromium.org> Cr-Commit-Position: refs/heads/main@{#985266}
chromium · Mar 25, 2022 · a272df4 · a272df4
1 parent e232089
commit a272df4
Show file tree

Hide file tree

Showing 16 changed files with 192 additions and 10 deletions.
diff --git a/chrome/browser/lacros/account_manager/account_profile_mapper.cc b/chrome/browser/lacros/account_manager/account_profile_mapper.cc
@@ -600,6 +600,56 @@ bool AccountProfileMapper::ShouldDeleteProfile(
   return primary_account_deleted;
 }
 
+void AccountProfileMapper::RemoveAllAccounts(
+    const base::FilePath& profile_path) {
+  DCHECK(!profile_path.empty());
+  if (!initialized_) {
+    // Note: On Initialization 'RemoveAllAccounts' may run before or after
+    // ProfileOAuth2TokenServiceDelegateChromeOS::OnGetAccounts which will
+    // influence the order of OnRefreshTokenRevoked and OnRefreshTokensLoaded.
+    // The'ProfileOAuth2TokenServiceDelegateChromeOS' ensures the integrity of
+    // accounts with refresh tokens through 'pending_accounts_'.
+    initialization_callbacks_.push_back(
+        base::BindOnce(&AccountProfileMapper::RemoveAllAccounts,
+                       weak_factory_.GetWeakPtr(), profile_path));
+    return;
+  }
+
+  // Profile might be deleted during initialization.
+  ProfileAttributesEntry* entry =
+      profile_attributes_storage_->GetProfileAttributesWithPath(profile_path);
+
+  if (!entry) {
+    DLOG(ERROR) << "Profile with profile path: " << profile_path
+                << "does not exist";
+    return;
+  }
+
+  if (Profile::IsMainProfilePath(entry->GetPath())) {
+    DLOG(ERROR)
+        << "The primary account should not be removed from the main profile";
+    return;
+  }
+
+  std::vector<account_manager::Account> removed_accounts;
+  for (const std::string& gaia_id : entry->GetGaiaIds()) {
+    const account_manager::Account* account =
+        account_cache_.FindAccountByGaiaId(gaia_id);
+    if (!account) {
+      DLOG(ERROR) << "Account " << gaia_id << " missing.";
+      continue;
+    }
+    removed_accounts.push_back(*account);
+  }
+
+  entry->SetGaiaIds({});
+  for (auto& obs : observers_) {
+    for (const account_manager::Account& account : removed_accounts) {
+      obs.OnAccountRemoved(profile_path, account);
+    }
+  }
+}
+
 void AccountProfileMapper::MigrateOldProfiles() {
   for (ProfileAttributesEntry* entry :
        profile_attributes_storage_->GetAllProfilesAttributes()) {

diff --git a/chrome/browser/lacros/account_manager/account_profile_mapper.h b/chrome/browser/lacros/account_manager/account_profile_mapper.h
@@ -140,6 +140,9 @@ class AccountProfileMapper
   void CreateNewProfileWithAccount(const account_manager::AccountKey& account,
                                    AddAccountCallback callback);
 
+  // Remove all accounts from profile with profile_path.
+  void RemoveAllAccounts(const base::FilePath& profile_path);
+
   // account_manager::AccountManagerFacade::Observer:
   void OnAccountUpserted(const account_manager::Account& account) override;
   void OnAccountRemoved(const account_manager::Account& account) override;

diff --git a/chrome/browser/lacros/account_manager/account_profile_mapper_unittest.cc b/chrome/browser/lacros/account_manager/account_profile_mapper_unittest.cc
@@ -944,6 +944,99 @@ TEST_F(AccountProfileMapperTest, RemovePrimaryAccountFromPrimaryProfile) {
                        {{main_path(), {"B"}}});
 }
 
+// Tests removing all accounts from a secondary profile (User signed out from
+// chrome) before initialization.
+// TODO(crbug.com/1260291): Update this comment when secondary profiles are
+// not deleted when their primary account is removed from the OS to reflect that
+// RemoveAllAccounts would also be called in this case.
+TEST_F(AccountProfileMapperTest,
+       RemoveAllAccountsFromSecondaryProfile_BeforeInitialization) {
+  base::FilePath other_path = GetProfilePath("Other");
+  AccountProfileMapper* mapper = CreateMapperNonInitialized(
+      {{main_path(), {"A"}}, {other_path, {"B", "C"}}});
+  MockAccountProfileMapperObserver mock_observer;
+  base::ScopedObservation<AccountProfileMapper, AccountProfileMapper::Observer>
+      observation{&mock_observer};
+  observation.Observe(mapper);
+  ExpectOnAccountRemoved(&mock_observer, {{other_path, {"B", "C"}}});
+  mapper->RemoveAllAccounts(other_path);
+  CompleteFacadeGetAccountsGaia({"A", "B", "C"});
+  testing::Mock::VerifyAndClearExpectations(&mock_observer);
+  VerifyAccountsInStorage({{main_path(), {"A"}}, {other_path, {}}});
+}
+
+// Tests removing all accounts from a secondary profile and account removed from
+// the OS before initialization.
+TEST_F(
+    AccountProfileMapperTest,
+    RemoveAllAccountsFromSecondaryProfile_OSAccountsChanged_BeforeInitialization) {
+  base::FilePath other_path = GetProfilePath("Other");
+  AccountProfileMapper* mapper = CreateMapperNonInitialized(
+      {{main_path(), {"A"}}, {other_path, {"B", "C"}}});
+  MockAccountProfileMapperObserver mock_observer;
+  base::ScopedObservation<AccountProfileMapper, AccountProfileMapper::Observer>
+      observation{&mock_observer};
+  observation.Observe(mapper);
+  // "C" is removed from the ProfileAttributeEntry by RemoveStaleAccounts.
+  ExpectOnAccountRemoved(&mock_observer, {{other_path, {"B"}}});
+  mapper->RemoveAllAccounts(other_path);
+  CompleteFacadeGetAccountsGaia({"A", "B"});
+  testing::Mock::VerifyAndClearExpectations(&mock_observer);
+  VerifyAccountsInStorage({{main_path(), {"A"}}, {other_path, {}}});
+}
+
+// Tests removing all accounts from a secondary profile.
+TEST_F(AccountProfileMapperTest, RemoveAllAccountsFromSecondaryProfile) {
+  base::FilePath other_path = GetProfilePath("Other");
+  AccountProfileMapper* mapper =
+      CreateMapper({{main_path(), {"A"}}, {other_path, {"B", "C"}}});
+  MockAccountProfileMapperObserver mock_observer;
+  base::ScopedObservation<AccountProfileMapper, AccountProfileMapper::Observer>
+      observation{&mock_observer};
+  observation.Observe(mapper);
+  ExpectOnAccountRemoved(&mock_observer, {{other_path, {"B", "C"}}});
+  mapper->RemoveAllAccounts(other_path);
+  testing::Mock::VerifyAndClearExpectations(&mock_observer);
+  VerifyAccountsInStorage({{main_path(), {"A"}}, {other_path, {}}});
+}
+
+// Tests removing all accounts from main profile is not allowed.
+TEST_F(AccountProfileMapperTest, RemoveAllAccountsFromPrimaryProfile) {
+  AccountProfileMapper* mapper = CreateMapper({{main_path(), {"A", "B"}}});
+  SetPrimaryAccountForProfile(main_path(), "A");
+  MockAccountProfileMapperObserver mock_observer;
+  base::ScopedObservation<AccountProfileMapper, AccountProfileMapper::Observer>
+      observation{&mock_observer};
+  ExpectOnAccountRemoved(&mock_observer, {});
+  mapper->RemoveAllAccounts(main_path());
+  testing::Mock::VerifyAndClearExpectations(&mock_observer);
+  VerifyAccountsInStorage({{main_path(), {"A", "B"}}});
+}
+
+// Tests removing all accounts from profile before initialization but profile
+// is deleted during initialization.
+TEST_F(
+    AccountProfileMapperTest,
+    RemoveAllAccountsFromSecondaryProfile_ProfileDeletedDuringInitialization) {
+  base::FilePath other_path = GetProfilePath("Other");
+  AccountProfileMapper* mapper = CreateMapperNonInitialized(
+      {{main_path(), {"A"}}, {other_path, {"B", "C"}}});
+  SetPrimaryAccountForProfile(other_path, "B");
+  MockAccountProfileMapperObserver mock_observer;
+  base::ScopedObservation<AccountProfileMapper, AccountProfileMapper::Observer>
+      observation{&mock_observer};
+  observation.Observe(mapper);
+  ExpectOnAccountRemoved(&mock_observer, {});
+  mapper->RemoveAllAccounts(other_path);
+  // Removing the primary account will delete the profile.
+  // TODO(crbug.com/1260291): Rely on managed profile.
+  CompleteFacadeGetAccountsGaia({"A", "C"});
+  VerifyAccountsInPrefs({{main_path(), {"A"}}, {base::FilePath(), {"C"}}});
+  ProfileAttributesStorageTestObserver(attributes_storage())
+      .WaitForProfileBeingDeleted(other_path);
+  testing::Mock::VerifyAndClearExpectations(&mock_observer);
+}
+
 // Tests that accounts from deleted profile remain unassigned.
 TEST_F(AccountProfileMapperTest, DeleteProfile) {
   base::FilePath other_path = GetProfilePath("Other");

diff --git a/chrome/browser/signin/chrome_signin_client.cc b/chrome/browser/signin/chrome_signin_client.cc
@@ -329,6 +329,18 @@ absl::optional<bool> ChromeSigninClient::IsInitialPrimaryAccountChild() const {
       crosapi::mojom::SessionType::kChildSession;
   return is_child_session;
 }
+
+void ChromeSigninClient::RemoveAllAccounts() {
+  if (GetInitialPrimaryAccount().has_value()) {
+    DLOG(ERROR) << "It is not allowed to remove the initial primary account.";
+    return;
+  }
+
+  DCHECK(!profile_->IsMainProfile());
+  g_browser_process->profile_manager()
+      ->GetAccountProfileMapper()
+      ->RemoveAllAccounts(profile_->GetPath());
+}
 #endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
 
 void ChromeSigninClient::SetURLLoaderFactoryForTest(

diff --git a/chrome/browser/signin/chrome_signin_client.h b/chrome/browser/signin/chrome_signin_client.h
@@ -75,6 +75,7 @@ class ChromeSigninClient
 #if BUILDFLAG(IS_CHROMEOS_LACROS)
   absl::optional<account_manager::Account> GetInitialPrimaryAccount() override;
   absl::optional<bool> IsInitialPrimaryAccountChild() const override;
+  void RemoveAllAccounts() override;
 #endif
 
   // Used in tests to override the URLLoaderFactory returned by

diff --git a/components/signin/internal/identity_manager/profile_oauth2_token_service_builder.cc b/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/...onents/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos.cc b/...onents/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/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos.h b/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/...gnin/internal/identity_manager/profile_oauth2_token_service_delegate_chromeos_unittest.cc b/...gnin/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/...s/signin/internal/identity_manager/test_profile_oauth2_token_service_delegate_chromeos.cc b/...s/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/...ts/signin/internal/identity_manager/test_profile_oauth2_token_service_delegate_chromeos.h b/...ts/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/components/signin/public/base/signin_client.h b/components/signin/public/base/signin_client.h
@@ -103,6 +103,9 @@ class SigninClient : public KeyedService {
   // 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/components/signin/public/base/test_signin_client.cc b/components/signin/public/base/test_signin_client.cc
@@ -133,4 +133,6 @@ void TestSigninClient::SetInitialPrimaryAccountForTests(
   is_initial_primary_account_child_ = is_child;
 }
 
+void TestSigninClient::RemoveAllAccounts() {}
+
 #endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
diff --git a/components/signin/public/base/test_signin_client.h b/components/signin/public/base/test_signin_client.h
@@ -104,6 +104,7 @@ class TestSigninClient : public SigninClient {
 
   void SetInitialPrimaryAccountForTests(const account_manager::Account& account,
                                         const absl::optional<bool>& is_child);
+  void RemoveAllAccounts() override;
 #endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
 
  private:

diff --git a/components/signin/public/identity_manager/identity_manager_unittest.cc b/components/signin/public/identity_manager/identity_manager_unittest.cc
@@ -383,7 +383,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 =

diff --git a/components/signin/public/identity_manager/identity_test_environment.cc b/components/signin/public/identity_manager/identity_test_environment.cc
@@ -278,7 +278,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));