Revert "Add server token revocation in AccountManager"

This reverts commit 6bc98f91.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 566961 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzZiYzk4ZjkxODRkYjQzZmFlYWE5N2YwODZhMTFlYmEyNGZkMWMxZjkM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.memory/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/27907

Sample Failed Step: unit_tests

Original change's description:
> Add server token revocation in AccountManager
> 
> Bug: 820046
> Test: chromeos_unittests --gtest_filter="*AccountManager*"
> Test: unit_tests --gtest_filter="*CrOSOAuthDelegateTest*"
> Change-Id: I017bb9e9f9ed789d77235336be8fa465fd4289b8
> Reviewed-on: https://chromium-review.googlesource.com/1087455
> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
> Reviewed-by: Lutz Justen <ljusten@chromium.org>
> Reviewed-by: David Roger <droger@chromium.org>
> Commit-Queue: Kush Sinha <sinhak@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#566961}

Change-Id: Ie71a2d159ed50aa3196bf43b488877747b1f88b8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 820046
Reviewed-on: https://chromium-review.googlesource.com/1100515
Cr-Commit-Position: refs/heads/master@{#567210}

chrome/browser/chromeos/oauth2_token_service_delegate_unittest.cc

@@ -11,7 +11,6 @@
 
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
-#include "base/memory/scoped_refptr.h"
 #include "base/stl_util.h"
 #include "base/test/scoped_task_environment.h"
 #include "chromeos/account_manager/account_manager.h"
@@ -89,10 +88,7 @@ class CrOSOAuthDelegateTest : public testing::Test {
   void SetUp() override {
     ASSERT_TRUE(tmp_dir_.CreateUniqueTempDir());
 
-    request_context_ = new net::TestURLRequestContextGetter(
-        scoped_task_environment_.GetMainThreadTaskRunner());
-    account_manager_.Initialize(tmp_dir_.GetPath(), request_context_.get(),
-                                immediate_callback_runner_);
+    account_manager_.Initialize(tmp_dir_.GetPath());
     scoped_task_environment_.RunUntilIdle();
 
     pref_service_.registry()->RegisterListPref(
@@ -143,14 +139,10 @@ class CrOSOAuthDelegateTest : public testing::Test {
   base::test::ScopedTaskEnvironment scoped_task_environment_;
 
   base::ScopedTempDir tmp_dir_;
-  scoped_refptr<net::URLRequestContextGetter> request_context_;
   AccountInfo account_info_;
   AccountTrackerService account_tracker_service_;
   AccountManager account_manager_;
   std::unique_ptr<ChromeOSOAuth2TokenServiceDelegate> delegate_;
-  AccountManager::DelayNetworkCallRunner immediate_callback_runner_ =
-      base::BindRepeating(
-          [](const base::RepeatingClosure& closure) -> void { closure.Run(); });
 
  private:
   sync_preferences::TestingPrefServiceSyncable pref_service_;
@@ -264,8 +256,7 @@ TEST_F(CrOSOAuthDelegateTest, BatchChangeObserversAreNotifiedOncePerBatch) {
   AccountManager account_manager;
   // AccountManager will not be fully initialized until
   // |scoped_task_environment_.RunUntilIdle()| is called.
-  account_manager.Initialize(tmp_dir_.GetPath(), request_context_.get(),
-                             immediate_callback_runner_);
+  account_manager.Initialize(tmp_dir_.GetPath());
 
   // Register callbacks before AccountManager has been fully initialized.
   auto delegate = std::make_unique<ChromeOSOAuth2TokenServiceDelegate>(

chrome/browser/profiles/profile_impl.cc

@@ -142,7 +142,6 @@
 #include "chrome/browser/chromeos/cryptauth/gcm_device_info_provider_impl.h"
 #include "chrome/browser/chromeos/locale_change_guard.h"
 #include "chrome/browser/chromeos/login/session/user_session_manager.h"
-#include "chrome/browser/chromeos/net/delay_network_call.h"
 #include "chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h"
 #include "chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.h"
 #include "chrome/browser/chromeos/preferences.h"
@@ -434,11 +433,7 @@ ProfileImpl::ProfileImpl(
         g_browser_process->platform_part()->GetAccountManagerFactory();
     chromeos::AccountManager* account_manager =
         factory->GetAccountManager(path.value());
-    account_manager->Initialize(
-        path, g_browser_process->system_request_context(),
-        base::BindRepeating(&chromeos::DelayNetworkCall,
-                            base::TimeDelta::FromMilliseconds(
-                                chromeos::kDefaultNetworkRetryDelayMS)));
+    account_manager->Initialize(path);
   }
 #endif
 

chromeos/account_manager/account_manager.cc

@@ -14,12 +14,7 @@
 #include "base/sequenced_task_runner.h"
 #include "base/task_runner_util.h"
 #include "base/task_scheduler/post_task.h"
-#include "base/threading/sequenced_task_runner_handle.h"
-#include "google_apis/gaia/gaia_auth_consumer.h"
-#include "google_apis/gaia/gaia_auth_fetcher.h"
-#include "google_apis/gaia/gaia_constants.h"
 #include "google_apis/gaia/oauth2_access_token_fetcher_impl.h"
-#include "net/url_request/url_request_context_getter.h"
 #include "third_party/protobuf/src/google/protobuf/message_lite.h"
 
 namespace chromeos {
@@ -30,35 +25,6 @@ constexpr base::FilePath::CharType kTokensFileName[] =
     FILE_PATH_LITERAL("AccountManagerTokens.bin");
 constexpr int kTokensFileMaxSizeInBytes = 100000;  // ~100 KB
 
-// A helper class for revoking GAIA tokens. This class is meant to be used in a
-// one-shot fashion and deletes itself when its work is done.
-class GaiaTokenRevocationHelper : public GaiaAuthConsumer {
- public:
-  GaiaTokenRevocationHelper(
-      net::URLRequestContextGetter* request_context,
-      AccountManager::DelayNetworkCallRunner delay_network_call_runner,
-      const std::string& refresh_token) {
-    DCHECK(!refresh_token.empty());
-    base::RepeatingClosure start_revoke_token = base::BindRepeating(
-        &GaiaAuthFetcher::StartRevokeOAuth2Token,
-        std::make_unique<GaiaAuthFetcher>(this, GaiaConstants::kChromeOSSource,
-                                          request_context),
-        refresh_token);
-    delay_network_call_runner.Run(start_revoke_token);
-  }
-
-  ~GaiaTokenRevocationHelper() override = default;
-
-  // GaiaAuthConsumer overrides.
-  void OnOAuth2RevokeTokenCompleted(TokenRevocationStatus status) override {
-    VLOG(1) << "GaiaTokenRevocationHelper::OnOAuth2RevokeTokenCompleted";
-    base::SequencedTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(GaiaTokenRevocationHelper);
-};
-
 AccountManager::TokenMap LoadTokensFromDisk(
     const base::FilePath& tokens_file_path) {
   AccountManager::TokenMap tokens;
@@ -145,20 +111,14 @@ AccountManager::Observer::~Observer() = default;
 
 AccountManager::AccountManager() : weak_factory_(this) {}
 
-void AccountManager::Initialize(
-    const base::FilePath& home_dir,
-    net::URLRequestContextGetter* request_context,
-    DelayNetworkCallRunner delay_network_call_runner) {
-  Initialize(
-      home_dir, request_context, std::move(delay_network_call_runner),
-      base::CreateSequencedTaskRunnerWithTraits(
-          {base::TaskShutdownBehavior::BLOCK_SHUTDOWN, base::MayBlock()}));
+void AccountManager::Initialize(const base::FilePath& home_dir) {
+  Initialize(home_dir, base::CreateSequencedTaskRunnerWithTraits(
+                           {base::TaskShutdownBehavior::BLOCK_SHUTDOWN,
+                            base::MayBlock()}));
 }
 
 void AccountManager::Initialize(
     const base::FilePath& home_dir,
-    net::URLRequestContextGetter* request_context,
-    DelayNetworkCallRunner delay_network_call_runner,
     scoped_refptr<base::SequencedTaskRunner> task_runner) {
   VLOG(1) << "AccountManager::Initialize";
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -173,8 +133,6 @@ void AccountManager::Initialize(
   }
 
   init_state_ = InitializationState::kInProgress;
-  request_context_ = request_context;
-  delay_network_call_runner_ = std::move(delay_network_call_runner);
   task_runner_ = task_runner;
   writer_ = std::make_unique<base::ImportantFileWriter>(
       home_dir.Append(kTokensFileName), task_runner_);
@@ -253,21 +211,9 @@ void AccountManager::RemoveAccountInternal(const AccountKey& account_key) {
     return;
   }
 
-  const std::string token = std::move(it->second);
   tokens_.erase(it);
   PersistTokensAsync();
   NotifyAccountRemovalObservers(account_key);
-
-  // Revoke the token iff it is a GAIA account and the token is not empty.
-  // Stored tokens can be empty for accounts recently migrated to
-  // AccountManager, for which we do not have LSTs yet. These accounts require
-  // re-authentication from the user, but are in a valid state (and hence don't
-  // do a DCHECK here for |!token.empty()|).
-  if (account_key.account_type ==
-          account_manager::AccountType::ACCOUNT_TYPE_GAIA &&
-      !token.empty()) {
-    RevokeGaiaTokenOnServer(token);
-  }
 }
 
 void AccountManager::UpsertToken(const AccountKey& account_key,
@@ -328,11 +274,6 @@ void AccountManager::RemoveObserver(AccountManager::Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
-net::URLRequestContextGetter* AccountManager::GetUrlRequestContext() {
-  DCHECK(request_context_);
-  return request_context_;
-}
-
 std::unique_ptr<OAuth2AccessTokenFetcher>
 AccountManager::CreateAccessTokenFetcher(
     const AccountKey& account_key,
@@ -356,12 +297,6 @@ bool AccountManager::IsTokenAvailable(const AccountKey& account_key) const {
   return it != tokens_.end() && !it->second.empty();
 }
 
-void AccountManager::RevokeGaiaTokenOnServer(const std::string& refresh_token) {
-  // GaiaTokenRevocationHelper will delete itself when its work is over.
-  new GaiaTokenRevocationHelper(GetUrlRequestContext(),
-                                delay_network_call_runner_, refresh_token);
-}
-
 CHROMEOS_EXPORT std::ostream& operator<<(
     std::ostream& os,
     const AccountManager::AccountKey& account_key) {

chromeos/account_manager/account_manager.h

@@ -58,9 +58,6 @@ class CHROMEOS_EXPORT AccountManager {
   // A callback for list of |AccountKey|s.
   using AccountListCallback = base::OnceCallback<void(std::vector<AccountKey>)>;
 
-  using DelayNetworkCallRunner =
-      base::RepeatingCallback<void(const base::RepeatingClosure&)>;
-
   class Observer {
    public:
     Observer();
@@ -88,18 +85,11 @@ class CHROMEOS_EXPORT AccountManager {
 
   // Note: |Initialize| MUST be called at least once on this object.
   AccountManager();
-  virtual ~AccountManager();
+  ~AccountManager();
 
   // |home_dir| is the path of the Device Account's home directory (root of the
-  // user's cryptohome).
-  // |request_context| is a non-owning pointer.
-  // |delay_network_call_runner| is basically a wrapper for
-  // |chromeos::DelayNetworkCall|. Cannot use |chromeos::DelayNetworkCall| due
-  // to linking/dependency constraints.
-  // This method MUST be called at least once in the lifetime of AccountManager.
-  void Initialize(const base::FilePath& home_dir,
-                  net::URLRequestContextGetter* request_context,
-                  DelayNetworkCallRunner delay_network_call_runner);
+  // user's cryptohome). This method MUST be called at least once.
+  void Initialize(const base::FilePath& home_dir);
 
   // Gets (async) a list of account keys known to |AccountManager|.
   void GetAccounts(AccountListCallback callback);
@@ -108,9 +98,6 @@ class CHROMEOS_EXPORT AccountManager {
   // |AccountManager|.
   // Observers are notified about an account removal through
   // |Observer::OnAccountRemoved|.
-  // If the account being removed is a GAIA account, a token revocation with
-  // GAIA is also attempted, on a best effort basis. Even if token revocation
-  // with GAIA fails, AccountManager will forget the account.
   void RemoveAccount(const AccountKey& account_key);
 
   // Updates or inserts a token, for the account corresponding to the given
@@ -124,9 +111,6 @@ class CHROMEOS_EXPORT AccountManager {
   // not in the list of known observers.
   void RemoveObserver(Observer* observer);
 
-  // Gets AccountManager's URL Request Context.
-  net::URLRequestContextGetter* GetUrlRequestContext();
-
   // Creates and returns an |OAuth2AccessTokenFetcher| using the refresh token
   // stored for |account_key|. |IsTokenAvailable| should be |true| for
   // |account_key|, otherwise a |nullptr| is returned.
@@ -151,11 +135,12 @@ class CHROMEOS_EXPORT AccountManager {
 
   friend class AccountManagerTest;
   FRIEND_TEST_ALL_PREFIXES(AccountManagerTest, TestInitialization);
+  FRIEND_TEST_ALL_PREFIXES(AccountManagerTest, TestPersistence);
+  FRIEND_TEST_ALL_PREFIXES(AccountManagerTest, AccountRemovalIsPersistedToDisk);
 
-  // Same as the public |Initialize| except for a |task_runner|.
+  // Initializes |AccountManager| with the provided |task_runner| and location
+  // of the user's home directory.
   void Initialize(const base::FilePath& home_dir,
-                  net::URLRequestContextGetter* request_context,
-                  DelayNetworkCallRunner delay_network_call_runner,
                   scoped_refptr<base::SequencedTaskRunner> task_runner);
 
   // Reads tokens from |tokens| and inserts them in |tokens_| and runs all
@@ -190,21 +175,9 @@ class CHROMEOS_EXPORT AccountManager {
   // Notify |Observer|s about an account removal.
   void NotifyAccountRemovalObservers(const AccountKey& account_key);
 
-  // Revokes |refresh_token| with GAIA. Virtual for testing.
-  virtual void RevokeGaiaTokenOnServer(const std::string& refresh_token);
-
   // Status of this object's initialization.
   InitializationState init_state_ = InitializationState::kNotStarted;
 
-  // All tokens, if channel bound, are bound to |request_context_|. This is a
-  // non-owning pointer.
-  net::URLRequestContextGetter* request_context_ = nullptr;
-
-  // An indirect way to access |chromeos::DelayNetworkCall|. We cannot use
-  // |chromeos::DelayNetworkCall| directly here due to linking/dependency
-  // issues.
-  DelayNetworkCallRunner delay_network_call_runner_;
-
   // A task runner for disk I/O.
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
   std::unique_ptr<base::ImportantFileWriter> writer_;

chromeos/account_manager/account_manager_unittest.cc

@@ -12,29 +12,13 @@
 #include "base/bind.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
-#include "base/memory/scoped_refptr.h"
 #include "base/run_loop.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/threading/sequenced_task_runner_handle.h"
-#include "net/url_request/url_request_test_util.h"
-#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using ::testing::_;
-
 namespace chromeos {
 
-class AccountManagerSpy : public AccountManager {
- public:
-  AccountManagerSpy() = default;
-  ~AccountManagerSpy() override = default;
-
-  MOCK_METHOD1(RevokeGaiaTokenOnServer, void(const std::string& refresh_token));
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(AccountManagerSpy);
-};
-
 class AccountManagerTest : public testing::Test {
  public:
   AccountManagerTest() = default;
@@ -43,9 +27,9 @@ class AccountManagerTest : public testing::Test {
  protected:
   void SetUp() override {
     ASSERT_TRUE(tmp_dir_.CreateUniqueTempDir());
-    request_context_ = new net::TestURLRequestContextGetter(
-        scoped_task_environment_.GetMainThreadTaskRunner());
-    ResetAndInitializeAccountManager();
+    account_manager_ = std::make_unique<AccountManager>();
+    account_manager_->Initialize(tmp_dir_.GetPath(),
+                                 base::SequencedTaskRunnerHandle::Get());
   }
 
   // Gets the list of accounts stored in |account_manager_|.
@@ -66,26 +50,13 @@ class AccountManagerTest : public testing::Test {
     return accounts;
   }
 
-  // Helper method to reset and initialize |account_manager_| with default
-  // parameters.
-  void ResetAndInitializeAccountManager() {
-    account_manager_ = std::make_unique<AccountManagerSpy>();
-    account_manager_->Initialize(tmp_dir_.GetPath(), request_context_.get(),
-                                 immediate_callback_runner_,
-                                 base::SequencedTaskRunnerHandle::Get());
-  }
-
   // Check base/test/scoped_task_environment.h. This must be the first member /
   // declared before any member that cares about tasks.
   base::test::ScopedTaskEnvironment scoped_task_environment_;
   base::ScopedTempDir tmp_dir_;
-  scoped_refptr<net::URLRequestContextGetter> request_context_;
-  std::unique_ptr<AccountManagerSpy> account_manager_;
+  std::unique_ptr<AccountManager> account_manager_;
   const AccountManager::AccountKey kAccountKey_{
       "111", account_manager::AccountType::ACCOUNT_TYPE_GAIA};
-  AccountManager::DelayNetworkCallRunner immediate_callback_runner_ =
-      base::BindRepeating(
-          [](const base::RepeatingClosure& closure) -> void { closure.Run(); });
 
  private:
   DISALLOW_COPY_AND_ASSIGN(AccountManagerTest);
@@ -134,8 +105,7 @@ TEST_F(AccountManagerTest, TestInitialization) {
 
   EXPECT_EQ(account_manager.init_state_,
             AccountManager::InitializationState::kNotStarted);
-  account_manager.Initialize(tmp_dir_.GetPath(), request_context_.get(),
-                             immediate_callback_runner_,
+  account_manager.Initialize(tmp_dir_.GetPath(),
                              base::SequencedTaskRunnerHandle::Get());
   scoped_task_environment_.RunUntilIdle();
   EXPECT_EQ(account_manager.init_state_,
@@ -155,7 +125,10 @@ TEST_F(AccountManagerTest, TestPersistence) {
   account_manager_->UpsertToken(kAccountKey_, "123");
   scoped_task_environment_.RunUntilIdle();
 
-  ResetAndInitializeAccountManager();
+  account_manager_ = std::make_unique<AccountManager>();
+  account_manager_->Initialize(tmp_dir_.GetPath(),
+                               base::SequencedTaskRunnerHandle::Get());
+
   std::vector<AccountManager::AccountKey> accounts = GetAccountsBlocking();
 
   EXPECT_EQ(1UL, accounts.size());
@@ -228,7 +201,9 @@ TEST_F(AccountManagerTest, AccountRemovalIsPersistedToDisk) {
   account_manager_->RemoveAccount(kAccountKey_);
   scoped_task_environment_.RunUntilIdle();
 
-  ResetAndInitializeAccountManager();
+  account_manager_ = std::make_unique<AccountManager>();
+  account_manager_->Initialize(tmp_dir_.GetPath(),
+                               base::SequencedTaskRunnerHandle::Get());
 
   std::vector<AccountManager::AccountKey> accounts = GetAccountsBlocking();
 
@@ -249,37 +224,4 @@ TEST_F(AccountManagerTest, ObserversAreNotifiedOnAccountRemoval) {
   account_manager_->RemoveObserver(observer.get());
 }
 
-TEST_F(AccountManagerTest, TokenRevocationIsAttemptedForGaiaAccounts) {
-  const std::string kToken = "123";
-  ResetAndInitializeAccountManager();
-  EXPECT_CALL(*account_manager_.get(), RevokeGaiaTokenOnServer(kToken));
-
-  account_manager_->UpsertToken(kAccountKey_, kToken);
-  scoped_task_environment_.RunUntilIdle();
-
-  account_manager_->RemoveAccount(kAccountKey_);
-}
-
-TEST_F(AccountManagerTest, TokenRevocationIsNotAttemptedForNonGaiaAccounts) {
-  ResetAndInitializeAccountManager();
-  EXPECT_CALL(*account_manager_.get(), RevokeGaiaTokenOnServer(_)).Times(0);
-
-  const AccountManager::AccountKey adAccountKey{
-      "999", account_manager::AccountType::ACCOUNT_TYPE_ACTIVE_DIRECTORY};
-  account_manager_->UpsertToken(adAccountKey, "123");
-  scoped_task_environment_.RunUntilIdle();
-
-  account_manager_->RemoveAccount(adAccountKey);
-}
-
-TEST_F(AccountManagerTest, TokenRevocationIsNotAttemptedForEmptyTokens) {
-  ResetAndInitializeAccountManager();
-  EXPECT_CALL(*account_manager_.get(), RevokeGaiaTokenOnServer(_)).Times(0);
-
-  account_manager_->UpsertToken(kAccountKey_, std::string());
-  scoped_task_environment_.RunUntilIdle();
-
-  account_manager_->RemoveAccount(kAccountKey_);
-}
-
 }  // namespace chromeos