Changes to PairingRegistry to facilitate per-platform implementions.

Implementing the Linux pairing registry delegate required some changes to the cross-platform code. I've isolated them in this CL for clarity:

* Added a timestamp to the Pairing struct and made it a class with a Create method to generate the random information and timestamp.
* Added a callback to AddPairing. For now, this is only used in the unit test.

I also fixed a couple of unrelated linter errors in protocol_mock_objects.

BUG=156182

Review URL: https://chromiumcodereview.appspot.com/17063003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@206947 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 119 insertions, 52 deletions

diff --git a/remoting/protocol/negotiating_authenticator_unittest.cc b/remoting/protocol/negotiating_authenticator_unittest.cc
index 8a18f2d..fb7ed34 100644
--- a/remoting/protocol/negotiating_authenticator_unittest.cc
+++ b/remoting/protocol/negotiating_authenticator_unittest.cc
@@ -31,6 +31,7 @@ const int kMessages = 1;
 
 const char kNoClientId[] = "";
 const char kNoPairedSecret[] = "";
+const char kTestClientName[] = "client-name";
 const char kTestClientId[] = "client-id";
 const char kTestHostId[] = "12345678910123456";
 
@@ -85,10 +86,10 @@ class NegotiatingAuthenticatorTest : public AuthenticatorTestBase {
   void CreatePairingRegistry(bool with_paired_client) {
     mock_delegate_ = new MockPairingRegistryDelegate;
     if (with_paired_client) {
-      PairingRegistry::Pairing pairing;
-      pairing.client_id = kTestClientId;
-      pairing.shared_secret = kTestPairedSecret;
-      mock_delegate_->AddPairing(pairing);
+      PairingRegistry::Pairing pairing(
+          base::Time(), kTestClientName, kTestClientId, kTestPairedSecret);
+      mock_delegate_->AddPairing(pairing,
+                                 PairingRegistry::AddPairingCallback());
     }
     pairing_registry_ = new PairingRegistry(
         scoped_ptr<PairingRegistry::Delegate>(mock_delegate_));
diff --git a/remoting/protocol/pairing_host_authenticator.cc b/remoting/protocol/pairing_host_authenticator.cc
index 75db3a7a..bd1f75d 100644
--- a/remoting/protocol/pairing_host_authenticator.cc
+++ b/remoting/protocol/pairing_host_authenticator.cc
@@ -97,7 +97,7 @@ void PairingHostAuthenticator::ProcessMessageWithPairing(
     const base::Closure& resume_callback,
     PairingRegistry::Pairing pairing) {
   waiting_for_paired_secret_ = false;
-  std::string paired_secret = pairing.shared_secret;
+  std::string paired_secret = pairing.shared_secret();
   if (paired_secret.empty()) {
     LOG(INFO) << "Unknown client id";
     error_message_ = "unknown-client-id";
diff --git a/remoting/protocol/pairing_registry.cc b/remoting/protocol/pairing_registry.cc
index 10098b0..6c9cc66 100644
--- a/remoting/protocol/pairing_registry.cc
+++ b/remoting/protocol/pairing_registry.cc
@@ -12,35 +12,63 @@
 namespace remoting {
 namespace protocol {
 
-// How many bytes of random data to use for the client id and shared secret.
+// How many bytes of random data to use for the shared secret.
 const int kKeySize = 16;
 
-PairingRegistry::PairingRegistry(scoped_ptr<Delegate> delegate)
-    : delegate_(delegate.Pass()) {
-  DCHECK(delegate_);
+PairingRegistry::Pairing::Pairing() {
 }
 
-PairingRegistry::~PairingRegistry() {
+PairingRegistry::Pairing::Pairing(const base::Time& created_time,
+                                  const std::string& client_name,
+                                  const std::string& client_id,
+                                  const std::string& shared_secret)
+    : created_time_(created_time),
+      client_name_(client_name),
+      client_id_(client_id),
+      shared_secret_(shared_secret) {
 }
 
-PairingRegistry::Pairing PairingRegistry::CreatePairing(
+PairingRegistry::Pairing PairingRegistry::Pairing::Create(
     const std::string& client_name) {
-  DCHECK(CalledOnValidThread());
-
-  Pairing result;
-  result.client_name = client_name;
-  result.client_id = base::GenerateGUID();
-
-  // Create a random shared secret to authenticate this client.
+  base::Time created_time = base::Time::Now();
+  std::string client_id = base::GenerateGUID();
+  std::string shared_secret;
   char buffer[kKeySize];
   crypto::RandBytes(buffer, arraysize(buffer));
   if (!base::Base64Encode(base::StringPiece(buffer, arraysize(buffer)),
-                          &result.shared_secret)) {
+                          &shared_secret)) {
     LOG(FATAL) << "Base64Encode failed.";
   }
+  return Pairing(created_time, client_name, client_id, shared_secret);
+}
+
+PairingRegistry::Pairing::~Pairing() {
+}
+
+bool PairingRegistry::Pairing::operator==(const Pairing& other) const {
+  return created_time_ == other.created_time_ &&
+         client_id_ == other.client_id_ &&
+         client_name_ == other.client_name_ &&
+         shared_secret_ == other.shared_secret_;
+}
+
+bool PairingRegistry::Pairing::is_valid() const {
+  return !client_id_.empty() && !shared_secret_.empty();
+}
+
+PairingRegistry::PairingRegistry(scoped_ptr<Delegate> delegate)
+    : delegate_(delegate.Pass()) {
+  DCHECK(delegate_);
+}
+
+PairingRegistry::~PairingRegistry() {
+}
 
-  // Save the result via the Delegate and return it to the caller.
-  delegate_->AddPairing(result);
+PairingRegistry::Pairing PairingRegistry::CreatePairing(
+    const std::string& client_name) {
+  DCHECK(CalledOnValidThread());
+  Pairing result = Pairing::Create(client_name);
+  delegate_->AddPairing(result, AddPairingCallback());
   return result;
 }
 
@@ -51,8 +79,12 @@ void PairingRegistry::GetPairing(const std::string& client_id,
 }
 
 void NotImplementedPairingRegistryDelegate::AddPairing(
-    const PairingRegistry::Pairing& new_paired_client) {
+    const PairingRegistry::Pairing& new_paired_client,
+    const PairingRegistry::AddPairingCallback& callback) {
   NOTIMPLEMENTED();
+  if (!callback.is_null()) {
+    callback.Run(false);
+  }
 }
 
 void NotImplementedPairingRegistryDelegate::GetPairing(
@@ -62,6 +94,5 @@ void NotImplementedPairingRegistryDelegate::GetPairing(
   callback.Run(PairingRegistry::Pairing());
 }
 
-
 }  // namespace protocol
 }  // namespace remoting
diff --git a/remoting/protocol/pairing_registry.h b/remoting/protocol/pairing_registry.h
index c32c54f..3544524 100644
--- a/remoting/protocol/pairing_registry.h
+++ b/remoting/protocol/pairing_registry.h
@@ -12,10 +12,14 @@
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/threading/non_thread_safe.h"
+#include "base/time.h"
 
 namespace remoting {
 namespace protocol {
 
+// TODO(jamiewalch): This class is little more than a wrapper around the
+// Pairing and Delegate classes. Refactor it away.
+
 // PairingRegistry holds information about paired clients to support
 // PIN-less authentication. For each paired client, the registry holds
 // the following information:
@@ -29,28 +33,55 @@ class PairingRegistry : public base::RefCountedThreadSafe<PairingRegistry>,
                         public base::NonThreadSafe {
  public:
   struct Pairing {
-    std::string client_id;
-    std::string client_name;
-    std::string shared_secret;
+    Pairing();
+    Pairing(const base::Time& created_time,
+            const std::string& client_name,
+            const std::string& client_id,
+            const std::string& shared_secret);
+    ~Pairing();
+
+    static Pairing Create(const std::string& client_name);
+
+    bool operator==(const Pairing& other) const;
+
+    bool is_valid() const;
+
+    base::Time created_time() const { return created_time_; }
+    std::string client_id() const { return client_id_; }
+    std::string client_name() const { return client_name_; }
+    std::string shared_secret() const { return shared_secret_; }
+
+   private:
+    base::Time created_time_;
+    std::string client_name_;
+    std::string client_id_;
+    std::string shared_secret_;
   };
 
   // Mapping from client id to pairing information.
   typedef std::map<std::string, Pairing> PairedClients;
 
-  // Delegate::GetPairing callback.
-  typedef base::Callback<void(Pairing)> GetPairingCallback;
+  // Delegate callbacks.
+  typedef base::Callback<void(Pairing client_information)> GetPairingCallback;
+  typedef base::Callback<void(bool success)> AddPairingCallback;
 
   // Interface representing the persistent storage back-end.
   class Delegate {
    public:
     virtual ~Delegate() {}
 
-    // Add pairing information to persistent storage. Must not block.
-    virtual void AddPairing(const Pairing& new_paired_client) = 0;
-
-    // Retrieve the Pairing for the specified client id. If none is
-    // found, invoke the callback with a Pairing in which (at least)
-    // the shared_secret is empty.
+    // Add pairing information to persistent storage. If a non-NULL callback
+    // is provided, invoke it on completion to indicate success or failure.
+    // Must not block.
+    //
+    // TODO(jamiewalch): Plumb the callback into the RequestPairing flow so
+    // that the client isn't sent the pairing information until it has been
+    // saved.
+    virtual void AddPairing(const Pairing& new_paired_client,
+                            const AddPairingCallback& callback) = 0;
+
+    // Retrieve the Pairing for the specified client id. If none is found,
+    // invoke the callback with a default pairing. Must not block.
     virtual void GetPairing(const std::string& client_id,
                             const GetPairingCallback& callback) = 0;
   };
@@ -80,7 +111,8 @@ class PairingRegistry : public base::RefCountedThreadSafe<PairingRegistry>,
 class NotImplementedPairingRegistryDelegate : public PairingRegistry::Delegate {
  public:
   virtual void AddPairing(
-      const PairingRegistry::Pairing& paired_clients) OVERRIDE;
+      const PairingRegistry::Pairing& paired_clients,
+      const PairingRegistry::AddPairingCallback& callback) OVERRIDE;
   virtual void GetPairing(
       const std::string& client_id,
       const PairingRegistry::GetPairingCallback& callback) OVERRIDE;
diff --git a/remoting/protocol/pairing_registry_unittest.cc b/remoting/protocol/pairing_registry_unittest.cc
index 523bdaa..a5d8bf2 100644
--- a/remoting/protocol/pairing_registry_unittest.cc
+++ b/remoting/protocol/pairing_registry_unittest.cc
@@ -23,7 +23,7 @@ class PairingRegistryTest : public testing::Test {
 
   void CompareSecret(const std::string& expected,
                      PairingRegistry::Pairing actual) {
-    EXPECT_EQ(actual.shared_secret, expected);
+    EXPECT_EQ(actual.shared_secret(), expected);
     got_secret_ = true;
   }
 
@@ -32,22 +32,19 @@ class PairingRegistryTest : public testing::Test {
 };
 
 TEST_F(PairingRegistryTest, GetPairing) {
-  PairingRegistry::Pairing client_info = {
-    "client_id",
-    "client_name",
-    "shared_secret"
-  };
+  PairingRegistry::Pairing client_info =
+      PairingRegistry::Pairing::Create("client_name");
   MockPairingRegistryDelegate* mock_delegate =
       new MockPairingRegistryDelegate();
-  mock_delegate->AddPairing(client_info);
+  mock_delegate->AddPairing(client_info, PairingRegistry::AddPairingCallback());
   scoped_ptr<PairingRegistry::Delegate> delegate(mock_delegate);
 
   scoped_refptr<PairingRegistry> registry(new PairingRegistry(delegate.Pass()));
 
-  registry->GetPairing(client_info.client_id,
+  registry->GetPairing(client_info.client_id(),
                        base::Bind(&PairingRegistryTest::CompareSecret,
                                   base::Unretained(this),
-                                  client_info.shared_secret));
+                                  client_info.shared_secret()));
   mock_delegate->RunCallback();
   EXPECT_TRUE(got_secret_);
 }
@@ -69,8 +66,8 @@ TEST_F(PairingRegistryTest, AddPairing) {
   ASSERT_EQ(clients.size(), 2u);
   PairingRegistry::Pairing first = clients.begin()->second;
   PairingRegistry::Pairing second = (++clients.begin())->second;
-  EXPECT_EQ(first.client_name, second.client_name);
-  EXPECT_NE(first.client_id, second.client_id);
+  EXPECT_EQ(first.client_name(), second.client_name());
+  EXPECT_NE(first.client_id(), second.client_id());
 }
 
 }  // namespace protocol
diff --git a/remoting/protocol/protocol_mock_objects.cc b/remoting/protocol/protocol_mock_objects.cc
index eb4022a..1902d83 100644
--- a/remoting/protocol/protocol_mock_objects.cc
+++ b/remoting/protocol/protocol_mock_objects.cc
@@ -55,8 +55,12 @@ MockPairingRegistryDelegate::~MockPairingRegistryDelegate() {
 }
 
 void MockPairingRegistryDelegate::AddPairing(
-    const PairingRegistry::Pairing& new_paired_client) {
-  paired_clients_[new_paired_client.client_id] = new_paired_client;
+    const PairingRegistry::Pairing& new_paired_client,
+    const PairingRegistry::AddPairingCallback& callback) {
+  paired_clients_[new_paired_client.client_id()] = new_paired_client;
+  if (!callback.is_null()) {
+    callback.Run(true);
+  }
 }
 
 void MockPairingRegistryDelegate::GetPairing(
diff --git a/remoting/protocol/protocol_mock_objects.h b/remoting/protocol/protocol_mock_objects.h
index 7262ae5..0f1e24d 100644
--- a/remoting/protocol/protocol_mock_objects.h
+++ b/remoting/protocol/protocol_mock_objects.h
@@ -187,18 +187,19 @@ class MockSessionManager : public SessionManager {
       Authenticator* authenticator,
       CandidateSessionConfig* config));
   MOCK_METHOD0(Close, void());
-  MOCK_METHOD1(set_authenticator_factory_ptr, void(AuthenticatorFactory*));
+  MOCK_METHOD1(set_authenticator_factory_ptr,
+               void(AuthenticatorFactory* factory));
   virtual scoped_ptr<Session> Connect(
       const std::string& host_jid,
       scoped_ptr<Authenticator> authenticator,
       scoped_ptr<CandidateSessionConfig> config) {
     return scoped_ptr<Session>(ConnectPtr(
         host_jid, authenticator.get(), config.get()));
-  };
+  }
   virtual void set_authenticator_factory(
       scoped_ptr<AuthenticatorFactory> authenticator_factory) {
     set_authenticator_factory_ptr(authenticator_factory.release());
-  };
+  }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockSessionManager);
@@ -216,7 +217,8 @@ class MockPairingRegistryDelegate : public PairingRegistry::Delegate {
 
   // PairingRegistry::Delegate implementation.
   virtual void AddPairing(
-      const PairingRegistry::Pairing& new_paired_client) OVERRIDE;
+      const PairingRegistry::Pairing& new_paired_client,
+      const PairingRegistry::AddPairingCallback& callback) OVERRIDE;
   virtual void GetPairing(
       const std::string& client_id,
       const PairingRegistry::GetPairingCallback& callback) OVERRIDE;