Linux pairing registry delegate implementation

Linux pairing registry delegate implementation

BUG=156182
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208123

[Jamie, 2013-05-31 01:14:22]

ptal

https://codereview.chromium.org/15709005/diff/1/remoting/protocol/pairing_reg...
File remoting/protocol/pairing_registry.h (right):

https://codereview.chromium.org/15709005/diff/1/remoting/protocol/pairing_reg...
remoting/protocol/pairing_registry.h:39: base::Time created_time;
Not strictly related to this CL, and I'm not sure if we'll ever use this
information, but it seems like a useful thing to save. I can split it into a
separate CL if you'd prefer.

[Sergey Ulanov, 2013-05-31 18:45:46]

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
File remoting/host/pairing_registry_delegate_linux.cc (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:16: const char kConfigDir[] =
".config/chrome-remote-desktop";
There is remoting::GetConfigDir() that returns path to the config directory.

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:17: const char
kRegistryFilename[] = "paired-clients";
Maybe paired-clients.json?
For host config we add a hash of the host name to deal with the case when home
directory is shared between multiple machines. I think it might be useful here
too. 
Config file path with the suffix normally comes from command line. One possible
solution is to replace --host-config option with --config-suffix and use the
suffix for both files.

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:43: void
PairingRegistryDelegateLinux::SaveOnCurrentThread(
Why is it "OnCurrentThread"? Should it be something like OnIoThread? Or maybe
just call it DoSave()

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:51: for
(PairingRegistry::PairedClients::const_iterator i =
It looks to me that most of the code in this class can be shared between
platforms. All formatting/parsing code can be shared. Is it possible to avoid
duplication? E.g. PairingRegistry could format the data, so Save() would accept
already formatted base::Value and so it would just need to save it at the right
place.
But I'm actually not sure why delegate needs to be platform specific. The only
part that's differs between platforms is the path to the file, everything else
will be the same, right?

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:63: JSONFileValueSerializer
registry(registry_path);
It's better to use base::ImportantFileWriter - it helps to avoid damaging the
file in case something goes wrong.

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:104:
(static_cast<int64>(created_time_high) << 32) + created_time_low);
I'm not sure this will always work correctly. Particularly when 32'nd bit is set
- you'll get a negative value for the low part. It's better to use double to
store time instead of pair of integers.

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

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
File remoting/host/pairing_registry_delegate_linux.cc (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:51: for
(PairingRegistry::PairedClients::const_iterator i =
This serialization code looks like it would be the same on Linux/Mac/Win. Only
difference between Linux and Mac is the choice of filename. I guess on Windows
we might store the list in the Registry somehow, but wouldn't we just save the
whole JSON string into a Registry key? If it's too big for that, we'd probably
want to save it as a file on Windows too?

Maybe just have per-platform SaveJson()/LoadJson() implementations, and keep the
rest of the code cross-platform?

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:64: registry.Serialize(root);
I don't think JSONFileValueSerializer creates the directory. If you need to
create it here, you can use file_util::CreateDirectory() which looks like it
also sets the permissions we want.

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
File remoting/host/pairing_registry_delegate_linux_unittest.cc (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux_unittest.cc:9: #include
"testing/gmock/include/gmock/gmock.h"
Remove gmock.h ?

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
File remoting/protocol/pairing_registry.cc (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
remoting/protocol/pairing_registry.cc:15: bool
PairingRegistry::Pairing::operator==(const Pairing& other) const {
nit: move this below const int kKeySize (or move constant to unnamed namespace).

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
File remoting/protocol/pairing_registry.h (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
remoting/protocol/pairing_registry.h:37: bool operator==(const Pairing& other)
const;
Is this used only in unittest? If so, consider moving it there.

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
remoting/protocol/pairing_registry.h:56: // Load the pairing information
synchronously from persistent storage.
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.

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
remoting/protocol/pairing_registry.h:75: // Return a platform-specific Delegate
that will save to permanent storage
Can this be moved to remoting/host ?

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
remoting/protocol/pairing_registry.h:96: // Temporary delegate that just logs
NOTIMPLEMENTED for Load/Save.
Can we remove this class as well?

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
File remoting/protocol/pairing_registry_unittest.cc (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
remoting/protocol/pairing_registry_unittest.cc:35: EXPECT_TRUE(false);
nit: ADD_FAILURE(); is cleaner.

https://codereview.chromium.org/15709005/diff/2001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/remoting.gyp#newc...
remoting/remoting.gyp:423: 'host/pairing_registry_delegate_linux.cc',
nit: "pairing" before "pam"

[Lambros, 2013-05-31 19:55:26]

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
File remoting/host/pairing_registry_delegate_linux.cc (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:104:
(static_cast<int64>(created_time_high) << 32) + created_time_low);
On 2013/05/31 18:45:46, Sergey Ulanov wrote:
> I'm not sure this will always work correctly. Particularly when 32'nd bit is
set
> - you'll get a negative value for the low part. It's better to use double to
> store time instead of pair of integers.  
Ah yes, I missed this! I think created_time_low/high need to be declared as
uint32.

Or serialize as decimal string using base/strings/string_number_conversions.h.

[Wez, 2013-06-01 01:34:41]

Drive-by...

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
File remoting/host/pairing_registry_delegate_linux.h (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.h:32: void
SetFilenameForTesting(const base::FilePath &filename);
nit: Add comments to explain these members, e.g. what a false return from
GetRegistryFilePath means

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.h:35:
base::WeakPtrFactory<PairingRegistryDelegateLinux> weak_factory_;
This member should be the last in the class.

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

PTAL. I think I've addressed all the comments, but this version implements a
different Delegate interface than the previous, so it's probably best to treat
it as a completely new CL.

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
File remoting/host/pairing_registry_delegate_linux.cc (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:16: const char kConfigDir[] =
".config/chrome-remote-desktop";
On 2013/05/31 18:45:46, Sergey Ulanov wrote:
> There is remoting::GetConfigDir() that returns path to the config directory.

Done.

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:17: const char
kRegistryFilename[] = "paired-clients";
On 2013/05/31 18:45:46, Sergey Ulanov wrote:
> Maybe paired-clients.json?

Done.

> For host config we add a hash of the host name to deal with the case when home
> directory is shared between multiple machines. I think it might be useful here
> too. 
> Config file path with the suffix normally comes from command line. One
possible
> solution is to replace --host-config option with --config-suffix and use the
> suffix for both files.

I think we're planning on moving to a model where the config directory is under
/var, in which case the suffix won't be needed, so I prefer not to add one.

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:43: void
PairingRegistryDelegateLinux::SaveOnCurrentThread(
On 2013/05/31 18:45:46, Sergey Ulanov wrote:
> Why is it "OnCurrentThread"? Should it be something like OnIoThread? Or maybe
> just call it DoSave()

The new interface has different functions, but I've adopted the Do* naming
convention for the implementation as you suggest.

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:51: for
(PairingRegistry::PairedClients::const_iterator i =
On 2013/05/31 18:45:46, Sergey Ulanov wrote:
> It looks to me that most of the code in this class can be shared between
> platforms. All formatting/parsing code can be shared. Is it possible to avoid
> duplication? E.g. PairingRegistry could format the data, so Save() would
accept
> already formatted base::Value and so it would just need to save it at the
right
> place.
> But I'm actually not sure why delegate needs to be platform specific. The only
> part that's differs between platforms is the path to the file, everything else
> will be the same, right?

I was concerned about permissions. For example, on Mac we can't support pairing
at all right now because the process needing write access to it might be running
as root or as the user who started the host. Similarly, on Windows, we'll
probably need a completely different implementation that works with the
multi-process architecture (based on a short discussion with Alex, it's likely
that this will be registry-based).

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:51: for
(PairingRegistry::PairedClients::const_iterator i =
On 2013/05/31 18:51:59, Lambros wrote:
> This serialization code looks like it would be the same on Linux/Mac/Win. Only
> difference between Linux and Mac is the choice of filename. I guess on Windows
> we might store the list in the Registry somehow, but wouldn't we just save the
> whole JSON string into a Registry key? If it's too big for that, we'd probably
> want to save it as a file on Windows too?
> 
> Maybe just have per-platform SaveJson()/LoadJson() implementations, and keep
the
> rest of the code cross-platform?

I don't think all platforms will use JSON. On Windows we might use the registry,
which has support for structured data.

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:63: JSONFileValueSerializer
registry(registry_path);
On 2013/05/31 18:45:46, Sergey Ulanov wrote:
> It's better to use base::ImportantFileWriter - it helps to avoid damaging the
> file in case something goes wrong.

Done.

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:64: registry.Serialize(root);
On 2013/05/31 18:51:59, Lambros wrote:
> I don't think JSONFileValueSerializer creates the directory. If you need to
> create it here, you can use file_util::CreateDirectory() which looks like it
> also sets the permissions we want.

Done.

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:104:
(static_cast<int64>(created_time_high) << 32) + created_time_low);
On 2013/05/31 19:55:26, Lambros wrote:
> On 2013/05/31 18:45:46, Sergey Ulanov wrote:
> > I'm not sure this will always work correctly. Particularly when 32'nd bit is
> set
> > - you'll get a negative value for the low part. It's better to use double to
> > store time instead of pair of integers.  
> Ah yes, I missed this! I think created_time_low/high need to be declared as
> uint32.
> 
> Or serialize as decimal string using base/strings/string_number_conversions.h.

Done.

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:104:
(static_cast<int64>(created_time_high) << 32) + created_time_low);
On 2013/05/31 18:45:46, Sergey Ulanov wrote:
> I'm not sure this will always work correctly. Particularly when 32'nd bit is
set
> - you'll get a negative value for the low part. It's better to use double to
> store time instead of pair of integers.  

I've followed Lambros's suggestion of using string_number_conversions.h.

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
File remoting/host/pairing_registry_delegate_linux.h (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.h:32: void
SetFilenameForTesting(const base::FilePath &filename);
On 2013/06/01 01:34:41, Wez wrote:
> nit: Add comments to explain these members, e.g. what a false return from
> GetRegistryFilePath means

Done.

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.h:35:
base::WeakPtrFactory<PairingRegistryDelegateLinux> weak_factory_;
On 2013/06/01 01:34:41, Wez wrote:
> This member should be the last in the class.

Done.

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
File remoting/host/pairing_registry_delegate_linux_unittest.cc (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux_unittest.cc:9: #include
"testing/gmock/include/gmock/gmock.h"
On 2013/05/31 18:51:59, Lambros wrote:
> Remove gmock.h ?

Done.

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
File remoting/protocol/pairing_registry.cc (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
remoting/protocol/pairing_registry.cc:15: bool
PairingRegistry::Pairing::operator==(const Pairing& other) const {
On 2013/05/31 18:51:59, Lambros wrote:
> nit: move this below const int kKeySize (or move constant to unnamed
namespace).

Done.

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
File remoting/protocol/pairing_registry.h (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
remoting/protocol/pairing_registry.h:37: bool operator==(const Pairing& other)
const;
On 2013/05/31 18:51:59, Lambros wrote:
> Is this used only in unittest? If so, consider moving it there.

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.

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
remoting/protocol/pairing_registry.h:56: // Load the pairing information
synchronously from persistent storage.
On 2013/05/31 18:51:59, Lambros wrote:
> 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.

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.

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
remoting/protocol/pairing_registry.h:75: // Return a platform-specific Delegate
that will save to permanent storage
On 2013/05/31 18:51:59, Lambros wrote:
> Can this be moved to remoting/host ?

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.

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
remoting/protocol/pairing_registry.h:96: // Temporary delegate that just logs
NOTIMPLEMENTED for Load/Save.
On 2013/05/31 18:51:59, Lambros wrote:
> Can we remove this class as well?

Done.

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
File remoting/protocol/pairing_registry_unittest.cc (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
remoting/protocol/pairing_registry_unittest.cc:35: EXPECT_TRUE(false);
On 2013/05/31 18:51:59, Lambros wrote:
> nit: ADD_FAILURE(); is cleaner.

This is gone in the current version.

https://codereview.chromium.org/15709005/diff/2001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/remoting.gyp#newc...
remoting/remoting.gyp:423: 'host/pairing_registry_delegate_linux.cc',
On 2013/05/31 18:51:59, Lambros wrote:
> nit: "pairing" before "pam"

Done.

[Sergey Ulanov, 2013-06-19 18:23:49]

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
File remoting/host/pairing_registry_delegate_linux.cc (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
remoting/host/pairing_registry_delegate_linux.cc:51: for
(PairingRegistry::PairedClients::const_iterator i =
On 2013/06/19 17:56:44, Jamie wrote:
> On 2013/05/31 18:51:59, Lambros wrote:
> > This serialization code looks like it would be the same on Linux/Mac/Win.
Only
> > difference between Linux and Mac is the choice of filename. I guess on
Windows
> > we might store the list in the Registry somehow, but wouldn't we just save
the
> > whole JSON string into a Registry key? If it's too big for that, we'd
probably
> > want to save it as a file on Windows too?
> > 
> > Maybe just have per-platform SaveJson()/LoadJson() implementations, and keep
> the
> > rest of the code cross-platform?
> 
> I don't think all platforms will use JSON. On Windows we might use the
registry,
> which has support for structured data.

What are the benefits of using registry instead of storing that data in the file
system on windows too?
We already use JSON file to store host config on all platforms. Similarly chrome
doesn't store settings in the registry. So I think it's better to store the
pairing data in the file system for consistency, and that will allow more code
to be reused across platforms (meaning there will be fewer bugs and it will be
easier to maintain).

[Jamie, 2013-06-19 18:39:02]

On 2013/06/19 18:23:49, Sergey Ulanov wrote:
>
https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
> File remoting/host/pairing_registry_delegate_linux.cc (right):
> 
>
https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
> remoting/host/pairing_registry_delegate_linux.cc:51: for
> (PairingRegistry::PairedClients::const_iterator i =
> On 2013/06/19 17:56:44, Jamie wrote:
> > On 2013/05/31 18:51:59, Lambros wrote:
> > > This serialization code looks like it would be the same on Linux/Mac/Win.
> Only
> > > difference between Linux and Mac is the choice of filename. I guess on
> Windows
> > > we might store the list in the Registry somehow, but wouldn't we just save
> the
> > > whole JSON string into a Registry key? If it's too big for that, we'd
> probably
> > > want to save it as a file on Windows too?
> > > 
> > > Maybe just have per-platform SaveJson()/LoadJson() implementations, and
keep
> > the
> > > rest of the code cross-platform?
> > 
> > I don't think all platforms will use JSON. On Windows we might use the
> registry,
> > which has support for structured data.
> 
> What are the benefits of using registry instead of storing that data in the
file
> system on windows too?
> We already use JSON file to store host config on all platforms. Similarly
chrome
> doesn't store settings in the registry. So I think it's better to store the
> pairing data in the file system for consistency, and that will allow more code
> to be reused across platforms (meaning there will be fewer bugs and it will be
> easier to maintain).

On Windows, the network process that needs to write the file is unprivileged,
but we want the pairing data to be secure. That can be achieved using the
registry by having the privileged daemon process pass an HKEY with appropriate
permissions to the network process. We might be able to do the same thing with
an HFILE, but I think it would be complicated to deal with changes to the file
when the web-app revokes pairings (it's less of an issue on Linux because we
reopen the file each time). I'm happy to revisit this per-platform architecture
as part of the Windows implementation, but my gut feeling is that the different
security models on the different platforms means we're going to need different
implementations here.

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

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
File remoting/protocol/pairing_registry.h (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
remoting/protocol/pairing_registry.h:75: // Return a platform-specific Delegate
that will save to permanent storage
On 2013/06/19 17:56:44, Jamie wrote:
> On 2013/05/31 18:51:59, Lambros wrote:
> > Can this be moved to remoting/host ?
> 
> 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.

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.

https://codereview.chromium.org/15709005/diff/24001/remoting/host/pairing_reg...
File remoting/host/pairing_registry_delegate_linux.cc (right):

https://codereview.chromium.org/15709005/diff/24001/remoting/host/pairing_reg...
remoting/host/pairing_registry_delegate_linux.cc:42: const
PairingRegistry::AddPairingCallback& callback) {
nit: indentation

https://codereview.chromium.org/15709005/diff/24001/remoting/host/pairing_reg...
remoting/host/pairing_registry_delegate_linux.cc:48:
base::Bind(&PairingRegistryDelegateLinux::RunAddPairingCallbackOnThread,
RunAddPairingCallbackOnThread doesn't access any members. Might as well make it
static, and then you don't need to bind it to a WeakPtr. I wonder if this could
be further simplified, since that method is just a one-liner, maybe can combine
the two Binds into one? Or look at TaskRunner::PostTaskAndReply().

https://codereview.chromium.org/15709005/diff/24001/remoting/host/pairing_reg...
remoting/host/pairing_registry_delegate_linux.cc:63: const 
PairingRegistry::GetPairingCallback& callback) {
nit: remove extra space

https://codereview.chromium.org/15709005/diff/24001/remoting/host/pairing_reg...
File remoting/host/pairing_registry_delegate_linux_unittest.cc (right):

https://codereview.chromium.org/15709005/diff/24001/remoting/host/pairing_reg...
remoting/host/pairing_registry_delegate_linux_unittest.cc:24: // Load the
pairings again to make sure we get what we've just written.
I think it would be better to delete the delegate here and create a new one.
Otherwise, a delegate could pass this test by pretending to save, then returning
its own internal cached data without actually loading it from disk.

https://codereview.chromium.org/15709005/diff/24001/remoting/host/pairing_reg...
remoting/host/pairing_registry_delegate_linux_unittest.cc:49:
file_util::Delete(temp_dir, false);
Better not to delete the temp dir here. Instead, do something like
temp_file = temp_dir.Append("xxx").Append("registry.json");

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

ptal

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
File remoting/protocol/pairing_registry.h (right):

https://codereview.chromium.org/15709005/diff/2001/remoting/protocol/pairing_...
remoting/protocol/pairing_registry.h:75: // Return a platform-specific Delegate
that will save to permanent storage
On 2013/06/20 17:59:20, Lambros wrote:
> On 2013/06/19 17:56:44, Jamie wrote:
> > On 2013/05/31 18:51:59, Lambros wrote:
> > > Can this be moved to remoting/host ?
> > 
> > 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.
> 
> 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.

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

https://codereview.chromium.org/15709005/diff/24001/remoting/host/pairing_reg...
File remoting/host/pairing_registry_delegate_linux.cc (right):

https://codereview.chromium.org/15709005/diff/24001/remoting/host/pairing_reg...
remoting/host/pairing_registry_delegate_linux.cc:42: const
PairingRegistry::AddPairingCallback& callback) {
On 2013/06/20 17:59:20, Lambros wrote:
> nit: indentation

Done.

https://codereview.chromium.org/15709005/diff/24001/remoting/host/pairing_reg...
remoting/host/pairing_registry_delegate_linux.cc:48:
base::Bind(&PairingRegistryDelegateLinux::RunAddPairingCallbackOnThread,
On 2013/06/20 17:59:20, Lambros wrote:
> RunAddPairingCallbackOnThread doesn't access any members. Might as well make
it
> static, and then you don't need to bind it to a WeakPtr. I wonder if this
could
> be further simplified, since that method is just a one-liner, maybe can
combine
> the two Binds into one? Or look at TaskRunner::PostTaskAndReply().

I'm made the methods static, but I don't see how to simplify further, since
PostTaskAndReply (and similarly any attempt to combine the Binds) requires a
closure, and we only have a partially-bound function.

https://codereview.chromium.org/15709005/diff/24001/remoting/host/pairing_reg...
remoting/host/pairing_registry_delegate_linux.cc:63: const 
PairingRegistry::GetPairingCallback& callback) {
On 2013/06/20 17:59:20, Lambros wrote:
> nit: remove extra space

Done.

https://codereview.chromium.org/15709005/diff/24001/remoting/host/pairing_reg...
File remoting/host/pairing_registry_delegate_linux_unittest.cc (right):

https://codereview.chromium.org/15709005/diff/24001/remoting/host/pairing_reg...
remoting/host/pairing_registry_delegate_linux_unittest.cc:24: // Load the
pairings again to make sure we get what we've just written.
On 2013/06/20 17:59:20, Lambros wrote:
> I think it would be better to delete the delegate here and create a new one.
> Otherwise, a delegate could pass this test by pretending to save, then
returning
> its own internal cached data without actually loading it from disk.

Done.

https://codereview.chromium.org/15709005/diff/24001/remoting/host/pairing_reg...
remoting/host/pairing_registry_delegate_linux_unittest.cc:49:
file_util::Delete(temp_dir, false);
On 2013/06/20 17:59:20, Lambros wrote:
> Better not to delete the temp dir here. Instead, do something like
> temp_file = temp_dir.Append("xxx").Append("registry.json");
> 

Done.

[Lambros, 2013-06-20 21:40:51]

LGTM with comment.

I still think some common code can be pulled out of the Linux implementation,
but it can wait for the Mac version, when  we'll know how much code they have in
common.

https://codereview.chromium.org/15709005/diff/33001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/15709005/diff/33001/remoting/remoting.gyp#new...
remoting/remoting.gyp:420: 'host/pairing_registry_delegate_linux.cc',
Add pairing_registry_delegate.h

[Sergey Ulanov, 2013-06-20 22:26:52]

On 2013/06/19 18:39:02, Jamie wrote:
> On 2013/06/19 18:23:49, Sergey Ulanov wrote:
> >
>
https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
> > File remoting/host/pairing_registry_delegate_linux.cc (right):
> > 
> >
>
https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
> > remoting/host/pairing_registry_delegate_linux.cc:51: for
> > (PairingRegistry::PairedClients::const_iterator i =
> > On 2013/06/19 17:56:44, Jamie wrote:
> > > On 2013/05/31 18:51:59, Lambros wrote:
> > > > This serialization code looks like it would be the same on
Linux/Mac/Win.
> > Only
> > > > difference between Linux and Mac is the choice of filename. I guess on
> > Windows
> > > > we might store the list in the Registry somehow, but wouldn't we just
save
> > the
> > > > whole JSON string into a Registry key? If it's too big for that, we'd
> > probably
> > > > want to save it as a file on Windows too?
> > > > 
> > > > Maybe just have per-platform SaveJson()/LoadJson() implementations, and
> keep
> > > the
> > > > rest of the code cross-platform?
> > > 
> > > I don't think all platforms will use JSON. On Windows we might use the
> > registry,
> > > which has support for structured data.
> > 
> > What are the benefits of using registry instead of storing that data in the
> file
> > system on windows too?
> > We already use JSON file to store host config on all platforms. Similarly
> chrome
> > doesn't store settings in the registry. So I think it's better to store the
> > pairing data in the file system for consistency, and that will allow more
code
> > to be reused across platforms (meaning there will be fewer bugs and it will
be
> > easier to maintain).
> 
> On Windows, the network process that needs to write the file is unprivileged,
> but we want the pairing data to be secure. That can be achieved using the
> registry by having the privileged daemon process pass an HKEY with appropriate
> permissions to the network process. We might be able to do the same thing with
> an HFILE, but I think it would be complicated to deal with changes to the file
> when the web-app revokes pairings (it's less of an issue on Linux because we
> reopen the file each time). I'm happy to revisit this per-platform
architecture
> as part of the Windows implementation, but my gut feeling is that the
different
> security models on the different platforms means we're going to need different
> implementations here.

Even if the network process can't store the data on the disk directly, I think
it's still possible to avoid duplication of all of the storage logic:
 - The data can be saved in the registry as a JSON blob
(PairingRegistry::Delegate implementation will not need to worry about
formatting/parsing it) instead of as a set of values.
 - The network process can talk to the daemon process to store/load the data -
it already does it to get updated config file. I would prefer this solution
because it avoids platform-specific details in the IPC interface, and so it can
be reused later, e.g. on Mac.

[Jamie, 2013-06-21 02:33:04]

On 2013/06/20 22:26:52, Sergey Ulanov wrote:
> On 2013/06/19 18:39:02, Jamie wrote:
> > On 2013/06/19 18:23:49, Sergey Ulanov wrote:
> > >
> >
>
https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
> > > File remoting/host/pairing_registry_delegate_linux.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/15709005/diff/2001/remoting/host/pairing_regi...
> > > remoting/host/pairing_registry_delegate_linux.cc:51: for
> > > (PairingRegistry::PairedClients::const_iterator i =
> > > On 2013/06/19 17:56:44, Jamie wrote:
> > > > On 2013/05/31 18:51:59, Lambros wrote:
> > > > > This serialization code looks like it would be the same on
> Linux/Mac/Win.
> > > Only
> > > > > difference between Linux and Mac is the choice of filename. I guess on
> > > Windows
> > > > > we might store the list in the Registry somehow, but wouldn't we just
> save
> > > the
> > > > > whole JSON string into a Registry key? If it's too big for that, we'd
> > > probably
> > > > > want to save it as a file on Windows too?
> > > > > 
> > > > > Maybe just have per-platform SaveJson()/LoadJson() implementations,
and
> > keep
> > > > the
> > > > > rest of the code cross-platform?
> > > > 
> > > > I don't think all platforms will use JSON. On Windows we might use the
> > > registry,
> > > > which has support for structured data.
> > > 
> > > What are the benefits of using registry instead of storing that data in
the
> > file
> > > system on windows too?
> > > We already use JSON file to store host config on all platforms. Similarly
> > chrome
> > > doesn't store settings in the registry. So I think it's better to store
the
> > > pairing data in the file system for consistency, and that will allow more
> code
> > > to be reused across platforms (meaning there will be fewer bugs and it
will
> be
> > > easier to maintain).
> > 
> > On Windows, the network process that needs to write the file is
unprivileged,
> > but we want the pairing data to be secure. That can be achieved using the
> > registry by having the privileged daemon process pass an HKEY with
appropriate
> > permissions to the network process. We might be able to do the same thing
with
> > an HFILE, but I think it would be complicated to deal with changes to the
file
> > when the web-app revokes pairings (it's less of an issue on Linux because we
> > reopen the file each time). I'm happy to revisit this per-platform
> architecture
> > as part of the Windows implementation, but my gut feeling is that the
> different
> > security models on the different platforms means we're going to need
different
> > implementations here.
> 
> Even if the network process can't store the data on the disk directly, I think
> it's still possible to avoid duplication of all of the storage logic:
>  - The data can be saved in the registry as a JSON blob
> (PairingRegistry::Delegate implementation will not need to worry about
> formatting/parsing it) instead of as a set of values.

I like this idea, and it fits in well with the future work that's going to be
needed to implement the Javascript show/revoke UI--that's probably going to end
up using JSON anyway.

I've reimplemented the Delegate interface to use simpler Load/Save semantics and
moved all the JSON parsing logic into PairingRegistry. WDYT?

>  - The network process can talk to the daemon process to store/load the data -
> it already does it to get updated config file. I would prefer this solution
> because it avoids platform-specific details in the IPC interface, and so it
can
> be reused later, e.g. on Mac.

Alex and I discussed this possibility. I don't like it because it involves
adding a lot of code to a security-sensitive IPC interface.

[Jamie, 2013-06-21 02:33:21]

fyi

https://codereview.chromium.org/15709005/diff/33001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/15709005/diff/33001/remoting/remoting.gyp#new...
remoting/remoting.gyp:420: 'host/pairing_registry_delegate_linux.cc',
On 2013/06/20 21:40:52, Lambros wrote:
> Add pairing_registry_delegate.h

Done.

[Sergey Ulanov, 2013-06-21 23:27:54]

LGTM.

https://codereview.chromium.org/15709005/diff/54001/remoting/host/pairing_reg...
File remoting/host/pairing_registry_delegate_linux.cc (right):

https://codereview.chromium.org/15709005/diff/54001/remoting/host/pairing_reg...
remoting/host/pairing_registry_delegate_linux.cc:103: 
nit: don't need this empty line.

https://codereview.chromium.org/15709005/diff/54001/remoting/protocol/pairing...
File remoting/protocol/pairing_registry.cc (right):

https://codereview.chromium.org/15709005/diff/54001/remoting/protocol/pairing...
remoting/protocol/pairing_registry.cc:115: PairingRegistry::DecodeJson(const
std::string& pairings_json) {
nit: normally function name is wrapped only when it doesn't fit on one line with
the type, so more conventional formatting for this function would be

PairingRegistry::PairedClients PairingRegistry::DecodeJson(
    const std::string& pairings_json) {

[Jamie, 2013-06-21 23:33:25]

fyi

https://codereview.chromium.org/15709005/diff/54001/remoting/host/pairing_reg...
File remoting/host/pairing_registry_delegate_linux.cc (right):

https://codereview.chromium.org/15709005/diff/54001/remoting/host/pairing_reg...
remoting/host/pairing_registry_delegate_linux.cc:103: 
On 2013/06/21 23:27:55, Sergey Ulanov wrote:
> nit: don't need this empty line.

Done.

https://codereview.chromium.org/15709005/diff/54001/remoting/protocol/pairing...
File remoting/protocol/pairing_registry.cc (right):

https://codereview.chromium.org/15709005/diff/54001/remoting/protocol/pairing...
remoting/protocol/pairing_registry.cc:115: PairingRegistry::DecodeJson(const
std::string& pairings_json) {
On 2013/06/21 23:27:55, Sergey Ulanov wrote:
> nit: normally function name is wrapped only when it doesn't fit on one line
with
> the type, so more conventional formatting for this function would be
> 
> PairingRegistry::PairedClients PairingRegistry::DecodeJson(
>     const std::string& pairings_json) {

Done.

[commit-bot: I haz the power, 2013-06-21 23:33:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/15709005/66001

[commit-bot: I haz the power, 2013-06-21 23:33:42]

Failed to apply patch for remoting/host/remoting_me2me_host.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file remoting/host/remoting_me2me_host.cc
  Hunk #1 FAILED at 58.
  1 out of 2 hunks FAILED -- saving rejects to file
remoting/host/remoting_me2me_host.cc.rej

Patch:       remoting/host/remoting_me2me_host.cc
Index: remoting/host/remoting_me2me_host.cc
diff --git a/remoting/host/remoting_me2me_host.cc
b/remoting/host/remoting_me2me_host.cc
index
252390ab7f997413a65a0df8fde59a859ae06232..8775bfc776c4c7a6c57376916ca20fece146e4d4
100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -58,6 +58,7 @@
 #include "remoting/host/logging.h"
 #include "remoting/host/me2me_desktop_environment.h"
 #include "remoting/host/network_settings.h"
+#include "remoting/host/pairing_registry_delegate.h"
 #include "remoting/host/policy_hack/policy_watcher.h"
 #include "remoting/host/service_urls.h"
 #include "remoting/host/session_manager_factory.h"
@@ -478,12 +479,9 @@ void HostProcess::CreateAuthenticatorFactory() {
 
   // TODO(jamiewalch): Add a pairing registry here once all the code
   // is committed.
-  scoped_refptr<remoting::protocol::PairingRegistry> pairing_registry;
-  //scoped_refptr<protocol::PairingRegistry> pairing_registry(
-  //    new protocol::PairingRegistry(
-  //        scoped_ptr<protocol::PairingRegistry::Delegate>(
-  //            new protocol::NotImplementedPairingRegistryDelegate),
-  //        protocol::PairingRegistry::PairedClients()));
+  scoped_refptr<remoting::protocol::PairingRegistry> pairing_registry(
+      new remoting::protocol::PairingRegistry(
+          CreatePairingRegistryDelegate(NULL)));
 
   scoped_ptr<protocol::AuthenticatorFactory> factory;

[commit-bot: I haz the power, 2013-06-22 00:03:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/15709005/63002

[commit-bot: I haz the power, 2013-06-22 00:47:50]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-22 00:58:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/15709005/58003

[commit-bot: I haz the power, 2013-06-22 02:29:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/15709005/58003

[commit-bot: I haz the power, 2013-06-22 03:25:20]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, chromedriver2_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, nacl_integration, net_unittests, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-06-22 17:48:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/15709005/58003

[commit-bot: I haz the power, 2013-06-22 18:19:52]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-23 05:47:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/15709005/90001

[commit-bot: I haz the power, 2013-06-23 14:10:31]

Change committed as 208123