Do not allow simultaneous computations and uploads of the EID

In some situations, Start() would be called from both the constructor
and the observer notification method quickly, yielding to multiple
concurrent flows. Prevent those.

In addition, redo the tests so that they test both work from the
observer notification method and from an existing policy. Also add
a test that triggers the concurrency situation and verifies that
there is only one flow running in that situation.

BUG=chromium:887683
TEST=unit_tests --gtest_filter=*EnrollmentPolicyObserverTest*
CQ-DEPEND=CL:1234373

Change-Id: Ice6b46e19bf77a837b4c4908f332e9af6c5189f5
Reviewed-on: https://chromium-review.googlesource.com/1237242
Commit-Queue: Yves Arrouye <drcrash@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594751}

chrome/browser/chromeos/attestation/enrollment_policy_observer.cc

@@ -10,6 +10,8 @@
 #include "base/callback.h"
 #include "base/location.h"
 #include "base/optional.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
 #include "base/task/post_task.h"
 #include "base/time/time.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ -109,6 +111,7 @@ void EnrollmentPolicyObserver::Start() {
   // contain an EID).
   if (did_upload_empty_eid_)
     return;
+
   // If identification for enrollment isn't needed, there is nothing to do.
   const enterprise_management::PolicyData* policy_data =
       device_settings_service_->policy_data();
@@ -121,6 +124,11 @@ void EnrollmentPolicyObserver::Start() {
     return;
   }
 
+  // Do not allow multiple concurrent starts.
+  if (request_in_flight_)
+    return;
+  request_in_flight_ = true;
+
   if (!cryptohome_client_)
     cryptohome_client_ = DBusThreadManager::Get()->GetCryptohomeClient();
 
@@ -160,20 +168,26 @@ void EnrollmentPolicyObserver::RescheduleGetEnrollmentId() {
         base::TimeDelta::FromSeconds(retry_delay_));
   } else {
     LOG(WARNING) << "EnrollmentPolicyObserver: Retry limit exceeded.";
+    request_in_flight_ = false;
   }
 }
 
 void EnrollmentPolicyObserver::OnUploadComplete(
     const std::string& enrollment_id,
     bool status) {
+  const std::string& printable_enrollment_id = base::ToLowerASCII(
+      base::HexEncode(enrollment_id.data(), enrollment_id.size()));
+  request_in_flight_ = false;
   if (status) {
     if (enrollment_id.empty())
       did_upload_empty_eid_ = true;
   } else {
-    LOG(ERROR) << "Failed to upload Enrollment Identifier to DMServer.";
+    LOG(ERROR) << "Failed to upload Enrollment Identifier \""
+               << printable_enrollment_id << "\" to DMServer.";
     return;
   }
-  VLOG(1) << "Enrollment Identifier uploaded to DMServer.";
+  VLOG(1) << "Enrollment Identifier \"" << printable_enrollment_id
+          << "\" uploaded to DMServer.";
 }
 
 }  // namespace attestation

chrome/browser/chromeos/attestation/enrollment_policy_observer.h

@@ -73,6 +73,8 @@ class EnrollmentPolicyObserver : public DeviceSettingsService::Observer {
   int num_retries_;
   int retry_limit_;
   int retry_delay_;
+  // Whether we are requesting an EID right now.
+  bool request_in_flight_ = false;
   // Used to remember we uploaded an empty identifier this session for
   // devices that can't obtain the identifier until they are powerwashed or
   // updated and rebooted (see http://crbug.com/867724).

chrome/browser/chromeos/attestation/enrollment_policy_observer_unittest.cc

@@ -4,7 +4,10 @@
 
 #include <stdint.h>
 
+#include <memory>
+#include <queue>
 #include <string>
+#include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -37,6 +40,42 @@ void StatusCallbackSuccess(
                                                 base::BindOnce(callback, true));
 }
 
+// A FakeCryptohomeClient that can hold call until told to flush them all.
+class CallsHoldingFakeCryptohomeClient : public FakeCryptohomeClient {
+ public:
+  void set_hold_calls(bool hold_calls) { hold_calls_ = hold_calls; }
+
+  void TpmAttestationGetEnrollmentId(
+      bool ignore_cache,
+      DBusMethodCallback<TpmAttestationDataResult> callback) override {
+    if (hold_calls_) {
+      held_calls_.push(base::BindOnce(
+          &CallsHoldingFakeCryptohomeClient::DoTpmAttestationGetEnrollmentId,
+          base::Unretained(this), ignore_cache, std::move(callback)));
+    } else {
+      DoTpmAttestationGetEnrollmentId(ignore_cache, std::move(callback));
+    }
+  }
+
+  void FlushCalls() {
+    while (!held_calls_.empty()) {
+      std::move(held_calls_.front()).Run();
+      held_calls_.pop();
+    }
+  }
+
+ private:
+  void DoTpmAttestationGetEnrollmentId(
+      bool ignore_cache,
+      DBusMethodCallback<TpmAttestationDataResult> callback) {
+    FakeCryptohomeClient::TpmAttestationGetEnrollmentId(ignore_cache,
+                                                        std::move(callback));
+  }
+
+  bool hold_calls_ = false;
+  std::queue<base::OnceClosure> held_calls_;
+};
+
 }  // namespace
 
 class EnrollmentPolicyObserverTest : public DeviceSettingsTestBase {
@@ -56,68 +95,110 @@ class EnrollmentPolicyObserverTest : public DeviceSettingsTestBase {
   static constexpr char kEnrollmentId[] =
       "6fcc0ebddec3db9500cf82476d594f4d60db934c5b47fa6085c707b2a93e205b";
 
-  void SetUpEnrollmentIdNeeded(bool enrollment_id_needed) {
-    if (enrollment_id_needed) {
-      EXPECT_CALL(policy_client_,
-                  UploadEnterpriseEnrollmentId(enrollment_id_, _))
-          .WillOnce(WithArgs<1>(Invoke(StatusCallbackSuccess)));
-    }
-    SetUpDevicePolicy(enrollment_id_needed);
+  void SetUpObserver() {
+    observer_ = std::make_unique<EnrollmentPolicyObserver>(
+        &policy_client_, &device_settings_service_, &cryptohome_client_);
+    observer_->set_retry_limit(3);
+    observer_->set_retry_delay(0);
+  }
+
+  void ExpectUploadEnterpriseEnrollmentId(int times) {
+    EXPECT_CALL(policy_client_, UploadEnterpriseEnrollmentId(enrollment_id_, _))
+        .Times(times)
+        .WillRepeatedly(WithArgs<1>(Invoke(StatusCallbackSuccess)));
   }
 
   void SetUpDevicePolicy(bool enrollment_id_needed) {
     device_policy_.policy_data().set_enrollment_id_needed(enrollment_id_needed);
+  }
+
+  void PropagateDevicePolicy() {
     ReloadDevicePolicy();
     ReloadDeviceSettings();
   }
 
   void Run() {
-    EnrollmentPolicyObserver observer(
-        &policy_client_, &device_settings_service_, &cryptohome_client_);
-    observer.set_retry_limit(3);
-    observer.set_retry_delay(0);
     base::RunLoop().RunUntilIdle();
   }
 
-  FakeCryptohomeClient cryptohome_client_;
+  CallsHoldingFakeCryptohomeClient cryptohome_client_;
   StrictMock<policy::MockCloudPolicyClient> policy_client_;
+  std::unique_ptr<EnrollmentPolicyObserver> observer_;
   std::string enrollment_id_;
 };
 
 constexpr char EnrollmentPolicyObserverTest::kEnrollmentId[];
 
 TEST_F(EnrollmentPolicyObserverTest, UploadEnterpriseEnrollmentId) {
-  SetUpEnrollmentIdNeeded(true);
+  SetUpDevicePolicy(true);
+  ExpectUploadEnterpriseEnrollmentId(1);
+  SetUpObserver();
+  PropagateDevicePolicy();
+  Run();
+}
+
+TEST_F(EnrollmentPolicyObserverTest,
+       UploadEnterpriseEnrollmentIdFromExistingPolicy) {
+  // This test will trigger the observer work twice in a row: when the
+  // observer is created, and when it gets notified later on.
+  SetUpDevicePolicy(true);
+  PropagateDevicePolicy();
+  ExpectUploadEnterpriseEnrollmentId(2);
+  SetUpObserver();
+  PropagateDevicePolicy();
+  Run();
+}
+
+TEST_F(EnrollmentPolicyObserverTest,
+       UploadEnterpriseEnrollmentIdWithDelayedCallbacks) {
+  // We hold calls to cryptohome so that one is still pending by the time the
+  // observer gets notified. We expect only one upload despite the concurrent
+  // calls.
+  cryptohome_client_.set_hold_calls(true);
+  SetUpDevicePolicy(true);
+  ExpectUploadEnterpriseEnrollmentId(1);
+  PropagateDevicePolicy();
+  SetUpObserver();
+  cryptohome_client_.FlushCalls();
   Run();
 }
 
 TEST_F(EnrollmentPolicyObserverTest, FeatureDisabled) {
-  SetUpEnrollmentIdNeeded(false);
+  SetUpDevicePolicy(false);
+  SetUpObserver();
+  PropagateDevicePolicy();
   Run();
 }
 
 TEST_F(EnrollmentPolicyObserverTest, UnregisteredPolicyClient) {
   policy_client_.SetDMToken("");
+  SetUpDevicePolicy(true);
+  SetUpObserver();
+  PropagateDevicePolicy();
   Run();
 }
 
 TEST_F(EnrollmentPolicyObserverTest, DBusFailureRetry) {
-  SetUpEnrollmentIdNeeded(true);
-
   // Simulate a DBus failure.
   cryptohome_client_.SetServiceIsAvailable(false);
 
+  ExpectUploadEnterpriseEnrollmentId(1);
+
+  SetUpDevicePolicy(true);
+  PropagateDevicePolicy();
+  SetUpObserver();
+
   // Emulate delayed service initialization.
-  // Run() instantiates an Observer, which synchronously calls
-  // TpmAttestationDoesKeyExist() and fails. During this call, we make the
-  // service available in the next run, so on retry, it will successfully
-  // return the result.
+  // The observer we create synchronously calls TpmAttestationGetEnrollmentId()
+  // and fails. During this call, we make the service available in the next
+  // run, so on retry, it will successfully return the result.
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(
                      [](FakeCryptohomeClient* cryptohome_client) {
                        cryptohome_client->SetServiceIsAvailable(true);
                      },
                      base::Unretained(&cryptohome_client_)));
+
   Run();
 }
 

chromeos/dbus/fake_cryptohome_client.cc

@@ -317,13 +317,14 @@ void FakeCryptohomeClient::TpmAttestationIsPrepared(
 void FakeCryptohomeClient::TpmAttestationGetEnrollmentId(
     bool ignore_cache,
     DBusMethodCallback<TpmAttestationDataResult> callback) {
+  auto result =
+      service_is_available_
+          ? base::make_optional(TpmAttestationDataResult{
+                true, ignore_cache ? tpm_attestation_enrollment_id_ignore_cache_
+                                   : tpm_attestation_enrollment_id_})
+          : base::nullopt;
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(
-          std::move(callback),
-          TpmAttestationDataResult{
-              true, ignore_cache ? tpm_attestation_enrollment_id_ignore_cache_
-                                 : tpm_attestation_enrollment_id_}));
+      FROM_HERE, base::BindOnce(std::move(callback), result));
 }
 
 void FakeCryptohomeClient::TpmAttestationIsEnrolled(