Verify that xmpp_login specified in the config matches auth token.

Verify that xmpp_login specified in the config matches auth token.

Previously the host would ignore the case when the oauth token doesn't
match the xmpp_login specified in the config. That would lead to situations
when host is connected with one account, but verifies incoming connections
with a different account. Fix this by verifying that the JID that we receive 
from XMPP server matches the value in the config.


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

[simonmorris, 2012-05-11 19:57:25]

http://codereview.chromium.org/10378110/diff/1/remoting/host/signaling_connec...
File remoting/host/signaling_connector.cc (right):

http://codereview.chromium.org/10378110/diff/1/remoting/host/signaling_connec...
remoting/host/signaling_connector.cc:73: // If authentication failed then it
means that we have invalid
"it means that" -> ""
"have invalid" -> "have an invalid"

http://codereview.chromium.org/10378110/diff/1/remoting/host/signaling_connec...
remoting/host/signaling_connector.cc:76: auth_failed_callback_.Run();
Optional suggestion: don't schedule retries in this case.

http://codereview.chromium.org/10378110/diff/1/remoting/jingle_glue/signal_st...
File remoting/jingle_glue/signal_strategy.h (right):

http://codereview.chromium.org/10378110/diff/1/remoting/jingle_glue/signal_st...
remoting/jingle_glue/signal_strategy.h:47: // specified state is set to
DISCONNECTED.
There's no |error| now. Maybe something like "If the state is DISCONNECTED, then
GetError() will return the reason for the disconnection"?

http://codereview.chromium.org/10378110/diff/1/remoting/jingle_glue/xmpp_sign...
File remoting/jingle_glue/xmpp_signal_strategy.cc (right):

http://codereview.chromium.org/10378110/diff/1/remoting/jingle_glue/xmpp_sign...
remoting/jingle_glue/xmpp_signal_strategy.cc:170: // Verify that the JID that
we've received doesn't match the
"doesn't match" -> "matches".

http://codereview.chromium.org/10378110/diff/1/remoting/jingle_glue/xmpp_sign...
remoting/jingle_glue/xmpp_signal_strategy.cc:171: // username that we have. That
probably means that the OAuth token
"That probably means" -> "If it doesn't, then... was probably issued".

http://codereview.chromium.org/10378110/diff/1/remoting/jingle_glue/xmpp_sign...
remoting/jingle_glue/xmpp_signal_strategy.cc:174: if
(!StartsWithASCII(GetLocalJid(), username_, false)) {
Other cases may be worth considering. I'll follow up by email.

[Sergey Ulanov, 2012-05-11 20:48:05]

http://codereview.chromium.org/10378110/diff/1/remoting/host/signaling_connec...
File remoting/host/signaling_connector.cc (right):

http://codereview.chromium.org/10378110/diff/1/remoting/host/signaling_connec...
remoting/host/signaling_connector.cc:73: // If authentication failed then it
means that we have invalid
On 2012/05/11 19:57:25, simonmorris wrote:
> "it means that" -> ""
> "have invalid" -> "have an invalid"

Done.

http://codereview.chromium.org/10378110/diff/1/remoting/host/signaling_connec...
remoting/host/signaling_connector.cc:76: auth_failed_callback_.Run();
On 2012/05/11 19:57:25, simonmorris wrote:
> Optional suggestion: don't schedule retries in this case.

Done.

http://codereview.chromium.org/10378110/diff/1/remoting/jingle_glue/signal_st...
File remoting/jingle_glue/signal_strategy.h (right):

http://codereview.chromium.org/10378110/diff/1/remoting/jingle_glue/signal_st...
remoting/jingle_glue/signal_strategy.h:47: // specified state is set to
DISCONNECTED.
On 2012/05/11 19:57:25, simonmorris wrote:
> There's no |error| now. Maybe something like "If the state is DISCONNECTED,
then
> GetError() will return the reason for the disconnection"?

Done.

http://codereview.chromium.org/10378110/diff/1/remoting/jingle_glue/xmpp_sign...
File remoting/jingle_glue/xmpp_signal_strategy.cc (right):

http://codereview.chromium.org/10378110/diff/1/remoting/jingle_glue/xmpp_sign...
remoting/jingle_glue/xmpp_signal_strategy.cc:170: // Verify that the JID that
we've received doesn't match the
On 2012/05/11 19:57:25, simonmorris wrote:
> "doesn't match" -> "matches".

Done.

http://codereview.chromium.org/10378110/diff/1/remoting/jingle_glue/xmpp_sign...
remoting/jingle_glue/xmpp_signal_strategy.cc:171: // username that we have. That
probably means that the OAuth token
On 2012/05/11 19:57:25, simonmorris wrote:
> "That probably means" -> "If it doesn't, then... was probably issued".

Done.

[simonmorris, 2012-05-11 22:59:03]

LGTM with optional suggestion.

http://codereview.chromium.org/10378110/diff/6001/remoting/jingle_glue/xmpp_s...
File remoting/jingle_glue/xmpp_signal_strategy.cc (right):

http://codereview.chromium.org/10378110/diff/6001/remoting/jingle_glue/xmpp_s...
remoting/jingle_glue/xmpp_signal_strategy.cc:172: // issued for a different
account, so we treat is a an auth error.
Maybe add a TODO to handle unusual cases where the received JID doesn't start
with an email address?

[Sergey Ulanov, 2012-05-11 23:33:41]

http://codereview.chromium.org/10378110/diff/6001/remoting/jingle_glue/xmpp_s...
File remoting/jingle_glue/xmpp_signal_strategy.cc (right):

http://codereview.chromium.org/10378110/diff/6001/remoting/jingle_glue/xmpp_s...
remoting/jingle_glue/xmpp_signal_strategy.cc:172: // issued for a different
account, so we treat is a an auth error.
On 2012/05/11 22:59:03, simonmorris wrote:
> Maybe add a TODO to handle unusual cases where the received JID doesn't start
> with an email address?

Done.

[commit-bot: I haz the power, 2012-05-11 23:34:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10378110/7013

[commit-bot: I haz the power, 2012-05-11 23:51:13]

Try job failure for 10378110-7013 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-05-12 00:13:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10378110/6002

[commit-bot: I haz the power, 2012-05-12 02:02:18]

Change committed as 136745