Linux pairing registry delegate implementation

remoting/protocol/pairing_registry.h

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef REMOTING_PROTOCOL_PAIRING_REGISTRY_H_
 #define REMOTING_PROTOCOL_PAIRING_REGISTRY_H_
 
 #include <map>
 #include <string>
 
 #include "base/callback.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/threading/non_thread_safe.h"
+#include "base/time.h"
+
+namespace base {
+class TaskRunner;
+}  // namespace base
 
 namespace remoting {
 namespace protocol {
 
 // PairingRegistry holds information about paired clients to support
 // PIN-less authentication. For each paired client, the registry holds
 // the following information:
 //   * The name of the client. This is supplied by the client and is not
 //     guaranteed to be unique.
 //   * The unique id of the client. This is generated on-demand by this
 //     class and sent in plain-text by the client during authentication.
 //   * The shared secret for the client. This is generated on-demand by this
 //     class and used in the SPAKE2 exchange to mutually verify identity.
 class PairingRegistry : public base::RefCountedThreadSafe<PairingRegistry>,
                         public base::NonThreadSafe {
  public:
   struct Pairing {
+    bool operator==(const Pairing& other) const;

[Lambros, 2013/05/31 18:51:59]

Is this used only in unittest? If so, consider moving it there.

[Jamie, 2013/06/19 17:56:44]

It is only used in the unit test, but I've now made Pairing into a full class,
so I think it's better to keep it part of the class, if only to reduce the
chance of it getting out-of-sync with the data members.

+
+    base::Time created_time;
     std::string client_id;
     std::string client_name;
     std::string shared_secret;
   };
 
   // Mapping from client id to pairing information.
   typedef std::map<std::string, Pairing> PairedClients;
 
   // Interface representing the persistent storage back-end.
   class Delegate {
    public:
     virtual ~Delegate() {}
 
     // Save pairing information to persistent storage. Must not block.
     virtual void Save(const PairedClients& paired_clients) = 0;
+
+    // Load the pairing information synchronously from persistent storage.

[Lambros, 2013/05/31 18:51:59]

nit: Loads (and elsewhere)
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Consider updating all function-comments here to be compliant with style guide.

[Jamie, 2013/06/19 17:56:44]

I've updated the implementation methods as you suggest. I think the interface
methods make more sense as instructions. For example, both methods end with
"Must not block.", which is clearly an instruction to the implementer, and I
think it makes more sense for the rest of the comment to follow that style.

+    // Note that this method is not used by the PairingRegistry class; instead
+    // it should be called on the result of CreateDelegate prior to creating
+    // a PairingRegistry. Unlike Save, LoadOnCurrentThread may block while
+    // loading the pairings--it is the caller's responsibility to make sure
+    // it is called on a suitable thread.
+    virtual PairedClients LoadOnCurrentThread() = 0;
   };
 
   explicit PairingRegistry(scoped_ptr<Delegate> delegate,
                            const PairedClients& paired_clients);
 
   // Create a pairing for a new client and save it to disk.
   const Pairing& CreatePairing(const std::string& client_name);
 
   // Look up the shared secret for the specified client id. Returns an empty
   // string if the client id is not known.
   std::string GetSecret(const std::string &client_id) const;
 
+  // Return a platform-specific Delegate that will save to permanent storage

[Lambros, 2013/05/31 18:51:59]

Can this be moved to remoting/host ?

[Jamie, 2013/06/19 17:56:44]

It could, but the Create pattern is very common in our code-base IIRC, so it
seems more consistent to define it here. If I'm mistaken, please let me know.

[Lambros, 2013/06/20 17:59:20]

The pattern is fine, but it doesn't belong in remoting/protocol/.

The difference here is that nothing else in remoting/protocol/ calls
CreateDelegate(). In the other cases where we use the pattern, the Create()
method is defined in the same place as its per-platform implementations.

If you built libprotocol.a, nothing in that library calls into CreateDelegate().
And that library doesn't (and shouldn't) implement it itself. It's a dead
function that the linker would completely remove.

[Jamie, 2013/06/20 20:17:09]

Good point. There were actually quite a lot of classes defined in the wrong
namespace here.

+  // using the specified TaskRunner. Return NULL on platforms that don't
+  // support pairing.
+  static scoped_ptr<Delegate> CreateDelegate(
+      scoped_refptr<base::TaskRunner> task_runner);
+
  private:
   friend class base::RefCountedThreadSafe<PairingRegistry>;
 
   virtual ~PairingRegistry();
 
   // Callback for the Delegate::Load method. Invoked when the stored pairings
   // have been loaded.
   void OnLoad(const PairedClients& paired_clients);
 
   scoped_ptr<Delegate> delegate_;
   PairedClients paired_clients_;
 
   DISALLOW_COPY_AND_ASSIGN(PairingRegistry);
 };
 
 // Temporary delegate that just logs NOTIMPLEMENTED for Load/Save.

[Lambros, 2013/05/31 18:51:59]

Can we remove this class as well?

[Jamie, 2013/06/19 17:56:44]

Done.

 // TODO(jamiewalch): Delete once Delegates are implemented for all platforms.
 class NotImplementedPairingRegistryDelegate : public PairingRegistry::Delegate {
  public:
   virtual void Save(
       const PairingRegistry::PairedClients& paired_clients) OVERRIDE;
+  virtual PairingRegistry::PairedClients LoadOnCurrentThread() OVERRIDE;
 };
 
 }  // namespace protocol
 }  // namespace remoting
 
 #endif  // REMOTING_PROTOCOL_PAIRING_REGISTRY_H_