ChromeCryptAuthService: only perform device sync once enrollment is complete.

chrome/browser/cryptauth/chrome_cryptauth_service.cc

 // Copyright 2017 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/cryptauth/chrome_cryptauth_service.h"
 
 #include "base/guid.h"
 #include "base/memory/ptr_util.h"
 #include "base/sys_info.h"
 #include "base/time/default_clock.h"
 #include "base/version.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_content_browser_client.h"
 #include "chrome/browser/gcm/gcm_profile_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/common/pref_names.h"
 #include "components/cryptauth/cryptauth_client.h"
 #include "components/cryptauth/cryptauth_client_impl.h"
 #include "components/cryptauth/cryptauth_device_manager.h"
 #include "components/cryptauth/cryptauth_enroller.h"
 #include "components/cryptauth/cryptauth_enroller_impl.h"
-#include "components/cryptauth/cryptauth_enrollment_manager.h"
 #include "components/cryptauth/cryptauth_enrollment_utils.h"
 #include "components/cryptauth/cryptauth_gcm_manager_impl.h"
 #include "components/cryptauth/proto/cryptauth_api.pb.h"
 #include "components/cryptauth/secure_message_delegate.h"
 #include "components/gcm_driver/gcm_profile_service.h"
 #include "components/prefs/pref_service.h"
 #include "components/proximity_auth/logging/logging.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/translate/core/browser/translate_download_manager.h"
@@ skipping 184 matching lines @@
       device_manager_(std::move(device_manager)),
       profile_(profile),
       token_service_(token_service) {
   gcm_manager_->StartListening();
 
   if (!token_service_->RefreshTokenIsAvailable(GetAccountId())) {
     PA_LOG(INFO) << "Refresh token not yet available; "
                  << "waiting before starting CryptAuth managers.";
     token_service_->AddObserver(this);
   } else {
-    enrollment_manager_->Start();
-    device_manager_->Start();
+    PerformEnrollmentAndDeviceSync();
   }
 }
 
 ChromeCryptAuthService::~ChromeCryptAuthService() {}
 
 void ChromeCryptAuthService::Shutdown() {
   token_service_->RemoveObserver(this);
   enrollment_manager_.reset();
   device_manager_.reset();
   gcm_manager_.reset();
@@ skipping 20 matching lines @@
 std::unique_ptr<cryptauth::SecureMessageDelegate>
 ChromeCryptAuthService::CreateSecureMessageDelegate() {
   return CreateSecureMessageDelegateImpl();
 }
 
 std::unique_ptr<cryptauth::CryptAuthClientFactory>
 ChromeCryptAuthService::CreateCryptAuthClientFactory() {
   return CreateCryptAuthClientFactoryImpl(profile_);
 }
 
+void ChromeCryptAuthService::OnEnrollmentStarted() {}
+
+void ChromeCryptAuthService::OnEnrollmentFinished(bool success) {

[khorimoto, 2017/05/30 21:38:18]

If !success, the device manager won't actually work, right? Should we be
retrying the enrollment attempt here?

[Tim Song, 2017/05/31 00:04:21]

The enrollment manager should have its own retry logic, so we don't need to
retry the enrollment.

However, I agree that we should check that success == true.

[Ryan Hansberry, 2017/05/31 17:08:09]

Done.

+  device_manager_->Start();
+  enrollment_manager_->RemoveObserver(this);
+}
+
 void ChromeCryptAuthService::OnRefreshTokenAvailable(
     const std::string& account_id) {
+  PA_LOG(ERROR) << "ChromeCAS: OnRefreshTokenAvailable";

[khorimoto, 2017/05/30 21:38:18]

Remove these logs.

[Ryan Hansberry, 2017/05/31 17:08:09]

Oops, removed.

   if (account_id == GetAccountId()) {
+    PA_LOG(ERROR) << "ChromeCAS: OnRefreshTokenAvailable starting managers";
     token_service_->RemoveObserver(this);
-    enrollment_manager_->Start();
-    device_manager_->Start();
+    PerformEnrollmentAndDeviceSync();
   }
 }
+
+void ChromeCryptAuthService::PerformEnrollmentAndDeviceSync() {
+  // If enrollment is not valid, wait for the new enrollment attempt to finish

[khorimoto, 2017/05/30 21:38:18]

nit: This comment should probably be moved to the else block.

[Ryan Hansberry, 2017/05/31 17:08:09]

Done.

+  // before starting CryptAuthDeviceManager. See OnEnrollmentFinished(),
+  if (enrollment_manager_->IsEnrollmentValid())
+    device_manager_->Start();
+  else
+    enrollment_manager_->AddObserver(this);
+
+  enrollment_manager_->Start();

[khorimoto, 2017/05/30 21:38:18]

Do we still need to call Start() here if the enrollment is already valid above?

[Ryan Hansberry, 2017/05/31 17:08:09]

Yes, in order for EnrollmentManager to schedule its next enrollment.

[khorimoto, 2017/05/31 17:53:38]

nit: Thanks for the explanation - please add a comment explaining why we need to
Start() regardless.

[Ryan Hansberry, 2017/05/31 18:14:32]

Done.

+}