Disconnect all users if too many connection requests are received for It2Me

Disconnect all users if too many connection requests are received for It2Me

This change adds logic to disconnect the current session if multiple remote
users attempt to use the same Access Code with It2Me.  It adds a new rejection
state and updates the It2MeHost unit tests.  The previous behavior was to
crash which was not the right thing to do.  This CL also moves the point of
validation from the 'incoming' state to the 'accepted' state.

BUG=683317
Review-Url: https://codereview.chromium.org/2724223003
Cr-Commit-Position: refs/heads/master@{#458075}
Committed: https://chromium.googlesource.com/chromium/src/+/f38c5d70b59fe17e1d3479081dd3f6bc1bfc1cb5

[joedow, 2017-03-02 19:25:15]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 19:25:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joedow, 2017-03-02 19:28:35]

joedow@chromium.org changed reviewers:
+ sergeyu@chromium.org

[joedow, 2017-03-02 19:28:35]

PTAL!

[commit-bot: I haz the power, 2017-03-02 20:33:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 20:33:54]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2017-03-02 23:07:29]

https://codereview.chromium.org/2724223003/diff/20001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/2724223003/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:520: if (state_ != kReceivedAccessCode) {
I don't think we want to shutdown the host when the user mistypes access code.
Currently the host keeps running and client can correct access code and try
connecting again. I'd like to keep this behavior.

Currently ValidatingAuthenticator calls validation callback before processing
the first message. Can it be updated to perform validation after base
authentication is complete? I think it should be enough to fix this bug.

[joedow, 2017-03-13 21:26:50]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 21:27:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 21:34:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 21:34:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[joedow, 2017-03-13 21:44:10]

Description was changed from

==========
Disconnect all users if too many connection requests are received for It2Me

This change adds logic to disconnect the current session if multiple remote
users attempt to use the same Access Code with It2Me.  It adds a new rejection
state and updates the It2MeHost unit tests (they weren't validating the
current host state accurately).  The previous behavior was to crash which
was not the right thing to do.

BUG=683317
==========

to

==========
Disconnect all users if too many connection requests are received for It2Me

This change adds logic to disconnect the current session if multiple remote
users attempt to use the same Access Code with It2Me.  It adds a new rejection
state and updates the It2MeHost unit tests.  The previous behavior was to
crash which was not the right thing to do.

Another change is to split the single validation callback into two callbacks
which are called at different points in the connection process.  The first
callback (the 'incoming' connection callback) is calkled before any message
processing happens.  This allows basic validation to occur on the JID or other
details before lower level authenticators handle things like access code
validation.  If the lower-level authenticators all accept the connection, then
one final callback (the accepted callback) is used for final validation before
the connection is accepted / completed.  This second callback is where the
confirmation dialog logic lives now.

BUG=683317
==========

[joedow, 2017-03-13 21:44:18]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 21:45:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 21:52:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 21:52:26]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[joedow, 2017-03-13 22:17:01]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 22:18:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 23:07:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 23:07:34]

Dry run: This issue passed the CQ dry run.

[joedow, 2017-03-13 23:26:57]

PTAL!

https://codereview.chromium.org/2724223003/diff/20001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/2724223003/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:520: if (state_ != kReceivedAccessCode) {
On 2017/03/02 23:07:29, Sergey Ulanov wrote:
> I don't think we want to shutdown the host when the user mistypes access code.
> Currently the host keeps running and client can correct access code and try
> connecting again. I'd like to keep this behavior.
> 
> Currently ValidatingAuthenticator calls validation callback before processing
> the first message. Can it be updated to perform validation after base
> authentication is complete? I think it should be enough to fix this bug.

I missed that scenario so thanks for pointing that out.  I've broken validation
into two steps, one which is run on incoming connections to weed out obviously
wrong ones before doing additional validation and a second step which occurs
after the lower level authenticators have all succeeded.  This second step is
the right place for the confirmation dialog as the JID has been validated, a
connection method has been negotiated, and we know the access code is valid.

[joedow, 2017-03-15 15:37:32]

Ping!

[Sergey Ulanov, 2017-03-15 22:25:33]

https://codereview.chromium.org/2724223003/diff/60001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/2724223003/diff/60001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:57: bool GetUsernameFromJid(const std::string&
remote_jid,
Maybe use base::Optional<std::string> for the result and remove client_username
argument?

https://codereview.chromium.org/2724223003/diff/60001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:519: // Check the client domain policy.
Does this check need to be separate from ShowConfirmationDialog()? I.e. Could we
authenticate connection using the access code and check the domain policy after
that? That would make ValidatingAuthenticator simpler.

https://codereview.chromium.org/2724223003/diff/60001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:548: LOG(ERROR) << "Received too many
connection requests.";
Can we DCHECK here that state_ is kConnecting? I don't think it can be anything
else here.

https://codereview.chromium.org/2724223003/diff/60001/remoting/protocol/authe...
File remoting/protocol/authenticator.h (right):

https://codereview.chromium.org/2724223003/diff/60001/remoting/protocol/authe...
remoting/protocol/authenticator.h:71: TOO_MANY_CONNECTIONS,
Maybe add comments for each rejection reason? They were not necessary when this
enum had just 2 values (INVALID_CREDENTIALS and PROTOCOL_ERROR), but that's not
the case anymore, and it's more ambiguous now.

[joedow, 2017-03-16 21:01:39]

Description was changed from

==========
Disconnect all users if too many connection requests are received for It2Me

This change adds logic to disconnect the current session if multiple remote
users attempt to use the same Access Code with It2Me.  It adds a new rejection
state and updates the It2MeHost unit tests.  The previous behavior was to
crash which was not the right thing to do.

Another change is to split the single validation callback into two callbacks
which are called at different points in the connection process.  The first
callback (the 'incoming' connection callback) is calkled before any message
processing happens.  This allows basic validation to occur on the JID or other
details before lower level authenticators handle things like access code
validation.  If the lower-level authenticators all accept the connection, then
one final callback (the accepted callback) is used for final validation before
the connection is accepted / completed.  This second callback is where the
confirmation dialog logic lives now.

BUG=683317
==========

to

==========
Disconnect all users if too many connection requests are received for It2Me

This change adds logic to disconnect the current session if multiple remote
users attempt to use the same Access Code with It2Me.  It adds a new rejection
state and updates the It2MeHost unit tests.  The previous behavior was to
crash which was not the right thing to do.  This CL also moves the point of
validation from the 'incoming' state to the 'accepted' state.

BUG=683317
==========

[joedow, 2017-03-16 21:04:38]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 21:05:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joedow, 2017-03-16 21:32:18]

PTAL!

https://codereview.chromium.org/2724223003/diff/60001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/2724223003/diff/60001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:57: bool GetUsernameFromJid(const std::string&
remote_jid,
On 2017/03/15 22:25:33, Sergey Ulanov wrote:
> Maybe use base::Optional<std::string> for the result and remove
client_username
> argument?

Acknowledged.

https://codereview.chromium.org/2724223003/diff/60001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:519: // Check the client domain policy.
On 2017/03/15 22:25:33, Sergey Ulanov wrote:
> Does this check need to be separate from ShowConfirmationDialog()? I.e. Could
we
> authenticate connection using the access code and check the domain policy
after
> that? That would make ValidatingAuthenticator simpler.

Done.

https://codereview.chromium.org/2724223003/diff/60001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:548: LOG(ERROR) << "Received too many
connection requests.";
On 2017/03/15 22:25:33, Sergey Ulanov wrote:
> Can we DCHECK here that state_ is kConnecting? I don't think it can be
anything
> else here.

Done.

https://codereview.chromium.org/2724223003/diff/60001/remoting/protocol/authe...
File remoting/protocol/authenticator.h (right):

https://codereview.chromium.org/2724223003/diff/60001/remoting/protocol/authe...
remoting/protocol/authenticator.h:71: TOO_MANY_CONNECTIONS,
On 2017/03/15 22:25:33, Sergey Ulanov wrote:
> Maybe add comments for each rejection reason? They were not necessary when
this
> enum had just 2 values (INVALID_CREDENTIALS and PROTOCOL_ERROR), but that's
not
> the case anymore, and it's more ambiguous now.

Done.

[commit-bot: I haz the power, 2017-03-16 21:36:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 21:36:31]

Dry run: This issue passed the CQ dry run.

[kelvinp, 2017-03-17 17:33:24]

kelvinp@chromium.org changed reviewers:
+ kelvinp@chromium.org

[kelvinp, 2017-03-17 17:33:25]

Drive by comment, please make sure, we report the new error in our telemetry as
well.

[joedow, 2017-03-17 17:36:47]

The new error is translated to an existing error further up in the stack so
telemetry shouldn't need to be changed.

[joedow, 2017-03-17 20:42:51]

Ping!

[Sergey Ulanov, 2017-03-17 21:11:28]

LGTM when my comment is addressed

https://codereview.chromium.org/2724223003/diff/80001/remoting/protocol/valid...
File remoting/protocol/validating_authenticator.cc (right):

https://codereview.chromium.org/2724223003/diff/80001/remoting/protocol/valid...
remoting/protocol/validating_authenticator.cc:131: validation_callback_.Run(
I think we want to stay in PROCESSING_MESSAGE state while the callback is still
pending. Update state() to handle this case?

[joedow, 2017-03-20 15:09:07]

Thanks!

https://codereview.chromium.org/2724223003/diff/80001/remoting/protocol/valid...
File remoting/protocol/validating_authenticator.cc (right):

https://codereview.chromium.org/2724223003/diff/80001/remoting/protocol/valid...
remoting/protocol/validating_authenticator.cc:131: validation_callback_.Run(
On 2017/03/17 21:11:28, Sergey Ulanov wrote:
> I think we want to stay in PROCESSING_MESSAGE state while the callback is
still
> pending. Update state() to handle this case?

That makes sense, good suggestion!

[joedow, 2017-03-20 15:09:12]

The CQ bit was checked by joedow@chromium.org

[joedow, 2017-03-20 15:09:13]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2724223003/#ps100001
(title: "Addressing CR Feedback")

[commit-bot: I haz the power, 2017-03-20 15:09:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 15:55:54]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490022552934110,
"parent_rev": "c801d71d54cdc098f53132adbcd53d24e89fc4e3", "commit_rev":
"f38c5d70b59fe17e1d3479081dd3f6bc1bfc1cb5"}

[commit-bot: I haz the power, 2017-03-20 15:56:38]

Description was changed from

==========
Disconnect all users if too many connection requests are received for It2Me

This change adds logic to disconnect the current session if multiple remote
users attempt to use the same Access Code with It2Me.  It adds a new rejection
state and updates the It2MeHost unit tests.  The previous behavior was to
crash which was not the right thing to do.  This CL also moves the point of
validation from the 'incoming' state to the 'accepted' state.

BUG=683317
==========

to

==========
Disconnect all users if too many connection requests are received for It2Me

This change adds logic to disconnect the current session if multiple remote
users attempt to use the same Access Code with It2Me.  It adds a new rejection
state and updates the It2MeHost unit tests.  The previous behavior was to
crash which was not the right thing to do.  This CL also moves the point of
validation from the 'incoming' state to the 'accepted' state.

BUG=683317

Review-Url: https://codereview.chromium.org/2724223003
Cr-Commit-Position: refs/heads/master@{#458075}
Committed:
https://chromium.googlesource.com/chromium/src/+/f38c5d70b59fe17e1d3479081dd3...
==========

[commit-bot: I haz the power, 2017-03-20 15:56:39]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f38c5d70b59fe17e1d3479081dd3...