Added method to host daemon to exchange an auth code for just an OAuth

Added method to host daemon to exchange an auth code for just an OAuth
refresh token.

This change is necessary because, if the host daemon attempts to
retrieve an email address for an auth code from GCD, the request
fails, causing the host daemon to report failure and not return a
refresh token.

BUG=471928
Committed: https://crrev.com/ec9abae8e2c437970e89e5565aaf7d9ea19ab9d0
Cr-Commit-Position: refs/heads/master@{#326895}

[John Williams, 2015-04-22 17:29:35]

jrw@chromium.org changed reviewers:
+ lambroslambrou@chromium.org, weitaosu@chromium.org

[John Williams, 2015-04-22 17:29:35]

My first C++ CL at Google.

[Lambros, 2015-04-22 18:24:34]

LG so far, but I have a concern about version-skew.
Do we need to add a new "capability" for this new "getTokenFromAuthCode"
request, and have the Chrome app test for the capability before making the
request to the native-messaging-host? Or perhaps it's OK for the request to fail
naturally?

https://codereview.chromium.org/1076093003/diff/1/remoting/host/setup/oauth_c...
File remoting/host/setup/oauth_client.h (right):

https://codereview.chromium.org/1076093003/diff/1/remoting/host/setup/oauth_c...
remoting/host/setup/oauth_client.h:39: // |refresh_token| and |access_token|,
then, if |need_user_info| is
s/_info/_email/

[Lambros, 2015-04-22 18:28:52]

https://codereview.chromium.org/1076093003/diff/1/remoting/host/setup/me2me_n...
File remoting/host/setup/me2me_native_messaging_host.cc (right):

https://codereview.chromium.org/1076093003/diff/1/remoting/host/setup/me2me_n...
remoting/host/setup/me2me_native_messaging_host.cc:165: } else if (type ==
"getTokenFromAuthCode") {
Please update me2me_native_messaging_host_unittest.cc for this new request type.
You can simply validate that this new request triggers a correctly-formatted
response, similar to what we do for the other request types.

[John Williams, 2015-04-22 20:10:51]

On 2015/04/22 18:24:34, Lambros wrote:
> LG so far, but I have a concern about version-skew.
> Do we need to add a new "capability" for this new "getTokenFromAuthCode"
> request, and have the Chrome app test for the capability before making the
> request to the native-messaging-host? Or perhaps it's OK for the request to
fail
> naturally?

Testing for the capability is a possible optimization.  Without testing first,
the webapp will register the host with GCD and then immediately un-register it
when the call to getTokenFromAuthCode fails.  Probably worth doing in a future
CL, so I'll add a TODO.

[John Williams, 2015-04-22 20:11:25]

https://codereview.chromium.org/1076093003/diff/1/remoting/host/setup/me2me_n...
File remoting/host/setup/me2me_native_messaging_host.cc (right):

https://codereview.chromium.org/1076093003/diff/1/remoting/host/setup/me2me_n...
remoting/host/setup/me2me_native_messaging_host.cc:165: } else if (type ==
"getTokenFromAuthCode") {
On 2015/04/22 18:28:52, Lambros wrote:
> Please update me2me_native_messaging_host_unittest.cc for this new request
type.
> You can simply validate that this new request triggers a correctly-formatted
> response, similar to what we do for the other request types.

It looks like to do this, I will have to make remoting::OAuthClient abstract and
write a mock class.  Is that what I should do?  And if so, does this breakdown
of files/classes work for you?

  OAuthClient(abstract)   oauth_client.{cc,h}
  GaiaOAuthClient         oauth_client.{cc,h}
  MockOAuthClient         mock_oauth_client.{cc,h}

https://codereview.chromium.org/1076093003/diff/1/remoting/host/setup/oauth_c...
File remoting/host/setup/oauth_client.h (right):

https://codereview.chromium.org/1076093003/diff/1/remoting/host/setup/oauth_c...
remoting/host/setup/oauth_client.h:39: // |refresh_token| and |access_token|,
then, if |need_user_info| is
On 2015/04/22 18:24:34, Lambros wrote:
> s/_info/_email/

Done.

[Lambros, 2015-04-22 21:23:00]

In the case of version-skew, if we're going to end up registering then
de-registering the host, I'm inclined to think we should test for the capability
first. I don't feel particularly strongly - let's see what others think.

https://codereview.chromium.org/1076093003/diff/1/remoting/host/setup/me2me_n...
File remoting/host/setup/me2me_native_messaging_host.cc (right):

https://codereview.chromium.org/1076093003/diff/1/remoting/host/setup/me2me_n...
remoting/host/setup/me2me_native_messaging_host.cc:165: } else if (type ==
"getTokenFromAuthCode") {
On 2015/04/22 20:11:25, John Williams wrote:
> On 2015/04/22 18:28:52, Lambros wrote:
> > Please update me2me_native_messaging_host_unittest.cc for this new request
> type.
> > You can simply validate that this new request triggers a correctly-formatted
> > response, similar to what we do for the other request types.
> 
> It looks like to do this, I will have to make remoting::OAuthClient abstract
and
> write a mock class.  Is that what I should do?  And if so, does this breakdown
> of files/classes work for you?
> 
>   OAuthClient(abstract)   oauth_client.{cc,h}
>   GaiaOAuthClient         oauth_client.{cc,h}
>   MockOAuthClient         mock_oauth_client.{cc,h}

OK, I thought it would be a simple addition to the existing unittests. But it
seems we've only mocked the DaemonController but not yet the OAuthClient. Your
suggestion looks good - we definitely want to have this, but I think it's fine
to do it as a separate CL.

[Sergey Ulanov, 2015-04-22 21:31:01]

On 2015/04/22 20:10:51, John Williams wrote:
> On 2015/04/22 18:24:34, Lambros wrote:
> > LG so far, but I have a concern about version-skew.
> > Do we need to add a new "capability" for this new "getTokenFromAuthCode"
> > request, and have the Chrome app test for the capability before making the
> > request to the native-messaging-host? Or perhaps it's OK for the request to
> fail
> > naturally?
> 
> Testing for the capability is a possible optimization.  Without testing first,
> the webapp will register the host with GCD and then immediately un-register it
> when the call to getTokenFromAuthCode fails.  Probably worth doing in a future
> CL, so I'll add a TODO.

In general we try to make all API changes backwards compatible, so you need to
add capability in this CL - it's just 2 extra lines of code, easier to add than
a TODO :)

[Sergey Ulanov, 2015-04-22 21:31:27]

On 2015/04/22 21:31:01, Sergey Ulanov wrote:
> On 2015/04/22 20:10:51, John Williams wrote:
> > On 2015/04/22 18:24:34, Lambros wrote:
> > > LG so far, but I have a concern about version-skew.
> > > Do we need to add a new "capability" for this new "getTokenFromAuthCode"
> > > request, and have the Chrome app test for the capability before making the
> > > request to the native-messaging-host? Or perhaps it's OK for the request
to
> > fail
> > > naturally?
> > 
> > Testing for the capability is a possible optimization.  Without testing
first,
> > the webapp will register the host with GCD and then immediately un-register
it
> > when the call to getTokenFromAuthCode fails.  Probably worth doing in a
future
> > CL, so I'll add a TODO.
> 
> In general we try to make all API changes backwards compatible, so you need to
> add capability in this CL - it's just 3 extra lines in this CL, easier to add
than
> a TODO :)

[Sergey Ulanov, 2015-04-22 21:55:40]

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

[Sergey Ulanov, 2015-04-22 21:55:41]

https://codereview.chromium.org/1076093003/diff/40001/remoting/host/setup/me2...
File remoting/host/setup/me2me_native_messaging_host.cc (right):

https://codereview.chromium.org/1076093003/diff/40001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host.cc:523:
response->SetString("userEmail", user_email);
This will set an empty email when processing getTokenFromAuthCode().

https://codereview.chromium.org/1076093003/diff/40001/remoting/host/setup/oau...
File remoting/host/setup/oauth_client.h (right):

https://codereview.chromium.org/1076093003/diff/40001/remoting/host/setup/oau...
remoting/host/setup/oauth_client.h:45: void GetCredentialsFromAuthCode(
OAuthClient is just a wrapper for GaiaOAuthClient. in GaiaOAuthClient there are
two separate calls - one to get refresh token and another one to get e-mail. I
suggest separating these two functions here too. I.e. there can get
GetRefreshToken() and separate GetUserEmail()

https://codereview.chromium.org/1076093003/diff/40001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/host_daemon_facade.js (right):

https://codereview.chromium.org/1076093003/diff/40001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/host_daemon_facade.js:521:
remoting.HostDaemonFacade.prototype.getTokenFromAuthCode =
Maybe call it GetRefreshTokenFromAuthCode() to make clear what kind of token we
get back?

[John Williams, 2015-04-24 00:00:47]

https://codereview.chromium.org/1076093003/diff/40001/remoting/host/setup/me2...
File remoting/host/setup/me2me_native_messaging_host.cc (right):

https://codereview.chromium.org/1076093003/diff/40001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host.cc:523:
response->SetString("userEmail", user_email);
On 2015/04/22 21:55:40, Sergey Ulanov wrote:
> This will set an empty email when processing getTokenFromAuthCode().

Fixed.

https://codereview.chromium.org/1076093003/diff/40001/remoting/host/setup/oau...
File remoting/host/setup/oauth_client.h (right):

https://codereview.chromium.org/1076093003/diff/40001/remoting/host/setup/oau...
remoting/host/setup/oauth_client.h:45: void GetCredentialsFromAuthCode(
On 2015/04/22 21:55:40, Sergey Ulanov wrote:
> OAuthClient is just a wrapper for GaiaOAuthClient. in GaiaOAuthClient there
are
> two separate calls - one to get refresh token and another one to get e-mail. I
> suggest separating these two functions here too. I.e. there can get
> GetRefreshToken() and separate GetUserEmail() 

I agree that would be cleaner, but I would rather not introduce a new method,
because modifying C++ code is a hassle, and once we switch over to using GCD,
there will be no need for it.  Eventually I plan to just rename this method and
remove the need_user_email parameter.

https://codereview.chromium.org/1076093003/diff/40001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/host_daemon_facade.js (right):

https://codereview.chromium.org/1076093003/diff/40001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/host_daemon_facade.js:521:
remoting.HostDaemonFacade.prototype.getTokenFromAuthCode =
On 2015/04/22 21:55:41, Sergey Ulanov wrote:
> Maybe call it GetRefreshTokenFromAuthCode() to make clear what kind of token
we
> get back?

Done.

[Lambros, 2015-04-24 17:27:16]

lgtm

[John Williams, 2015-04-24 21:10:38]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-04-24 21:10:51]

The CQ bit was unchecked by jrw@chromium.org

[John Williams, 2015-04-24 21:11:06]

The CQ bit was checked by jrw@chromium.org

[commit-bot: I haz the power, 2015-04-24 21:11:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1076093003/60001

[commit-bot: I haz the power, 2015-04-24 21:57:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-24 21:58:00]

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