Refactored PairingRegistry::Delegate such that it can retrieve/modify for a single client.

remoting/host/pairing_registry_delegate_linux.cc

Index: remoting/host/pairing_registry_delegate_linux.cc
diff --git a/remoting/host/pairing_registry_delegate_linux.cc b/remoting/host/pairing_registry_delegate_linux.cc
index bca9b4fb95a03470d89f6e2eb7d5f6dbc36da793..6eba04dd014a520200ecd6b9c8ff8f0b21088e0e 100644
--- a/remoting/host/pairing_registry_delegate_linux.cc
+++ b/remoting/host/pairing_registry_delegate_linux.cc
@@ -6,14 +6,25 @@
 
 #include "base/bind.h"
 #include "base/file_util.h"
+#include "base/files/file_enumerator.h"
 #include "base/files/important_file_writer.h"
+#include "base/json/json_file_value_serializer.h"
+#include "base/json/json_string_value_serializer.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
+#include "base/strings/stringprintf.h"
 #include "base/thread_task_runner_handle.h"
+#include "base/values.h"
 #include "remoting/host/branding.h"
 
 namespace {
-const char kRegistryFilename[] = "paired-clients.json";
+
+// The pairing registry path relative to the configuration directory.
+const char kRegistryDirectory[] = "pairing-registry";

[Jamie, 2013/07/30 21:35:07]

I think paired-clients is sufficient for the directory name ("registry" doesn't
make it as clear what the contents are used for).

[alexeypa (please no reviews), 2013/07/31 21:31:24]

Done.

+
+const char kPairingFilenameFormat[] = "%s.json";
+const char kPairingFilenamePattern[] = "*.json";
+
 }  // namespace
 
 namespace remoting {
@@ -29,97 +40,244 @@ PairingRegistryDelegateLinux::PairingRegistryDelegateLinux(
 PairingRegistryDelegateLinux::~PairingRegistryDelegateLinux() {
 }
 
-void PairingRegistryDelegateLinux::Save(
-    const std::string& pairings_json,
-    const PairingRegistry::SaveCallback& callback) {
-  // If a callback was supplied, wrap it in a helper function that will
-  // run it on this thread.
-  PairingRegistry::SaveCallback run_callback_on_this_thread;
-  if (!callback.is_null()) {
-    run_callback_on_this_thread =
-        base::Bind(&PairingRegistryDelegateLinux::RunSaveCallbackOnThread,
-                   base::ThreadTaskRunnerHandle::Get(),
-                   callback);
-  }
+void PairingRegistryDelegateLinux::LoadAll(
+    const protocol::PairingRegistry::GetAllPairingsCallback& callback) {
+  // Wrap the callback in a helper function that will run it on this thread.
+  PairingRegistry::GetAllPairingsCallback run_callback_on_this_thread =
+      base::Bind(
+          &PairingRegistryDelegateLinux::RunGetAllPairingsCallbackOnThread,
+          base::ThreadTaskRunnerHandle::Get(),
+          callback);
   task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(&PairingRegistryDelegateLinux::DoSave,
+      base::Bind(&PairingRegistryDelegateLinux::DoLoadAll,
+                 weak_factory_.GetWeakPtr(),
+                 run_callback_on_this_thread));
+}
+
+void PairingRegistryDelegateLinux::DeleteAll(
+    const protocol::PairingRegistry::DoneCallback& callback) {
+  // Wrap the callback in a helper function that will run it on this thread.
+  PairingRegistry::DoneCallback run_callback_on_this_thread =
+      base::Bind(&PairingRegistryDelegateLinux::RunDoneCallbackOnThread,
+                 base::ThreadTaskRunnerHandle::Get(),
+                 callback);
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&PairingRegistryDelegateLinux::DoDeleteAll,
                  weak_factory_.GetWeakPtr(),
-                 pairings_json,
                  run_callback_on_this_thread));
 }
 
 void PairingRegistryDelegateLinux::Load(
-    const PairingRegistry::LoadCallback& callback) {
+    const std::string& client_id,
+    const protocol::PairingRegistry::GetPairingCallback& callback) {
   // Wrap the callback in a helper function that will run it on this thread.
-  // Note that, unlike AddPairing, the GetPairing callback is mandatory.
-  PairingRegistry::LoadCallback run_callback_on_this_thread =
-      base::Bind(&PairingRegistryDelegateLinux::RunLoadCallbackOnThread,
+  PairingRegistry::GetPairingCallback run_callback_on_this_thread =
+      base::Bind(&PairingRegistryDelegateLinux::RunGetPairingCallbackOnThread,
                  base::ThreadTaskRunnerHandle::Get(),
                  callback);
   task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&PairingRegistryDelegateLinux::DoLoad,
                  weak_factory_.GetWeakPtr(),
+                 client_id,
                  run_callback_on_this_thread));
 }
 
-void PairingRegistryDelegateLinux::RunSaveCallbackOnThread(
-    scoped_refptr<base::TaskRunner> task_runner,
-    const PairingRegistry::SaveCallback& callback,
-    bool success) {
-  task_runner->PostTask(FROM_HERE, base::Bind(callback, success));
+void PairingRegistryDelegateLinux::Save(
+    const protocol::PairingRegistry::Pairing& pairing,
+    const protocol::PairingRegistry::DoneCallback& callback) {
+  // Wrap the callback in a helper function that will run it on this thread.
+  PairingRegistry::DoneCallback run_callback_on_this_thread =
+      base::Bind(&PairingRegistryDelegateLinux::RunDoneCallbackOnThread,
+                 base::ThreadTaskRunnerHandle::Get(),
+                 callback);
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&PairingRegistryDelegateLinux::DoSave,
+                 weak_factory_.GetWeakPtr(),
+                 pairing,
+                 run_callback_on_this_thread));
 }
 
-void PairingRegistryDelegateLinux::RunLoadCallbackOnThread(
-      scoped_refptr<base::TaskRunner> task_runner,
-      const PairingRegistry::LoadCallback& callback,
-      const std::string& pairings_json) {
-  task_runner->PostTask(FROM_HERE, base::Bind(callback, pairings_json));
+void PairingRegistryDelegateLinux::Delete(
+    const std::string& client_id,
+    const protocol::PairingRegistry::DoneCallback& callback) {
+  // Wrap the callback in a helper function that will run it on this thread.
+  PairingRegistry::DoneCallback run_callback_on_this_thread =
+      base::Bind(&PairingRegistryDelegateLinux::RunDoneCallbackOnThread,
+                 base::ThreadTaskRunnerHandle::Get(),
+                 callback);
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&PairingRegistryDelegateLinux::DoDelete,
+                 weak_factory_.GetWeakPtr(),
+                 client_id,
+                 run_callback_on_this_thread));
+}
+
+void PairingRegistryDelegateLinux::DoLoadAll(
+    const protocol::PairingRegistry::GetAllPairingsCallback& callback) {
+  scoped_ptr<base::ListValue> pairings(new base::ListValue());
+
+  // Enumerate all pairing files in the pairing registry.
+  base::FilePath registry_path = GetRegistryPath();
+  base::FileEnumerator enumerator(registry_path, false,
+                                  base::FileEnumerator::FILES,
+                                  kPairingFilenamePattern);
+  for (base::FilePath pairing_file = enumerator.Next(); !pairing_file.empty();
+       pairing_file = enumerator.Next()) {
+    // Read the JSON containing pairing data.
+    JSONFileValueSerializer serializer(pairing_file);
+    int error_code;
+    std::string error_message;
+    scoped_ptr<base::Value> pairing_json(
+        serializer.Deserialize(&error_code, &error_message));
+    if (!pairing_json) {
+      LOG(WARNING) << "Failed to load '" << pairing_file.value() << "' ("
+                   << error_code << ").";
+      continue;
+    }
+
+    // Parse the pairing data.
+    protocol::PairingRegistry::Pairing pairing =
+        protocol::PairingRegistry::Pairing::CreateFromJson(*pairing_json);
+    if (!pairing.is_valid()) {
+      LOG(WARNING) << "Could not parse '" << pairing_file.value() << "'";
+      continue;
+    }
+
+    // Clear the shared secrect and append the pairing data to the list.
+    protocol::PairingRegistry::Pairing sanitized_pairing(
+        pairing.created_time(),
+        pairing.client_name(),
+        pairing.client_id(),
+        "");

[Jamie, 2013/07/30 21:35:07]

Would it make sense to make the extraction of the secret optional in
CreateFromJson? It would avoid duplicating it in every delegate.

[alexeypa (please no reviews), 2013/07/31 21:31:24]

Done.

+    pairings->Append(sanitized_pairing.EncodeJson().release());
+  }
+
+  callback.Run(pairings.Pass());
+}
+
+void PairingRegistryDelegateLinux::DoDeleteAll(
+    const protocol::PairingRegistry::DoneCallback& callback) {
+  // Delete all pairing files in the pairing registry.
+  base::FilePath registry_path = GetRegistryPath();
+  base::FileEnumerator enumerator(registry_path, false,
+                                  base::FileEnumerator::FILES,
+                                  kPairingFilenamePattern);
+
+  for (base::FilePath pairing_file = enumerator.Next(); !pairing_file.empty();
+       pairing_file = enumerator.Next()) {
+    base::DeleteFile(pairing_file, false);

[Jamie, 2013/07/30 21:35:07]

DeleteFile can fail. I think we should try to continue deleting the other files
in this case, but pass false to the callback.

[alexeypa (please no reviews), 2013/07/31 21:31:24]

Done.

+  }
+
+  callback.Run(true);
+}
+
+void PairingRegistryDelegateLinux::DoLoad(
+    const std::string& client_id,
+    const protocol::PairingRegistry::GetPairingCallback& callback) {
+  base::FilePath registry_path = GetRegistryPath();
+  base::FilePath pairing_file = registry_path.Append(
+      base::StringPrintf(kPairingFilenameFormat, client_id.c_str()));
+
+  JSONFileValueSerializer serializer(pairing_file);
+  int error_code;
+  std::string error_message;
+  scoped_ptr<base::Value> pairing(
+      serializer.Deserialize(&error_code, &error_message));
+  if (!pairing) {
+    LOG(WARNING) << "Failed to load pairing information: " << error_message
+                 << " (" << error_code << ").";
+    callback.Run(protocol::PairingRegistry::Pairing());
+    return;
+  }
+
+  callback.Run(protocol::PairingRegistry::Pairing::CreateFromJson(*pairing));
 }
 
 void PairingRegistryDelegateLinux::DoSave(
-    const std::string& pairings_json,
-    const PairingRegistry::SaveCallback& callback) {
-  base::FilePath registry_path = GetRegistryFilePath();
-  base::FilePath parent_directory = registry_path.DirName();
+    const protocol::PairingRegistry::Pairing& pairing,
+    const protocol::PairingRegistry::DoneCallback& callback) {
+  base::FilePath registry_path = GetRegistryPath();
   base::PlatformFileError error;
-  if (!file_util::CreateDirectoryAndGetError(parent_directory, &error)) {
+  if (!file_util::CreateDirectoryAndGetError(registry_path, &error)) {
     LOG(ERROR) << "Could not create pairing registry directory: " << error;
+    callback.Run(false);
     return;
   }
-  if (!base::ImportantFileWriter::WriteFileAtomically(registry_path,
-                                                      pairings_json)) {
-    LOG(ERROR) << "Could not save pairing registry.";
+
+  std::string pairing_json;
+  JSONStringValueSerializer serializer(&pairing_json);
+  if (!serializer.Serialize(*pairing.EncodeJson())) {
+    LOG(ERROR) << "Failed to serialize pairing data for "
+               << pairing.client_id();
+    callback.Run(false);
+    return;
   }
 
-  if (!callback.is_null()) {
-    callback.Run(true);
+  base::FilePath pairing_file = registry_path.Append(
+      base::StringPrintf(kPairingFilenameFormat, pairing.client_id().c_str()));
+  if (!base::ImportantFileWriter::WriteFileAtomically(pairing_file,
+                                                      pairing_json)) {
+    LOG(ERROR) << "Could not save pairing data for " << pairing.client_id();
+    callback.Run(false);
+    return;
   }
+
+  callback.Run(true);
 }
 
-void PairingRegistryDelegateLinux::DoLoad(
-    const PairingRegistry::LoadCallback& callback) {
-  base::FilePath registry_path = GetRegistryFilePath();
-  std::string result;
-  if (!file_util::ReadFileToString(registry_path, &result)) {
-    LOG(ERROR) << "Load failed.";
-  }
-  callback.Run(result);
+void PairingRegistryDelegateLinux::DoDelete(
+    const std::string& client_id,
+    const protocol::PairingRegistry::DoneCallback& callback) {
+  base::FilePath registry_path = GetRegistryPath();
+  base::FilePath pairing_file = registry_path.Append(
+      base::StringPrintf(kPairingFilenameFormat, client_id.c_str()));
+
+  base::DeleteFile(pairing_file, false);
+  callback.Run(true);
 }
 
-base::FilePath PairingRegistryDelegateLinux::GetRegistryFilePath() {
-  if (!filename_for_testing_.empty()) {
-    return filename_for_testing_;
+void PairingRegistryDelegateLinux::RunDoneCallbackOnThread(
+    scoped_refptr<base::TaskRunner> task_runner,
+    const protocol::PairingRegistry::DoneCallback& callback,
+    bool success) {
+  // The callback is optional when a pairing is created.
+  if (!callback.is_null())
+    task_runner->PostTask(FROM_HERE, base::Bind(callback, success));

[Jamie, 2013/07/30 21:35:07]

Would it be worth moving these Run*CallbackOnThread methods into the
PairingRegistry so that they don't need to be duplicated in all delegates?

[alexeypa (please no reviews), 2013/07/31 21:31:24]

I made PairingRegistry::Delegate a synchronous class and moved all threading to
PairingRegistry.

+}
+
+void PairingRegistryDelegateLinux::RunGetAllPairingsCallbackOnThread(
+    scoped_refptr<base::TaskRunner> task_runner,
+    const protocol::PairingRegistry::GetAllPairingsCallback& callback,
+    scoped_ptr<base::ListValue> pairings) {
+  task_runner->PostTask(FROM_HERE,
+                        base::Bind(callback, base::Passed(&pairings)));
+}
+
+void PairingRegistryDelegateLinux::RunGetPairingCallbackOnThread(
+    scoped_refptr<base::TaskRunner> task_runner,
+    const protocol::PairingRegistry::GetPairingCallback& callback,
+    protocol::PairingRegistry::Pairing pairing) {
+  task_runner->PostTask(FROM_HERE, base::Bind(callback, pairing));
+}
+
+
+base::FilePath PairingRegistryDelegateLinux::GetRegistryPath() {
+  if (!registry_path_for_testing_.empty()) {
+    return registry_path_for_testing_;
   }
 
   base::FilePath config_dir = remoting::GetConfigDir();
-  return config_dir.Append(kRegistryFilename);
+  return config_dir.Append(kRegistryDirectory);
 }
 
-void PairingRegistryDelegateLinux::SetFilenameForTesting(
-    const base::FilePath &filename) {
-  filename_for_testing_ = filename;
+void PairingRegistryDelegateLinux::SetRegistryPathForTesting(
+    const base::FilePath& registry_path) {
+  registry_path_for_testing_ = registry_path;
 }