[CrOS MultiDevice] Refactor two CryptAuth classes.

This CL modifies CryptAuthEnrollerImpl and CryptAuthDeviceManagerImpl
to take a CryptAuthClientFactory* instead of a
std::unique_ptr<CryptAuthClientFactory> in their constructors. It was
unnecessary to have each class own its own instance of the factory, and
this refactor will help as I add an instance of the factory to the
DeviceSync service.

Bug: 824568, 752273
Change-Id: Ie3818cb7d81439689e4edb60f6c024f0e5069435
Reviewed-on: https://chromium-review.googlesource.com/1017307
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551798}

chrome/browser/chromeos/cryptauth/chrome_cryptauth_service.cc

@@ -57,16 +57,18 @@ CreateCryptAuthClientFactoryImpl(Profile* profile) {
 class CryptAuthEnrollerFactoryImpl
     : public cryptauth::CryptAuthEnrollerFactory {
  public:
-  explicit CryptAuthEnrollerFactoryImpl(Profile* profile) : profile_(profile) {}
+  explicit CryptAuthEnrollerFactoryImpl(
+      cryptauth::CryptAuthClientFactory* client_factory)
+      : client_factory_(client_factory) {}
 
   std::unique_ptr<cryptauth::CryptAuthEnroller> CreateInstance() override {
     return std::make_unique<cryptauth::CryptAuthEnrollerImpl>(
-        CreateCryptAuthClientFactoryImpl(profile_),
+        client_factory_,
         cryptauth::SecureMessageDelegateImpl::Factory::NewInstance());
   }
 
  private:
-  Profile* profile_;
+  cryptauth::CryptAuthClientFactory* client_factory_;
 };
 
 }  // namespace
@@ -74,6 +76,9 @@ class CryptAuthEnrollerFactoryImpl
 // static
 std::unique_ptr<ChromeCryptAuthService> ChromeCryptAuthService::Create(
     Profile* profile) {
+  std::unique_ptr<cryptauth::CryptAuthClientFactory> client_factory =
+      CreateCryptAuthClientFactoryImpl(profile);
+
   std::unique_ptr<cryptauth::CryptAuthGCMManager> gcm_manager =
       cryptauth::CryptAuthGCMManagerImpl::Factory::NewInstance(
           gcm::GCMProfileServiceFactory::GetForProfile(profile)->driver(),
@@ -81,14 +86,13 @@ std::unique_ptr<ChromeCryptAuthService> ChromeCryptAuthService::Create(
 
   std::unique_ptr<cryptauth::CryptAuthDeviceManager> device_manager =
       cryptauth::CryptAuthDeviceManagerImpl::Factory::NewInstance(
-          base::DefaultClock::GetInstance(),
-          CreateCryptAuthClientFactoryImpl(profile), gcm_manager.get(),
-          profile->GetPrefs());
+          base::DefaultClock::GetInstance(), client_factory.get(),
+          gcm_manager.get(), profile->GetPrefs());
 
   std::unique_ptr<cryptauth::CryptAuthEnrollmentManager> enrollment_manager =
       cryptauth::CryptAuthEnrollmentManagerImpl::Factory::NewInstance(
           base::DefaultClock::GetInstance(),
-          std::make_unique<CryptAuthEnrollerFactoryImpl>(profile),
+          std::make_unique<CryptAuthEnrollerFactoryImpl>(client_factory.get()),
           cryptauth::SecureMessageDelegateImpl::Factory::NewInstance(),
           GcmDeviceInfoProviderImpl::GetInstance()->GetGcmDeviceInfo(),
           gcm_manager.get(), profile->GetPrefs());
@@ -104,11 +108,13 @@ std::unique_ptr<ChromeCryptAuthService> ChromeCryptAuthService::Create(
       SigninManagerFactory::GetForProfile(profile);
 
   return base::WrapUnique(new ChromeCryptAuthService(
-      std::move(gcm_manager), std::move(device_manager),
-      std::move(enrollment_manager), profile, token_service, signin_manager));
+      std::move(client_factory), std::move(gcm_manager),
+      std::move(device_manager), std::move(enrollment_manager), profile,
+      token_service, signin_manager));
 }
 
 ChromeCryptAuthService::ChromeCryptAuthService(
+    std::unique_ptr<cryptauth::CryptAuthClientFactory> client_factory,
     std::unique_ptr<cryptauth::CryptAuthGCMManager> gcm_manager,
     std::unique_ptr<cryptauth::CryptAuthDeviceManager> device_manager,
     std::unique_ptr<cryptauth::CryptAuthEnrollmentManager> enrollment_manager,
@@ -117,6 +123,7 @@ ChromeCryptAuthService::ChromeCryptAuthService(
     SigninManagerBase* signin_manager)
     : KeyedService(),
       cryptauth::CryptAuthService(),
+      client_factory_(std::move(client_factory)),
       gcm_manager_(std::move(gcm_manager)),
       enrollment_manager_(std::move(enrollment_manager)),
       device_manager_(std::move(device_manager)),

chrome/browser/chromeos/cryptauth/chrome_cryptauth_service.h

@@ -55,6 +55,7 @@ class ChromeCryptAuthService
   // Note: ChromeCryptAuthServiceFactory DependsOn(OAuth2TokenServiceFactory),
   // so |token_service| is guaranteed to outlast this service.
   ChromeCryptAuthService(
+      std::unique_ptr<cryptauth::CryptAuthClientFactory> client_factory,
       std::unique_ptr<cryptauth::CryptAuthGCMManager> gcm_manager,
       std::unique_ptr<cryptauth::CryptAuthDeviceManager> device_manager,
       std::unique_ptr<cryptauth::CryptAuthEnrollmentManager> enrollment_manager,
@@ -74,6 +75,7 @@ class ChromeCryptAuthService
   bool IsEnrollmentAllowedByPolicy();
   void OnPrefsChanged();
 
+  std::unique_ptr<cryptauth::CryptAuthClientFactory> client_factory_;
   std::unique_ptr<cryptauth::CryptAuthGCMManager> gcm_manager_;
   std::unique_ptr<cryptauth::CryptAuthEnrollmentManager> enrollment_manager_;
   std::unique_ptr<cryptauth::CryptAuthDeviceManager> device_manager_;

chromeos/services/device_sync/cryptauth_enroller_factory_impl.cc

@@ -6,7 +6,6 @@
 
 #include <memory>
 
-#include "chromeos/services/device_sync/cryptauth_client_factory_impl.h"
 #include "components/cryptauth/cryptauth_enroller_impl.h"
 #include "components/cryptauth/secure_message_delegate_impl.h"
 
@@ -15,20 +14,15 @@ namespace chromeos {
 namespace device_sync {
 
 CryptAuthEnrollerFactoryImpl::CryptAuthEnrollerFactoryImpl(
-    identity::IdentityManager* identity_manager,
-    scoped_refptr<net::URLRequestContextGetter> url_request_context,
-    const cryptauth::DeviceClassifier& device_classifier)
-    : identity_manager_(identity_manager),
-      url_request_context_(url_request_context),
-      device_classifier_(device_classifier) {}
+    cryptauth::CryptAuthClientFactory* cryptauth_client_factory)
+    : cryptauth_client_factory_(cryptauth_client_factory) {}
 
-CryptAuthEnrollerFactoryImpl::~CryptAuthEnrollerFactoryImpl() {}
+CryptAuthEnrollerFactoryImpl::~CryptAuthEnrollerFactoryImpl() = default;
 
 std::unique_ptr<cryptauth::CryptAuthEnroller>
 CryptAuthEnrollerFactoryImpl::CreateInstance() {
   return std::make_unique<cryptauth::CryptAuthEnrollerImpl>(
-      std::make_unique<CryptAuthClientFactoryImpl>(
-          identity_manager_, url_request_context_, device_classifier_),
+      cryptauth_client_factory_,
       cryptauth::SecureMessageDelegateImpl::Factory::NewInstance());
 }
 

chromeos/services/device_sync/cryptauth_enroller_factory_impl.h

@@ -5,18 +5,11 @@
 #ifndef CHROMEOS_SERVICES_DEVICE_SYNC_CRYPTAUTH_ENROLLER_FACTORY_IMPL_H_
 #define CHROMEOS_SERVICES_DEVICE_SYNC_CRYPTAUTH_ENROLLER_FACTORY_IMPL_H_
 
-#include "base/memory/ref_counted.h"
 #include "components/cryptauth/cryptauth_enroller.h"
-#include "components/cryptauth/proto/cryptauth_api.pb.h"
-#include "net/url_request/url_request_context_getter.h"
 
-namespace identity {
-class IdentityManager;
-}  // namespace identity
-
-namespace net {
-class URLRequestContextGetter;
-}  // namespace net
+namespace cryptauth {
+class CryptAuthClientFactory;
+}  // namespace cryptauth
 
 namespace chromeos {
 
@@ -27,18 +20,14 @@ class CryptAuthEnrollerFactoryImpl
     : public cryptauth::CryptAuthEnrollerFactory {
  public:
   CryptAuthEnrollerFactoryImpl(
-      identity::IdentityManager* identity_manager,
-      scoped_refptr<net::URLRequestContextGetter> url_request_context,
-      const cryptauth::DeviceClassifier& device_classifier);
+      cryptauth::CryptAuthClientFactory* cryptauth_client_factory);
   ~CryptAuthEnrollerFactoryImpl() override;
 
   // cryptauth::CryptAuthEnrollerFactory:
   std::unique_ptr<cryptauth::CryptAuthEnroller> CreateInstance() override;
 
  private:
-  identity::IdentityManager* identity_manager_;
-  const scoped_refptr<net::URLRequestContextGetter> url_request_context_;
-  const cryptauth::DeviceClassifier device_classifier_;
+  cryptauth::CryptAuthClientFactory* cryptauth_client_factory_;
 };
 
 }  // namespace device_sync

chromeos/services/device_sync/device_sync_impl.cc

@@ -257,15 +257,16 @@ void DeviceSyncImpl::InitializeCryptAuthManagementObjects() {
           gcm_driver_, pref_service_.get());
   cryptauth_gcm_manager_->StartListening();
 
+  cryptauth_client_factory_ = std::make_unique<CryptAuthClientFactoryImpl>(
+      identity_manager_, url_request_context_,
+      cryptauth::device_classifier_util::GetDeviceClassifier());
+
   // Initialize |crypauth_device_manager_| and start observing. Start() is not
   // called yet since the device has not completed enrollment.
   cryptauth_device_manager_ =
       cryptauth::CryptAuthDeviceManagerImpl::Factory::NewInstance(
-          clock_,
-          std::make_unique<CryptAuthClientFactoryImpl>(
-              identity_manager_, url_request_context_,
-              cryptauth::device_classifier_util::GetDeviceClassifier()),
-          cryptauth_gcm_manager_.get(), pref_service_.get());
+          clock_, cryptauth_client_factory_.get(), cryptauth_gcm_manager_.get(),
+          pref_service_.get());
 
   // Initialize |cryptauth_enrollment_manager_| and start observing, then call
   // Start() immediately to schedule enrollment.
@@ -273,8 +274,7 @@ void DeviceSyncImpl::InitializeCryptAuthManagementObjects() {
       cryptauth::CryptAuthEnrollmentManagerImpl::Factory::NewInstance(
           clock_,
           std::make_unique<CryptAuthEnrollerFactoryImpl>(
-              identity_manager_, url_request_context_,
-              cryptauth::device_classifier_util::GetDeviceClassifier()),
+              cryptauth_client_factory_.get()),
           cryptauth::SecureMessageDelegateImpl::Factory::NewInstance(),
           gcm_device_info_provider_->GetGcmDeviceInfo(),
           cryptauth_gcm_manager_.get(), pref_service_.get());

chromeos/services/device_sync/device_sync_impl.h

@@ -25,6 +25,7 @@ class Clock;
 }  // namespace base
 
 namespace cryptauth {
+class CryptAuthClientFactory;
 class CryptAuthDeviceManager;
 class GcmDeviceInfoProvider;
 }  // namespace cryptauth
@@ -157,6 +158,7 @@ class DeviceSyncImpl : public mojom::DeviceSync,
   std::unique_ptr<PrefService> pref_service_;
 
   std::unique_ptr<cryptauth::CryptAuthGCMManager> cryptauth_gcm_manager_;
+  std::unique_ptr<cryptauth::CryptAuthClientFactory> cryptauth_client_factory_;
   std::unique_ptr<cryptauth::CryptAuthEnrollmentManager>
       cryptauth_enrollment_manager_;
   std::unique_ptr<cryptauth::CryptAuthDeviceManager> cryptauth_device_manager_;

chromeos/services/device_sync/device_sync_service_unittest.cc

@@ -122,7 +122,7 @@ class FakeCryptAuthDeviceManagerFactory
   // cryptauth::CryptAuthDeviceManagerImpl::Factory:
   std::unique_ptr<cryptauth::CryptAuthDeviceManager> BuildInstance(
       base::Clock* clock,
-      std::unique_ptr<cryptauth::CryptAuthClientFactory> client_factory,
+      cryptauth::CryptAuthClientFactory* client_factory,
       cryptauth::CryptAuthGCMManager* gcm_manager,
       PrefService* pref_service) override {
     EXPECT_EQ(simple_test_clock_, clock);

components/cryptauth/cryptauth_device_manager_impl.cc

@@ -325,13 +325,13 @@ CryptAuthDeviceManagerImpl::Factory*
 std::unique_ptr<CryptAuthDeviceManager>
 CryptAuthDeviceManagerImpl::Factory::NewInstance(
     base::Clock* clock,
-    std::unique_ptr<CryptAuthClientFactory> client_factory,
+    CryptAuthClientFactory* cryptauth_client_factory,
     CryptAuthGCMManager* gcm_manager,
     PrefService* pref_service) {
   if (!factory_instance_)
     factory_instance_ = new Factory();
 
-  return factory_instance_->BuildInstance(clock, std::move(client_factory),
+  return factory_instance_->BuildInstance(clock, cryptauth_client_factory,
                                           gcm_manager, pref_service);
 }
 
@@ -346,20 +346,20 @@ CryptAuthDeviceManagerImpl::Factory::~Factory() = default;
 std::unique_ptr<CryptAuthDeviceManager>
 CryptAuthDeviceManagerImpl::Factory::BuildInstance(
     base::Clock* clock,
-    std::unique_ptr<CryptAuthClientFactory> client_factory,
+    CryptAuthClientFactory* cryptauth_client_factory,
     CryptAuthGCMManager* gcm_manager,
     PrefService* pref_service) {
   return base::WrapUnique(new CryptAuthDeviceManagerImpl(
-      clock, std::move(client_factory), gcm_manager, pref_service));
+      clock, cryptauth_client_factory, gcm_manager, pref_service));
 }
 
 CryptAuthDeviceManagerImpl::CryptAuthDeviceManagerImpl(
     base::Clock* clock,
-    std::unique_ptr<CryptAuthClientFactory> client_factory,
+    CryptAuthClientFactory* cryptauth_client_factory,
     CryptAuthGCMManager* gcm_manager,
     PrefService* pref_service)
     : clock_(clock),
-      client_factory_(std::move(client_factory)),
+      cryptauth_client_factory_(cryptauth_client_factory),
       gcm_manager_(gcm_manager),
       pref_service_(pref_service),
       scheduler_(CreateSyncScheduler(this)),
@@ -554,7 +554,7 @@ void CryptAuthDeviceManagerImpl::OnSyncRequested(
   NotifySyncStarted();
 
   sync_request_ = std::move(sync_request);
-  cryptauth_client_ = client_factory_->CreateInstance();
+  cryptauth_client_ = cryptauth_client_factory_->CreateInstance();
 
   InvocationReason invocation_reason = INVOCATION_REASON_UNKNOWN;
 

components/cryptauth/cryptauth_device_manager_impl.h

@@ -31,7 +31,7 @@ class CryptAuthDeviceManagerImpl : public CryptAuthDeviceManager,
    public:
     static std::unique_ptr<CryptAuthDeviceManager> NewInstance(
         base::Clock* clock,
-        std::unique_ptr<CryptAuthClientFactory> client_factory,
+        CryptAuthClientFactory* cryptauth_client_factory,
         CryptAuthGCMManager* gcm_manager,
         PrefService* pref_service);
 
@@ -41,7 +41,7 @@ class CryptAuthDeviceManagerImpl : public CryptAuthDeviceManager,
     virtual ~Factory();
     virtual std::unique_ptr<CryptAuthDeviceManager> BuildInstance(
         base::Clock* clock,
-        std::unique_ptr<CryptAuthClientFactory> client_factory,
+        CryptAuthClientFactory* cryptauth_client_factory,
         CryptAuthGCMManager* gcm_manager,
         PrefService* pref_service);
 
@@ -67,17 +67,17 @@ class CryptAuthDeviceManagerImpl : public CryptAuthDeviceManager,
  protected:
   // Creates the manager:
   // |clock|: Used to determine the time between sync attempts.
-  // |client_factory|: Creates CryptAuthClient instances to perform each sync.
-  // |gcm_manager|: Notifies when GCM push messages trigger device syncs.
+  // |cryptauth_client_factory|: Creates CryptAuthClient instances to perform
+  // each sync. |gcm_manager|: Notifies when GCM push messages trigger device
+  // syncs.
   //                Not owned and must outlive this instance.
   // |pref_service|: Stores syncing metadata and unlock key information to
   //                 persist across browser restarts. Must already be registered
   //                 with RegisterPrefs().
-  CryptAuthDeviceManagerImpl(
-      base::Clock* clock,
-      std::unique_ptr<CryptAuthClientFactory> client_factory,
-      CryptAuthGCMManager* gcm_manager,
-      PrefService* pref_service);
+  CryptAuthDeviceManagerImpl(base::Clock* clock,
+                             CryptAuthClientFactory* cryptauth_client_factory,
+                             CryptAuthGCMManager* gcm_manager,
+                             PrefService* pref_service);
 
   void SetSyncSchedulerForTest(std::unique_ptr<SyncScheduler> sync_scheduler);
 
@@ -100,7 +100,7 @@ class CryptAuthDeviceManagerImpl : public CryptAuthDeviceManager,
   base::Clock* clock_;
 
   // Creates CryptAuthClient instances for each sync attempt.
-  std::unique_ptr<CryptAuthClientFactory> client_factory_;
+  CryptAuthClientFactory* cryptauth_client_factory_;
 
   // Notifies when GCM push messages trigger device sync. Not owned and must
   // outlive this instance.

components/cryptauth/cryptauth_device_manager_impl_unittest.cc

@@ -277,13 +277,12 @@ void ExpectSyncedDevicesAndPrefAreEqual(
 // Harness for testing CryptAuthDeviceManager.
 class TestCryptAuthDeviceManager : public CryptAuthDeviceManagerImpl {
  public:
-  TestCryptAuthDeviceManager(
-      base::Clock* clock,
-      std::unique_ptr<CryptAuthClientFactory> client_factory,
-      CryptAuthGCMManager* gcm_manager,
-      PrefService* pref_service)
+  TestCryptAuthDeviceManager(base::Clock* clock,
+                             CryptAuthClientFactory* client_factory,
+                             CryptAuthGCMManager* gcm_manager,
+                             PrefService* pref_service)
       : CryptAuthDeviceManagerImpl(clock,
-                                   std::move(client_factory),
+                                   client_factory,
                                    gcm_manager,
                                    pref_service),
         scoped_sync_scheduler_(new NiceMock<MockSyncScheduler>()),
@@ -319,7 +318,7 @@ class CryptAuthDeviceManagerImplTest
       public MockCryptAuthClientFactory::Observer {
  protected:
   CryptAuthDeviceManagerImplTest()
-      : client_factory_(new MockCryptAuthClientFactory(
+      : client_factory_(std::make_unique<MockCryptAuthClientFactory>(
             MockCryptAuthClientFactory::MockType::MAKE_STRICT_MOCKS)),
         gcm_manager_("existing gcm registration id") {
     client_factory_->AddObserver(this);
@@ -408,8 +407,7 @@ class CryptAuthDeviceManagerImplTest
     }
 
     device_manager_.reset(new TestCryptAuthDeviceManager(
-        &clock_, base::WrapUnique(client_factory_), &gcm_manager_,
-        &pref_service_));
+        &clock_, client_factory_.get(), &gcm_manager_, &pref_service_));
     device_manager_->AddObserver(this);
 
     get_my_devices_response_.add_devices()->CopyFrom(devices_in_response_[0]);
@@ -470,8 +468,7 @@ class CryptAuthDeviceManagerImplTest
 
   base::SimpleTestClock clock_;
 
-  // Owned by |device_manager_|.
-  MockCryptAuthClientFactory* client_factory_;
+  std::unique_ptr<MockCryptAuthClientFactory> client_factory_;
 
   TestingPrefServiceSimple pref_service_;
 
@@ -537,11 +534,8 @@ TEST_F(CryptAuthDeviceManagerImplTest, InitWithDefaultPrefs) {
   TestingPrefServiceSimple pref_service;
   CryptAuthDeviceManager::RegisterPrefs(pref_service.registry());
 
-  TestCryptAuthDeviceManager device_manager(
-      &clock,
-      std::make_unique<MockCryptAuthClientFactory>(
-          MockCryptAuthClientFactory::MockType::MAKE_STRICT_MOCKS),
-      &gcm_manager_, &pref_service);
+  TestCryptAuthDeviceManager device_manager(&clock, client_factory_.get(),
+                                            &gcm_manager_, &pref_service);
 
   EXPECT_CALL(
       *(device_manager.GetSyncScheduler()),

components/cryptauth/cryptauth_enroller_impl.cc

@@ -55,9 +55,9 @@ std::string CreateEnrollmentPublicMetadata() {
 }  // namespace
 
 CryptAuthEnrollerImpl::CryptAuthEnrollerImpl(
-    std::unique_ptr<CryptAuthClientFactory> client_factory,
+    CryptAuthClientFactory* client_factory,
     std::unique_ptr<SecureMessageDelegate> secure_message_delegate)
-    : client_factory_(std::move(client_factory)),
+    : client_factory_(client_factory),
       secure_message_delegate_(std::move(secure_message_delegate)),
       weak_ptr_factory_(this) {}
 

components/cryptauth/cryptauth_enroller_impl.h

@@ -32,7 +32,7 @@ class CryptAuthEnrollerImpl : public CryptAuthEnroller {
   // |client_factory| creates CryptAuthClient instances for making API calls.
   // |crypto_delegate| is responsible for SecureMessage operations.
   CryptAuthEnrollerImpl(
-      std::unique_ptr<CryptAuthClientFactory> client_factory,
+      CryptAuthClientFactory* client_factory,
       std::unique_ptr<SecureMessageDelegate> secure_message_delegate);
   ~CryptAuthEnrollerImpl() override;
 
@@ -62,7 +62,7 @@ class CryptAuthEnrollerImpl : public CryptAuthEnroller {
   void OnOuterSecureMessageCreated(const std::string& outer_message);
 
   // Creates the CryptAuthClient instances to make API requests.
-  std::unique_ptr<CryptAuthClientFactory> client_factory_;
+  CryptAuthClientFactory* client_factory_;
 
   // Handles SecureMessage operations.
   std::unique_ptr<SecureMessageDelegate> secure_message_delegate_;

components/cryptauth/cryptauth_enroller_impl_unittest.cc

@@ -103,10 +103,10 @@ class CryptAuthEnrollerTest
       public MockCryptAuthClientFactory::Observer {
  public:
   CryptAuthEnrollerTest()
-      : client_factory_(new MockCryptAuthClientFactory(
+      : client_factory_(std::make_unique<MockCryptAuthClientFactory>(
             MockCryptAuthClientFactory::MockType::MAKE_NICE_MOCKS)),
         secure_message_delegate_(new FakeSecureMessageDelegate()),
-        enroller_(base::WrapUnique(client_factory_),
+        enroller_(client_factory_.get(),
                   base::WrapUnique(secure_message_delegate_)) {
     client_factory_->AddObserver(this);
 
@@ -243,7 +243,7 @@ class CryptAuthEnrollerTest
   std::string user_private_key_;
 
   // Owned by |enroller_|.
-  MockCryptAuthClientFactory* client_factory_;
+  std::unique_ptr<MockCryptAuthClientFactory> client_factory_;
   // Owned by |enroller_|.
   FakeSecureMessageDelegate* secure_message_delegate_;
   // The CryptAuthEnroller under test.