Webapp changes to support third party authentication

Webapp changes to support third party authentication

This uses an OAuth flow on the server to fetch the token and shared secret. There are two implementations for this:

* The current one manually opens a tab and asks for a redirect to a blank page in talkgadget, which we content-script to sendmessage the token/secret back to the extension (fairly similar to our OAuth trampoline)

* Once we're running on appsv2, and identity is out of experimental, we can use launchWebAuthFlow to do this.

This includes an interstitial to ask for an optional permission to the given host. The window.open method doesn't actually need this, but the identity API one does, so I thought I'd leave it in to make its behavior match closely the one of the identity API, which is the one we'll use in the future.

Most of the code is shared between these two versions, the only different pieces are the mechanics to open the window/launchWebFlow, and to send the redirectedUrl back to the webapp for parsing.

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

[rmsousa, 2013-03-27 23:47:21]

The associated client plugin changes are at
https://codereview.chromium.org/12475020/

This is missing the part where we match the Token URL against a whitelist/regex
for the domain (provided by the directory server). Other than that, it's fully
functional.

[Jamie, 2013-03-28 23:29:36]

https://codereview.chromium.org/12905012/diff/1/remoting/resources/remoting_s...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/12905012/diff/1/remoting/resources/remoting_s...
remoting/resources/remoting_strings.grd:147: </message>
I've referred elsewhere to the app simply as "Chrome Remote Desktop", rather
than "the Chrome Remote Desktop app". But in any case, you'll need to run this
past the UI leads before submitting. Follow up with me offline for the
procedure.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/client_sessio...
File remoting/webapp/client_session.js (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:715: var that = this;
I think this would be clearer using bind:

this.plugin.onThirdPartyTokenFetched.bind(this.plugin)

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/cs_third_part...
File remoting/webapp/cs_third_party_auth_trampoline.js (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/cs_third_part...
remoting/webapp/cs_third_party_auth_trampoline.js:8:
chrome.extension.sendMessage(window.location.href);
It would be more future-proof to make this a structured message type. Something
like:

{type:'thirdPartyAuthToken', url:window.local.href}

Or it might be cleaner to parse it here--I don't have a strong opinion on that.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/jscompiler_ha...
File remoting/webapp/jscompiler_hacks.js (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/jscompiler_ha...
remoting/webapp/jscompiler_hacks.js:184: /** @type {Object} */
You can declare this as type chrome.Event and add removeListener to the
prototype above.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/manifest.json
File remoting/webapp/manifest.json (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/manifest.json...
remoting/webapp/manifest.json:1: {
I'm a bit surprised not to see any changes to appsv2.patch, since you want to do
something different in appsv2 mode. Does appsv2.patch still apply cleanly after
this change?

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/session_conne...
File remoting/webapp/session_connector.js (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/session_conne...
remoting/webapp/session_connector.js:140: this.hostPublicKey_ = host.publicKey;
This feels like it should be a separate fix. Is it the case that the existing
auth methods didn't need this to be set, but thirdparty does?

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
File remoting/webapp/third_party_auth.js (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:11: * user to authenticate themselves). The
client then sends only the token to the
Can you clarify the parenthetical remark?

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:27: * @param {string} tokenUrl Token issue
URL received from the host.
s/Token issue/Token-issue/

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:39: this.clientId_ = hostPublicKey;
Could the member variable and the parameter have the same name?

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:43: '.chromiumapp.org/ThirdPartyAuth';
Why chromiumapp.org? It's not clear from the docs, but I think that might just
be an example. I would expect us to use THIRD_PARTY_AUTH_REDIRECT_URI here too.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:54: (new
remoting.ThirdPartyHostPermissions(this.tokenUrl_)).getPermission(
Please add a variable to store the result of 'new', for readability.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:55: this.fetchTokenInternal_.bind(this),
No need to bind a callback (see below for why it doesn't need to be a member
function).

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:56:
this.onThirdPartyTokenFetched_.bind(this, '', ''));
No need for bind here either.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:63:
remoting.ThirdPartyTokenFetcher.prototype.fetchTokenInternal_ = function() {};
You seem to be usig fetchTokenInternal_ both as a member variable in the ctor
and as a member function. I think the former is what you want, so you don't need
this declaration.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:77: (this.redirectUri_ + '#')) {
Would this be clearer as responseUrl.search(this.redirectUri_ + '#') == 0?

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:80: var queryArgs = {};
Can you explicitly declare this as type Object.<String> to avoid the type
coercion below?

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:87: // that can be exchanged for an
'access_token'. For our purposes, 'code' is
Is this naming under our control? It might be clearer not to reuse the term
'code'. Perhaps something like 'ticket' might be better?

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:90: if (state == this.xsrfToken_ && 'code'
in queryArgs) {
'states' and 'tokens' are very different concepts. Why are we comparing them for
equality here? It seems that one or the other has a misleading name.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:93: sharedSecret = /** @type string */
queryArgs['access_token'];
{} around the type, if it's needed at all (see above).

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:123: /** @type
remoting.ThirdPartyTokenFetcher */
{} around types, here and elsewhere.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:149: that.parseRedirectUrl_(responseUrl);
});
This would be clearer with bind.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:176: if (allowed) {
Nit: Indentation.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:230: var that = this;
You don't seem to be using this.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/ui_mode.js
File remoting/webapp/ui_mode.js (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/ui_mode.js#ne...
remoting/webapp/ui_mode.js:43: CONFIRM_FETCH_TOKEN: 'home.confirm-fetch-token',
You don't seem to be using this.

[rmsousa, 2013-03-29 04:08:21]

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/client_sessio...
File remoting/webapp/client_session.js (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:715: var that = this;
On 2013/03/28 23:29:36, Jamie wrote:
> I think this would be clearer using bind:
> 
> this.plugin.onThirdPartyTokenFetched.bind(this.plugin)

Done.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/cs_third_part...
File remoting/webapp/cs_third_party_auth_trampoline.js (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/cs_third_part...
remoting/webapp/cs_third_party_auth_trampoline.js:8:
chrome.extension.sendMessage(window.location.href);
On 2013/03/28 23:29:36, Jamie wrote:
> It would be more future-proof to make this a structured message type.
Something
> like:
> 
> {type:'thirdPartyAuthToken', url:window.local.href}
> 
> Or it might be cleaner to parse it here--I don't have a strong opinion on
that.

The sendMessage parameter is string typed. passing the redirectURL as the
message allows us to maintain the same interface as launchWebAuthFlow (that
calls its callback with the redirectURL when done).

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/jscompiler_ha...
File remoting/webapp/jscompiler_hacks.js (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/jscompiler_ha...
remoting/webapp/jscompiler_hacks.js:184: /** @type {Object} */
On 2013/03/28 23:29:36, Jamie wrote:
> You can declare this as type chrome.Event and add removeListener to the
> prototype above.

Done.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/manifest.json
File remoting/webapp/manifest.json (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/manifest.json...
remoting/webapp/manifest.json:1: {
On 2013/03/28 23:29:36, Jamie wrote:
> I'm a bit surprised not to see any changes to appsv2.patch, since you want to
do
> something different in appsv2 mode. Does appsv2.patch still apply cleanly
after
> this change? 

I'll update the patch to apply cleanly once the rest of the code is final.
There's no logic that depends specifically on appsv2 - the different behavior is
based on whether the identity API is available or not, and it tests for that
where appropriate.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/session_conne...
File remoting/webapp/session_connector.js (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/session_conne...
remoting/webapp/session_connector.js:140: this.hostPublicKey_ = host.publicKey;
On 2013/03/28 23:29:36, Jamie wrote:
> This feels like it should be a separate fix. Is it the case that the existing
> auth methods didn't need this to be set, but thirdparty does?

Yes. Aside from ThirdPartyAuth, hostPublicKey is just stored in ConnectionToHost
and then never used.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
File remoting/webapp/third_party_auth.js (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:11: * user to authenticate themselves). The
client then sends only the token to the
On 2013/03/28 23:29:36, Jamie wrote:
> Can you clarify the parenthetical remark?

Done.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:27: * @param {string} tokenUrl Token issue
URL received from the host.
On 2013/03/28 23:29:36, Jamie wrote:
> s/Token issue/Token-issue/

Done.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:39: this.clientId_ = hostPublicKey;
On 2013/03/28 23:29:36, Jamie wrote:
> Could the member variable and the parameter have the same name?

Done.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:39: this.clientId_ = hostPublicKey;
On 2013/03/28 23:29:36, Jamie wrote:
> Could the member variable and the parameter have the same name?

Yeah, I had kinda mixed OAuth param names with actual contents here. Changed it
so that all member variables are named for the actual contents.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:43: '.chromiumapp.org/ThirdPartyAuth';
On 2013/03/28 23:29:36, Jamie wrote:
> Why chromiumapp.org? It's not clear from the docs, but I think that might just
> be an example. I would expect us to use THIRD_PARTY_AUTH_REDIRECT_URI here
too.

It's not just an example. The identity API captures a redirect to
https://<extension-id>.chromiumapp.org, and gives the resulting URL to the app
with <extension-id> to parse. See
http://developer.chrome.com/apps/app_identity.html#non and
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:54: (new
remoting.ThirdPartyHostPermissions(this.tokenUrl_)).getPermission(
On 2013/03/28 23:29:36, Jamie wrote:
> Please add a variable to store the result of 'new', for readability.

Done.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:55: this.fetchTokenInternal_.bind(this),
On 2013/03/28 23:29:36, Jamie wrote:
> No need to bind a callback (see below for why it doesn't need to be a member
> function).

The implementations reference |this|, so they need to be bound.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:56:
this.onThirdPartyTokenFetched_.bind(this, '', ''));
On 2013/03/28 23:29:36, Jamie wrote:
> No need for bind here either.

I need to bind the extra parameters (the empty strings).

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:63:
remoting.ThirdPartyTokenFetcher.prototype.fetchTokenInternal_ = function() {};
On 2013/03/28 23:29:36, Jamie wrote:
> You seem to be usig fetchTokenInternal_ both as a member variable in the ctor
> and as a member function. I think the former is what you want, so you don't
need
> this declaration.

Done.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:77: (this.redirectUri_ + '#')) {
On 2013/03/28 23:29:36, Jamie wrote:
> Would this be clearer as responseUrl.search(this.redirectUri_ + '#') == 0?

Done.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:80: var queryArgs = {};
On 2013/03/28 23:29:36, Jamie wrote:
> Can you explicitly declare this as type Object.<String> to avoid the type
> coercion below?

Done.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:87: // that can be exchanged for an
'access_token'. For our purposes, 'code' is
On 2013/03/28 23:29:36, Jamie wrote:
> Is this naming under our control? It might be clearer not to reuse the term
> 'code'. Perhaps something like 'ticket' might be better?

The OAuth terms are not under our control. "code" and "token" refer to specific
types of response and semantics in OAuth (a "code" is something the user gets
when it authenticates, and a "token" is what the application must exchange the
"code" for in order to authenticate".

In our case, basically the OAuth "code" semantics is what we desire for what we
call internally our "token", and the OAuth "token" semantics is what we want for
our "sharedSecret". This piece of code traverses between the two naming schemes,
and tries to explain that mapping. I updated the comment to explain the
mapping/semantics, and tried to make at least the naming that is under our
control consistent with our internal usage (even though it results in a variable
ending up with the generic name "token").

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:90: if (state == this.xsrfToken_ && 'code'
in queryArgs) {
On 2013/03/28 23:29:36, Jamie wrote:
> 'states' and 'tokens' are very different concepts. Why are we comparing them
for
> equality here? It seems that one or the other has a misleading name.

Again, OAuth terminology vs. reality. The name "state" is the name of the OAuth
parameter that the client sends to the server and receives verbatim on the
redirect URL. What we (and pretty much everyone who uses OAuth) use that
parameter for is for XSRF prevention, by means of an XSRF token. See
http://tools.ietf.org/html/draft-ietf-oauth-v2-31#section-10.12

We can change the name of the variable to xsrfToken, but there will still be a
"var xsrfToken = decodeURIComponent(queryArgs['state'])" line above.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:93: sharedSecret = /** @type string */
queryArgs['access_token'];
On 2013/03/28 23:29:36, Jamie wrote:
> {} around the type, if it's needed at all (see above).

Done.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:123: /** @type
remoting.ThirdPartyTokenFetcher */
On 2013/03/28 23:29:36, Jamie wrote:
> {} around types, here and elsewhere.

Done.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:149: that.parseRedirectUrl_(responseUrl);
});
On 2013/03/28 23:29:36, Jamie wrote:
> This would be clearer with bind.

Done.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:176: if (allowed) {
On 2013/03/28 23:29:36, Jamie wrote:
> Nit: Indentation.

Done.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:230: var that = this;
On 2013/03/28 23:29:36, Jamie wrote:
> You don't seem to be using this.

Done.

[Jamie, 2013-03-29 18:19:23]

https://codereview.chromium.org/12905012/diff/1/remoting/resources/remoting_s...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/12905012/diff/1/remoting/resources/remoting_s...
remoting/resources/remoting_strings.grd:147: </message>
On 2013/03/28 23:29:36, Jamie wrote:
> I've referred elsewhere to the app simply as "Chrome Remote Desktop", rather
> than "the Chrome Remote Desktop app". But in any case, you'll need to run this
> past the UI leads before submitting. Follow up with me offline for the
> procedure.

Did you see this comment?

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/session_conne...
File remoting/webapp/session_connector.js (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/session_conne...
remoting/webapp/session_connector.js:140: this.hostPublicKey_ = host.publicKey;
On 2013/03/29 04:08:21, rmsousa wrote:
> On 2013/03/28 23:29:36, Jamie wrote:
> > This feels like it should be a separate fix. Is it the case that the
existing
> > auth methods didn't need this to be set, but thirdparty does?
> 
> Yes. Aside from ThirdPartyAuth, hostPublicKey is just stored in
ConnectionToHost
> and then never used.

Generally, things like this should be in a separate CL. That way, if we ever
need to revert this one for any reason, we don't end up reverting the bugfix as
well.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
File remoting/webapp/third_party_auth.js (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:55: this.fetchTokenInternal_.bind(this),
On 2013/03/29 04:08:21, rmsousa wrote:
> On 2013/03/28 23:29:36, Jamie wrote:
> > No need to bind a callback (see below for why it doesn't need to be a member
> > function).
> 
> The implementations reference |this|, so they need to be bound.

Okay, I see now. It's not clear at this point of the code that
fetchTokenInternal_ is actually a member function masquerading as a callback
pointer. I think it would be clearer to do the bind at the point where you're
setting the member variable, rather than here. It would be a better match with
how bind is used elsewhere (and with the semantics of base::Bind in C++).

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:56:
this.onThirdPartyTokenFetched_.bind(this, '', ''));
On 2013/03/29 04:08:21, rmsousa wrote:
> On 2013/03/28 23:29:36, Jamie wrote:
> > No need for bind here either.
> 
> I need to bind the extra parameters (the empty strings).

I think it would be clearer to have a separate member variable for the failure
case and bind it at the point of assignment.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:90: if (state == this.xsrfToken_ && 'code'
in queryArgs) {
On 2013/03/29 04:08:21, rmsousa wrote:
> On 2013/03/28 23:29:36, Jamie wrote:
> > 'states' and 'tokens' are very different concepts. Why are we comparing them
> for
> > equality here? It seems that one or the other has a misleading name.
> 
> Again, OAuth terminology vs. reality. The name "state" is the name of the
OAuth
> parameter that the client sends to the server and receives verbatim on the
> redirect URL. What we (and pretty much everyone who uses OAuth) use that
> parameter for is for XSRF prevention, by means of an XSRF token. See
> http://tools.ietf.org/html/draft-ietf-oauth-v2-31#section-10.12
> 
> We can change the name of the variable to xsrfToken, but there will still be a
> "var xsrfToken = decodeURIComponent(queryArgs['state'])" line above.

I think that would be clearer, since that's the line where we're taking
something named by OAuth and storing it in something where we choose the name.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/ui_mode.js
File remoting/webapp/ui_mode.js (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/ui_mode.js#ne...
remoting/webapp/ui_mode.js:43: CONFIRM_FETCH_TOKEN: 'home.confirm-fetch-token',
On 2013/03/28 23:29:36, Jamie wrote:
> You don't seem to be using this.

Did you see this comment?

https://codereview.chromium.org/12905012/diff/9001/remoting/webapp/third_part...
File remoting/webapp/third_party_auth.js (right):

https://codereview.chromium.org/12905012/diff/9001/remoting/webapp/third_part...
remoting/webapp/third_party_auth.js:144: /** @param {string} responseUrl The
full redirection URL. */
I don't think you need this annotation any more.

[rmsousa, 2013-03-29 20:55:48]

https://codereview.chromium.org/12905012/diff/1/remoting/resources/remoting_s...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/12905012/diff/1/remoting/resources/remoting_s...
remoting/resources/remoting_strings.grd:147: </message>
On 2013/03/29 18:19:24, Jamie wrote:
> On 2013/03/28 23:29:36, Jamie wrote:
> > I've referred elsewhere to the app simply as "Chrome Remote Desktop", rather
> > than "the Chrome Remote Desktop app". But in any case, you'll need to run
this
> > past the UI leads before submitting. Follow up with me offline for the
> > procedure.
> 
> Did you see this comment?

Yup, I had left it alone since it'll likely change after the review anyway. I'll
prepare some screenshots for the UI review.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
File remoting/webapp/third_party_auth.js (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:55: this.fetchTokenInternal_.bind(this),
On 2013/03/29 18:19:24, Jamie wrote:
> On 2013/03/29 04:08:21, rmsousa wrote:
> > On 2013/03/28 23:29:36, Jamie wrote:
> > > No need to bind a callback (see below for why it doesn't need to be a
member
> > > function).
> > 
> > The implementations reference |this|, so they need to be bound.
> 
> Okay, I see now. It's not clear at this point of the code that
> fetchTokenInternal_ is actually a member function masquerading as a callback
> pointer. I think it would be clearer to do the bind at the point where you're
> setting the member variable, rather than here. It would be a better match with
> how bind is used elsewhere (and with the semantics of base::Bind in C++).

Agreed.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:56:
this.onThirdPartyTokenFetched_.bind(this, '', ''));
On 2013/03/29 18:19:24, Jamie wrote:
> On 2013/03/29 04:08:21, rmsousa wrote:
> > On 2013/03/28 23:29:36, Jamie wrote:
> > > No need for bind here either.
> > 
> > I need to bind the extra parameters (the empty strings).
> 
> I think it would be clearer to have a separate member variable for the failure
> case and bind it at the point of assignment.

Done.
Since I was cleaning this up, I decided to use function() {
onThirdPartyTokenFetched_('','') } rather than bind, to avoid passing "this" as
a bind parameter to a callback.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/third_party_a...
remoting/webapp/third_party_auth.js:90: if (state == this.xsrfToken_ && 'code'
in queryArgs) {
On 2013/03/29 18:19:24, Jamie wrote:
> On 2013/03/29 04:08:21, rmsousa wrote:
> > On 2013/03/28 23:29:36, Jamie wrote:
> > > 'states' and 'tokens' are very different concepts. Why are we comparing
them
> > for
> > > equality here? It seems that one or the other has a misleading name.
> > 
> > Again, OAuth terminology vs. reality. The name "state" is the name of the
> OAuth
> > parameter that the client sends to the server and receives verbatim on the
> > redirect URL. What we (and pretty much everyone who uses OAuth) use that
> > parameter for is for XSRF prevention, by means of an XSRF token. See
> > http://tools.ietf.org/html/draft-ietf-oauth-v2-31#section-10.12
> > 
> > We can change the name of the variable to xsrfToken, but there will still be
a
> > "var xsrfToken = decodeURIComponent(queryArgs['state'])" line above.
> 
> I think that would be clearer, since that's the line where we're taking
> something named by OAuth and storing it in something where we choose the name.

Done.

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/ui_mode.js
File remoting/webapp/ui_mode.js (right):

https://codereview.chromium.org/12905012/diff/1/remoting/webapp/ui_mode.js#ne...
remoting/webapp/ui_mode.js:43: CONFIRM_FETCH_TOKEN: 'home.confirm-fetch-token',
On 2013/03/29 18:19:24, Jamie wrote:
> On 2013/03/28 23:29:36, Jamie wrote:
> > You don't seem to be using this.
> 
> Did you see this comment?

Oops, I had missed this one, sorry, done.

https://codereview.chromium.org/12905012/diff/9001/remoting/webapp/third_part...
File remoting/webapp/third_party_auth.js (right):

https://codereview.chromium.org/12905012/diff/9001/remoting/webapp/third_part...
remoting/webapp/third_party_auth.js:144: /** @param {string} responseUrl The
full redirection URL. */
On 2013/03/29 18:19:24, Jamie wrote:
> I don't think you need this annotation any more.

Done.

[Jamie, 2013-03-29 21:34:53]

Looks good once the string review is finalized and the patch has been updated.

https://codereview.chromium.org/12905012/diff/1/remoting/resources/remoting_s...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/12905012/diff/1/remoting/resources/remoting_s...
remoting/resources/remoting_strings.grd:147: </message>
On 2013/03/29 20:55:48, rmsousa wrote:
> On 2013/03/29 18:19:24, Jamie wrote:
> > On 2013/03/28 23:29:36, Jamie wrote:
> > > I've referred elsewhere to the app simply as "Chrome Remote Desktop",
rather
> > > than "the Chrome Remote Desktop app". But in any case, you'll need to run
> this
> > > past the UI leads before submitting. Follow up with me offline for the
> > > procedure.
> > 
> > Did you see this comment?
> 
> Yup, I had left it alone since it'll likely change after the review anyway.
I'll
> prepare some screenshots for the UI review.

Okay, fair enough. But you might as well change it to "Chrome Remote Desktop"
(get rid of "the" and "app"), since that's the first thing I'm going to ask for
on the review thread :)

https://codereview.chromium.org/12905012/diff/20001/remoting/webapp/client_pl...
File remoting/webapp/client_plugin_async.js (right):

https://codereview.chromium.org/12905012/diff/20001/remoting/webapp/client_pl...
remoting/webapp/client_plugin_async.js:44: * @param {string} tokenUrl Token
request URL, received from the host.
Sorry, I missed this one: s/Token request/Token-request/ please.

[rmsousa, 2013-04-15 23:56:57]

PTAL (again)

I've added the verification of the token URL against the domain-permitted
patterns provided by the directory-server. Aside from that, I've also refactored
the code around a bit, to organize it the same way as the pin-fetch codepath
(since, aside from the UI/implementation bits, their (code) interface is very
similar).

https://codereview.chromium.org/12905012/diff/1/remoting/resources/remoting_s...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/12905012/diff/1/remoting/resources/remoting_s...
remoting/resources/remoting_strings.grd:147: </message>
On 2013/03/29 21:34:53, Jamie wrote:
> On 2013/03/29 20:55:48, rmsousa wrote:
> > On 2013/03/29 18:19:24, Jamie wrote:
> > > On 2013/03/28 23:29:36, Jamie wrote:
> > > > I've referred elsewhere to the app simply as "Chrome Remote Desktop",
> rather
> > > > than "the Chrome Remote Desktop app". But in any case, you'll need to
run
> > this
> > > > past the UI leads before submitting. Follow up with me offline for the
> > > > procedure.
> > > 
> > > Did you see this comment?
> > 
> > Yup, I had left it alone since it'll likely change after the review anyway.
> I'll
> > prepare some screenshots for the UI review.
> 
> Okay, fair enough. But you might as well change it to "Chrome Remote Desktop"
> (get rid of "the" and "app"), since that's the first thing I'm going to ask
for
> on the review thread :)

Done.

https://codereview.chromium.org/12905012/diff/20001/remoting/webapp/client_pl...
File remoting/webapp/client_plugin_async.js (right):

https://codereview.chromium.org/12905012/diff/20001/remoting/webapp/client_pl...
remoting/webapp/client_plugin_async.js:44: * @param {string} tokenUrl Token
request URL, received from the host.
On 2013/03/29 21:34:53, Jamie wrote:
> Sorry, I missed this one: s/Token request/Token-request/ please.

Done.

[Jamie, 2013-04-16 20:57:20]

https://codereview.chromium.org/12905012/diff/29001/remoting/resources/remoti...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/12905012/diff/29001/remoting/resources/remoti...
remoting/resources/remoting_strings.grd:146: This remote host requires you to
authenticate to a third-party website. To continue, you must grant Chrome Remote
Desktop additional permissions to access this address:
s/This/The/, here and below.

https://codereview.chromium.org/12905012/diff/29001/remoting/webapp/third_par...
File remoting/webapp/third_party_auth.js (right):

https://codereview.chromium.org/12905012/diff/29001/remoting/webapp/third_par...
remoting/webapp/third_party_auth.js:71: 
Missing return here?

https://codereview.chromium.org/12905012/diff/29001/remoting/webapp/third_par...
remoting/webapp/third_party_auth.js:75: console.error("Token URL does not match
the domain's allowed URL patterns. " +
Single quotes for JS string, please.

https://codereview.chromium.org/12905012/diff/29001/remoting/webapp/third_par...
remoting/webapp/third_party_auth.js:98: queryArgs[pair[0]] = pair[1];
I think it would be better to use decodeURIComponent when storing the value, not
when retrieving it. You should probably also call it for the keys as well as the
values.

https://codereview.chromium.org/12905012/diff/29001/remoting/webapp/third_par...
remoting/webapp/third_party_auth.js:168: {'url': fullTokenUrl, 'interactive':
true},
For the non-third-party flow, ISTR that you have to try with interactive: false
first, otherwise it won't work if you're already authenticated. Does that
limitation not also apply to the third-party flow?

https://codereview.chromium.org/12905012/diff/29001/remoting/webapp/third_par...
remoting/webapp/third_party_auth.js:178: remoting.ThirdPartyHostPermissions =
function(url) {
I think this class is big enough to warrant a separate source file.

[rmsousa, 2013-04-17 03:48:46]

https://codereview.chromium.org/12905012/diff/29001/remoting/webapp/third_par...
File remoting/webapp/third_party_auth.js (right):

https://codereview.chromium.org/12905012/diff/29001/remoting/webapp/third_par...
remoting/webapp/third_party_auth.js:71: 
On 2013/04/16 20:57:20, Jamie wrote:
> Missing return here?

Done.

https://codereview.chromium.org/12905012/diff/29001/remoting/webapp/third_par...
remoting/webapp/third_party_auth.js:75: console.error("Token URL does not match
the domain's allowed URL patterns. " +
On 2013/04/16 20:57:20, Jamie wrote:
> Single quotes for JS string, please.

Done.

https://codereview.chromium.org/12905012/diff/29001/remoting/webapp/third_par...
remoting/webapp/third_party_auth.js:98: queryArgs[pair[0]] = pair[1];
On 2013/04/16 20:57:20, Jamie wrote:
> I think it would be better to use decodeURIComponent when storing the value,
not
> when retrieving it. You should probably also call it for the keys as well as
the
> values.

Done.

https://codereview.chromium.org/12905012/diff/29001/remoting/webapp/third_par...
remoting/webapp/third_party_auth.js:168: {'url': fullTokenUrl, 'interactive':
true},
On 2013/04/16 20:57:20, Jamie wrote:
> For the non-third-party flow, ISTR that you have to try with interactive:
false
> first, otherwise it won't work if you're already authenticated. Does that
> limitation not also apply to the third-party flow?

Is that related to trying non-interactive first, or to being executed in a user
gesture context?

Right now launchWebAuthFlow doesn't seem to have a problem with running directly
with interactive: true, and the page
(http://developer.chrome.com/trunk/apps/app_identity.html) mentions that the
gesture requirement is not enforced. I'd rather keep it simple for now, rather
than adding another user-gesture interstitial.

https://codereview.chromium.org/12905012/diff/29001/remoting/webapp/third_par...
remoting/webapp/third_party_auth.js:178: remoting.ThirdPartyHostPermissions =
function(url) {
On 2013/04/16 20:57:20, Jamie wrote:
> I think this class is big enough to warrant a separate source file.

Done.

But it's quite closely related to the third party auth, and not really reusable
anywhere else, as it uses the third party auth specific strings/UI for the user
consent dialog.

[Jamie, 2013-04-17 19:16:51]

lgtm

https://codereview.chromium.org/12905012/diff/29001/remoting/webapp/third_par...
File remoting/webapp/third_party_auth.js (right):

https://codereview.chromium.org/12905012/diff/29001/remoting/webapp/third_par...
remoting/webapp/third_party_auth.js:168: {'url': fullTokenUrl, 'interactive':
true},
On 2013/04/17 03:48:46, rmsousa wrote:
> On 2013/04/16 20:57:20, Jamie wrote:
> > For the non-third-party flow, ISTR that you have to try with interactive:
> false
> > first, otherwise it won't work if you're already authenticated. Does that
> > limitation not also apply to the third-party flow?
> 
> Is that related to trying non-interactive first, or to being executed in a
user
> gesture context?

You may be right. It's been a while since I played with it. In any case, what
you're doing here seems fine.

[commit-bot: I haz the power, 2013-04-25 22:44:31]

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

[commit-bot: I haz the power, 2013-04-26 02:35:54]

Change committed as 196580