Report the Auth method for me2me connections.

Report the Auth method for me2me connections.

As of M46, 20% of the connection failure are due to invalid access code.
We should record the auth method so we can understand if there are
correlations between auth failures and auth methods.

Summary of changes:
1. Reports the AuthMethod in CredentialsProvider.
2. Adds an unittest.
3. Merge the enum remoting.MockClientPlugin.AuthMethod
   with remoting.ChromotingEvent.AuthMethod

BUG=539928
Committed: https://crrev.com/cb01fddc7697aa1fdbda295d9503c88614d85291
Cr-Commit-Position: refs/heads/master@{#353345}

[kelvinp, 2015-10-07 23:43:41]

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

[kelvinp, 2015-10-07 23:44:43]

PTAL

[kelvinp, 2015-10-08 21:49:52]

Ping

[Jamie, 2015-10-08 22:22:42]

https://codereview.chromium.org/1397463003/diff/1/remoting/webapp/base/js/cre...
File remoting/webapp/base/js/credentials_provider.js (right):

https://codereview.chromium.org/1397463003/diff/1/remoting/webapp/base/js/cre...
remoting/webapp/base/js/credentials_provider.js:61: this.authMethod_ =
remoting.ChromotingEvent.AuthMethod.ACCESS_CODE;
This isn't the right place to set this. Aside from the strangeness of having a
getter update state, you're relying on the fact that a caller will never invoke
this method unless the authentication method was ACCESS_CODE. That's a fragile
assumption to make--you'd be better off storing the authMethod field somewhere
else or having an explicit setAuthMethod method on this class.

[kelvinp, 2015-10-09 00:45:41]

https://codereview.chromium.org/1397463003/diff/1/remoting/webapp/base/js/cre...
File remoting/webapp/base/js/credentials_provider.js (right):

https://codereview.chromium.org/1397463003/diff/1/remoting/webapp/base/js/cre...
remoting/webapp/base/js/credentials_provider.js:61: this.authMethod_ =
remoting.ChromotingEvent.AuthMethod.ACCESS_CODE;
On 2015/10/08 22:22:42, Jamie wrote:
> This isn't the right place to set this. Aside from the strangeness of having a
> getter update state, you're relying on the fact that a caller will never
invoke
> this method unless the authentication method was ACCESS_CODE. That's a fragile
> assumption to make--you'd be better off storing the authMethod field somewhere
> else or having an explicit setAuthMethod method on this class.

I agree that the correct long term fix will be in the plugin layer.
However, that would require reverse the ownership of the authenticator thru a
couple layers, which is quite risky to do after
a branch point.  I will prefer to land this fix now and clean it up
in trunk.

[Jamie, 2015-10-09 01:17:21]

LGTM once the new test is added.

https://codereview.chromium.org/1397463003/diff/1/remoting/webapp/base/js/cli...
File remoting/webapp/base/js/client_session.js (right):

https://codereview.chromium.org/1397463003/diff/1/remoting/webapp/base/js/cli...
remoting/webapp/base/js/client_session.js:566: this.state_ ==
remoting.ClientSession.State.FAILED) {
Rather than relying on this, couldn't you have getAuthMethod return null? Feel
free to ignore if this code will change substantially when you refactor the
plugin code to notify the auth method explicitly.

https://codereview.chromium.org/1397463003/diff/1/remoting/webapp/base/js/cre...
File remoting/webapp/base/js/credentials_provider.js (right):

https://codereview.chromium.org/1397463003/diff/1/remoting/webapp/base/js/cre...
remoting/webapp/base/js/credentials_provider.js:61: this.authMethod_ =
remoting.ChromotingEvent.AuthMethod.ACCESS_CODE;
On 2015/10/09 00:45:41, kelvinp wrote:
> On 2015/10/08 22:22:42, Jamie wrote:
> > This isn't the right place to set this. Aside from the strangeness of having
a
> > getter update state, you're relying on the fact that a caller will never
> invoke
> > this method unless the authentication method was ACCESS_CODE. That's a
fragile
> > assumption to make--you'd be better off storing the authMethod field
somewhere
> > else or having an explicit setAuthMethod method on this class.
> 
> I agree that the correct long term fix will be in the plugin layer.
> However, that would require reverse the ownership of the authenticator thru a
> couple layers, which is quite risky to do after
> a branch point.  I will prefer to land this fix now and clean it up
> in trunk.

Agreed, thanks for investigating.

https://codereview.chromium.org/1397463003/diff/1/remoting/webapp/crd/js/me2m...
File remoting/webapp/crd/js/me2me_telemetry_integration_test.js (right):

https://codereview.chromium.org/1397463003/diff/1/remoting/webapp/crd/js/me2m...
remoting/webapp/crd/js/me2me_telemetry_integration_test.js:480: 
Add a test that the auth method is set for failed connections too?

[kelvinp, 2015-10-09 18:00:34]

FYI

https://codereview.chromium.org/1397463003/diff/1/remoting/webapp/base/js/cli...
File remoting/webapp/base/js/client_session.js (right):

https://codereview.chromium.org/1397463003/diff/1/remoting/webapp/base/js/cli...
remoting/webapp/base/js/client_session.js:566: this.state_ ==
remoting.ClientSession.State.FAILED) {
On 2015/10/09 01:17:21, Jamie wrote:
> Rather than relying on this, couldn't you have getAuthMethod return null? Feel
> free to ignore if this code will change substantially when you refactor the
> plugin code to notify the auth method explicitly.

The scenario that I am trying to cover is the following:

If you have a pairing info in local storage (but pairing is disabled on the host
side), we will always read the pairing info from the credentials provider the
session.  Due to the limitations that you have pointed out before, the session
will be first reported with pairing as the auth method.  However, upon
authentication, it fails and the host will ask for a PIN.  As a result, the
CONNECTING events of the session will be reported using pairing whereas
AUTHENTICATED will be reported using PIN.

https://codereview.chromium.org/1397463003/diff/1/remoting/webapp/base/js/cre...
File remoting/webapp/base/js/credentials_provider.js (right):

https://codereview.chromium.org/1397463003/diff/1/remoting/webapp/base/js/cre...
remoting/webapp/base/js/credentials_provider.js:61: this.authMethod_ =
remoting.ChromotingEvent.AuthMethod.ACCESS_CODE;
On 2015/10/09 01:17:21, Jamie wrote:
> On 2015/10/09 00:45:41, kelvinp wrote:
> > On 2015/10/08 22:22:42, Jamie wrote:
> > > This isn't the right place to set this. Aside from the strangeness of
having
> a
> > > getter update state, you're relying on the fact that a caller will never
> > invoke
> > > this method unless the authentication method was ACCESS_CODE. That's a
> fragile
> > > assumption to make--you'd be better off storing the authMethod field
> somewhere
> > > else or having an explicit setAuthMethod method on this class.
> > 
> > I agree that the correct long term fix will be in the plugin layer.
> > However, that would require reverse the ownership of the authenticator thru
a
> > couple layers, which is quite risky to do after
> > a branch point.  I will prefer to land this fix now and clean it up
> > in trunk.
> 
> Agreed, thanks for investigating.

Done.

https://codereview.chromium.org/1397463003/diff/1/remoting/webapp/crd/js/me2m...
File remoting/webapp/crd/js/me2me_telemetry_integration_test.js (right):

https://codereview.chromium.org/1397463003/diff/1/remoting/webapp/crd/js/me2m...
remoting/webapp/crd/js/me2me_telemetry_integration_test.js:480: 
On 2015/10/09 01:17:21, Jamie wrote:
> Add a test that the auth method is set for failed connections too?

Done.

[kelvinp, 2015-10-09 18:00:38]

The CQ bit was checked by kelvinp@chromium.org

[kelvinp, 2015-10-09 18:00:38]

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/1397463003/#ps20001
(title: "Reviewer's feedback")

[commit-bot: I haz the power, 2015-10-09 18:07:30]

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

[commit-bot: I haz the power, 2015-10-09 18:58:03]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-09 18:58:58]

Patchset 2 (id:??) landed as
https://crrev.com/cb01fddc7697aa1fdbda295d9503c88614d85291
Cr-Commit-Position: refs/heads/master@{#353345}