Handle pairing_client_id in the negotiating authenticators.

Handle pairing client ID in the negotiating authenticators.

Previously PairingAuthenticator classes were responsible for handling
client ID. This meant that in case there are multiple pairing
authentication methods the client would have to resend the client_id
if the host chooses different authentication method. Now the client
ID is handled by the negotiating layer, so client_id needs to be sent
only once.

Also now NegotiatingAuthenticators create PairingAuthenticators even
when falling back to PIN. This ensures that <pairing-failed> tag is
always present when falling back to PIN.

BUG=593123
Committed: https://crrev.com/843ef124414456330cd297383d171bd3c8ee4c91
Cr-Commit-Position: refs/heads/master@{#381633}

[Sergey Ulanov, 2016-03-14 18:26:18]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2016-03-14 18:26:26]

Patchset #1 (id:20001) has been deleted

[Sergey Ulanov, 2016-03-14 18:34:19]

Description was changed from

==========
Always create pairing authenticators when using spake2_pair method

Previosly V2Authenticator was created when using spake2_pair. Now
Pairing[Client|Host]Authenticator is alwasy created when using
spake2_pair method on the wire.

BUG=593123
==========

to

==========
Handle pairing client ID in the negotiating authenticators.

Previously PairingAuthenticator classes were responsible for handling
client ID. This meant that in case there are multiple pairing
authentication methods the client would have to resend the client_id
if the host chooses different authentication method. Now the client
ID is handled by the negotiating layer, so client_id needs to be sent
only once.

Also now NegotiatingAuthenticators create PairingAuthenticators even
when falling back to PIN. This ensures that <pairing-failed> tag is
always present when falling back to PIN.

BUG=593123
==========

[Sergey Ulanov, 2016-03-14 18:34:19]

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

[Sergey Ulanov, 2016-03-14 18:34:38]

Patchset #1 (id:40001) has been deleted

[Jamie, 2016-03-14 19:04:22]

the client id is an implementation detail of the pairing protocol; it feels like
a violation of encapsulation for the negotiating authenticator to need to know
about it. If we can already have multiple pairing protocols running, each
expecting its own client id, then this seems like a protocol-breaking change. On
the other hand, if it's a hypothetical concern, then I don't think we should
pollute the negotiating authenticator with the client id.

[Sergey Ulanov, 2016-03-16 18:39:44]

As we discussed offline we already have dependency between negotiating and
pairing authentication layers. This CL makes that dependency explicit and
expressed in form of a parameter passed between the layers instead dependency on
behavior. It will also allow to add new version of the pairing auth method,
without adding extra message in the case when there is no pairing. PTAL.

[Jamie, 2016-03-17 00:36:22]

lgtm

https://codereview.chromium.org/1781173005/diff/80001/remoting/protocol/pairi...
File remoting/protocol/pairing_client_authenticator.cc (right):

https://codereview.chromium.org/1781173005/diff/80001/remoting/protocol/pairi...
remoting/protocol/pairing_client_authenticator.cc:30: error_message_ =
"not-paired";
It's not obvious why this needs to be set here (ie, nothing has gone wrong yet).
I think a comment would help.

https://codereview.chromium.org/1781173005/diff/80001/remoting/protocol/pairi...
File remoting/protocol/pairing_client_authenticator.h (right):

https://codereview.chromium.org/1781173005/diff/80001/remoting/protocol/pairi...
remoting/protocol/pairing_client_authenticator.h:27: // cases.
Make it clear why both are needed/useful; ie. "StartPaired is synchronous, but
can only be used... Start() handles both cases, but requires a resume callback."

[Sergey Ulanov, 2016-03-17 00:49:21]

https://codereview.chromium.org/1781173005/diff/80001/remoting/protocol/pairi...
File remoting/protocol/pairing_client_authenticator.cc (right):

https://codereview.chromium.org/1781173005/diff/80001/remoting/protocol/pairi...
remoting/protocol/pairing_client_authenticator.cc:30: error_message_ =
"not-paired";
On 2016/03/17 00:36:22, Jamie wrote:
> It's not obvious why this needs to be set here (ie, nothing has gone wrong
yet).
> I think a comment would help.

Done.

https://codereview.chromium.org/1781173005/diff/80001/remoting/protocol/pairi...
File remoting/protocol/pairing_client_authenticator.h (right):

https://codereview.chromium.org/1781173005/diff/80001/remoting/protocol/pairi...
remoting/protocol/pairing_client_authenticator.h:27: // cases.
On 2016/03/17 00:36:22, Jamie wrote:
> Make it clear why both are needed/useful; ie. "StartPaired is synchronous, but
> can only be used... Start() handles both cases, but requires a resume
callback."

Done.

[Sergey Ulanov, 2016-03-17 00:49:41]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-03-17 00:49:41]

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

[commit-bot: I haz the power, 2016-03-17 00:50:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1781173005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1781173005/100001

[commit-bot: I haz the power, 2016-03-17 01:44:31]

Description was changed from

==========
Handle pairing client ID in the negotiating authenticators.

Previously PairingAuthenticator classes were responsible for handling
client ID. This meant that in case there are multiple pairing
authentication methods the client would have to resend the client_id
if the host chooses different authentication method. Now the client
ID is handled by the negotiating layer, so client_id needs to be sent
only once.

Also now NegotiatingAuthenticators create PairingAuthenticators even
when falling back to PIN. This ensures that <pairing-failed> tag is
always present when falling back to PIN.

BUG=593123
==========

to

==========
Handle pairing client ID in the negotiating authenticators.

Previously PairingAuthenticator classes were responsible for handling
client ID. This meant that in case there are multiple pairing
authentication methods the client would have to resend the client_id
if the host chooses different authentication method. Now the client
ID is handled by the negotiating layer, so client_id needs to be sent
only once.

Also now NegotiatingAuthenticators create PairingAuthenticators even
when falling back to PIN. This ensures that <pairing-failed> tag is
always present when falling back to PIN.

BUG=593123
==========

[commit-bot: I haz the power, 2016-03-17 01:44:32]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-03-17 01:45:37]

Description was changed from

==========
Handle pairing client ID in the negotiating authenticators.

Previously PairingAuthenticator classes were responsible for handling
client ID. This meant that in case there are multiple pairing
authentication methods the client would have to resend the client_id
if the host chooses different authentication method. Now the client
ID is handled by the negotiating layer, so client_id needs to be sent
only once.

Also now NegotiatingAuthenticators create PairingAuthenticators even
when falling back to PIN. This ensures that <pairing-failed> tag is
always present when falling back to PIN.

BUG=593123
==========

to

==========
Handle pairing client ID in the negotiating authenticators.

Previously PairingAuthenticator classes were responsible for handling
client ID. This meant that in case there are multiple pairing
authentication methods the client would have to resend the client_id
if the host chooses different authentication method. Now the client
ID is handled by the negotiating layer, so client_id needs to be sent
only once.

Also now NegotiatingAuthenticators create PairingAuthenticators even
when falling back to PIN. This ensures that <pairing-failed> tag is
always present when falling back to PIN.

BUG=593123

Committed: https://crrev.com/843ef124414456330cd297383d171bd3c8ee4c91
Cr-Commit-Position: refs/heads/master@{#381633}
==========

[commit-bot: I haz the power, 2016-03-17 01:45:38]

Patchset 3 (id:??) landed as
https://crrev.com/843ef124414456330cd297383d171bd3c8ee4c91
Cr-Commit-Position: refs/heads/master@{#381633}