Allow IdentityTestEnvironment to be used in Profile-based unittests

Currently IdentityTestEnvironment only works if it can internally
construct the IdentityManager instance that it uses. This is definitely
the cleanest mode of operation (and long-term the only targeted one)
However, it prevents IdentityTestEnvironment's usage in //chrome-level
unittests wherein the IdentityManager instance that the test interacts
with must the one constructed via the Profile (i.e., because the
production code being tested obtains IdentityManager from the Profile).

This CL adds support for using IdentityTestEnvironment in this context.
Specifically, it adds a new IdentityTestEnvironment constructor that
takes in an IdentityManager instance as well as its dependencies and
a //chrome-level adaptor that configures Profile to build the necessary
fakes and wraps an IdentityTestEnvironment instance using this new
constructor.

Bug: 882865
Change-Id: If45f29643fc5a2cacf357ac159c709b068443e9c
Reviewed-on: https://chromium-review.googlesource.com/c/1280264
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600350}

chrome/browser/BUILD.gn

@@ -5120,6 +5120,8 @@ static_library("test_support") {
     "signin/fake_profile_oauth2_token_service_builder.h",
     "signin/fake_signin_manager_builder.cc",
     "signin/fake_signin_manager_builder.h",
+    "signin/identity_test_environment_profile_adaptor.cc",
+    "signin/identity_test_environment_profile_adaptor.h",
     "signin/scoped_account_consistency.cc",
     "signin/scoped_account_consistency.h",
     "ssl/ssl_client_auth_requestor_mock.cc",
@@ -5160,6 +5162,7 @@ static_library("test_support") {
     "//content/test:test_support",
     "//google_apis:test_support",
     "//net:test_support",
+    "//services/identity/public/cpp:test_support",
     "//services/preferences/public/cpp/tracked:test_support",
     "//skia",
     "//testing/gmock",

chrome/browser/signin/identity_test_environment_profile_adaptor.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/signin/identity_test_environment_profile_adaptor.h"
+
+#include "chrome/browser/signin/account_tracker_service_factory.h"
+#include "chrome/browser/signin/fake_gaia_cookie_manager_service_builder.h"
+#include "chrome/browser/signin/fake_profile_oauth2_token_service_builder.h"
+#include "chrome/browser/signin/fake_signin_manager_builder.h"
+#include "chrome/browser/signin/gaia_cookie_manager_service_factory.h"
+#include "chrome/browser/signin/identity_manager_factory.h"
+#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
+#include "chrome/browser/signin/signin_manager_factory.h"
+
+// static
+void IdentityTestEnvironmentProfileAdaptor::
+    AppendIdentityTestEnvironmentFactories(
+        TestingProfile::TestingFactories* factories_to_append_to) {
+  TestingProfile::TestingFactories identity_factories = {
+      {GaiaCookieManagerServiceFactory::GetInstance(),
+       base::BindRepeating(&BuildFakeGaiaCookieManagerService)},
+      {ProfileOAuth2TokenServiceFactory::GetInstance(),
+       base::BindRepeating(&BuildFakeProfileOAuth2TokenService)},
+      {SigninManagerFactory::GetInstance(),
+       base::BindRepeating(&BuildFakeSigninManagerForTesting)}};
+  factories_to_append_to->insert(factories_to_append_to->end(),
+                                 identity_factories.begin(),
+                                 identity_factories.end());
+}
+
+IdentityTestEnvironmentProfileAdaptor::IdentityTestEnvironmentProfileAdaptor(
+    Profile* profile)
+    : identity_test_env_(
+          AccountTrackerServiceFactory::GetForProfile(profile),
+          static_cast<FakeProfileOAuth2TokenService*>(
+              ProfileOAuth2TokenServiceFactory::GetForProfile(profile)),
+#if defined(OS_CHROMEOS)
+          static_cast<FakeSigninManagerBase*>(
+              SigninManagerFactory::GetForProfile(profile)),
+#else
+          static_cast<FakeSigninManager*>(
+              SigninManagerFactory::GetForProfile(profile)),
+#endif
+          static_cast<FakeGaiaCookieManagerService*>(
+              GaiaCookieManagerServiceFactory::GetForProfile(profile)),
+          IdentityManagerFactory::GetForProfile(profile)) {
+}

chrome/browser/signin/identity_test_environment_profile_adaptor.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_SIGNIN_IDENTITY_TEST_ENVIRONMENT_PROFILE_ADAPTOR_H_
+#define CHROME_BROWSER_SIGNIN_IDENTITY_TEST_ENVIRONMENT_PROFILE_ADAPTOR_H_
+
+#include "chrome/test/base/testing_profile.h"
+#include "services/identity/public/cpp/identity_test_environment.h"
+
+// Adaptor that supports identity::IdentityTestEnvironment's usage in testing
+// contexts where the relevant fake objects must be injected via the
+// BrowserContextKeyedServiceFactory infrastructure as the production code
+// accesses IdentityManager via that infrastructure. Before using this
+// class, please consider whether you can change the production code in question
+// to take in the relevant dependencies directly rather than obtaining them from
+// the Profile; this is both cleaner in general and allows for direct usage of
+// identity::IdentityTestEnvironment in the test.
+class IdentityTestEnvironmentProfileAdaptor {
+ public:
+  // This static method must be called as part of configuring the TestingProfile
+  // that the test is using. It will append the set of
+  // testing factories that identity::IdentityTestEnvironment
+  // requires to |factories_to_append_to|, which should be the set of testing
+  // factories supplied to TestingProfile (via one of the various mechanisms for
+  // doing so, e.g. using a TestingProfile::Builder).
+  static void AppendIdentityTestEnvironmentFactories(
+      TestingProfile::TestingFactories* factories_to_append_to);
+
+  // Constructs an adaptor that associates an IdentityTestEnvironment instance
+  // with |profile| via the relevant backing objects. Note that
+  // AppendIdentityTestEnvironmentFactories() must have previously been invoked
+  // on the set of testing factories used to configure |profile|.
+  // |profile| must outlive this object.
+  explicit IdentityTestEnvironmentProfileAdaptor(Profile* profile);
+  ~IdentityTestEnvironmentProfileAdaptor() {}
+
+  // Returns the IdentityTestEnvironment associated with this object (and
+  // implicitly with the Profile passed to this object's constructor).
+  identity::IdentityTestEnvironment* identity_test_env() {
+    return &identity_test_env_;
+  }
+
+ private:
+  identity::IdentityTestEnvironment identity_test_env_;
+
+  DISALLOW_COPY_AND_ASSIGN(IdentityTestEnvironmentProfileAdaptor);
+};
+
+#endif  // CHROME_BROWSER_SIGNIN_IDENTITY_TEST_ENVIRONMENT_PROFILE_ADAPTOR_H_

services/identity/public/cpp/identity_test_environment.cc

@@ -102,7 +102,8 @@ IdentityTestEnvironment::IdentityTestEnvironment(
           /*signin_manager=*/nullptr,
           /*gaia_cookie_manager_service=*/nullptr,
           std::make_unique<IdentityManagerDependenciesOwner>(
-              use_fake_url_loader_for_gaia_cookie_manager)) {}
+              use_fake_url_loader_for_gaia_cookie_manager),
+          /*identity_manager=*/nullptr) {}
 
 IdentityTestEnvironment::IdentityTestEnvironment(
     AccountTrackerService* account_tracker_service,
@@ -113,17 +114,32 @@ IdentityTestEnvironment::IdentityTestEnvironment(
                               token_service,
                               signin_manager,
                               gaia_cookie_manager_service,
-                              /*dependency_owner=*/nullptr) {}
+                              /*dependency_owner=*/nullptr,
+                              /*identity_manager=*/nullptr) {}
 
 IdentityTestEnvironment::IdentityTestEnvironment(
     AccountTrackerService* account_tracker_service,
     FakeProfileOAuth2TokenService* token_service,
     SigninManagerForTest* signin_manager,
     FakeGaiaCookieManagerService* gaia_cookie_manager_service,
-    std::unique_ptr<IdentityManagerDependenciesOwner> dependencies_owner) {
+    IdentityManager* identity_manager)
+    : IdentityTestEnvironment(account_tracker_service,
+                              token_service,
+                              signin_manager,
+                              gaia_cookie_manager_service,
+                              /*dependency_owner=*/nullptr,
+                              identity_manager) {}
+
+IdentityTestEnvironment::IdentityTestEnvironment(
+    AccountTrackerService* account_tracker_service,
+    FakeProfileOAuth2TokenService* token_service,
+    SigninManagerForTest* signin_manager,
+    FakeGaiaCookieManagerService* gaia_cookie_manager_service,
+    std::unique_ptr<IdentityManagerDependenciesOwner> dependencies_owner,
+    IdentityManager* identity_manager) {
   if (dependencies_owner) {
     DCHECK(!(account_tracker_service || token_service || signin_manager ||
-             gaia_cookie_manager_service));
+             gaia_cookie_manager_service || identity_manager));
 
     dependencies_owner_ = std::move(dependencies_owner);
 
@@ -143,19 +159,27 @@ IdentityTestEnvironment::IdentityTestEnvironment(
     gaia_cookie_manager_service_ = gaia_cookie_manager_service;
   }
 
-  identity_manager_ = std::make_unique<IdentityManager>(
-      signin_manager_, token_service_, account_tracker_service_,
-      gaia_cookie_manager_service_);
+  if (identity_manager) {
+    raw_identity_manager_ = identity_manager;
+  } else {
+    owned_identity_manager_ = std::make_unique<IdentityManager>(
+        signin_manager_, token_service_, account_tracker_service_,
+        gaia_cookie_manager_service_);
+  }
 
-  identity_manager_->AddDiagnosticsObserver(this);
+  this->identity_manager()->AddDiagnosticsObserver(this);
 }
 
 IdentityTestEnvironment::~IdentityTestEnvironment() {
-  identity_manager_->RemoveDiagnosticsObserver(this);
+  identity_manager()->RemoveDiagnosticsObserver(this);
 }
 
 IdentityManager* IdentityTestEnvironment::identity_manager() {
-  return identity_manager_.get();
+  DCHECK(raw_identity_manager_ || owned_identity_manager_);
+  DCHECK(!(raw_identity_manager_ && owned_identity_manager_));
+
+  return raw_identity_manager_ ? raw_identity_manager_
+                               : owned_identity_manager_.get();
 }
 
 AccountInfo IdentityTestEnvironment::SetPrimaryAccount(

services/identity/public/cpp/identity_test_environment.h

@@ -13,6 +13,8 @@
 #include "services/identity/public/cpp/identity_manager.h"
 #include "services/identity/public/cpp/identity_test_utils.h"
 
+class IdentityTestEnvironmentProfileAdaptor;
+
 namespace identity {
 
 // Internal class that creates and owns dependencies of IdentityManager
@@ -52,9 +54,10 @@ class IdentityTestEnvironment : public IdentityManager::DiagnosticsObserver {
       FakeProfileOAuth2TokenService* token_service,
       SigninManagerForTest* signin_manager,
       FakeGaiaCookieManagerService* gaia_cookie_manager_service);
+
   ~IdentityTestEnvironment() override;
 
-  // The IdentityManager instance created and owned by this instance.
+  // The IdentityManager instance associated with this instance.
   IdentityManager* identity_manager();
 
   // Sets the primary account for the given email address, generating a GAIA ID
@@ -197,6 +200,8 @@ class IdentityTestEnvironment : public IdentityManager::DiagnosticsObserver {
   void SetCallbackForNextAccessTokenRequest(base::OnceClosure callback);
 
  private:
+  friend class ::IdentityTestEnvironmentProfileAdaptor;
+
   struct AccessTokenRequestState {
     AccessTokenRequestState();
     ~AccessTokenRequestState();
@@ -211,16 +216,46 @@ class IdentityTestEnvironment : public IdentityManager::DiagnosticsObserver {
     base::OnceClosure on_available;
   };
 
-  // Constructs this object and its IdentityManager instance from the supplied
-  // dependencies of IdentityManager, which must be either:
-  // (1) non-null instances of the backing classes, OR
+  // Constructor that takes in an IdentityManager instance as well as instances
+  // of the dependencies of that IdentityManager. For use only in contexts where
+  // IdentityManager and its dependencies are all unavoidably created by the
+  // embedder (e.g., //chrome-level unittests that use the
+  // ProfileKeyedServiceFactory infrastructure).
+  // When using this constructor, the invoker is responsible for ensuring the
+  // following:
+  // - That all of these objects outlive this object
+  // - That the dependencies being passed in were in fact the objects used to
+  //   construct |identity_manager|
+  // - That the passed-in dependencies of |identity_manager| outlive it
+  // NOTE: This constructor is for usage only in the special case of embedder
+  // unittests that must use the IdentityManager instance associated with the
+  // Profile/ChromeBrowserState. If you think you have another use case for it,
+  // contact blundell@chromium.org.
+  IdentityTestEnvironment(
+      AccountTrackerService* account_tracker_service,
+      FakeProfileOAuth2TokenService* token_service,
+      SigninManagerForTest* signin_manager,
+      FakeGaiaCookieManagerService* gaia_cookie_manager_service,
+      IdentityManager* identity_manager);
+
+  // Constructs this object from the supplied
+  // dependencies of IdentityManager and potentially IdentityManager itself.
+  // The supplied dependencies must be either:
+  // (1) non-null instances of the backing classes,
   // (2) a non-null instance of |dependencies_owner|.
+  // In the case of (1), |identity_manager| can be non-null, in which case it
+  // must point to an object created via these dependencies. In the case of 2,
+  // |identity_manager| must be null. If |identity_manager| is non-null, it will
+  // be the IdentityManager instance associated with this object. Otherwise,
+  // this object will create and own an IdentityManager instance from the
+  // supplied dependencies.
   IdentityTestEnvironment(
       AccountTrackerService* account_tracker_service,
       FakeProfileOAuth2TokenService* token_service,
       SigninManagerForTest* signin_manager,
       FakeGaiaCookieManagerService* gaia_cookie_manager_service,
-      std::unique_ptr<IdentityManagerDependenciesOwner> dependencies_owner);
+      std::unique_ptr<IdentityManagerDependenciesOwner> dependencies_owner,
+      IdentityManager* identity_manager);
 
   // IdentityManager::DiagnosticsObserver:
   void OnAccessTokenRequested(
@@ -249,7 +284,12 @@ class IdentityTestEnvironment : public IdentityManager::DiagnosticsObserver {
   FakeProfileOAuth2TokenService* token_service_ = nullptr;
   SigninManagerForTest* signin_manager_ = nullptr;
   FakeGaiaCookieManagerService* gaia_cookie_manager_service_ = nullptr;
-  std::unique_ptr<IdentityManager> identity_manager_;
+
+  // Depending on which constructor is used, exactly one of these will be
+  // non-null. See the documentation on the constructor wherein IdentityManager
+  // is passed in for required lifetime invariants in that case.
+  std::unique_ptr<IdentityManager> owned_identity_manager_;
+  IdentityManager* raw_identity_manager_ = nullptr;
 
   base::OnceClosure on_access_token_requested_callback_;
   std::vector<AccessTokenRequestState> requesters_;