Multi-step authentication support in JingleSession.

Multi-step authentication support in JingleSession.

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

[Wez, 2011-12-03 00:21:05]

LGTM, although you might add a test for the case where an authenticator sends
multiple messages in-between receiving messages.

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/authentica...
File remoting/protocol/authenticator.h (right):

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/authentica...
remoting/protocol/authenticator.h:56: // Returns true if |message| is an
authenticator message.
nit: Authenticator?

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/authentica...
remoting/protocol/authenticator.h:59: // Creates an empty authenticator message.
Passes ownership of the
nit: Creates an empty authenticator message, owned by the caller.

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/authentica...
remoting/protocol/authenticator.h:63: // Finds authenticator message among child
elements of |message|.
And if there isn't one?

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/content_de...
File remoting/protocol/content_description.cc (right):

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/content_de...
remoting/protocol/content_description.cc:9: #include
"remoting/protocol/authenticator.h"
This goes below remoting/base/constants.h

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.cc:256: if (state_ != CONNECTED ||
It might be clearer to have AUTHENTICATING and CONNECTED states, rather than
CONNECTED and AUTHENTICATED?

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_unittest.cc (right):

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_unittest.cc:354: // authentication.
nit: premature line-wrap?

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_unittest.cc:366: // Verify that connection is
terminted when multi-step auth fails.
nit: terminated

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_unittest.cc:412: // Verify that we can connect
channgels with multistep auth.
nit: channels

[Sergey Ulanov, 2011-12-06 19:37:07]

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/authentica...
File remoting/protocol/authenticator.h (right):

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/authentica...
remoting/protocol/authenticator.h:56: // Returns true if |message| is an
authenticator message.
On 2011/12/03 00:21:05, Wez wrote:
> nit: Authenticator?

Done.

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/authentica...
remoting/protocol/authenticator.h:59: // Creates an empty authenticator message.
Passes ownership of the
On 2011/12/03 00:21:05, Wez wrote:
> nit: Creates an empty authenticator message, owned by the caller.

Done.

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/authentica...
remoting/protocol/authenticator.h:63: // Finds authenticator message among child
elements of |message|.
On 2011/12/03 00:21:05, Wez wrote:
> And if there isn't one?

Done.

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/content_de...
File remoting/protocol/content_description.cc (right):

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/content_de...
remoting/protocol/content_description.cc:9: #include
"remoting/protocol/authenticator.h"
On 2011/12/03 00:21:05, Wez wrote:
> This goes below remoting/base/constants.h

Done.

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.cc:256: if (state_ != CONNECTED ||
On 2011/12/03 00:21:05, Wez wrote:
> It might be clearer to have AUTHENTICATING and CONNECTED states, rather than
> CONNECTED and AUTHENTICATED?

I agree, but it is not related to this CL.

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_unittest.cc (right):

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_unittest.cc:354: // authentication.
On 2011/12/03 00:21:05, Wez wrote:
> nit: premature line-wrap?

Done.

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_unittest.cc:366: // Verify that connection is
terminted when multi-step auth fails.
On 2011/12/03 00:21:05, Wez wrote:
> nit: terminated

Done.

http://codereview.chromium.org/8774031/diff/6002/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_unittest.cc:412: // Verify that we can connect
channgels with multistep auth.
On 2011/12/03 00:21:05, Wez wrote:
> nit: channels

Done.

[commit-bot: I haz the power, 2011-12-13 05:25:38]

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

[commit-bot: I haz the power, 2011-12-13 06:29:01]

Change committed as 114187