CrOS: Add success/failure indicators for biod methods with no callback.

A couple methods in biod_client.h do not have any indicators of success or failure. Modify these functions to uses VoidDBusMethodCallbaks so users have some indication of D-Bus failures.

TEST=chromeos_unittests --gtest_filter="BiodClientTest.*"
BUG=700933

Review-Url: https://codereview.chromium.org/2799043007
Cr-Commit-Position: refs/heads/master@{#463385}

chromeos/dbus/biod/biod_client.cc

@@ -16,6 +16,16 @@
 
 namespace chromeos {
 
+namespace {
+
+// D-Bus response handler for methods that use void callbacks.
+void OnVoidResponse(const VoidDBusMethodCallback& callback,
+                    dbus::Response* response) {
+  callback.Run(response ? DBUS_METHOD_CALL_SUCCESS : DBUS_METHOD_CALL_FAILURE);
+}
+
+}  // namespace
+
 // The BiodClient implementation used in production.
 class BiodClientImpl : public BiodClient {
  public:
@@ -66,14 +76,14 @@ class BiodClientImpl : public BiodClient {
                    weak_ptr_factory_.GetWeakPtr(), callback));
   }
 
-  void DestroyAllRecords() override {
+  void DestroyAllRecords(const VoidDBusMethodCallback& callback) override {
     dbus::MethodCall method_call(
         biod::kBiometricsManagerInterface,
         biod::kBiometricsManagerDestroyAllRecordsMethod);
 
     biod_proxy_->CallMethod(&method_call,
                             dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-                            dbus::ObjectProxy::EmptyResponseCallback());
+                            base::Bind(&OnVoidResponse, callback));
   }
 
   void StartAuthSession(const ObjectPathCallback& callback) override {
@@ -100,19 +110,20 @@ class BiodClientImpl : public BiodClient {
                    weak_ptr_factory_.GetWeakPtr(), callback));
   }
 
-  void CancelEnrollSession(
-      const dbus::ObjectPath& enroll_session_path) override {
+  void CancelEnrollSession(const dbus::ObjectPath& enroll_session_path,
+                           const VoidDBusMethodCallback& callback) override {
     dbus::MethodCall method_call(biod::kEnrollSessionInterface,
                                  biod::kEnrollSessionCancelMethod);
 
     dbus::ObjectProxy* enroll_session_proxy =
         bus_->GetObjectProxy(biod::kBiodServiceName, enroll_session_path);
-    enroll_session_proxy->CallMethod(
-        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        dbus::ObjectProxy::EmptyResponseCallback());
+    enroll_session_proxy->CallMethod(&method_call,
+                                     dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+                                     base::Bind(&OnVoidResponse, callback));
   }
 
-  void EndAuthSession(const dbus::ObjectPath& auth_session_path) override {
+  void EndAuthSession(const dbus::ObjectPath& auth_session_path,
+                      const VoidDBusMethodCallback& callback) override {
     dbus::MethodCall method_call(biod::kAuthSessionInterface,
                                  biod::kAuthSessionEndMethod);
 
@@ -120,11 +131,12 @@ class BiodClientImpl : public BiodClient {
         bus_->GetObjectProxy(biod::kBiodServiceName, auth_session_path);
     auth_session_proxy->CallMethod(&method_call,
                                    dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-                                   dbus::ObjectProxy::EmptyResponseCallback());
+                                   base::Bind(&OnVoidResponse, callback));
   }
 
   void SetRecordLabel(const dbus::ObjectPath& record_path,
-                      const std::string& label) override {
+                      const std::string& label,
+                      const VoidDBusMethodCallback& callback) override {
     dbus::MethodCall method_call(biod::kRecordInterface,
                                  biod::kRecordSetLabelMethod);
     dbus::MessageWriter writer(&method_call);
@@ -134,10 +146,11 @@ class BiodClientImpl : public BiodClient {
         bus_->GetObjectProxy(biod::kBiodServiceName, record_path);
     record_proxy->CallMethod(&method_call,
                              dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-                             dbus::ObjectProxy::EmptyResponseCallback());
+                             base::Bind(&OnVoidResponse, callback));
   }
 
-  void RemoveRecord(const dbus::ObjectPath& record_path) override {
+  void RemoveRecord(const dbus::ObjectPath& record_path,
+                    const VoidDBusMethodCallback& callback) override {
     dbus::MethodCall method_call(biod::kRecordInterface,
                                  biod::kRecordRemoveMethod);
 
@@ -145,7 +158,7 @@ class BiodClientImpl : public BiodClient {
         bus_->GetObjectProxy(biod::kBiodServiceName, record_path);
     record_proxy->CallMethod(&method_call,
                              dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-                             dbus::ObjectProxy::EmptyResponseCallback());
+                             base::Bind(&OnVoidResponse, callback));
   }
 
   void RequestRecordLabel(const dbus::ObjectPath& record_path,

chromeos/dbus/biod/biod_client.h

@@ -95,8 +95,9 @@ class CHROMEOS_EXPORT BiodClient : public DBusClient {
   virtual void GetRecordsForUser(const std::string& user_id,
                                  const UserRecordsCallback& callback) = 0;
 
-  // Irreversibly destroys all records registered.
-  virtual void DestroyAllRecords() = 0;
+  // Irreversibly destroys all records registered. |callback| is called
+  // asynchronously with the result.
+  virtual void DestroyAllRecords(const VoidDBusMethodCallback& callback) = 0;
 
   // Starts the biometric auth session. |callback| is called with the object
   // path of the auth session after the method succeeds.
@@ -106,20 +107,27 @@ class CHROMEOS_EXPORT BiodClient : public DBusClient {
   // type after the method succeeds.
   virtual void RequestType(const BiometricTypeCallback& callback) = 0;
 
-  // Cancels the enroll session at |enroll_session_path|.
-  virtual void CancelEnrollSession(
-      const dbus::ObjectPath& enroll_session_path) = 0;
+  // Cancels the enroll session at |enroll_session_path|. |callback| is called
+  // asynchronously with the result.
+  virtual void CancelEnrollSession(const dbus::ObjectPath& enroll_session_path,
+                                   const VoidDBusMethodCallback& callback) = 0;
 
-  // Ends the auth session at |auth_session_path|.
-  virtual void EndAuthSession(const dbus::ObjectPath& auth_session_path) = 0;
+  // Ends the auth session at |auth_session_path|. |callback| is called
+  // asynchronously with the result.
+  virtual void EndAuthSession(const dbus::ObjectPath& auth_session_path,
+                              const VoidDBusMethodCallback& callback) = 0;
 
-  // Changes the label of the record at |record_path| to |label|.
+  // Changes the label of the record at |record_path| to |label|. |callback| is
+  // called asynchronously with the result.
   virtual void SetRecordLabel(const dbus::ObjectPath& record_path,
-                              const std::string& label) = 0;
+                              const std::string& label,
+                              const VoidDBusMethodCallback& callback) = 0;
 
   // Removes the record at |record_path|. This record will no longer be able to
-  // used for authentication.
-  virtual void RemoveRecord(const dbus::ObjectPath& record_path) = 0;
+  // used for authentication. |callback| is called asynchronously with the
+  // result.
+  virtual void RemoveRecord(const dbus::ObjectPath& record_path,
+                            const VoidDBusMethodCallback& callback) = 0;
 
   // Requests the label of the record at |record_path|. |callback| is called
   // with the label of the record.

chromeos/dbus/biod/biod_client_unittest.cc

@@ -52,6 +52,18 @@ void CopyString(std::string* dest_str, const std::string& src_str) {
   *dest_str = src_str;
 }
 
+void CopyDBusMethodCallStatus(DBusMethodCallStatus* dest_status,
+                              DBusMethodCallStatus src_status) {
+  CHECK(dest_status);
+  *dest_status = src_status;
+}
+
+void CopyBiometricType(biod::BiometricType* dest_type,
+                       biod::BiometricType src_type) {
+  CHECK(dest_type);
+  *dest_type = src_type;
+}
+
 // Matcher that verifies that a dbus::Message has member |name|.
 MATCHER_P(HasMember, name, "") {
   if (arg->GetMember() != name) {
@@ -135,7 +147,7 @@ class BiodClientTest : public testing::Test {
 
  protected:
   // Add an expectation for method with |method_name| to be called. When the
-  // method is called the response shoudl match |response|.
+  // method is called the response should match |response|.
   void AddMethodExpectation(const std::string& method_name,
                             std::unique_ptr<dbus::Response> response) {
     ASSERT_FALSE(pending_method_calls_.count(method_name));
@@ -258,7 +270,7 @@ TEST_F(BiodClientTest, TestStartEnrollSession) {
   EXPECT_EQ(kFakeObjectPath, returned_path);
 
   // Verify that by sending a empty reponse or a improperly formatted one, the
-  // response is an empty object path. Also, logs will get printed.
+  // response is an empty object path.
   AddMethodExpectation(biod::kBiometricsManagerStartEnrollSessionMethod,
                        nullptr);
   returned_path = dbus::ObjectPath(kInvalidTestPath);
@@ -302,7 +314,7 @@ TEST_F(BiodClientTest, TestGetRecordsForUser) {
   EXPECT_EQ(kFakeObjectPaths, returned_object_paths);
 
   // Verify that by sending a empty reponse the response is an empty array of
-  // object paths. Also, logs will get printed.
+  // object paths.
   AddMethodExpectation(biod::kBiometricsManagerGetRecordsForUserMethod,
                        nullptr);
   returned_object_paths = {dbus::ObjectPath(kInvalidTestPath)};
@@ -312,6 +324,29 @@ TEST_F(BiodClientTest, TestGetRecordsForUser) {
   EXPECT_EQ(std::vector<dbus::ObjectPath>(), returned_object_paths);
 }
 
+TEST_F(BiodClientTest, TestDestroyAllRecords) {
+  std::unique_ptr<dbus::Response> response(dbus::Response::CreateEmpty());
+  dbus::MessageWriter writer(response.get());
+
+  // Create an empty response to simulate success.
+  AddMethodExpectation(biod::kBiometricsManagerDestroyAllRecordsMethod,
+                       std::move(response));
+  DBusMethodCallStatus returned_status = static_cast<DBusMethodCallStatus>(-1);
+  client_->DestroyAllRecords(
+      base::Bind(&CopyDBusMethodCallStatus, &returned_status));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(DBUS_METHOD_CALL_SUCCESS, returned_status);
+
+  // Return a null response to simulate failure.
+  AddMethodExpectation(biod::kBiometricsManagerDestroyAllRecordsMethod,
+                       nullptr);
+  returned_status = static_cast<DBusMethodCallStatus>(-1);
+  client_->DestroyAllRecords(
+      base::Bind(&CopyDBusMethodCallStatus, &returned_status));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(DBUS_METHOD_CALL_FAILURE, returned_status);
+}
+
 TEST_F(BiodClientTest, TestStartAuthentication) {
   const dbus::ObjectPath kFakeObjectPath(std::string("/fake/object/path"));
 
@@ -329,7 +364,7 @@ TEST_F(BiodClientTest, TestStartAuthentication) {
   EXPECT_EQ(kFakeObjectPath, returned_path);
 
   // Verify that by sending a empty reponse or a improperly formatted one, the
-  // response is an empty object path. Also, logs will get printed.
+  // response is an empty object path.
   AddMethodExpectation(biod::kBiometricsManagerStartAuthSessionMethod, nullptr);
   returned_path = dbus::ObjectPath(kInvalidTestPath);
   client_->StartAuthSession(base::Bind(&CopyObjectPath, &returned_path));
@@ -347,6 +382,60 @@ TEST_F(BiodClientTest, TestStartAuthentication) {
   EXPECT_EQ(dbus::ObjectPath(), returned_path);
 }
 
+TEST_F(BiodClientTest, TestRequestBiometricType) {
+  const biod::BiometricType kFakeBiometricType =
+      biod::BIOMETRIC_TYPE_FINGERPRINT;
+
+  std::unique_ptr<dbus::Response> response(dbus::Response::CreateEmpty());
+  dbus::MessageWriter writer(response.get());
+  writer.AppendVariantOfUint32(static_cast<uint32_t>(kFakeBiometricType));
+
+  // Create a fake response with biometric type. The get label call should
+  // return this exact biometric type.
+  biod::BiometricType returned_biometric_type = biod::BIOMETRIC_TYPE_MAX;
+  AddMethodExpectation(dbus::kDBusPropertiesGet, std::move(response));
+  client_->RequestType(
+      base::Bind(&CopyBiometricType, &returned_biometric_type));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(kFakeBiometricType, returned_biometric_type);
+
+  // Verify that by sending a null reponse, the result is an unknown biometric
+  // type.
+  returned_biometric_type = biod::BIOMETRIC_TYPE_MAX;
+  AddMethodExpectation(dbus::kDBusPropertiesGet, nullptr);
+  client_->RequestType(
+      base::Bind(&CopyBiometricType, &returned_biometric_type));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(biod::BiometricType::BIOMETRIC_TYPE_UNKNOWN,
+            returned_biometric_type);
+}
+
+TEST_F(BiodClientTest, TestRequestRecordLabel) {
+  const std::string kFakeLabel("fakeLabel");
+  const dbus::ObjectPath kFakeRecordPath(std::string("/fake/record/path"));
+
+  std::unique_ptr<dbus::Response> response(dbus::Response::CreateEmpty());
+  dbus::MessageWriter writer(response.get());
+  writer.AppendString(kFakeLabel);
+
+  // Create a fake response with string. The get label call should return this
+  // exact string.
+  std::string returned_label = kInvalidString;
+  AddMethodExpectation(dbus::kDBusPropertiesGet, std::move(response));
+  client_->RequestRecordLabel(kFakeRecordPath,
+                              base::Bind(&CopyString, &returned_label));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(kFakeLabel, returned_label);
+
+  // Verify that by sending a null reponse, the result is an empty string.
+  returned_label = kInvalidString;
+  AddMethodExpectation(dbus::kDBusPropertiesGet, nullptr);
+  client_->RequestRecordLabel(kFakeRecordPath,
+                              base::Bind(&CopyString, &returned_label));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ("", returned_label);
+}
+
 // Verify when signals are mocked, an observer will catch the signals as
 // expected.
 TEST_F(BiodClientTest, TestNotifyObservers) {
@@ -378,31 +467,4 @@ TEST_F(BiodClientTest, TestNotifyObservers) {
   EXPECT_EQ(1, observer.num_auth_scans_received());
   EXPECT_EQ(1, observer.num_failures_received());
 }
-
-TEST_F(BiodClientTest, TestGetRecordLabel) {
-  const std::string kFakeLabel("fakeLabel");
-  const dbus::ObjectPath kFakeRecordPath(std::string("/fake/record/path"));
-
-  std::unique_ptr<dbus::Response> response(dbus::Response::CreateEmpty());
-  dbus::MessageWriter writer(response.get());
-  writer.AppendString(kFakeLabel);
-
-  // Create a fake response with string. The get label call should return this
-  // exact string.
-  std::string returned_label = kInvalidString;
-  AddMethodExpectation(dbus::kDBusPropertiesGet, std::move(response));
-  client_->RequestRecordLabel(kFakeRecordPath,
-                              base::Bind(&CopyString, &returned_label));
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(kFakeLabel, returned_label);
-
-  // Verify that by sending a empty reponse the response is an empty string.
-  // Also, logs will get printed.
-  returned_label = kInvalidString;
-  AddMethodExpectation(dbus::kDBusPropertiesGet, nullptr);
-  client_->RequestRecordLabel(kFakeRecordPath,
-                              base::Bind(&CopyString, &returned_label));
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ("", returned_label);
-}
 }  // namespace chromeos

chromeos/dbus/biod/fake_biod_client.cc

@@ -60,7 +60,10 @@ void FakeBiodClient::GetRecordsForUser(const std::string& /* user_id */,
       FROM_HERE, base::Bind(callback, std::vector<dbus::ObjectPath>()));
 }
 
-void FakeBiodClient::DestroyAllRecords() {}
+void FakeBiodClient::DestroyAllRecords(const VoidDBusMethodCallback& callback) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(callback, DBUS_METHOD_CALL_SUCCESS));
+}
 
 void FakeBiodClient::StartAuthSession(const ObjectPathCallback& callback) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ -74,15 +77,31 @@ void FakeBiodClient::RequestType(const BiometricTypeCallback& callback) {
 }
 
 void FakeBiodClient::CancelEnrollSession(
-    const dbus::ObjectPath& /* enroll_session_path */) {}
+    const dbus::ObjectPath& /* enroll_session_path */,
+    const VoidDBusMethodCallback& callback) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(callback, DBUS_METHOD_CALL_SUCCESS));
+}
 
 void FakeBiodClient::EndAuthSession(
-    const dbus::ObjectPath& /* auth_session_path */) {}
+    const dbus::ObjectPath& /* auth_session_path */,
+    const VoidDBusMethodCallback& callback) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(callback, DBUS_METHOD_CALL_SUCCESS));
+}
 
 void FakeBiodClient::SetRecordLabel(const dbus::ObjectPath& /* record_path */,
-                                    const std::string& /* label */) {}
+                                    const std::string& /* label */,
+                                    const VoidDBusMethodCallback& callback) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(callback, DBUS_METHOD_CALL_SUCCESS));
+}
 
-void FakeBiodClient::RemoveRecord(const dbus::ObjectPath& /* record_path */) {}
+void FakeBiodClient::RemoveRecord(const dbus::ObjectPath& /* record_path */,
+                                  const VoidDBusMethodCallback& callback) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(callback, DBUS_METHOD_CALL_SUCCESS));
+}
 
 void FakeBiodClient::RequestRecordLabel(
     const dbus::ObjectPath& /* record_path */,

chromeos/dbus/biod/fake_biod_client.h

@@ -40,15 +40,18 @@ class CHROMEOS_EXPORT FakeBiodClient : public BiodClient {
                           const ObjectPathCallback& callback) override;
   void GetRecordsForUser(const std::string& user_id,
                          const UserRecordsCallback& callback) override;
-  void DestroyAllRecords() override;
+  void DestroyAllRecords(const VoidDBusMethodCallback& callback) override;
   void StartAuthSession(const ObjectPathCallback& callback) override;
   void RequestType(const BiometricTypeCallback& callback) override;
-  void CancelEnrollSession(
-      const dbus::ObjectPath& enroll_session_path) override;
-  void EndAuthSession(const dbus::ObjectPath& auth_session_path) override;
+  void CancelEnrollSession(const dbus::ObjectPath& enroll_session_path,
+                           const VoidDBusMethodCallback& callback) override;
+  void EndAuthSession(const dbus::ObjectPath& auth_session_path,
+                      const VoidDBusMethodCallback& callback) override;
   void SetRecordLabel(const dbus::ObjectPath& record_path,
-                      const std::string& label) override;
-  void RemoveRecord(const dbus::ObjectPath& record_path) override;
+                      const std::string& label,
+                      const VoidDBusMethodCallback& callback) override;
+  void RemoveRecord(const dbus::ObjectPath& record_path,
+                    const VoidDBusMethodCallback& callback) override;
   void RequestRecordLabel(const dbus::ObjectPath& record_path,
                           const LabelCallback& callback) override;