ServiceConnection: Make urandom's parameter optional

With the initiative to create more sensible defaults for
cros_healthd's diagnostic routines, the urandom routine shouldn't
need to have its length_seconds parameter specified. If the
parameter is not specified, the routine will use a default value.

Bug: chromium:1131609
Change-Id: I9fbcedf35ea4d2cfbf44df79afb96510206c46f9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2533677
Commit-Queue: Paul Moy <pmoy@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827294}

chrome/browser/chromeos/policy/remote_commands/device_command_run_routine_job.cc

@@ -156,18 +156,23 @@ void DeviceCommandRunRoutineJob::RunImpl(CallbackWithResult succeeded_callback,
       constexpr char kLengthSecondsFieldName[] = "lengthSeconds";
       base::Optional<int> length_seconds =
           params_dict_.FindIntKey(kLengthSecondsFieldName);
-      // The urandom routine expects one integer >= 0.
-      if (!length_seconds.has_value() || length_seconds.value() < 0) {
-        SYSLOG(ERROR) << "Invalid parameters for Urandom routine.";
-        base::ThreadTaskRunnerHandle::Get()->PostTask(
-            FROM_HERE, base::BindOnce(std::move(failed_callback),
-                                      std::make_unique<Payload>(
-                                          MakeInvalidParametersResponse())));
-        break;
+      base::Optional<base::TimeDelta> routine_parameter;
+      if (length_seconds.has_value()) {
+        // If the optional integer parameter is specified, it must be >= 0.
+        int value = length_seconds.value();
+        if (value < 0) {
+          SYSLOG(ERROR) << "Invalid parameters for Urandom routine.";
+          base::ThreadTaskRunnerHandle::Get()->PostTask(
+              FROM_HERE, base::BindOnce(std::move(failed_callback),
+                                        std::make_unique<Payload>(
+                                            MakeInvalidParametersResponse())));
+          break;
+        }
+        routine_parameter = base::TimeDelta::FromSeconds(value);
       }
       chromeos::cros_healthd::ServiceConnection::GetInstance()
           ->RunUrandomRoutine(
-              length_seconds.value(),
+              routine_parameter,
               base::BindOnce(
                   &DeviceCommandRunRoutineJob::OnCrosHealthdResponseReceived,
                   weak_ptr_factory_.GetWeakPtr(), std::move(succeeded_callback),

chrome/browser/chromeos/policy/remote_commands/device_command_run_routine_job_unittest.cc

@@ -324,18 +324,22 @@ TEST_F(DeviceCommandRunRoutineJobTest, RunUrandomRoutineSuccess) {
              })));
 }
 
-// Test that leaving out the lengthSeconds parameter causes the urandom routine
-// to fail.
+// Test that the urandom routine handles the optional length_seconds parameter
+// being missing.
 TEST_F(DeviceCommandRunRoutineJobTest, RunUrandomRoutineMissingLengthSeconds) {
+  auto run_routine_response =
+      chromeos::cros_healthd::mojom::RunRoutineResponse::New(kId, kStatus);
+  chromeos::cros_healthd::FakeCrosHealthdClient::Get()
+      ->SetRunRoutineResponseForTesting(run_routine_response);
   base::Value params_dict(base::Value::Type::DICTIONARY);
   EXPECT_TRUE(
       RunJob(chromeos::cros_healthd::mojom::DiagnosticRoutineEnum::kUrandom,
              std::move(params_dict),
              base::BindLambdaForTesting([](RemoteCommandJob* job) {
-               EXPECT_EQ(job->status(), RemoteCommandJob::FAILED);
+               EXPECT_EQ(job->status(), RemoteCommandJob::SUCCEEDED);
                std::unique_ptr<std::string> payload = job->GetResultPayload();
                EXPECT_TRUE(payload);
-               EXPECT_EQ(CreateInvalidParametersFailurePayload(), *payload);
+               EXPECT_EQ(CreateSuccessPayload(kId, kStatus), *payload);
              })));
 }
 

chromeos/dbus/cros_healthd/fake_cros_healthd_service.cc

@@ -67,7 +67,7 @@ void FakeCrosHealthdService::GetRoutineUpdate(
 }
 
 void FakeCrosHealthdService::RunUrandomRoutine(
-    uint32_t length_seconds,
+    mojom::NullableUint32Ptr length_seconds,
     RunUrandomRoutineCallback callback) {
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,

chromeos/dbus/cros_healthd/fake_cros_healthd_service.h

@@ -55,7 +55,7 @@ class FakeCrosHealthdService final
                         mojom::DiagnosticRoutineCommandEnum command,
                         bool include_output,
                         GetRoutineUpdateCallback callback) override;
-  void RunUrandomRoutine(uint32_t length_seconds,
+  void RunUrandomRoutine(mojom::NullableUint32Ptr length_seconds,
                          RunUrandomRoutineCallback callback) override;
   void RunBatteryCapacityRoutine(
       RunBatteryCapacityRoutineCallback callback) override;

chromeos/services/cros_healthd/public/cpp/service_connection.cc

@@ -39,7 +39,7 @@ class ServiceConnectionImpl : public ServiceConnection {
       mojom::CrosHealthdDiagnosticsService::GetRoutineUpdateCallback callback)
       override;
   void RunUrandomRoutine(
-      uint32_t length_seconds,
+      const base::Optional<base::TimeDelta>& length_seconds,
       mojom::CrosHealthdDiagnosticsService::RunUrandomRoutineCallback callback)
       override;
   void RunBatteryCapacityRoutine(
@@ -225,12 +225,17 @@ void ServiceConnectionImpl::GetRoutineUpdate(
 }
 
 void ServiceConnectionImpl::RunUrandomRoutine(
-    uint32_t length_seconds,
+    const base::Optional<base::TimeDelta>& length_seconds,
     mojom::CrosHealthdDiagnosticsService::RunUrandomRoutineCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   BindCrosHealthdDiagnosticsServiceIfNeeded();
-  cros_healthd_diagnostics_service_->RunUrandomRoutine(length_seconds,
-                                                       std::move(callback));
+  chromeos::cros_healthd::mojom::NullableUint32Ptr routine_parameter;
+  if (length_seconds.has_value()) {
+    routine_parameter = chromeos::cros_healthd::mojom::NullableUint32::New(
+        length_seconds.value().InSeconds());
+  }
+  cros_healthd_diagnostics_service_->RunUrandomRoutine(
+      std::move(routine_parameter), std::move(callback));
 }
 
 void ServiceConnectionImpl::RunBatteryCapacityRoutine(

chromeos/services/cros_healthd/public/cpp/service_connection.h

@@ -12,6 +12,7 @@
 
 #include "base/callback_forward.h"
 #include "base/optional.h"
+#include "base/time/time.h"
 #include "chromeos/services/cros_healthd/public/mojom/cros_healthd.mojom.h"
 #include "chromeos/services/cros_healthd/public/mojom/cros_healthd_events.mojom.h"
 #include "chromeos/services/network_health/public/mojom/network_diagnostics.mojom.h"
@@ -57,7 +58,7 @@ class ServiceConnection {
   // src/chromeos/service/cros_healthd/public/mojom/cros_healthd.mojom for
   // details.
   virtual void RunUrandomRoutine(
-      uint32_t length_seconds,
+      const base::Optional<base::TimeDelta>& length_seconds,
       mojom::CrosHealthdDiagnosticsService::RunUrandomRoutineCallback
           callback) = 0;
 

chromeos/services/cros_healthd/public/cpp/service_connection_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/run_loop.h"
 #include "base/test/bind.h"
 #include "base/test/task_environment.h"
+#include "base/time/time.h"
 #include "chromeos/dbus/cros_healthd/cros_healthd_client.h"
 #include "chromeos/dbus/cros_healthd/fake_cros_healthd_client.h"
 #include "chromeos/services/cros_healthd/public/mojom/cros_healthd.mojom.h"
@@ -306,7 +307,7 @@ TEST_F(CrosHealthdServiceConnectionTest, RunUrandomRoutine) {
   FakeCrosHealthdClient::Get()->SetRunRoutineResponseForTesting(response);
   bool callback_done = false;
   ServiceConnection::GetInstance()->RunUrandomRoutine(
-      /*length_seconds=*/10,
+      /*length_seconds=*/base::nullopt,
       base::BindOnce(
           [](bool* callback_done, mojom::RunRoutineResponsePtr response) {
             EXPECT_EQ(response, MakeRunRoutineResponse());
@@ -322,13 +323,12 @@ TEST_F(CrosHealthdServiceConnectionTest, RunBatteryCapacityRoutine) {
   auto response = MakeRunRoutineResponse();
   FakeCrosHealthdClient::Get()->SetRunRoutineResponseForTesting(response);
   bool callback_done = false;
-  ServiceConnection::GetInstance()->RunBatteryCapacityRoutine(
-      base::BindOnce(
-          [](bool* callback_done, mojom::RunRoutineResponsePtr response) {
-            EXPECT_EQ(response, MakeRunRoutineResponse());
-            *callback_done = true;
-          },
-          &callback_done));
+  ServiceConnection::GetInstance()->RunBatteryCapacityRoutine(base::BindOnce(
+      [](bool* callback_done, mojom::RunRoutineResponsePtr response) {
+        EXPECT_EQ(response, MakeRunRoutineResponse());
+        *callback_done = true;
+      },
+      &callback_done));
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(callback_done);
 }
@@ -338,13 +338,12 @@ TEST_F(CrosHealthdServiceConnectionTest, RunBatteryHealthRoutine) {
   auto response = MakeRunRoutineResponse();
   FakeCrosHealthdClient::Get()->SetRunRoutineResponseForTesting(response);
   bool callback_done = false;
-  ServiceConnection::GetInstance()->RunBatteryHealthRoutine(
-      base::BindOnce(
-          [](bool* callback_done, mojom::RunRoutineResponsePtr response) {
-            EXPECT_EQ(response, MakeRunRoutineResponse());
-            *callback_done = true;
-          },
-          &callback_done));
+  ServiceConnection::GetInstance()->RunBatteryHealthRoutine(base::BindOnce(
+      [](bool* callback_done, mojom::RunRoutineResponsePtr response) {
+        EXPECT_EQ(response, MakeRunRoutineResponse());
+        *callback_done = true;
+      },
+      &callback_done));
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(callback_done);
 }

chromeos/services/cros_healthd/public/mojom/cros_healthd.mojom

@@ -13,6 +13,7 @@ module chromeos.cros_healthd.mojom;
 import "chromeos/services/cros_healthd/public/mojom/cros_healthd_diagnostics.mojom";
 import "chromeos/services/cros_healthd/public/mojom/cros_healthd_events.mojom";
 import "chromeos/services/cros_healthd/public/mojom/cros_healthd_probe.mojom";
+import "chromeos/services/cros_healthd/public/mojom/nullable_primitives.mojom";
 import "chromeos/services/network_health/public/mojom/network_diagnostics.mojom";
 import "chromeos/services/network_health/public/mojom/network_health.mojom";
 
@@ -72,12 +73,13 @@ interface CrosHealthdDiagnosticsService {
   // * |length_seconds| - length of time, in seconds, to run the urandom routine
   //                      for. If all reads from /dev/urandom for this length of
   //                      time are successful, then the test  will pass. This
-  //                      parameter needs to be strictly greater than zero.
+  //                      parameter needs to be strictly greater than zero. If
+  //                      not specified, the routine will default to 10 seconds.
   //
   // The response:
   // * |response| - contains a unique identifier and status for the created
   //                routine.
-  RunUrandomRoutine(uint32 length_seconds)
+  RunUrandomRoutine(NullableUint32? length_seconds)
       => (RunRoutineResponse response);
 
   // Requests that the BatteryCapacity routine is created and started on the