[Chromoting] Implement UpdateConfig() for the Windows host.

[Chromoting] Implement UpdateConfig() for the Windows host.

BUG=121539
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132598

[simonmorris, 2012-04-16 22:15:15]

ptal

[Jamie, 2012-04-16 22:26:48]

I think we'll want to use FilePathWatcher on all platforms eventually, but until
we do, I wonder if we'd be better off with that stuff #ifdef'd out for now?

http://codereview.chromium.org/10103010/diff/1/remoting/host/elevated_control...
File remoting/host/elevated_controller_win.cc (right):

http://codereview.chromium.org/10103010/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:342: if (config_dict->HasKey(kHostId)
|| config_dict->HasKey(kXmppLogin)) {
This is probably not a discussion for this CL (as it's a cross-platform issue),
but might we be better off having a whitelist of things you are allowed to
update (basically just the PIN, I think), rather than a blacklist?

http://codereview.chromium.org/10103010/diff/1/remoting/host/remoting_me2me_h...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10103010/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:127: class ConfigChangedDelegate : public
base::files::FilePathWatcher::Delegate {
Neat!

[alexeypa (please no reviews), 2012-04-16 22:36:34]

http://codereview.chromium.org/10103010/diff/1/remoting/host/elevated_control...
File remoting/host/elevated_controller_resource.h (right):

http://codereview.chromium.org/10103010/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_resource.h:6: #define
REMOTING_HOST_ELEVATED_CONTROLLER_RESOURCE_H 1
nit: I see this pattern in other header files:

#ifndef XXX_H_
#define XXX_H_
#endif  // XXX_H_

I.e. the guard should end with '_' and #define should not assign a value to the
define.

http://codereview.chromium.org/10103010/diff/1/remoting/host/elevated_control...
File remoting/host/elevated_controller_win.cc (right):

http://codereview.chromium.org/10103010/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:341: // The config must not contain
host_id or xmpp_login.
It is better to have an explicit list of banned keys so it would be easier to
add more keys later.

http://codereview.chromium.org/10103010/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:342: if (config_dict->HasKey(kHostId)
|| config_dict->HasKey(kXmppLogin)) {
On 2012/04/16 22:26:48, Jamie wrote:
> This is probably not a discussion for this CL (as it's a cross-platform
issue),
> but might we be better off having a whitelist of things you are allowed to
> update (basically just the PIN, I think), rather than a blacklist?

The last time we discussed this with sergeyu@ the conclusion was to blacklist
rather than to whitelist.

http://codereview.chromium.org/10103010/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:350: config_old.reset(new
base::DictionaryValue());
I don't think we should allow to update a non-existing config. It will result in
invalid config anyway.

Also not ignoring the error from ReadConfig will allow to get rid of
CreateDirectory call below.

[simonmorris, 2012-04-16 23:18:33]

On 2012/04/16 22:26:48, Jamie wrote:
> I think we'll want to use FilePathWatcher on all platforms eventually, but
until
> we do, I wonder if we'd be better off with that stuff #ifdef'd out for now?
> 

Done.

[simonmorris, 2012-04-16 23:20:23]

http://codereview.chromium.org/10103010/diff/1/remoting/host/elevated_control...
File remoting/host/elevated_controller_resource.h (right):

http://codereview.chromium.org/10103010/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_resource.h:6: #define
REMOTING_HOST_ELEVATED_CONTROLLER_RESOURCE_H 1
On 2012/04/16 22:36:35, alexeypa wrote:
> nit: I see this pattern in other header files:
> 
> #ifndef XXX_H_
> #define XXX_H_
> #endif  // XXX_H_
> 
> I.e. the guard should end with '_' and #define should not assign a value to
the
> define.

Done.

http://codereview.chromium.org/10103010/diff/1/remoting/host/elevated_control...
File remoting/host/elevated_controller_win.cc (right):

http://codereview.chromium.org/10103010/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:341: // The config must not contain
host_id or xmpp_login.
On 2012/04/16 22:36:35, alexeypa wrote:
> It is better to have an explicit list of banned keys so it would be easier to
> add more keys later.

Done.

http://codereview.chromium.org/10103010/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:350: config_old.reset(new
base::DictionaryValue());
On 2012/04/16 22:36:35, alexeypa wrote:
> I don't think we should allow to update a non-existing config. It will result
in
> invalid config anyway.
> 
> Also not ignoring the error from ReadConfig will allow to get rid of
> CreateDirectory call below.

Done.

[alexeypa (please no reviews), 2012-04-16 23:28:36]

LGTM with a couple of nits.

http://codereview.chromium.org/10103010/diff/4002/remoting/host/elevated_cont...
File remoting/host/elevated_controller_win.cc (right):

http://codereview.chromium.org/10103010/diff/4002/remoting/host/elevated_cont...
remoting/host/elevated_controller_win.cc:48: const char* kNonUpdateableKeys[] =
{ kHostId, kXmppLogin };
nit: kReadonlyKeys maybe?

http://codereview.chromium.org/10103010/diff/4002/remoting/host/elevated_cont...
remoting/host/elevated_controller_win.cc:347: return E_FAIL;
nit: I think returning HRESULT_FROM_WIN32(ERROR_ACCESS_DENIED) is slightly
better in this case.

[Sergey Ulanov, 2012-04-16 23:36:24]

http://codereview.chromium.org/10103010/diff/4002/remoting/host/remoting_me2m...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10103010/diff/4002/remoting/host/remoting_me2m...
remoting/host/remoting_me2me_host.cc:134: host_process_->ConfigUpdated();
It might be better to wait for a couple of seconds before reading the file to
make sure we don't try to read it while it is being written (use
base::DelayTimer for that).

[simonmorris, 2012-04-16 23:38:46]

fyi

http://codereview.chromium.org/10103010/diff/4002/remoting/host/elevated_cont...
File remoting/host/elevated_controller_win.cc (right):

http://codereview.chromium.org/10103010/diff/4002/remoting/host/elevated_cont...
remoting/host/elevated_controller_win.cc:48: const char* kNonUpdateableKeys[] =
{ kHostId, kXmppLogin };
On 2012/04/16 23:28:36, alexeypa wrote:
> nit: kReadonlyKeys maybe?

Done.

http://codereview.chromium.org/10103010/diff/4002/remoting/host/elevated_cont...
remoting/host/elevated_controller_win.cc:347: return E_FAIL;
On 2012/04/16 23:28:36, alexeypa wrote:
> nit: I think returning HRESULT_FROM_WIN32(ERROR_ACCESS_DENIED) is slightly
> better in this case.

Done.

[simonmorris, 2012-04-17 00:04:55]

http://codereview.chromium.org/10103010/diff/4002/remoting/host/remoting_me2m...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10103010/diff/4002/remoting/host/remoting_me2m...
remoting/host/remoting_me2me_host.cc:134: host_process_->ConfigUpdated();
On 2012/04/16 23:36:24, sergeyu wrote:
> It might be better to wait for a couple of seconds before reading the file to
> make sure we don't try to read it while it is being written (use
> base::DelayTimer for that).

Done, but please check the code.

[alexeypa (please no reviews), 2012-04-17 00:10:33]

http://codereview.chromium.org/10103010/diff/7010/remoting/host/remoting_me2m...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10103010/diff/7010/remoting/host/remoting_me2m...
remoting/host/remoting_me2me_host.cc:192: base::Unretained(this)));
Should it stop listening to config file changes (and cancel the timer) before
exiting Run()?

http://codereview.chromium.org/10103010/diff/7010/remoting/host/remoting_me2m...
remoting/host/remoting_me2me_host.cc:395:
scoped_ptr<base::DelayTimer<HostProcess>> config_updated_timer_;
There should be a space between > >. Otherwise it will not compile on older
compilers such as VS 2008.

[simonmorris, 2012-04-17 00:34:02]

http://codereview.chromium.org/10103010/diff/7010/remoting/host/remoting_me2m...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10103010/diff/7010/remoting/host/remoting_me2m...
remoting/host/remoting_me2me_host.cc:192: base::Unretained(this)));
On 2012/04/17 00:10:34, alexeypa wrote:
> Should it stop listening to config file changes (and cancel the timer) before
> exiting Run()?

I don't see a reason it needs to.

http://codereview.chromium.org/10103010/diff/7010/remoting/host/remoting_me2m...
remoting/host/remoting_me2me_host.cc:395:
scoped_ptr<base::DelayTimer<HostProcess>> config_updated_timer_;
On 2012/04/17 00:10:34, alexeypa wrote:
> There should be a space between > >. Otherwise it will not compile on older
> compilers such as VS 2008.

Done.

[Sergey Ulanov, 2012-04-17 05:42:18]

LGTM, but see my nits.

http://codereview.chromium.org/10103010/diff/5003/remoting/host/elevated_cont...
File remoting/host/elevated_controller_win.cc (right):

http://codereview.chromium.org/10103010/diff/5003/remoting/host/elevated_cont...
remoting/host/elevated_controller_win.cc:48: const char* kReadonlyKeys[] = {
kHostId, kXmppLogin };
nit: const char* const kReadonlyKeys[]

http://codereview.chromium.org/10103010/diff/5003/remoting/host/elevated_cont...
remoting/host/elevated_controller_win.cc:346: if
(config_dict->HasKey(kReadonlyKeys[i])) {
Not sure if this is really necessary to verify this here - HostScriptableObject
should check it. Or is this to make sure that the COM interface is not abused by
some other malicious  code?

http://codereview.chromium.org/10103010/diff/5003/remoting/host/plugin/daemon...
File remoting/host/plugin/daemon_controller_win.cc (right):

http://codereview.chromium.org/10103010/diff/5003/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_controller_win.cc:217: 
nit: don't need this empty line.

http://codereview.chromium.org/10103010/diff/5003/remoting/host/remoting_me2m...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10103010/diff/5003/remoting/host/remoting_me2m...
remoting/host/remoting_me2me_host.cc:92: config_updated_timer_.reset(new
base::DelayTimer<HostProcess>(
Add a comment here or in OnFilePathChanged() to explain what we need this delay
for.

[alexeypa (please no reviews), 2012-04-17 05:51:54]

LGTM as well

http://codereview.chromium.org/10103010/diff/5003/remoting/host/elevated_cont...
File remoting/host/elevated_controller_win.cc (right):

http://codereview.chromium.org/10103010/diff/5003/remoting/host/elevated_cont...
remoting/host/elevated_controller_win.cc:346: if
(config_dict->HasKey(kReadonlyKeys[i])) {
On 2012/04/17 05:42:18, sergeyu wrote:
> this to make sure that the COM interface is not abused by
> some other malicious  code?

Yes, pretty much. This check has to be on the more privileged side to make sure
it is secure.

[simonmorris, 2012-04-17 15:55:47]

http://codereview.chromium.org/10103010/diff/5003/remoting/host/elevated_cont...
File remoting/host/elevated_controller_win.cc (right):

http://codereview.chromium.org/10103010/diff/5003/remoting/host/elevated_cont...
remoting/host/elevated_controller_win.cc:48: const char* kReadonlyKeys[] = {
kHostId, kXmppLogin };
On 2012/04/17 05:42:18, sergeyu wrote:
> nit: const char* const kReadonlyKeys[]

Done.

http://codereview.chromium.org/10103010/diff/5003/remoting/host/plugin/daemon...
File remoting/host/plugin/daemon_controller_win.cc (right):

http://codereview.chromium.org/10103010/diff/5003/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_controller_win.cc:217: 
On 2012/04/17 05:42:18, sergeyu wrote:
> nit: don't need this empty line.

Done.

http://codereview.chromium.org/10103010/diff/5003/remoting/host/remoting_me2m...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10103010/diff/5003/remoting/host/remoting_me2m...
remoting/host/remoting_me2me_host.cc:92: config_updated_timer_.reset(new
base::DelayTimer<HostProcess>(
On 2012/04/17 05:42:18, sergeyu wrote:
> Add a comment here or in OnFilePathChanged() to explain what we need this
delay
> for.

Done.

[commit-bot: I haz the power, 2012-04-17 16:25:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10103010/1019

[commit-bot: I haz the power, 2012-04-17 17:46:02]

Change committed as 132598