[CrOS MultiDevice] Fix DeviceReenroller by using DeviceSyncClient::Observer callbacks

The current implementation of DeviceReenroller uses the incorrect
callbacks to signal when an enrollment and device sync are complete.
Specifically, the ForceEnrollmentNow() and ForceSyncNow() callbacks were
used instead of the DeviceSyncClient::Observer callbacks,
OnEnrollmentFinished() and OnNewDevicesSynced().

Here, we use to the correct callbacks along with a more appropriate and
simplified retry mechanism. Namely, the complete re-enrollment
process of enrollment --> device sync --> verification is retried every
5 minutes or until success.

Note: The OnEnrollmentFinished() and OnNewDevicesSynced() callbacks
might be invoked due to enrollments or device syncs external to the
DeviceReenroller class. Regardless of how the functions are called, the
two sets of supported software features (from GCM device info and local
device metadata) are compared. If they disagree, the re-enrollment
process is triggered.

Bug: 870770
Change-Id: If33320386cd8ce5d5639082009291b935ec6e471
Tested: Manual and unit tests
Reviewed-on: https://chromium-review.googlesource.com/1228311
Commit-Queue: Josh Nohle <nohle@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592816}

chromeos/services/multidevice_setup/BUILD.gn

@@ -62,6 +62,7 @@ static_library("multidevice_setup") {
     "//chromeos/services/secure_channel/public/cpp/client",
     "//chromeos/services/secure_channel/public/mojom",
     "//components/cryptauth",
+    "//components/cryptauth/proto:util",
     "//components/pref_registry",
     "//components/prefs:prefs",
     "//services/service_manager/public/cpp",

chromeos/services/multidevice_setup/device_reenroller.cc

@@ -7,8 +7,8 @@
 #include "base/containers/flat_set.h"
 #include "base/no_destructor.h"
 #include "chromeos/components/proximity_auth/logging/logging.h"
-#include "chromeos/services/device_sync/public/cpp/device_sync_client.h"
 #include "components/cryptauth/gcm_device_info_provider.h"
+#include "components/cryptauth/proto/enum_util.h"
 
 namespace chromeos {
 
@@ -16,12 +16,12 @@ namespace multidevice_setup {
 
 namespace {
 
-// The number of minutes to wait before retrying a failed re-enrollment or
-// device sync attempt.
+// The number of minutes to wait before retrying a failed re-enrollment attempt.
 const int kNumMinutesBetweenRetries = 5;
 
-std::vector<cryptauth::SoftwareFeature>
-ComputeSupportedSoftwareFeaturesSortedDedupedListFromGcmDeviceInfo(
+// Returns a sorted and deduped list of the supported software features from
+// GcmDeviceInfo.
+std::vector<cryptauth::SoftwareFeature> GetSupportedFeaturesFromGcmDeviceInfo(
     const cryptauth::GcmDeviceInfo& gcm_device_info) {
   base::flat_set<cryptauth::SoftwareFeature> sorted_and_deduped_set;
   for (int i = 0; i < gcm_device_info.supported_software_features_size(); ++i) {
@@ -32,21 +32,33 @@ ComputeSupportedSoftwareFeaturesSortedDedupedListFromGcmDeviceInfo(
                                                  sorted_and_deduped_set.end());
 }
 
-std::vector<cryptauth::SoftwareFeature>
-ComputeSupportedSoftwareFeaturesSortedDedupedListFromLocalDeviceMetadata(
-    const cryptauth::RemoteDeviceRef& local_device_metadata) {
-  base::flat_set<cryptauth::SoftwareFeature> sorted_and_deduped_set;
-  for (int i = cryptauth::SoftwareFeature_MIN;
-       i <= cryptauth::SoftwareFeature_MAX; ++i) {
-    cryptauth::SoftwareFeature feature =
-        static_cast<cryptauth::SoftwareFeature>(i);
-    if (local_device_metadata.GetSoftwareFeatureState(feature) !=
-        cryptauth::SoftwareFeatureState::kNotSupported) {
-      sorted_and_deduped_set.insert(feature);
+void OnForceEnrollmentNow(bool success) {
+  if (success) {
+    PA_LOG(VERBOSE) << "Forced enrollment was successfully requested.";
+    return;
   }
+  PA_LOG(WARNING) << "Forced enrollment was not successfully requested. "
+                  << "Waiting for " << kNumMinutesBetweenRetries << "-minute "
+                  << "re-enrollment retry timer to fire.";
+}
+
+void OnForceSyncNow(bool success) {
+  if (success) {
+    PA_LOG(VERBOSE) << "Forced device sync was successfully requested.";
+    return;
   }
-  return std::vector<cryptauth::SoftwareFeature>(sorted_and_deduped_set.begin(),
-                                                 sorted_and_deduped_set.end());
+  PA_LOG(WARNING) << "Forced device sync was not successfully requested. "
+                  << "Waiting for " << kNumMinutesBetweenRetries << "-minute "
+                  << "re-enrollment retry timer to fire.";
+}
+
+std::string CreateSoftwareFeaturesString(
+    const std::vector<cryptauth::SoftwareFeature>& software_features) {
+  std::stringstream ss;
+  for (cryptauth::SoftwareFeature feature : software_features) {
+    ss << feature << " ";
+  }
+  return ss.str();
 }
 
 }  // namespace
@@ -85,80 +97,89 @@ DeviceReenroller::DeviceReenroller(
     const cryptauth::GcmDeviceInfoProvider* gcm_device_info_provider,
     std::unique_ptr<base::OneShotTimer> timer)
     : device_sync_client_(device_sync_client),
-      gcm_supported_software_features_(
-          ComputeSupportedSoftwareFeaturesSortedDedupedListFromGcmDeviceInfo(
+      gcm_supported_software_features_(GetSupportedFeaturesFromGcmDeviceInfo(
           gcm_device_info_provider->GetGcmDeviceInfo())),
       timer_(std::move(timer)) {
-  // If the current set of supported software features from GcmDeviceInfo
-  // differs from that of the local device metadata on the CryptAuth server,
-  // attempt re-enrollment. Note: Both lists here are sorted and duplicate-free.
-  if (gcm_supported_software_features_ !=
-      ComputeSupportedSoftwareFeaturesSortedDedupedListFromLocalDeviceMetadata(
-          *device_sync_client_->GetLocalDeviceMetadata())) {
-    AttemptReenrollment();
-  }
+  DCHECK(device_sync_client_->is_ready());
+  device_sync_client_->AddObserver(this);
+  AttemptReenrollmentIfNecessary();
 }
 
-void DeviceReenroller::AttemptReenrollment() {
-  DCHECK(!timer_->IsRunning());
-  device_sync_client_->ForceEnrollmentNow(base::BindOnce(
-      &DeviceReenroller::OnForceEnrollmentNowComplete, base::Unretained(this)));
+void DeviceReenroller::AttemptReenrollmentIfNecessary() {
+  std::vector<cryptauth::SoftwareFeature> metadata_supported_software_features =
+      GetSupportedFeaturesForLocalDevice();
+
+  if (gcm_supported_software_features_ ==
+      metadata_supported_software_features) {
+    PA_LOG(VERBOSE) << "The supported software features of local device "
+                    << "metadata agree with those of GCM device info. No "
+                    << "further action taken.";
+    return;
+  }
+
+  PA_LOG(INFO)
+      << "Supported software feature mismatch. Attempting re-enrollment now."
+      << std::endl
+      << "    ---GcmDeviceInfo Supported Software Features---" << std::endl
+      << "    "
+      << CreateSoftwareFeaturesString(gcm_supported_software_features_)
+      << std::endl
+      << "    ---Local Device Metadata Supported Software Features---"
+      << std::endl
+      << "    "
+      << CreateSoftwareFeaturesString(metadata_supported_software_features);
+  // Attempt re-enrollment now and schedule a check-up in 5 minutes.
+  device_sync_client_->ForceEnrollmentNow(
+      base::BindOnce(&OnForceEnrollmentNow));
+  timer_->Start(
+      FROM_HERE, base::TimeDelta::FromMinutes(kNumMinutesBetweenRetries),
+      base::BindOnce(&DeviceReenroller::AttemptReenrollmentIfNecessary,
+                     base::Unretained(this)));
 }
 
-void DeviceReenroller::AttemptDeviceSync() {
-  DCHECK(!timer_->IsRunning());
-  device_sync_client_->ForceSyncNow(base::BindOnce(
-      &DeviceReenroller::OnForceSyncNowComplete, base::Unretained(this)));
+std::vector<cryptauth::SoftwareFeature>
+DeviceReenroller::GetSupportedFeaturesForLocalDevice() {
+  const cryptauth::RemoteDeviceRef& local_device_metadata =
+      *device_sync_client_->GetLocalDeviceMetadata();
+
+  base::flat_set<cryptauth::SoftwareFeature> sorted_and_deduped_set;
+  for (int i = cryptauth::SoftwareFeature_MIN;
+       i <= cryptauth::SoftwareFeature_MAX; ++i) {
+    cryptauth::SoftwareFeature feature =
+        static_cast<cryptauth::SoftwareFeature>(i);
+    if (local_device_metadata.GetSoftwareFeatureState(feature) !=
+        cryptauth::SoftwareFeatureState::kNotSupported) {
+      sorted_and_deduped_set.insert(feature);
+    }
+  }
+  return std::vector<cryptauth::SoftwareFeature>(sorted_and_deduped_set.begin(),
+                                                 sorted_and_deduped_set.end());
 }
 
-void DeviceReenroller::OnForceEnrollmentNowComplete(bool success) {
-  if (success) {
-    PA_LOG(INFO) << "DeviceReenroller::OnForceEnrollmentNowComplete(): "
-                 << "Forced enrollment attempt was successful. "
-                 << "Syncing devices now.";
-    AttemptDeviceSync();
+void DeviceReenroller::OnEnrollmentFinished() {
+  // Only sync devices if the features disagree. This is important to check
+  // because OnEnrollmentFinished() might be called due to an enrollment that
+  // was not started in DeviceReenroller.
+  if (gcm_supported_software_features_ ==
+      GetSupportedFeaturesForLocalDevice()) {
     return;
   }
-  PA_LOG(WARNING) << "DeviceReenroller::OnForceEnrollmentNowComplete(): "
-                  << "Forced enrollment attempt was unsuccessful. Retrying in "
-                  << kNumMinutesBetweenRetries << " minutes.";
-  timer_->Start(FROM_HERE,
-                base::TimeDelta::FromMinutes(kNumMinutesBetweenRetries),
-                base::BindOnce(&DeviceReenroller::AttemptReenrollment,
-                               base::Unretained(this)));
+  PA_LOG(VERBOSE) << "An enrollment finished. Syncing now.";
+  device_sync_client_->ForceSyncNow(base::BindOnce(&OnForceSyncNow));
 }
 
-void DeviceReenroller::OnForceSyncNowComplete(bool success) {
-  // This is used to track if the device sync properly updated the local device
-  // metadata to reflect the supported software features from GcmDeviceInfo.
-  bool local_device_metadata_agrees =
-      device_sync_client_->GetLocalDeviceMetadata() &&
-      ComputeSupportedSoftwareFeaturesSortedDedupedListFromLocalDeviceMetadata(
-          *device_sync_client_->GetLocalDeviceMetadata()) ==
-          gcm_supported_software_features_;
-
-  if (success && local_device_metadata_agrees) {
-    PA_LOG(INFO) << "DeviceReenroller::OnForceSyncNowComplete(): "
-                 << "Forced device sync attempt was successful.";
+void DeviceReenroller::OnNewDevicesSynced() {
+  PA_LOG(VERBOSE) << "A device sync finished. Waiting for verification.";
+  // If the retry timer is running, then wait for it to call
+  // AttemptReenrollmentIfNecessary(); otherwise, call it immediately. The timer
+  // should be running if a re-enrollment process that was triggered by
+  // DeviceReenroller (not externally) is still in progress. So, we do not want
+  // to spam the server with immediate retries should the features still not
+  // agree.
+  if (timer_->IsRunning()) {
     return;
   }
-  if (!success) {
-    PA_LOG(WARNING) << "DeviceReenroller::OnForceSyncNowComplete(): "
-                    << "Forced device sync attempt was unsuccessful. "
-                    << "Retrying in " << kNumMinutesBetweenRetries
-                    << " minutes.";
-  } else {
-    DCHECK(!local_device_metadata_agrees);
-    PA_LOG(WARNING) << "DeviceReenroller::OnForceSyncNowComplete(): "
-                    << "The local device metadata's supported software "
-                    << "features do not agree with the set extracted from GCM "
-                    << "device info. Retrying in " << kNumMinutesBetweenRetries
-                    << " minutes.";
-  }
-  timer_->Start(FROM_HERE,
-                base::TimeDelta::FromMinutes(kNumMinutesBetweenRetries),
-                base::BindOnce(&DeviceReenroller::AttemptDeviceSync,
-                               base::Unretained(this)));
+  AttemptReenrollmentIfNecessary();
 }
 
 }  // namespace multidevice_setup

chromeos/services/multidevice_setup/device_reenroller.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "chromeos/services/device_sync/public/cpp/device_sync_client.h"
 #include "components/cryptauth/proto/cryptauth_api.pb.h"
 
 namespace base {
@@ -20,16 +21,37 @@ class GcmDeviceInfoProvider;
 
 namespace chromeos {
 
-namespace device_sync {
-class DeviceSyncClient;
-}  // namespace device_sync
-
 namespace multidevice_setup {
 
-// The DeviceReenroller constructor re-enrolls and syncs the device if the set
-// of supported SoftwareFeatures in the current GCM device info differs from
-// that of the local device metadata on the CryptAuth server.
-class DeviceReenroller {
+// This class re-enrolls the device if the set of supported SoftwareFeatures in
+// the GCM device info differs from that of the local device metadata. This
+// condition is checked in the constructor and any time the
+// DeviceSyncClient::Observer callbacks--OnEnrollmentFinished() and
+// OnNewDevicesSync()--are called.
+//
+// The supported software features listed in GCM device info should be
+// considered the new source of truth. Enrollment updates a device's list of
+// supported software features (among other things) on the backend to conform
+// with the GCM device info. Then, a device sync is necessary to update the
+// cache of device information with the latest backend data. The local device
+// metadata is part of this cache.
+//
+// The flow of the class is as follows:
+//
+//   +-------------------------------------------------------------------------+
+//   |                                                                         |
+//   |              (From external enrollment)      (From external device sync)|
+//   |                         |                                    |          |
+//   V                         V                                    V          |
+// Start-->Enrollment-->OnEnrollmentFinished-->DeviceSync-->OnNewDevicesSynced-+
+//   |                         |
+//   |                         |
+//   V                         V
+//   If features agree, then done
+//
+// A five-minute retry timer is started at the beginning of the flow. Should any
+// step fail, the process will be re-started when the timer fires.
+class DeviceReenroller : public device_sync::DeviceSyncClient::Observer {
  public:
   class Factory {
    public:
@@ -46,7 +68,7 @@ class DeviceReenroller {
     static Factory* test_factory_;
   };
 
-  virtual ~DeviceReenroller();
+  ~DeviceReenroller() override;
 
  private:
   DeviceReenroller(
@@ -54,22 +76,18 @@ class DeviceReenroller {
       const cryptauth::GcmDeviceInfoProvider* gcm_device_info_provider,
       std::unique_ptr<base::OneShotTimer> timer);
 
-  void AttemptReenrollment();
-  void AttemptDeviceSync();
+  void AttemptReenrollmentIfNecessary();
+  // Returns a sorted and deduped list of the supported or enabled software
+  // features from DeviceSyncClient::GetLocalDeviceMetadata().
+  std::vector<cryptauth::SoftwareFeature> GetSupportedFeaturesForLocalDevice();
 
-  // If the re-enrollment was successful, force a device sync; otherwise, retry
-  // re-enrollment every 5 minutes or until success.
-  void OnForceEnrollmentNowComplete(bool success);
-  // If the device sync was successful and the list of supported software
-  // features on the CryptAuth server now agrees with the list of supported
-  // software features in GcmDeviceInfo, log the success; otherwise, retry
-  // device sync every 5 minutes or until success.
-  void OnForceSyncNowComplete(bool success);
+  // device_sync::DeviceSyncClient::Observer:
+  void OnEnrollmentFinished() override;
+  void OnNewDevicesSynced() override;
 
   device_sync::DeviceSyncClient* device_sync_client_;
-  // The sorted and deduped list of supported software features extracted from
-  // GcmDeviceInfo.
-  std::vector<cryptauth::SoftwareFeature> gcm_supported_software_features_;
+  const std::vector<cryptauth::SoftwareFeature>
+      gcm_supported_software_features_;
   std::unique_ptr<base::OneShotTimer> timer_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceReenroller);

chromeos/services/multidevice_setup/device_reenroller_unittest.cc

@@ -25,6 +25,7 @@ class MultiDeviceSetupDeviceReenrollerTest : public testing::Test {
   void SetUp() override {
     fake_device_sync_client_ =
         std::make_unique<device_sync::FakeDeviceSyncClient>();
+    fake_device_sync_client_->NotifyReady();
 
     fake_gcm_device_info_provider_ =
         std::make_unique<cryptauth::FakeGcmDeviceInfoProvider>(
@@ -66,6 +67,19 @@ class MultiDeviceSetupDeviceReenrollerTest : public testing::Test {
         std::move(mock_timer));
   }
 
+  // After a successful re-enrollment and device sync, there should be a timer
+  // running to confirm that everything worked as expected.
+  void FireTimerAndVerifyResults() {
+    // Check-up timer should be running.
+    EXPECT_TRUE(timer()->IsRunning());
+    // Check should now pass with no further action taken.
+    timer()->Fire();
+    EXPECT_EQ(
+        0, fake_device_sync_client()->GetForceEnrollmentNowCallbackQueueSize());
+    EXPECT_EQ(0, fake_device_sync_client()->GetForceSyncNowCallbackQueueSize());
+    EXPECT_FALSE(timer()->IsRunning());
+  }
+
  private:
   std::unique_ptr<DeviceReenroller> device_reenroller_;
 
@@ -126,8 +140,10 @@ TEST_F(MultiDeviceSetupDeviceReenrollerTest,
   // Assume successful enrollment, sync, and local device metadata update.
   EXPECT_EQ(
       1, fake_device_sync_client()->GetForceEnrollmentNowCallbackQueueSize());
+  EXPECT_TRUE(timer()->IsRunning());
   fake_device_sync_client()->InvokePendingForceEnrollmentNowCallback(
       true /* success */);
+  fake_device_sync_client()->NotifyEnrollmentFinished();
   EXPECT_EQ(1, fake_device_sync_client()->GetForceSyncNowCallbackQueueSize());
   SetLocalDeviceMetadataSoftwareFeaturesMap(
       std::map<cryptauth::SoftwareFeature, cryptauth::SoftwareFeatureState>{
@@ -135,8 +151,9 @@ TEST_F(MultiDeviceSetupDeviceReenrollerTest,
            cryptauth::SoftwareFeatureState::kSupported}});
   fake_device_sync_client()->InvokePendingForceSyncNowCallback(
       true /* success */);
-  // No other attempts should be scheduled.
-  EXPECT_FALSE(timer()->IsRunning());
+  fake_device_sync_client()->NotifyNewDevicesSynced();
+
+  FireTimerAndVerifyResults();
 }
 
 TEST_F(MultiDeviceSetupDeviceReenrollerTest,
@@ -161,8 +178,10 @@ TEST_F(MultiDeviceSetupDeviceReenrollerTest,
   // Assume successful enrollment, sync, and local device metadata update.
   EXPECT_EQ(
       1, fake_device_sync_client()->GetForceEnrollmentNowCallbackQueueSize());
+  EXPECT_TRUE(timer()->IsRunning());
   fake_device_sync_client()->InvokePendingForceEnrollmentNowCallback(
       true /* success */);
+  fake_device_sync_client()->NotifyEnrollmentFinished();
   EXPECT_EQ(1, fake_device_sync_client()->GetForceSyncNowCallbackQueueSize());
   SetLocalDeviceMetadataSoftwareFeaturesMap(
       std::map<cryptauth::SoftwareFeature, cryptauth::SoftwareFeatureState>{
@@ -174,12 +193,13 @@ TEST_F(MultiDeviceSetupDeviceReenrollerTest,
            cryptauth::SoftwareFeatureState::kSupported}});
   fake_device_sync_client()->InvokePendingForceSyncNowCallback(
       true /* success */);
-  // No other attempts should be scheduled.
-  EXPECT_FALSE(timer()->IsRunning());
+  fake_device_sync_client()->NotifyNewDevicesSynced();
+
+  FireTimerAndVerifyResults();
 }
 
 TEST_F(MultiDeviceSetupDeviceReenrollerTest,
-       IfReenrollmentFailsThenScheduleAnotherAttempt) {
+       IfEnrollmentCallFailsThenAnotherAttemptShouldBeScheduled) {
   // Set the current local device metadata to contain a sample of supported
   // software features.
   SetLocalDeviceMetadataSoftwareFeaturesMap(
@@ -200,22 +220,19 @@ TEST_F(MultiDeviceSetupDeviceReenrollerTest,
 
   EXPECT_EQ(
       1, fake_device_sync_client()->GetForceEnrollmentNowCallbackQueueSize());
-  // Assume unsuccessful re-enrollment attempt.
+  // Assume unsuccessful enrollment call.
   fake_device_sync_client()->InvokePendingForceEnrollmentNowCallback(
       false /* success */);
-  // No device sync call should have been made.
-  EXPECT_EQ(0, fake_device_sync_client()->GetForceSyncNowCallbackQueueSize());
   // Another re-enrollment attempt should be scheduled.
   EXPECT_TRUE(timer()->IsRunning());
   // This should trigger another enrollment attempt.
   timer()->Fire();
   EXPECT_EQ(
       1, fake_device_sync_client()->GetForceEnrollmentNowCallbackQueueSize());
-  EXPECT_EQ(0, fake_device_sync_client()->GetForceSyncNowCallbackQueueSize());
 }
 
 TEST_F(MultiDeviceSetupDeviceReenrollerTest,
-       IfDeviceSyncFailsThenScheduleAnotherAttempt) {
+       IfDeviceSyncCallFailsThenAnotherEnrollmentAttemptShouldBeScheduled) {
   // Set the current local device metadata to contain a sample of supported
   // software features.
   SetLocalDeviceMetadataSoftwareFeaturesMap(
@@ -234,26 +251,26 @@ TEST_F(MultiDeviceSetupDeviceReenrollerTest,
 
   CreateDeviceReenroller();
 
-  // Assume successful re-enrollment attempt.
+  // Assume successful enrollment attempt.
   EXPECT_EQ(
       1, fake_device_sync_client()->GetForceEnrollmentNowCallbackQueueSize());
   fake_device_sync_client()->InvokePendingForceEnrollmentNowCallback(
       true /* success */);
+  fake_device_sync_client()->NotifyEnrollmentFinished();
   EXPECT_EQ(1, fake_device_sync_client()->GetForceSyncNowCallbackQueueSize());
-  // Assume unsuccessful device sync attempt.
+  // Assume unsuccessful device sync call.
   fake_device_sync_client()->InvokePendingForceSyncNowCallback(
       false /* success */);
-  // Another device sync attempt should be scheduled.
+  // Another re-enrollment attempt should be scheduled.
   EXPECT_TRUE(timer()->IsRunning());
-  // This should trigger another device sync attempt.
+  // This should trigger another enrollment attempt.
   timer()->Fire();
   EXPECT_EQ(
-      0, fake_device_sync_client()->GetForceEnrollmentNowCallbackQueueSize());
-  EXPECT_EQ(1, fake_device_sync_client()->GetForceSyncNowCallbackQueueSize());
+      1, fake_device_sync_client()->GetForceEnrollmentNowCallbackQueueSize());
 }
 
 TEST_F(MultiDeviceSetupDeviceReenrollerTest,
-       IfLocalDeviceMetadataNotUpdatedCorrectlyThenScheduleAnotherSyncAttempt) {
+       IfMetadataNotUpdatedCorrectlyThenAnotherEnrollAttemptShouldBeScheduled) {
   // Set the current local device metadata to contain a sample of supported
   // software features.
   SetLocalDeviceMetadataSoftwareFeaturesMap(
@@ -271,11 +288,13 @@ TEST_F(MultiDeviceSetupDeviceReenrollerTest,
           cryptauth::SoftwareFeature::MAGIC_TETHER_CLIENT});
 
   CreateDeviceReenroller();
+
   // Assume successful enrollment and device sync.
   EXPECT_EQ(
       1, fake_device_sync_client()->GetForceEnrollmentNowCallbackQueueSize());
   fake_device_sync_client()->InvokePendingForceEnrollmentNowCallback(
       true /* success */);
+  fake_device_sync_client()->NotifyEnrollmentFinished();
   EXPECT_EQ(1, fake_device_sync_client()->GetForceSyncNowCallbackQueueSize());
   // Assume local device metadata was not updated correctly.
   SetLocalDeviceMetadataSoftwareFeaturesMap(
@@ -286,13 +305,13 @@ TEST_F(MultiDeviceSetupDeviceReenrollerTest,
            cryptauth::SoftwareFeatureState::kSupported}});
   fake_device_sync_client()->InvokePendingForceSyncNowCallback(
       true /* success */);
-  // Another device sync attempt should be scheduled.
+  fake_device_sync_client()->NotifyNewDevicesSynced();
+  // Another enrollment attempt should be scheduled.
   EXPECT_TRUE(timer()->IsRunning());
-  // This should trigger another device sync attempt.
+  // This should trigger another enrollment attempt.
   timer()->Fire();
   EXPECT_EQ(
-      0, fake_device_sync_client()->GetForceEnrollmentNowCallbackQueueSize());
-  EXPECT_EQ(1, fake_device_sync_client()->GetForceSyncNowCallbackQueueSize());
+      1, fake_device_sync_client()->GetForceEnrollmentNowCallbackQueueSize());
 }
 
 TEST_F(MultiDeviceSetupDeviceReenrollerTest,
@@ -324,6 +343,7 @@ TEST_F(MultiDeviceSetupDeviceReenrollerTest,
       1, fake_device_sync_client()->GetForceEnrollmentNowCallbackQueueSize());
   fake_device_sync_client()->InvokePendingForceEnrollmentNowCallback(
       true /* success */);
+  fake_device_sync_client()->NotifyEnrollmentFinished();
   EXPECT_EQ(1, fake_device_sync_client()->GetForceSyncNowCallbackQueueSize());
   SetLocalDeviceMetadataSoftwareFeaturesMap(
       std::map<cryptauth::SoftwareFeature, cryptauth::SoftwareFeatureState>{
@@ -337,8 +357,9 @@ TEST_F(MultiDeviceSetupDeviceReenrollerTest,
            cryptauth::SoftwareFeatureState::kSupported}});
   fake_device_sync_client()->InvokePendingForceSyncNowCallback(
       true /* success */);
-  // No other attempts should be scheduled.
-  EXPECT_FALSE(timer()->IsRunning());
+  fake_device_sync_client()->NotifyNewDevicesSynced();
+
+  FireTimerAndVerifyResults();
 }
 
 TEST_F(
@@ -372,6 +393,99 @@ TEST_F(
   EXPECT_FALSE(timer()->IsRunning());
 }
 
+TEST_F(MultiDeviceSetupDeviceReenrollerTest,
+       IfOnEnrollmentFinishedCalledWithAgreementThenNoReenrollment) {
+  // Set the current local device metadata to contain a sample of supported
+  // software features.
+  SetLocalDeviceMetadataSoftwareFeaturesMap(
+      std::map<cryptauth::SoftwareFeature, cryptauth::SoftwareFeatureState>{
+          {cryptauth::SoftwareFeature::BETTER_TOGETHER_CLIENT,
+           cryptauth::SoftwareFeatureState::kSupported},
+          {cryptauth::SoftwareFeature::EASY_UNLOCK_CLIENT,
+           cryptauth::SoftwareFeatureState::kSupported}});
+  // Set the current GcmDeviceInfo supported software features to contain the
+  // same set.
+  SetFakeGcmDeviceInfoProviderWithSupportedSoftwareFeatures(
+      std::vector<cryptauth::SoftwareFeature>{
+          cryptauth::SoftwareFeature::BETTER_TOGETHER_CLIENT,
+          cryptauth::SoftwareFeature::EASY_UNLOCK_CLIENT});
+
+  CreateDeviceReenroller();
+
+  fake_device_sync_client()->NotifyEnrollmentFinished();
+
+  // No enrollment or device sync attempts should have taken place nor should
+  // any be scheduled.
+  EXPECT_EQ(
+      0, fake_device_sync_client()->GetForceEnrollmentNowCallbackQueueSize());
+  EXPECT_EQ(0, fake_device_sync_client()->GetForceSyncNowCallbackQueueSize());
+  EXPECT_FALSE(timer()->IsRunning());
+}
+
+TEST_F(MultiDeviceSetupDeviceReenrollerTest,
+       IfOnNewDevicesSyncedCalledWithAgreementThenNoReenrollment) {
+  // Set the current local device metadata to contain a sample of supported
+  // software features.
+  SetLocalDeviceMetadataSoftwareFeaturesMap(
+      std::map<cryptauth::SoftwareFeature, cryptauth::SoftwareFeatureState>{
+          {cryptauth::SoftwareFeature::BETTER_TOGETHER_CLIENT,
+           cryptauth::SoftwareFeatureState::kSupported},
+          {cryptauth::SoftwareFeature::EASY_UNLOCK_CLIENT,
+           cryptauth::SoftwareFeatureState::kSupported}});
+  // Set the current GcmDeviceInfo supported software features to contain the
+  // same set.
+  SetFakeGcmDeviceInfoProviderWithSupportedSoftwareFeatures(
+      std::vector<cryptauth::SoftwareFeature>{
+          cryptauth::SoftwareFeature::BETTER_TOGETHER_CLIENT,
+          cryptauth::SoftwareFeature::EASY_UNLOCK_CLIENT});
+
+  CreateDeviceReenroller();
+
+  fake_device_sync_client()->NotifyNewDevicesSynced();
+
+  // No enrollment or device sync attempts should have taken place nor should
+  // any be scheduled.
+  EXPECT_EQ(
+      0, fake_device_sync_client()->GetForceEnrollmentNowCallbackQueueSize());
+  EXPECT_EQ(0, fake_device_sync_client()->GetForceSyncNowCallbackQueueSize());
+  EXPECT_FALSE(timer()->IsRunning());
+}
+
+TEST_F(MultiDeviceSetupDeviceReenrollerTest,
+       IfOnNewDevicesSyncedCalledWithDisagreementThenStartReenrollment) {
+  // Set the current local device metadata to contain a sample of supported
+  // software features.
+  SetLocalDeviceMetadataSoftwareFeaturesMap(
+      std::map<cryptauth::SoftwareFeature, cryptauth::SoftwareFeatureState>{
+          {cryptauth::SoftwareFeature::BETTER_TOGETHER_CLIENT,
+           cryptauth::SoftwareFeatureState::kSupported},
+          {cryptauth::SoftwareFeature::EASY_UNLOCK_CLIENT,
+           cryptauth::SoftwareFeatureState::kSupported}});
+  // Set the current GcmDeviceInfo supported software features to contain the
+  // same set.
+  SetFakeGcmDeviceInfoProviderWithSupportedSoftwareFeatures(
+      std::vector<cryptauth::SoftwareFeature>{
+          cryptauth::SoftwareFeature::BETTER_TOGETHER_CLIENT,
+          cryptauth::SoftwareFeature::EASY_UNLOCK_CLIENT});
+
+  CreateDeviceReenroller();
+
+  EXPECT_FALSE(timer()->IsRunning());
+
+  // Remove a feature from the metadata.
+  SetLocalDeviceMetadataSoftwareFeaturesMap(
+      std::map<cryptauth::SoftwareFeature, cryptauth::SoftwareFeatureState>{
+          {cryptauth::SoftwareFeature::EASY_UNLOCK_CLIENT,
+           cryptauth::SoftwareFeatureState::kSupported}});
+
+  fake_device_sync_client()->NotifyNewDevicesSynced();
+
+  // Start re-enrollment process.
+  EXPECT_EQ(
+      1, fake_device_sync_client()->GetForceEnrollmentNowCallbackQueueSize());
+  EXPECT_TRUE(timer()->IsRunning());
+}
+
 }  // namespace multidevice_setup
 
 }  // namespace chromeos