Basic user access check for chromoting host.

Basic user access check for chromoting host.
BUG=53984
TEST=unittests

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

[awong, 2010-09-01 01:45:39]

Yay!!!!

Comments inline.

http://codereview.chromium.org/3303001/diff/2001/3001
File remoting/host/access_verifier.cc (right):

http://codereview.chromium.org/3303001/diff/2001/3001#newcode15
remoting/host/access_verifier.cc:15: if
(!config->GetString(kXmppLoginConfigPath, &host_jid_)) {
Should we just check fail this?

If not, can we add an initialized flag and make Verify fail below?

Also, should we make sure that host_jid is not 0-lengthed?

http://codereview.chromium.org/3303001/diff/2001/3001#newcode25
remoting/host/access_verifier.cc:25: return client_jid.substr(0,
host_jid_.length() + 1) == (host_jid_ + '/');
Should we just store host_jid_ to be host_jid+ "/"?

I think there's an StartsWith function in string_util.h that could be useful.

http://codereview.chromium.org/3303001/diff/2001/3002
File remoting/host/access_verifier.h (right):

http://codereview.chromium.org/3303001/diff/2001/3002#newcode18
remoting/host/access_verifier.h:18: // TODO(sergeyu): AccessVerifier should
query directory to verify user
Add a newline before the TODO so it is easier to see.

http://codereview.chromium.org/3303001/diff/2001/3002#newcode24
remoting/host/access_verifier.h:24: bool Verify(const std::string client_jid);
use reference?

http://codereview.chromium.org/3303001/diff/2001/3003
File remoting/host/access_verifier_unittest.cc (right):

http://codereview.chromium.org/3303001/diff/2001/3003#newcode41
remoting/host/access_verifier_unittest.cc:41: target.Init(config_);
Yeah, using a CHECK seems better if we are never to going to test ath Init
fails.

http://codereview.chromium.org/3303001/diff/2001/3003#newcode47
remoting/host/access_verifier_unittest.cc:47:
EXPECT_FALSE(target.Verify("host@domain.co/saf"));
Can we add one test for host@domain.com.other/blah  or something where the
host's bare jid is a substring of the another bare jid?

http://codereview.chromium.org/3303001/diff/2001/3008
File remoting/host/in_memory_host_config.cc (right):

http://codereview.chromium.org/3303001/diff/2001/3008#newcode24
remoting/host/in_memory_host_config.cc:24: AutoLock auto_lock(lock_);
...I don't get why this is locked...

http://codereview.chromium.org/3303001/diff/2001/3008#newcode32
remoting/host/in_memory_host_config.cc:32: lock_.AssertAcquired();
This seems weird...if this is a public interface, how is the lock going to be
acquired?

http://codereview.chromium.org/3303001/diff/2001/3009
File remoting/host/in_memory_host_config.h (right):

http://codereview.chromium.org/3303001/diff/2001/3009#newcode21
remoting/host/in_memory_host_config.h:21: // In-memory host config. Used by
unittests.
Place this in a host/testing directory?

[Sergey Ulanov, 2010-09-01 04:57:47]

http://codereview.chromium.org/3303001/diff/2001/3001
File remoting/host/access_verifier.cc (right):

http://codereview.chromium.org/3303001/diff/2001/3001#newcode15
remoting/host/access_verifier.cc:15: if
(!config->GetString(kXmppLoginConfigPath, &host_jid_)) {
On 2010/09/01 01:45:39, awong wrote:
> Should we just check fail this?
IMO no. The config may be invalid, and host shouldn't just crash in this case.

> If not, can we add an initialized flag and make Verify fail below?
> 
> Also, should we make sure that host_jid is not 0-lengthed?
Done and done.

http://codereview.chromium.org/3303001/diff/2001/3001#newcode25
remoting/host/access_verifier.cc:25: return client_jid.substr(0,
host_jid_.length() + 1) == (host_jid_ + '/');
On 2010/09/01 01:45:39, awong wrote:
> Should we just store host_jid_ to be host_jid+ "/"?
Done. Also renamed it to host_jid_prefix_ to show that it is not really a jid.

> 
> I think there's an StartsWith function in string_util.h that could be useful.
Yes, just what i was looking for, thanks!

http://codereview.chromium.org/3303001/diff/2001/3002
File remoting/host/access_verifier.h (right):

http://codereview.chromium.org/3303001/diff/2001/3002#newcode18
remoting/host/access_verifier.h:18: // TODO(sergeyu): AccessVerifier should
query directory to verify user
On 2010/09/01 01:45:39, awong wrote:
> Add a newline before the TODO so it is easier to see.

Done.

http://codereview.chromium.org/3303001/diff/2001/3002#newcode24
remoting/host/access_verifier.h:24: bool Verify(const std::string client_jid);
On 2010/09/01 01:45:39, awong wrote:
> use reference?

Done.

http://codereview.chromium.org/3303001/diff/2001/3003
File remoting/host/access_verifier_unittest.cc (right):

http://codereview.chromium.org/3303001/diff/2001/3003#newcode41
remoting/host/access_verifier_unittest.cc:41: target.Init(config_);
On 2010/09/01 01:45:39, awong wrote:
> Yeah, using a CHECK seems better if we are never to going to test ath Init
> fails.
Added check for Init results

http://codereview.chromium.org/3303001/diff/2001/3003#newcode47
remoting/host/access_verifier_unittest.cc:47:
EXPECT_FALSE(target.Verify("host@domain.co/saf"));
On 2010/09/01 01:45:39, awong wrote:
> Can we add one test for host@domain.com.other/blah  or something where the
> host's bare jid is a substring of the another bare jid?

Done.

http://codereview.chromium.org/3303001/diff/2001/3008
File remoting/host/in_memory_host_config.cc (right):

http://codereview.chromium.org/3303001/diff/2001/3008#newcode24
remoting/host/in_memory_host_config.cc:24: AutoLock auto_lock(lock_);
On 2010/09/01 01:45:39, awong wrote:
> ...I don't get why this is locked...
Because HostConfig is thread-safe.

http://codereview.chromium.org/3303001/diff/2001/3008#newcode32
remoting/host/in_memory_host_config.cc:32: lock_.AssertAcquired();
On 2010/09/01 01:45:39, awong wrote:
> This seems weird...if this is a public interface, how is the lock going to be
> acquired?
Ah. This method must only be called from the task passed to Update(). It is
implemented this way to guarantee atomicity of config updates (in case you need
to change multiple values). I know it looks weird, I should probably simplify
this interface.

http://codereview.chromium.org/3303001/diff/2001/3009
File remoting/host/in_memory_host_config.h (right):

http://codereview.chromium.org/3303001/diff/2001/3009#newcode21
remoting/host/in_memory_host_config.h:21: // In-memory host config. Used by
unittests.
On 2010/09/01 01:45:39, awong wrote:
> Place this in a host/testing directory?
This doesn't seem to be a convention in chromium: tests code is usually placed
next to the code that is being tested. Am I wrong?

[awong, 2010-09-01 15:43:52]

http://codereview.chromium.org/3303001/diff/2001/3008
File remoting/host/in_memory_host_config.cc (right):

http://codereview.chromium.org/3303001/diff/2001/3008#newcode22
remoting/host/in_memory_host_config.cc:22: void InMemoryHostConfig::Update(Task*
task) {
rename task -> done

http://codereview.chromium.org/3303001/diff/2001/3008#newcode24
remoting/host/in_memory_host_config.cc:24: AutoLock auto_lock(lock_);
On 2010/09/01 04:57:48, sergeyu wrote:
> On 2010/09/01 01:45:39, awong wrote:
> > ...I don't get why this is locked...
> Because HostConfig is thread-safe.
> 

You're only accessing the parameter though.  It's unclear who else might also be
calling task...

http://codereview.chromium.org/3303001/diff/2001/3008#newcode32
remoting/host/in_memory_host_config.cc:32: lock_.AssertAcquired();
On 2010/09/01 04:57:48, sergeyu wrote:
> On 2010/09/01 01:45:39, awong wrote:
> > This seems weird...if this is a public interface, how is the lock going to
be
> > acquired?
> Ah. This method must only be called from the task passed to Update(). It is
> implemented this way to guarantee atomicity of config updates (in case you
need
> to change multiple values). I know it looks weird, I should probably simplify
> this interface.

Ah, I see what is happening.

I think this API does need a little work.  Can you at least add a TODO to
address this and the previous comment?

http://codereview.chromium.org/3303001/diff/2001/3009
File remoting/host/in_memory_host_config.h (right):

http://codereview.chromium.org/3303001/diff/2001/3009#newcode21
remoting/host/in_memory_host_config.h:21: // In-memory host config. Used by
unittests.
On 2010/09/01 04:57:48, sergeyu wrote:
> On 2010/09/01 01:45:39, awong wrote:
> > Place this in a host/testing directory?
> This doesn't seem to be a convention in chromium: tests code is usually placed
> next to the code that is being tested. Am I wrong?
You're right. I'm getting my Java conventions mixed up with this.

http://codereview.chromium.org/3303001/diff/11001/12001
File remoting/host/access_verifier.cc (right):

http://codereview.chromium.org/3303001/diff/11001/12001#newcode33
remoting/host/access_verifier.cc:33: DCHECK(initialized_);
In this case, I think a full-on if statement is better.  Since the normal code
path does not assert that initialization has to succeed, we should be
conservative and fail if the verification if the object was not correctly
initialized.

http://codereview.chromium.org/3303001/diff/11001/12002
File remoting/host/access_verifier.h (right):

http://codereview.chromium.org/3303001/diff/11001/12002#newcode18
remoting/host/access_verifier.h:18: 
Add a // at the beginning of the line. :P

http://codereview.chromium.org/3303001/diff/11001/12002#newcode25
remoting/host/access_verifier.h:25: bool Verify(const std::string& client_jid);
Thinking more, can we rename Verify() to  IsAuthorized(), HasAccess(), or
something similar?

http://codereview.chromium.org/3303001/diff/11001/12003
File remoting/host/access_verifier_unittest.cc (right):

http://codereview.chromium.org/3303001/diff/11001/12003#newcode33
remoting/host/access_verifier_unittest.cc:33: scoped_refptr<TestConfigUpdater>
config_updater(new TestConfigUpdater());
BTW, why can't we just call config->SetString directly?

[Sergey Ulanov, 2010-09-01 18:00:09]

http://codereview.chromium.org/3303001/diff/2001/3008
File remoting/host/in_memory_host_config.cc (right):

http://codereview.chromium.org/3303001/diff/2001/3008#newcode22
remoting/host/in_memory_host_config.cc:22: void InMemoryHostConfig::Update(Task*
task) {
On 2010/09/01 15:43:52, awong wrote:
> rename task -> done
No, this task is not to notify when update is finished.

http://codereview.chromium.org/3303001/diff/2001/3008#newcode24
remoting/host/in_memory_host_config.cc:24: AutoLock auto_lock(lock_);
On 2010/09/01 15:43:52, awong wrote:
> On 2010/09/01 04:57:48, sergeyu wrote:
> > On 2010/09/01 01:45:39, awong wrote:
> > > ...I don't get why this is locked...
> > Because HostConfig is thread-safe.
> > 
> 
> You're only accessing the parameter though.  It's unclear who else might also
be
> calling task...
Please read comments in host_config.h, particularly for the MutableHostConfig
interface.

http://codereview.chromium.org/3303001/diff/2001/3008#newcode32
remoting/host/in_memory_host_config.cc:32: lock_.AssertAcquired();
On 2010/09/01 15:43:52, awong wrote:
> On 2010/09/01 04:57:48, sergeyu wrote:
> > On 2010/09/01 01:45:39, awong wrote:
> > > This seems weird...if this is a public interface, how is the lock going to
> be
> > > acquired?
> > Ah. This method must only be called from the task passed to Update(). It is
> > implemented this way to guarantee atomicity of config updates (in case you
> need
> > to change multiple values). I know it looks weird, I should probably
simplify
> > this interface.
> 
> Ah, I see what is happening.
> 
> I think this API does need a little work.  Can you at least add a TODO to
> address this and the previous comment?

added TODO.

http://codereview.chromium.org/3303001/diff/11001/12001
File remoting/host/access_verifier.cc (right):

http://codereview.chromium.org/3303001/diff/11001/12001#newcode33
remoting/host/access_verifier.cc:33: DCHECK(initialized_);
On 2010/09/01 15:43:53, awong wrote:
> In this case, I think a full-on if statement is better.  Since the normal code
> path does not assert that initialization has to succeed, we should be
> conservative and fail if the verification if the object was not correctly
> initialized.
If the object wasn't initialized then Verify should never be called and this
DCHECK checks this.

http://codereview.chromium.org/3303001/diff/11001/12002
File remoting/host/access_verifier.h (right):

http://codereview.chromium.org/3303001/diff/11001/12002#newcode18
remoting/host/access_verifier.h:18: 
On 2010/09/01 15:43:53, awong wrote:
> Add a // at the beginning of the line. :P

Done.

http://codereview.chromium.org/3303001/diff/11001/12002#newcode25
remoting/host/access_verifier.h:25: bool Verify(const std::string& client_jid);
On 2010/09/01 15:43:53, awong wrote:
> Thinking more, can we rename Verify() to  IsAuthorized(), HasAccess(), or
> something similar?
In future it will be asynchronous, so it must be a verb. Renamed it to
VerifyPermissions().

http://codereview.chromium.org/3303001/diff/11001/12003
File remoting/host/access_verifier_unittest.cc (right):

http://codereview.chromium.org/3303001/diff/11001/12003#newcode33
remoting/host/access_verifier_unittest.cc:33: scoped_refptr<TestConfigUpdater>
config_updater(new TestConfigUpdater());
On 2010/09/01 15:43:53, awong wrote:
> BTW, why can't we just call config->SetString directly?
Because that is not the way MustableHostConfig interface works.

[awong, 2010-09-01 18:13:18]

LGTM

FYI, the MutableHostConfig interface still seems pretty odd to me...

http://codereview.chromium.org/3303001/diff/11001/12001
File remoting/host/access_verifier.cc (right):

http://codereview.chromium.org/3303001/diff/11001/12001#newcode33
remoting/host/access_verifier.cc:33: DCHECK(initialized_);
On 2010/09/01 18:00:09, sergeyu wrote:
> On 2010/09/01 15:43:53, awong wrote:
> > In this case, I think a full-on if statement is better.  Since the normal
code
> > path does not assert that initialization has to succeed, we should be
> > conservative and fail if the verification if the object was not correctly
> > initialized.
> If the object wasn't initialized then Verify should never be called and this
> DCHECK checks this.

I agree, that it shouldn't be called, but it'd rather have a check that isn't
disabled in release if we code in a bug.  Change to CHECK?

[Sergey Ulanov, 2010-09-01 18:20:48]

http://codereview.chromium.org/3303001/diff/11001/12001
File remoting/host/access_verifier.cc (right):

http://codereview.chromium.org/3303001/diff/11001/12001#newcode33
remoting/host/access_verifier.cc:33: DCHECK(initialized_);
On 2010/09/01 18:13:18, awong wrote:
> On 2010/09/01 18:00:09, sergeyu wrote:
> > On 2010/09/01 15:43:53, awong wrote:
> > > In this case, I think a full-on if statement is better.  Since the normal
> code
> > > path does not assert that initialization has to succeed, we should be
> > > conservative and fail if the verification if the object was not correctly
> > > initialized.
> > If the object wasn't initialized then Verify should never be called and this
> > DCHECK checks this.
> 
> I agree, that it shouldn't be called, but it'd rather have a check that isn't
> disabled in release if we code in a bug.  Change to CHECK?

Done.