[Draft] Fix canceling pin prompt causes host overload

Cause:
To prevent a malicious client from guessing the PIN by spamming the host with bogus logins, the chromoting host can throttle incoming requests after too many unsuccessful login attempts.  In the current implementation, every time when there is an incoming request, we start incrementing the bad login counter, regardless of whether the host has actually starts authenticating.

Fix:
This change adds an extra flag on the authenticator to indicate whether authentication has started.
The JingleSession checks the flag and progagates the message back all the way up to the host through the callback  Session::OnSessionAuthenticationBegin

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

[kelvinp, 2014-03-21 22:27:40]

Hi Sergey,

Can you help me to take a look and see if you have feedbacks?

I have done the following testing on my code
1. Run all UT
2. Login using a paired client with invalid secrets multiple times, expect host
overload
3. Login using invalid pin multiple times, expect host overload
4. Cancelling at pin prompt multiple times, expect no host overload.

This is my first review in the host area, so I won't be surprised if there are
lot of feedback. Feel free to let me know how I can make the code better. Thank
you for your help

Kelvin

[kelvinp, 2014-03-21 22:32:05]

Hi Sergey,

Can you help me to take a look and see if you have comments?

I have done the following testing on my code
1. Run all UT
2. Login using a paired client with invalid secrets multiple times, expect host
overload
3. Login using invalid pin multiple times, expect host overload
4. Cancelling at pin prompt multiple times, expect no host overload.

This is my first review in the host area, so I won't be surprised if there are
lot of feedback. Feel free to let me know how I can make the code better. I will
send one out to the team once you think this change is ready for review.

Thank
you for your help

Kelvin

[Sergey Ulanov, 2014-03-24 18:42:36]

Looks mostly good. I suggest adding a state in protocol::Session instead of
another callback method.

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/authen...
File remoting/protocol/authenticator.h (right):

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/authen...
remoting/protocol/authenticator.h:92: // Whether authentication has started
Please mention how this value should be used. E.g. that host can use it for the
backoff.

Also comments should end with '.'

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/authen...
remoting/protocol/authenticator.h:93: virtual bool has_auth_started() const = 0;
call it started()?

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/jingle...
File remoting/protocol/jingle_session.cc (left):

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:498: 
nit: keep this empty line

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:651: 
nit: keep this line

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/session.h
File remoting/protocol/session.h (right):

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/sessio...
remoting/protocol/session.h:74: virtual void OnSessionAuthenticationBegin() {};
Instead of adding this callback I think it would be cleaner to have another
state between CONNECTED and AUTHENTICATED.

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/sessio...
remoting/protocol/session.h:74: virtual void OnSessionAuthenticationBegin() {};
If you keep this method: s/Begin/Started/ for consistency.

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/third_...
File remoting/protocol/third_party_authenticator_base.cc (right):

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/third_...
remoting/protocol/third_party_authenticator_base.cc:40: return false;
Third-party auth is started as soon as a message is sent to the peer.
underlying_ state is irrelevant here. So I think you need a started_ field
that's set to true when AddTokenElements() is called in GetNextMessage().

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/v2_aut...
File remoting/protocol/v2_authenticator.h (right):

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/v2_aut...
remoting/protocol/v2_authenticator.h:47: virtual bool has_auth_started() const
OVERRIDE;
Please keep the same method order as in the interface definition.

[kelvinp, 2014-03-24 23:11:38]

Addressed all of Sergey's feedback.  I agreed that having another state makes
the code cleaner.  The logic in jingle_session.cc has a lot of if statements
based on the state, which makes my change risky.  I have made sure the major
scenarios (PIN, paired client, invalid PIN) still works with the extra state. 
Please take another look to make sure I didn't miss anything.

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/authen...
File remoting/protocol/authenticator.h (right):

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/authen...
remoting/protocol/authenticator.h:92: // Whether authentication has started
On 2014/03/24 18:42:36, Sergey Ulanov wrote:
> Please mention how this value should be used. E.g. that host can use it for
the
> backoff.
> 
> Also comments should end with '.'

Done.

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/authen...
remoting/protocol/authenticator.h:93: virtual bool has_auth_started() const = 0;
On 2014/03/24 18:42:36, Sergey Ulanov wrote:
> call it started()?

Done.

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/jingle...
File remoting/protocol/jingle_session.cc (left):

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:498: 
On 2014/03/24 18:42:36, Sergey Ulanov wrote:
> nit: keep this empty line

Done.

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:651: 
On 2014/03/24 18:42:36, Sergey Ulanov wrote:
> nit: keep this line

Done.

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/session.h
File remoting/protocol/session.h (right):

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/sessio...
remoting/protocol/session.h:74: virtual void OnSessionAuthenticationBegin() {};
On 2014/03/24 18:42:36, Sergey Ulanov wrote:
> Instead of adding this callback I think it would be cleaner to have another
> state between CONNECTED and AUTHENTICATED.

Done.

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/sessio...
remoting/protocol/session.h:74: virtual void OnSessionAuthenticationBegin() {};
On 2014/03/24 18:42:36, Sergey Ulanov wrote:
> If you keep this method: s/Begin/Started/ for consistency.

I have added the extra state instead

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/third_...
File remoting/protocol/third_party_authenticator_base.cc (right):

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/third_...
remoting/protocol/third_party_authenticator_base.cc:40: return false;
On 2014/03/24 18:42:36, Sergey Ulanov wrote:
> Third-party auth is started as soon as a message is sent to the peer.
> underlying_ state is irrelevant here. So I think you need a started_ field
> that's set to true when AddTokenElements() is called in GetNextMessage().

Done.

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/v2_aut...
File remoting/protocol/v2_authenticator.h (right):

https://codereview.chromium.org/205583011/diff/20001/remoting/protocol/v2_aut...
remoting/protocol/v2_authenticator.h:47: virtual bool has_auth_started() const
OVERRIDE;
On 2014/03/24 18:42:36, Sergey Ulanov wrote:
> Please keep the same method order as in the interface definition.

Done.

[Sergey Ulanov, 2014-03-26 01:49:15]

Looks good. I'd like to have unittests coverage for the new state before we land
this change. Specifically I think AuthenticatorTestBase and JingleSessionTest
need to be updated.

https://codereview.chromium.org/205583011/diff/40001/remoting/host/chromoting...
File remoting/host/chromoting_host_unittest.cc (right):

https://codereview.chromium.org/205583011/diff/40001/remoting/host/chromoting...
remoting/host/chromoting_host_unittest.cc:605:
host_->OnSessionAuthenticating(NULL);
This test will break when OnSessionAuthenticating() is changed to use the
parameter. So it's better to keep the code that connects the first session -
just call OnSessionAuthenticating() for it here.

https://codereview.chromium.org/205583011/diff/40001/remoting/host/client_ses...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/205583011/diff/40001/remoting/host/client_ses...
remoting/host/client_session.cc:215: DCHECK(event_handler_);
nit: don't need this DCHECK.

https://codereview.chromium.org/205583011/diff/40001/remoting/host/pam_author...
File remoting/host/pam_authorization_factory_posix.cc (right):

https://codereview.chromium.org/205583011/diff/40001/remoting/host/pam_author...
remoting/host/pam_authorization_factory_posix.cc:67: CHECK(underlying_);
don't need this. scoped_ptr::operator->() DCHECK that object is not NULL.

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/fake_a...
File remoting/protocol/fake_authenticator.cc (right):

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/fake_a...
remoting/protocol/fake_authenticator.cc:120: return false;
messages_ > 0?

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/jingle...
File remoting/protocol/jingle_session.cc (right):

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:189: return;
while you are here, please remove this line

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:591: SetState(AUTHENTICATING);
nit: 2-space indent relative to if

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:614: state_ == AUTHENTICATING || state_ ==
AUTHENTICATED) {
this is duplicated in OnTerminate(). Maybe add is_session_active() to avoid
duplication.

[rmsousa, 2014-03-26 21:37:34]

This generally looks very good, just a couple of comments.

(feel free to commit once you address them and sergeyu lgtms).

https://codereview.chromium.org/205583011/diff/20001/remoting/host/chromoting...
File remoting/host/chromoting_host.cc (right):

https://codereview.chromium.org/205583011/diff/20001/remoting/host/chromoting...
remoting/host/chromoting_host.cc:179: login_backoff_.InformOfRequest(false);
Not related to your CL, but might as well fix it: Since we always treat the
first request as a failure, kDefaultBackoffPolicy's number of attempts to ignore
(line 35) should be at least 1.

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/jingle...
File remoting/protocol/jingle_session.cc (right):

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:184: if (authenticator_->started()) {
Do we want to ensure that we pass through the AUTHENTICATING state between
CONNECTED and AUTHENTICATED? if so, this should be before the outer if block.

It might be useful to keep the backoff behavior consistent between
authenticators that start immediately or not, but I don't really see a practical
consequence if we don't do that.

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:590: if (authenticator_->started()) {
authenticator may set the started bit in GetNextMessage, so this should be after
the if block below.

[Sergey Ulanov, 2014-03-27 00:00:03]

https://codereview.chromium.org/205583011/diff/40001/remoting/host/chromoting...
File remoting/host/chromoting_host.cc (right):

https://codereview.chromium.org/205583011/diff/40001/remoting/host/chromoting...
remoting/host/chromoting_host.cc:174: void
ChromotingHost::OnSessionAuthenticating(ClientSession* client) {
Looking more at this code I think we should move the
login_backoff_.ShouldRejectRequest() here in case there are multiple
simultaneous connection attempts.

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/jingle...
File remoting/protocol/jingle_session.cc (right):

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:590: if (authenticator_->started()) {
On 2014/03/26 21:37:35, rmsousa wrote:
> authenticator may set the started bit in GetNextMessage, so this should be
after
> the if block below.

+1 I think it should be an else-if case in the end of this method.

[kelvinp, 2014-03-27 03:23:20]

Fix UT

Check for backoff during session authenticating and disconnect the client is
back off is active.  This prevents the following scenario

Client A connects (No backoff)
Client B connects (No backoff)
Client A authenticates (backoff is set)
Client B authenticates (this will still go through as there is no backoff check
at this point)

https://codereview.chromium.org/205583011/diff/20001/remoting/host/chromoting...
File remoting/host/chromoting_host.cc (right):

https://codereview.chromium.org/205583011/diff/20001/remoting/host/chromoting...
remoting/host/chromoting_host.cc:179: login_backoff_.InformOfRequest(false);
On 2014/03/26 21:37:34, rmsousa wrote:
> Not related to your CL, but might as well fix it: Since we always treat the
> first request as a failure, kDefaultBackoffPolicy's number of attempts to
ignore
> (line 35) should be at least 1.

Good point.  However, after some research, I believe the current code is
correct.  Setting num_errors_to_ignore to 0 means we will treat the first
request as a failure as
int effective_failure_count =
      std::max(0, failure_count_ - policy_->num_errors_to_ignore);
if we set it to 1, we will end up treating the 2nd request as failure

See
(https://code.google.com/p/chromium/codesearch#chromium/src/net/base/backoff_e...)

num_e

https://codereview.chromium.org/205583011/diff/40001/remoting/host/chromoting...
File remoting/host/chromoting_host.cc (right):

https://codereview.chromium.org/205583011/diff/40001/remoting/host/chromoting...
remoting/host/chromoting_host.cc:174: void
ChromotingHost::OnSessionAuthenticating(ClientSession* client) {
On 2014/03/27 00:00:03, Sergey Ulanov wrote:
> Looking more at this code I think we should move the
> login_backoff_.ShouldRejectRequest() here in case there are multiple
> simultaneous connection attempts.

Done.

https://codereview.chromium.org/205583011/diff/40001/remoting/host/chromoting...
File remoting/host/chromoting_host_unittest.cc (right):

https://codereview.chromium.org/205583011/diff/40001/remoting/host/chromoting...
remoting/host/chromoting_host_unittest.cc:605:
host_->OnSessionAuthenticating(NULL);
On 2014/03/26 01:49:16, Sergey Ulanov wrote:
> This test will break when OnSessionAuthenticating() is changed to use the
> parameter. So it's better to keep the code that connects the first session -
> just call OnSessionAuthenticating() for it here.

Done.

https://codereview.chromium.org/205583011/diff/40001/remoting/host/client_ses...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/205583011/diff/40001/remoting/host/client_ses...
remoting/host/client_session.cc:215: DCHECK(event_handler_);
On 2014/03/26 01:49:16, Sergey Ulanov wrote:
> nit: don't need this DCHECK.

Done.

https://codereview.chromium.org/205583011/diff/40001/remoting/host/pam_author...
File remoting/host/pam_authorization_factory_posix.cc (right):

https://codereview.chromium.org/205583011/diff/40001/remoting/host/pam_author...
remoting/host/pam_authorization_factory_posix.cc:67: CHECK(underlying_);
On 2014/03/26 01:49:16, Sergey Ulanov wrote:
> don't need this. scoped_ptr::operator->() DCHECK that object is not NULL.

Done.

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/fake_a...
File remoting/protocol/fake_authenticator.cc (right):

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/fake_a...
remoting/protocol/fake_authenticator.cc:120: return false;
On 2014/03/26 01:49:16, Sergey Ulanov wrote:
> messages_ > 0?

Done.

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/jingle...
File remoting/protocol/jingle_session.cc (right):

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:184: if (authenticator_->started()) {
On 2014/03/26 21:37:35, rmsousa wrote:
> Do we want to ensure that we pass through the AUTHENTICATING state between
> CONNECTED and AUTHENTICATED? if so, this should be before the outer if block.
> 
> It might be useful to keep the backoff behavior consistent between
> authenticators that start immediately or not, but I don't really see a
practical
> consequence if we don't do that.

After talking to Sergey, I think we can allow switching directly from connected
to authenticated.

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:189: return;
On 2014/03/26 01:49:16, Sergey Ulanov wrote:
> while you are here, please remove this line

Done.

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:590: if (authenticator_->started()) {
On 2014/03/27 00:00:03, Sergey Ulanov wrote:
> On 2014/03/26 21:37:35, rmsousa wrote:
> > authenticator may set the started bit in GetNextMessage, so this should be
> after
> > the if block below.
> 
> +1 I think it should be an else-if case in the end of this method.

Done.

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:590: if (authenticator_->started()) {
On 2014/03/26 21:37:35, rmsousa wrote:
> authenticator may set the started bit in GetNextMessage, so this should be
after
> the if block below.

Done.

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:591: SetState(AUTHENTICATING);
On 2014/03/26 01:49:16, Sergey Ulanov wrote:
> nit: 2-space indent relative to if
My bad. Fixed

Done.

https://codereview.chromium.org/205583011/diff/40001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:614: state_ == AUTHENTICATING || state_ ==
AUTHENTICATED) {
On 2014/03/26 01:49:16, Sergey Ulanov wrote:
> this is duplicated in OnTerminate(). Maybe add is_session_active() to avoid
> duplication.

Good Idea. Done.

[Sergey Ulanov, 2014-03-27 19:06:55]

https://codereview.chromium.org/205583011/diff/90001/remoting/host/chromoting...
File remoting/host/chromoting_host.cc (right):

https://codereview.chromium.org/205583011/diff/90001/remoting/host/chromoting...
remoting/host/chromoting_host.cc:180: client->DisconnectSession();
return here. We don't want to increment backoff on rejected attempts.

https://codereview.chromium.org/205583011/diff/90001/remoting/host/chromoting...
File remoting/host/chromoting_host_unittest.cc (right):

https://codereview.chromium.org/205583011/diff/90001/remoting/host/chromoting...
remoting/host/chromoting_host_unittest.cc:596: TEST_F(ChromotingHostTest,
IncomingSessionOverload) {
Please add another test for the case when a session is rejected from
OnSessionAuthenticating()

https://codereview.chromium.org/205583011/diff/90001/remoting/host/chromoting...
remoting/host/chromoting_host_unittest.cc:613:
host_->OnSessionAuthenticating(NULL);
s/NULL/host_->clients_.front()/

https://codereview.chromium.org/205583011/diff/90001/remoting/protocol/jingle...
File remoting/protocol/jingle_session.cc (right):

https://codereview.chromium.org/205583011/diff/90001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:601: // asychronously using a weak pointer.
Please add a unittests that tries to delete JingleSession on AUTHENTICATING
state change

https://codereview.chromium.org/205583011/diff/90001/remoting/protocol/jingle...
File remoting/protocol/jingle_session.h (right):

https://codereview.chromium.org/205583011/diff/90001/remoting/protocol/jingle...
remoting/protocol/jingle_session.h:134: 
Don't need this empty line.

https://codereview.chromium.org/205583011/diff/90001/remoting/protocol/jingle...
remoting/protocol/jingle_session.h:140: // Called after the authenticating step
is finished
nit: add '.' at the end.

https://codereview.chromium.org/205583011/diff/90001/remoting/protocol/jingle...
remoting/protocol/jingle_session.h:158: base::WeakPtrFactory<JingleSession>
weak_factory_;
We normally put weak_factory_ as last field - that's to ensure that all weak
pointers are invalidated before all other fields are destroyed.

[kelvinp, 2014-04-01 21:23:49]

https://codereview.chromium.org/205583011/diff/90001/remoting/host/chromoting...
File remoting/host/chromoting_host.cc (right):

https://codereview.chromium.org/205583011/diff/90001/remoting/host/chromoting...
remoting/host/chromoting_host.cc:180: client->DisconnectSession();
On 2014/03/27 19:06:55, Sergey Ulanov wrote:
> return here. We don't want to increment backoff on rejected attempts.

Done.

https://codereview.chromium.org/205583011/diff/90001/remoting/host/chromoting...
File remoting/host/chromoting_host_unittest.cc (right):

https://codereview.chromium.org/205583011/diff/90001/remoting/host/chromoting...
remoting/host/chromoting_host_unittest.cc:613:
host_->OnSessionAuthenticating(NULL);
On 2014/03/27 19:06:55, Sergey Ulanov wrote:
> s/NULL/host_->clients_.front()/

Done.

https://codereview.chromium.org/205583011/diff/90001/remoting/protocol/jingle...
File remoting/protocol/jingle_session.cc (right):

https://codereview.chromium.org/205583011/diff/90001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:601: // asychronously using a weak pointer.
On 2014/03/27 19:06:55, Sergey Ulanov wrote:
> Please add a unittests that tries to delete JingleSession on AUTHENTICATING
> state change

Done.

https://codereview.chromium.org/205583011/diff/90001/remoting/protocol/jingle...
File remoting/protocol/jingle_session.h (right):

https://codereview.chromium.org/205583011/diff/90001/remoting/protocol/jingle...
remoting/protocol/jingle_session.h:134: 
On 2014/03/27 19:06:55, Sergey Ulanov wrote:
> Don't need this empty line.

Done.

https://codereview.chromium.org/205583011/diff/90001/remoting/protocol/jingle...
remoting/protocol/jingle_session.h:140: // Called after the authenticating step
is finished
On 2014/03/27 19:06:55, Sergey Ulanov wrote:
> nit: add '.' at the end.

Done.

https://codereview.chromium.org/205583011/diff/90001/remoting/protocol/jingle...
remoting/protocol/jingle_session.h:158: base::WeakPtrFactory<JingleSession>
weak_factory_;
On 2014/03/27 19:06:55, Sergey Ulanov wrote:
> We normally put weak_factory_ as last field - that's to ensure that all weak
> pointers are invalidated before all other fields are destroyed.

Done.

[Sergey Ulanov, 2014-04-02 19:49:26]

lgtm when my comments are addressed

https://codereview.chromium.org/205583011/diff/130001/remoting/host/chromotin...
File remoting/host/chromoting_host.cc (right):

https://codereview.chromium.org/205583011/diff/130001/remoting/host/chromotin...
remoting/host/chromoting_host.cc:283: " an overload of failed login attempts.";
Please log session->jid() here.

https://codereview.chromium.org/205583011/diff/130001/remoting/protocol/fake_...
File remoting/protocol/fake_authenticator.h (right):

https://codereview.chromium.org/205583011/diff/130001/remoting/protocol/fake_...
remoting/protocol/fake_authenticator.h:61: FakeAuthenticator(Type type, int
round_trips, int messages_till_started,
It's better to avoid function overloading:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

I suggest adding set_message_till_started().

https://codereview.chromium.org/205583011/diff/130001/remoting/protocol/fake_...
remoting/protocol/fake_authenticator.h:86: // returns true.  Default to 0
nit: add '.' at the end of the comment

https://codereview.chromium.org/205583011/diff/130001/remoting/protocol/me2me...
File remoting/protocol/me2me_host_authenticator_factory.cc (right):

https://codereview.chromium.org/205583011/diff/130001/remoting/protocol/me2me...
remoting/protocol/me2me_host_authenticator_factory.cc:34: return false;
true

[kelvinp, 2014-04-07 18:48:01]

https://codereview.chromium.org/205583011/diff/130001/remoting/host/chromotin...
File remoting/host/chromoting_host.cc (right):

https://codereview.chromium.org/205583011/diff/130001/remoting/host/chromotin...
remoting/host/chromoting_host.cc:283: " an overload of failed login attempts.";
On 2014/04/02 19:49:26, Sergey Ulanov wrote:
> Please log session->jid() here.

Done.

https://codereview.chromium.org/205583011/diff/130001/remoting/protocol/fake_...
File remoting/protocol/fake_authenticator.h (right):

https://codereview.chromium.org/205583011/diff/130001/remoting/protocol/fake_...
remoting/protocol/fake_authenticator.h:86: // returns true.  Default to 0
On 2014/04/02 19:49:26, Sergey Ulanov wrote:
> nit: add '.' at the end of the comment

Done.

https://codereview.chromium.org/205583011/diff/130001/remoting/protocol/me2me...
File remoting/protocol/me2me_host_authenticator_factory.cc (right):

https://codereview.chromium.org/205583011/diff/130001/remoting/protocol/me2me...
remoting/protocol/me2me_host_authenticator_factory.cc:34: return false;
On 2014/04/02 19:49:26, Sergey Ulanov wrote:
> true

Done.

[kelvinp, 2014-04-07 18:48:13]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-04-07 18:49:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kelvinp@chromium.org/205583011/170001

[commit-bot: I haz the power, 2014-04-07 22:33:30]

Change committed as 262228