Third Party Authentication for Android Part I - TokenFetcherProxy

Third Party Authentication for Android Part I - TokenFetcherProxy.

This is part I of the effort to support third party authentication for android.

The TokenFetcherProxy is generalized version of the existing PepperTokenFetcher.  It is used by the ThirdPartyClientAuthenticator to fetch auth tokens from both the webapp and the android client.  

BUG=329109
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275386

[Sergey Ulanov, 2014-06-04 03:49:18]

https://codereview.chromium.org/311983003/diff/20001/remoting/client/jni/jni_...
File remoting/client/jni/jni_token_fetcher.h (right):

https://codereview.chromium.org/311983003/diff/20001/remoting/client/jni/jni_...
remoting/client/jni/jni_token_fetcher.h:18: class JniTokenFetcher
This looks a lot like PepperTokenFetcher. I think we can avoid duplicating this
logic here. I think it should be possible to refactor it so that both
ChromotingInstance and CHromotingJniInstance can use the same class. E.g.
instead of passing jni_instance you can pass a callback for
jni_instance_->FetchThirdPartyToken() (created with weak pointer).

[kelvinp, 2014-06-04 21:13:53]

PTAL

https://codereview.chromium.org/311983003/diff/20001/remoting/client/jni/jni_...
File remoting/client/jni/jni_token_fetcher.h (right):

https://codereview.chromium.org/311983003/diff/20001/remoting/client/jni/jni_...
remoting/client/jni/jni_token_fetcher.h:18: class JniTokenFetcher
On 2014/06/04 03:49:17, Sergey Ulanov wrote:
> This looks a lot like PepperTokenFetcher. I think we can avoid duplicating
this
> logic here. I think it should be possible to refactor it so that both
> ChromotingInstance and CHromotingJniInstance can use the same class. E.g.
> instead of passing jni_instance you can pass a callback for
> jni_instance_->FetchThirdPartyToken() (created with weak pointer).

Done.

[Lambros, 2014-06-04 22:20:19]

JNI changes LG, but I'll defer to Sergey on the rest.

https://codereview.chromium.org/311983003/diff/40001/remoting/client/jni/chro...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/311983003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:115: // android app (Android
Beyond Corp - Part II).
nit: Third Party Authentication (or just remove this bit).

https://codereview.chromium.org/311983003/diff/40001/remoting/client/token_fe...
File remoting/client/token_fetcher_proxy.h (right):

https://codereview.chromium.org/311983003/diff/40001/remoting/client/token_fe...
remoting/client/token_fetcher_proxy.h:5: #ifndef
REMOTING_PROTOCOL_TOKEN_FETCHER_PROXY_H_
nit: s/PROTOCOL/CLIENT (same at bottom of file)

[rmsousa, 2014-06-04 22:52:14]

lgtm

[anandc, 2014-06-05 18:19:25]

On 2014/06/04 22:52:14, rmsousa wrote:
> lgtm

Chiming in late here: are there any unit-tests we could add for this?

[kelvinp, 2014-06-05 18:30:05]

On 2014/06/05 18:19:25, anandc wrote:
> On 2014/06/04 22:52:14, rmsousa wrote:
> > lgtm
> 
> Chiming in late here: are there any unit-tests we could add for this?

We could potentially.  But since all we do in this class is to forward the call
to TokenFetcherCallback.  The unittest won't be very interesting here.

[Sergey Ulanov, 2014-06-05 22:04:01]

https://codereview.chromium.org/311983003/diff/40001/remoting/client/token_fe...
File remoting/client/token_fetcher_proxy.h (right):

https://codereview.chromium.org/311983003/diff/40001/remoting/client/token_fe...
remoting/client/token_fetcher_proxy.h:22: TokenFetcherProxy(TokenFetcherCallback
token_fetcher_impl,
nit: const reference

https://codereview.chromium.org/311983003/diff/40001/remoting/client/token_fe...
remoting/client/token_fetcher_proxy.h:37: std::string host_public_key_;
This is not used anywhere. Can it be removed (and the constructor argument)?

https://codereview.chromium.org/311983003/diff/40001/remoting/client/token_fe...
remoting/client/token_fetcher_proxy.h:39: TokenFetcherCallback
token_fetcher_impl_;
nit: move this to top of the list, where plugin_ used to be, so that all
initialization arguments come before everything else.

[Sergey Ulanov, 2014-06-05 22:06:18]

lgtm when my comments are addressed

https://codereview.chromium.org/311983003/diff/40001/remoting/client/token_fe...
File remoting/client/token_fetcher_proxy.h (right):

https://codereview.chromium.org/311983003/diff/40001/remoting/client/token_fe...
remoting/client/token_fetcher_proxy.h:23: const std::string& host_public_key);
please mark constructor as explicit if you remove the second parameter per my
comment about |host_public_key_|.

[kelvinp, 2014-06-05 23:53:14]

https://codereview.chromium.org/311983003/diff/40001/remoting/client/jni/chro...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/311983003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:115: // android app (Android
Beyond Corp - Part II).
On 2014/06/04 22:20:19, Lambros wrote:
> nit: Third Party Authentication (or just remove this bit).

Done.

https://codereview.chromium.org/311983003/diff/40001/remoting/client/token_fe...
File remoting/client/token_fetcher_proxy.h (right):

https://codereview.chromium.org/311983003/diff/40001/remoting/client/token_fe...
remoting/client/token_fetcher_proxy.h:5: #ifndef
REMOTING_PROTOCOL_TOKEN_FETCHER_PROXY_H_
On 2014/06/04 22:20:19, Lambros wrote:
> nit: s/PROTOCOL/CLIENT (same at bottom of file)

Done.

https://codereview.chromium.org/311983003/diff/40001/remoting/client/token_fe...
remoting/client/token_fetcher_proxy.h:22: TokenFetcherProxy(TokenFetcherCallback
token_fetcher_impl,
On 2014/06/05 22:04:01, Sergey Ulanov wrote:
> nit: const reference 

Done.

https://codereview.chromium.org/311983003/diff/40001/remoting/client/token_fe...
remoting/client/token_fetcher_proxy.h:23: const std::string& host_public_key);
On 2014/06/05 22:06:18, Sergey Ulanov wrote:
> please mark constructor as explicit if you remove the second parameter per my
> comment about |host_public_key_|.

Turns out |host_public_key| is needed.  See comment below.

https://codereview.chromium.org/311983003/diff/40001/remoting/client/token_fe...
remoting/client/token_fetcher_proxy.h:37: std::string host_public_key_;
On 2014/06/05 22:04:01, Sergey Ulanov wrote:
> This is not used anywhere. Can it be removed (and the constructor argument)?

Good catch.  This should be used when calling to the TokenProxy

https://codereview.chromium.org/311983003/diff/40001/remoting/client/token_fe...
remoting/client/token_fetcher_proxy.h:39: TokenFetcherCallback
token_fetcher_impl_;
On 2014/06/05 22:04:01, Sergey Ulanov wrote:
> nit: move this to top of the list, where plugin_ used to be, so that all
> initialization arguments come before everything else.

Done.

[kelvinp, 2014-06-05 23:53:22]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-06-05 23:56:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kelvinp@chromium.org/311983003/60001

[commit-bot: I haz the power, 2014-06-06 10:50:53]

Change committed as 275386