Client plugin changes to support third party authentication.

Client plugin changes to support third party authentication. 

This creates a TokenFetcher implementation that on the client that uses the webapp to direct the user to fetch a token and secret from the given token URL. If the URL requires interactive authentication, the webapp will open a tab/window for the user to authenticate. Once the webapp has a token and shared secret, it uses the callback to send it back to the authentication layer. 

This also glues the third party authentication method to the NegotiatingAuthenticator.

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

[rmsousa, 2013-03-27 23:44:37]

I split the webapp changes from the client plugin changes. The corresponding
webapp changes are at https://codereview.chromium.org/12905012

This is fairly straightforward, just piping data up and down to the webapp. The
PepperTokenFetcher class is basically a way to make sure that the authenticator
owns the tokenfetcher (and the associated callback), and won't receive a
callback after deletion.

[Sergey Ulanov, 2013-03-28 21:59:25]

Looks mostly good, but I think you don't need TokenFetcherFactory interface.

https://codereview.chromium.org/12475020/diff/2001/remoting/client/chromoting...
File remoting/client/chromoting_client.cc (right):

https://codereview.chromium.org/12475020/diff/2001/remoting/client/chromoting...
remoting/client/chromoting_client.cc:58: config_.host_public_key,
user_interface_->GetTokenFetcherFactory(),
Why do we need to pass the factory here? we never expect single authenticator to
need more than one TokenFetcher, so I think you can just pass the TokenFetcher
here?

https://codereview.chromium.org/12475020/diff/2001/remoting/client/client_use...
File remoting/client/client_user_interface.h (right):

https://codereview.chromium.org/12475020/diff/2001/remoting/client/client_use...
remoting/client/client_user_interface.h:42: GetTokenFetcherFactory() = 0;
nit: normally function name is not indented when wrapped after type (though it's
not in the style guide :( ).

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.cc:140: "asyncPin
onThirdPartyTokenFetched";
call it thirdPartyAuth?

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.cc:475:
PostChromotingMessage("fetchThirdPartyToken", data.Pass());
What happens if the webapp doesn't support this message? I assume webapp will
add thirdParty auth in the list of auth methods only when it's supported, by
both the webapp and the plugin, right?

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.cc:514:
ChromotingInstance::GetTokenFetcherFactory() {
nit: no need to indent this line

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.cc:748: VLOG(1) <<
nit: move << to the next line. If the message is too long you can split it into
two parts.

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
File remoting/client/plugin/chromoting_instance.h (right):

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.h:115: GetTokenFetcherFactory()
OVERRIDE;
nit: don't indent this line

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.h:120: 
nit: remove this empty line

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.h:189: const std::string& token,
const std::string& shared_secret);
nit: one argument per line.

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.h:239: const std::string&
shared_secret)> on_third_party_token_fetched_;
This is not used anywhere. Remove it?

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/pep...
File remoting/client/plugin/pepper_token_fetcher.h (right):

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/pep...
remoting/client/plugin/pepper_token_fetcher.h:28: 
nit: remove empty line.

https://codereview.chromium.org/12475020/diff/2001/remoting/protocol/third_pa...
File remoting/protocol/third_party_client_authenticator.h (right):

https://codereview.chromium.org/12475020/diff/2001/remoting/protocol/third_pa...
remoting/protocol/third_party_client_authenticator.h:64: const std::string&
host_public_key) = 0;
Why did you need to move host_public_key argument here? I think it's better to
keep it in FetchThirdPartyToken()

[rmsousa, 2013-03-28 22:27:01]

(no code change yet, just quickly replying to some comments)

https://codereview.chromium.org/12475020/diff/2001/remoting/client/chromoting...
File remoting/client/chromoting_client.cc (right):

https://codereview.chromium.org/12475020/diff/2001/remoting/client/chromoting...
remoting/client/chromoting_client.cc:58: config_.host_public_key,
user_interface_->GetTokenFetcherFactory(),
On 2013/03/28 21:59:25, sergeyu wrote:
> Why do we need to pass the factory here? we never expect single authenticator
to
> need more than one TokenFetcher, so I think you can just pass the TokenFetcher
> here?

We don't expect a single ThirdPartyClientAuthenticator to need more than one,
but NegotiatingAuthenticator may need to create more than one underlying
ThirdPartyClientAuthenticator (not at the same time, but the protocol allows
either side to flip flop between methods at will, creating/destroying
authenticators in the process)

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.cc:475:
PostChromotingMessage("fetchThirdPartyToken", data.Pass());
On 2013/03/28 21:59:25, sergeyu wrote:
> What happens if the webapp doesn't support this message? I assume webapp will
> add thirdParty auth in the list of auth methods only when it's supported, by
> both the webapp and the plugin, right?

the webapp will add it if it supports it. If the plugin doesn't support it, it
won't be able to understand it (AuthenticatorMethod::FromString will return
invalid) and it will be ignored.
If the webapp doesn't support it, it won't be in the list, so the plugin won't
try to use it, and will never call fetchThirdPartyToken.

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
File remoting/client/plugin/chromoting_instance.h (right):

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.h:239: const std::string&
shared_secret)> on_third_party_token_fetched_;
On 2013/03/28 21:59:25, sergeyu wrote:
> This is not used anywhere. Remove it?

Yeah, leftover.

https://codereview.chromium.org/12475020/diff/2001/remoting/protocol/third_pa...
File remoting/protocol/third_party_client_authenticator.h (right):

https://codereview.chromium.org/12475020/diff/2001/remoting/protocol/third_pa...
remoting/protocol/third_party_client_authenticator.h:64: const std::string&
host_public_key) = 0;
On 2013/03/28 21:59:25, sergeyu wrote:
> Why did you need to move host_public_key argument here? I think it's better to
> keep it in FetchThirdPartyToken()

Mostly some classification of the parameters:
token_url, token_scope come in the wire from the host, so they're passed in by
the authenticator.
host_public_key is known at construction time, so it goes directly into the
fetcher.
This is mostly symmetrical with the TokenValidator, where the keypair is also
known at construction time and lives in the validator itself. (and ValidateToken
only takes the token as parameter).

I'm fine with whichever approach, but we should also look at TokenValidator to
make sure they can be consistent.

[Sergey Ulanov, 2013-03-28 22:50:06]

https://codereview.chromium.org/12475020/diff/2001/remoting/client/chromoting...
File remoting/client/chromoting_client.cc (right):

https://codereview.chromium.org/12475020/diff/2001/remoting/client/chromoting...
remoting/client/chromoting_client.cc:58: config_.host_public_key,
user_interface_->GetTokenFetcherFactory(),
On 2013/03/28 22:27:01, rmsousa wrote:
> On 2013/03/28 21:59:25, sergeyu wrote:
> > Why do we need to pass the factory here? we never expect single
authenticator
> to
> > need more than one TokenFetcher, so I think you can just pass the
TokenFetcher
> > here?
> 
> We don't expect a single ThirdPartyClientAuthenticator to need more than one,
> but NegotiatingAuthenticator may need to create more than one underlying
> ThirdPartyClientAuthenticator (not at the same time, but the protocol allows
> either side to flip flop between methods at will, creating/destroying
> authenticators in the process)

I think this shouldn't be part of the protocol. The client sends list of
supported methods, host chooses one, that's it. After that the method should be
fixed. I think it's a bug that current implementation allows changing the method
mid-flight. Seems like something we should fix. It might be worth separating
NegotiatingAuthenticator into host and client versions.

[rmsousa, 2013-04-05 01:24:42]

https://codereview.chromium.org/12475020/diff/2001/remoting/client/chromoting...
File remoting/client/chromoting_client.cc (right):

https://codereview.chromium.org/12475020/diff/2001/remoting/client/chromoting...
remoting/client/chromoting_client.cc:58: config_.host_public_key,
user_interface_->GetTokenFetcherFactory(),
On 2013/03/28 22:50:06, sergeyu wrote:
> On 2013/03/28 22:27:01, rmsousa wrote:
> > On 2013/03/28 21:59:25, sergeyu wrote:
> > > Why do we need to pass the factory here? we never expect single
> authenticator
> > to
> > > need more than one TokenFetcher, so I think you can just pass the
> TokenFetcher
> > > here?
> > 
> > We don't expect a single ThirdPartyClientAuthenticator to need more than
one,
> > but NegotiatingAuthenticator may need to create more than one underlying
> > ThirdPartyClientAuthenticator (not at the same time, but the protocol allows
> > either side to flip flop between methods at will, creating/destroying
> > authenticators in the process)
> 
> I think this shouldn't be part of the protocol. The client sends list of
> supported methods, host chooses one, that's it. After that the method should
be
> fixed. I think it's a bug that current implementation allows changing the
method
> mid-flight. Seems like something we should fix. It might be worth separating
> NegotiatingAuthenticator into host and client versions.

Done.

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.cc:140: "asyncPin
onThirdPartyTokenFetched";
On 2013/03/28 21:59:25, sergeyu wrote:
> call it thirdPartyAuth?

Done.

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.cc:514:
ChromotingInstance::GetTokenFetcherFactory() {
On 2013/03/28 21:59:25, sergeyu wrote:
> nit: no need to indent this line

Done.

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.cc:748: VLOG(1) <<
On 2013/03/28 21:59:25, sergeyu wrote:
> nit: move << to the next line. If the message is too long you can split it
into
> two parts.

Done.

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
File remoting/client/plugin/chromoting_instance.h (right):

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.h:115: GetTokenFetcherFactory()
OVERRIDE;
On 2013/03/28 21:59:25, sergeyu wrote:
> nit: don't indent this line

Done.

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.h:120: 
On 2013/03/28 21:59:25, sergeyu wrote:
> nit: remove this empty line

I remvoed the factory interface, and FetchThridPartyToken was in the wrong place
(it wasn't part of the interface).

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.h:189: const std::string& token,
const std::string& shared_secret);
On 2013/03/28 21:59:25, sergeyu wrote:
> nit: one argument per line.

Done.

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/pep...
File remoting/client/plugin/pepper_token_fetcher.h (right):

https://codereview.chromium.org/12475020/diff/2001/remoting/client/plugin/pep...
remoting/client/plugin/pepper_token_fetcher.h:28: 
On 2013/03/28 21:59:25, sergeyu wrote:
> nit: remove empty line.
It's not part of the interface. I added a comment.

[Sergey Ulanov, 2013-04-05 18:34:56]

lgtm

https://codereview.chromium.org/12475020/diff/13001/remoting/client/plugin/ch...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/12475020/diff/13001/remoting/client/plugin/ch...
remoting/client/plugin/chromoting_instance.cc:741: VLOG(1) << "Ignored
OnThirdPartyTokenFetched without a pending fetch.";
might be better to use LOG(WARNING) here.

https://codereview.chromium.org/12475020/diff/13001/remoting/client/plugin/pe...
File remoting/client/plugin/pepper_token_fetcher.h (right):

https://codereview.chromium.org/12475020/diff/13001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_token_fetcher.h:19: explicit
PepperTokenFetcher(base::WeakPtr<ChromotingInstance> plugin,
remove explicit.

https://codereview.chromium.org/12475020/diff/13001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_token_fetcher.h:30: virtual void
OnTokenFetched(const std::string& token,
Does this need to be virtual?

https://codereview.chromium.org/12475020/diff/13001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_token_fetcher.h:36:
base::WeakPtrFactory<PepperTokenFetcher> weak_factory_;
nit: I often doesn't matter, but we usually put weak_factory_ as last field, so
that it's destroyed first.

[rmsousa, 2013-04-06 01:11:03]

https://codereview.chromium.org/12475020/diff/13001/remoting/client/plugin/ch...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/12475020/diff/13001/remoting/client/plugin/ch...
remoting/client/plugin/chromoting_instance.cc:741: VLOG(1) << "Ignored
OnThirdPartyTokenFetched without a pending fetch.";
On 2013/04/05 18:34:57, sergeyu wrote:
> might be better to use LOG(WARNING) here.

Done. Changed the pin fetcher one as well.

https://codereview.chromium.org/12475020/diff/13001/remoting/client/plugin/pe...
File remoting/client/plugin/pepper_token_fetcher.h (right):

https://codereview.chromium.org/12475020/diff/13001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_token_fetcher.h:19: explicit
PepperTokenFetcher(base::WeakPtr<ChromotingInstance> plugin,
On 2013/04/05 18:34:57, sergeyu wrote:
> remove explicit.

Done.

https://codereview.chromium.org/12475020/diff/13001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_token_fetcher.h:30: virtual void
OnTokenFetched(const std::string& token,
On 2013/04/05 18:34:57, sergeyu wrote:
> Does this need to be virtual?

Done.

https://codereview.chromium.org/12475020/diff/13001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_token_fetcher.h:36:
base::WeakPtrFactory<PepperTokenFetcher> weak_factory_;
On 2013/04/05 18:34:57, sergeyu wrote:
> nit: I often doesn't matter, but we usually put weak_factory_ as last field,
so
> that it's destroyed first.

Good to know. I had noticed that pattern in our code, but hadn't really thought
about why it was that way.

[commit-bot: I haz the power, 2013-04-06 01:11:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12475020/20001

[commit-bot: I haz the power, 2013-04-06 01:26:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12475020/24002

[commit-bot: I haz the power, 2013-04-06 05:01:03]

Change committed as 192705