Apps v2 identity integration.

Apps v2 identity integration.

This CL adds support for Chrome's identity API, part of the Apps v2 migration effort:
* Add identity.js, wrapping chrome.experimental.identity and handling user consent UI automatically.
* Since the consent UI is now shown on-demand, it's better handled as an explicit overlay rather than as a AppMode, so remove home.auth.
* Move access token and email address functionality out of oauth2.js into identiry.js.
* Add stub code to simplify the transition to using the new code.

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

[Jamie, 2013-01-04 02:17:45]

ptal

[rmsousa, 2013-01-04 02:44:33]

On 2013/01/04 02:17:45, Jamie wrote:
> ptal

Drive-by suggestion:
Could you apply the patch, upload the patched version, then unpatch and upload
again? That makes it easier to look at what the patched code will look like, and
use side-by-side to view the patch.

[Jamie, 2013-01-04 18:19:56]

On 2013/01/04 02:44:33, rmsousa wrote:
> On 2013/01/04 02:17:45, Jamie wrote:
> > ptal
> 
> Drive-by suggestion:
> Could you apply the patch, upload the patched version, then unpatch and upload
> again? That makes it easier to look at what the patched code will look like,
and
> use side-by-side to view the patch.

I'm not sure that would be particularly useful. I attached the patch so that it
would be easy to test this change. It doesn't make the web-app fully functional
for apps v2, nor is it a minimal change to test this CL--it's just what I
happened to be working with before I trimmed out stuff that wasn't needed to
support the identity API. I'll keep it up-to-date as I iterate on the apps v2
work.

[Wez, 2013-01-04 21:09:41]

I think it'd be cleaner to keep the Apps v2 patch separate, and perhaps upload
it to a CL for reference in this and related reviews.

[Jamie, 2013-01-04 21:27:47]

On 2013/01/04 21:09:41, Wez wrote:
> I think it'd be cleaner to keep the Apps v2 patch separate, and perhaps upload
> it to a CL for reference in this and related reviews.

Okay, I've removed the patch. I'll upload a minimal one (just what's required to
test the apps v2 features implemented so far) once this has landed.

[Wez, 2013-01-05 00:04:24]

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/host_scree...
File remoting/webapp/host_screen.js (right):

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/host_scree...
remoting/webapp/host_screen.js:48: /** @type {string}
*/(remoting.identity.getCachedEmail()),
nit: Do we still need this type annotation here?

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/host_setup...
File remoting/webapp/host_setup_dialog.js (right):

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/host_setup...
remoting/webapp/host_setup_dialog.js:162: remoting.showErrorMessage);
nit: Indentation.

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.js
File remoting/webapp/identity.js (right):

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
remoting/webapp/identity.js:16: * TODO(jamiewalch): Remove the remoting.OAuth2
possibility when the Apps v2
nit: "the" and "possibility" are redundant here.

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
remoting/webapp/identity.js:31: remoting.Identity = function(authDialog,
authButton) {
This arrangement is a little strange; this class shouldn't need to know about
details of the app's UI, I don't think.  Rather than passing authDialog and
authButton, how about passing a function that this class can call to trigger the
auth prompt dialog, to which it can pass this.onAuthContinue_.bind(this)?

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
remoting/webapp/identity.js:75: // TODO(ajwong): See if we can't find a JSON
endpoint.
What does this TODO mean?

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
remoting/webapp/identity.js:92: // TODO(ajwong): Update to new v2 API.
Does this mean use a new v2 Google API, or use an Apps v2 API equivalent?

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
remoting/webapp/identity.js:102: * has been made.
nit: Worth noting that this may return a valid email address even if the auth
token has subsequently become invalid?

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
remoting/webapp/identity.js:141: remoting.Identity.prototype.onAuthComplete_ =
function(interactive, token) {
nit: interactive -> was_interactive?

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
remoting/webapp/identity.js:145: this.pendingCallbacks_[i].onOk(token);
nit: Can you re-write this as a while(!empty) loop, so that it's more robust to
pendingCallbacks_ changes triggered by the onOk() callback?

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/jscompiler...
File remoting/webapp/jscompiler_hacks.js (right):

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/jscompiler...
remoting/webapp/jscompiler_hacks.js:74: /** @type {Object} */
nit: Consider moving these into their own apps_v2_api_types.js?

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/remoting.js
File remoting/webapp/remoting.js (right):

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/remoting.j...
remoting/webapp/remoting.js:122: if (!remoting.oauth2.isAuthenticated()) {
Is there an equivalent to isAuthenticated() for the identity API?

You're making the authentication dialog orthogonal to whatever content the app
is currently displaying, rather than a separate state.  Is that a safe thing to
do wrt what the rest of the page may be up to?  Does it matter that the OAuth2
flow currently navigates away from the current page, potentially losing state?

[Jamie, 2013-01-05 01:32:54]

ptal

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/host_scree...
File remoting/webapp/host_screen.js (right):

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/host_scree...
remoting/webapp/host_screen.js:48: /** @type {string}
*/(remoting.identity.getCachedEmail()),
On 2013/01/05 00:04:24, Wez wrote:
> nit: Do we still need this type annotation here?

Yes, getCachedEmail can return null if we're not authenticated, but
createPluginAndConnect can't accept a null argument. The annotation is a cast
because we know we've authenticated here.

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/host_setup...
File remoting/webapp/host_setup_dialog.js (right):

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/host_setup...
remoting/webapp/host_setup_dialog.js:162: remoting.showErrorMessage);
On 2013/01/05 00:04:24, Wez wrote:
> nit: Indentation.

Done.

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.js
File remoting/webapp/identity.js (right):

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
remoting/webapp/identity.js:16: * TODO(jamiewalch): Remove the remoting.OAuth2
possibility when the Apps v2
On 2013/01/05 00:04:24, Wez wrote:
> nit: "the" and "possibility" are redundant here.

Done.

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
remoting/webapp/identity.js:31: remoting.Identity = function(authDialog,
authButton) {
On 2013/01/05 00:04:24, Wez wrote:
> This arrangement is a little strange; this class shouldn't need to know about
> details of the app's UI, I don't think.  Rather than passing authDialog and
> authButton, how about passing a function that this class can call to trigger
the
> auth prompt dialog, to which it can pass this.onAuthContinue_.bind(this)?

Done.

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
remoting/webapp/identity.js:75: // TODO(ajwong): See if we can't find a JSON
endpoint.
On 2013/01/05 00:04:24, Wez wrote:
> What does this TODO mean?

It's essentially the same as the next TODO. I've removed this one.

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
remoting/webapp/identity.js:92: // TODO(ajwong): Update to new v2 API.
On 2013/01/05 00:04:24, Wez wrote:
> Does this mean use a new v2 Google API, or use an Apps v2 API equivalent?

There's an endpoint, https://www.googleapis.com/oauth2/v2/userinfo, that returns
JSON.

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
remoting/webapp/identity.js:102: * has been made.
On 2013/01/05 00:04:24, Wez wrote:
> nit: Worth noting that this may return a valid email address even if the auth
> token has subsequently become invalid?

I don't think that would add much.

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
remoting/webapp/identity.js:141: remoting.Identity.prototype.onAuthComplete_ =
function(interactive, token) {
On 2013/01/05 00:04:24, Wez wrote:
> nit: interactive -> was_interactive?

I've left this as it was, but added some descriptions to the doc comment.

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
remoting/webapp/identity.js:145: this.pendingCallbacks_[i].onOk(token);
On 2013/01/05 00:04:24, Wez wrote:
> nit: Can you re-write this as a while(!empty) loop, so that it's more robust
to
> pendingCallbacks_ changes triggered by the onOk() callback?

Done, and also below.

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/jscompiler...
File remoting/webapp/jscompiler_hacks.js (right):

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/jscompiler...
remoting/webapp/jscompiler_hacks.js:74: /** @type {Object} */
On 2013/01/05 00:04:24, Wez wrote:
> nit: Consider moving these into their own apps_v2_api_types.js?

I'd rather not, unless you feel strongly about it.

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/remoting.js
File remoting/webapp/remoting.js (right):

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/remoting.j...
remoting/webapp/remoting.js:122: if (!remoting.oauth2.isAuthenticated()) {
On 2013/01/05 00:04:24, Wez wrote:
> Is there an equivalent to isAuthenticated() for the identity API?

I don't think so.

> You're making the authentication dialog orthogonal to whatever content the app
> is currently displaying, rather than a separate state.  Is that a safe thing
to
> do wrt what the rest of the page may be up to?  Does it matter that the OAuth2
> flow currently navigates away from the current page, potentially losing state?

I think that making it orthogonal makes more sense because requesting a token
may change the UI, and showing a dialog is more easily undoable than switching
to a new UI mode.

There's no way to avoid losing state if the page changes, so I don't think
that's a concern.

[rmsousa, 2013-01-05 05:21:11]

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/remoting.js
File remoting/webapp/remoting.js (right):

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/remoting.j...
remoting/webapp/remoting.js:122: if (!remoting.oauth2.isAuthenticated()) {
On 2013/01/05 01:32:54, Jamie wrote:
> On 2013/01/05 00:04:24, Wez wrote:
> > Is there an equivalent to isAuthenticated() for the identity API?
> 
> I don't think so.

I'm not sure we need to do this here at all. For appsv2, we call several APIs on
startup (getEmail on init(), hostlist refresh here) - any one of them will cause
a callWithToken flow to run (which will eventually will show the auth dialog
with the correct button callback if it needs to be shown).

For non-appsv2, now that we don't have a separate unauthenticated mode, we don't
need to do this isAuthenticated check here, oauth2 could take the same
constructor parameter as identity (consentRequired), and, while it's being
initialized, check if it's authenticated and call
consentRequired(doAuthRedirect) if not. That should have the same effect as this
code (show the auth dialog if needed), but make the two implementations more
similar.

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity.js
File remoting/webapp/identity.js (right):

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity....
remoting/webapp/identity.js:48: this.pendingCallbacks_.push(new
remoting.Identity.Callbacks(onOk, onError));
Do we really want to do mint a new token on every API call? (I tried this out,
and it does go to the OAuth endpoint and issue a new, distinct token for every
getAuthToken call).
I can see why we may not want to save these tokens to persistent storage, but
saving them to a member variable while the app is running (and minting a new one
on start up) should be fine. This seems to be what the gdocs sample does.

If there's a reason why we're doing it this way (e.g. to avoid having to deal
with token expiration/revocation), please add a comment explaining the
rationale.

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity....
remoting/webapp/identity.js:66: /** @type {remoting.Identity} */
Did you mean to remove these from oauth2.js? (If you do so, you'll need to
change the non-appsv2 code, as it relies on calling these functions on the
remoting.OAuth2 instance in remoting.identity).

Keeping them in either one of the files is OK, but I don't like having them
duplicated (but I can live with just a big TODO() in oauth2.js to delete it once
we switch to using identity.js)

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/remoting.js
File remoting/webapp/remoting.js (right):

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/remoting....
remoting/webapp/remoting.js:218: remoting.oauth2.clear();
What are we planning to do here for appsv2? experimental.identity doesn't seem
to offer a token revocation wrapper. Should we assume that users are intended to
manage that through chrome and remove the option to sign out from the app?
Or should we copy/move the manual token revocation from oauth2.js to
identity.js? (Revoking any one of the issued tokens seems to revoke all tokens
and require the app to ask permission again, so it should work as a sign-out
mechanism).

[Jamie, 2013-01-07 23:52:20]

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/remoting.js
File remoting/webapp/remoting.js (right):

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/remoting.j...
remoting/webapp/remoting.js:122: if (!remoting.oauth2.isAuthenticated()) {
On 2013/01/05 05:21:11, rmsousa wrote:
> On 2013/01/05 01:32:54, Jamie wrote:
> > On 2013/01/05 00:04:24, Wez wrote:
> > > Is there an equivalent to isAuthenticated() for the identity API?
> > 
> > I don't think so.
> 
> I'm not sure we need to do this here at all. For appsv2, we call several APIs
on
> startup (getEmail on init(), hostlist refresh here) - any one of them will
cause
> a callWithToken flow to run (which will eventually will show the auth dialog
> with the correct button callback if it needs to be shown).
> 
> For non-appsv2, now that we don't have a separate unauthenticated mode, we
don't
> need to do this isAuthenticated check here, oauth2 could take the same
> constructor parameter as identity (consentRequired), and, while it's being
> initialized, check if it's authenticated and call
> consentRequired(doAuthRedirect) if not. That should have the same effect as
this
> code (show the auth dialog if needed), but make the two implementations more
> similar.

The problem with that approach is that the apps v2 flow happens entirely within
the app, so callWithToken will always invoke one or other of the callbacks. For
apps v1, it might redirect to another page. I think an explicit doAuthRedirect
call is more appropriate in that case.

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity.js
File remoting/webapp/identity.js (right):

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity....
remoting/webapp/identity.js:48: this.pendingCallbacks_.push(new
remoting.Identity.Callbacks(onOk, onError));
On 2013/01/05 05:21:11, rmsousa wrote:
> Do we really want to do mint a new token on every API call? (I tried this out,
> and it does go to the OAuth endpoint and issue a new, distinct token for every
> getAuthToken call).
> I can see why we may not want to save these tokens to persistent storage, but
> saving them to a member variable while the app is running (and minting a new
one
> on start up) should be fine. This seems to be what the gdocs sample does.
> 
> If there's a reason why we're doing it this way (e.g. to avoid having to deal
> with token expiration/revocation), please add a comment explaining the
> rationale.

Thanks for checking on this. I think you're right that minting a new token each
time is not ideal, but this code doesn't have enough information to cache it (it
doesn't know when it expires). I've started a conversation with munjal@, which
will hopefully yield either a caching implementation within getAuthToken or at
least access to expiry information so that we can implement it ourselves. I've
added a TODO for the time being.

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity....
remoting/webapp/identity.js:66: /** @type {remoting.Identity} */
On 2013/01/05 05:21:11, rmsousa wrote:
> Did you mean to remove these from oauth2.js? (If you do so, you'll need to
> change the non-appsv2 code, as it relies on calling these functions on the
> remoting.OAuth2 instance in remoting.identity).
> 
> Keeping them in either one of the files is OK, but I don't like having them
> duplicated (but I can live with just a big TODO() in oauth2.js to delete it
once
> we switch to using identity.js)

oauth2.js will be deleted completely once we're a v2 app. I've added a comment
to that effect.

[Wez, 2013-01-07 23:52:44]

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.js
File remoting/webapp/identity.js (right):

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
remoting/webapp/identity.js:92: // TODO(ajwong): Update to new v2 API.
On 2013/01/05 01:32:54, Jamie wrote:
> On 2013/01/05 00:04:24, Wez wrote:
> > Does this mean use a new v2 Google API, or use an Apps v2 API equivalent?
> 
> There's an endpoint, https://www.googleapis.com/oauth2/v2/userinfo, that
returns
> JSON.

OK, great - can you create a bug for migrating to that, or replace the TODO with
a non-TODO just stating there's a newer API we could use?

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/remoting.js
File remoting/webapp/remoting.js (right):

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/remoting.j...
remoting/webapp/remoting.js:122: if (!remoting.oauth2.isAuthenticated()) {
On 2013/01/05 05:21:11, rmsousa wrote:
> On 2013/01/05 01:32:54, Jamie wrote:
> > On 2013/01/05 00:04:24, Wez wrote:
> > > Is there an equivalent to isAuthenticated() for the identity API?
> > 
> > I don't think so.
> 
> I'm not sure we need to do this here at all. For appsv2, we call several APIs
on
> startup (getEmail on init(), hostlist refresh here) - any one of them will
cause
> a callWithToken flow to run (which will eventually will show the auth dialog
> with the correct button callback if it needs to be shown).
> 
> For non-appsv2, now that we don't have a separate unauthenticated mode, we
don't
> need to do this isAuthenticated check here, oauth2 could take the same
> constructor parameter as identity (consentRequired), and, while it's being
> initialized, check if it's authenticated and call
> consentRequired(doAuthRedirect) if not. That should have the same effect as
this
> code (show the auth dialog if needed), but make the two implementations more
> similar.

The one distinction between the identity and OAuth2 flows is that the OAuth2
authorization flow navigates the tab away to the OAuth pages, so we lose any
application state.  I don't think that should be a problem so long as we
getEmail() early on in the startup process.

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity.js
File remoting/webapp/identity.js (right):

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity....
remoting/webapp/identity.js:26: *     which must be called from a "click" event
handler.
nit: Is that the actual requirement, or will any ser-interaction suffice?

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity....
remoting/webapp/identity.js:48: this.pendingCallbacks_.push(new
remoting.Identity.Callbacks(onOk, onError));
On 2013/01/05 05:21:11, rmsousa wrote:
> Do we really want to do mint a new token on every API call? (I tried this out,
> and it does go to the OAuth endpoint and issue a new, distinct token for every
> getAuthToken call).
> I can see why we may not want to save these tokens to persistent storage, but
> saving them to a member variable while the app is running (and minting a new
one
> on start up) should be fine. This seems to be what the gdocs sample does.
> 
> If there's a reason why we're doing it this way (e.g. to avoid having to deal
> with token expiration/revocation), please add a comment explaining the
> rationale.

Access tokens last about an hour, I think, so during a long session they might
expire - we really want to trap and only request a new access token in that
case.

Jamie, does the identity API take care of caching tokens under-the-hood, do you
know?

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/remoting.js
File remoting/webapp/remoting.js (right):

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/remoting....
remoting/webapp/remoting.js:32: * Show the consent UI and register a one-shot
event handler to continue
nit: consent -> authorization consent

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/remoting....
remoting/webapp/remoting.js:63: // TODO(jamiewalch): Reinstate this when we
migrate to apps v2.
nit: Reference the v2 app bug.

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/remoting....
remoting/webapp/remoting.js:218: remoting.oauth2.clear();
On 2013/01/05 05:21:11, rmsousa wrote:
> What are we planning to do here for appsv2? experimental.identity doesn't seem
> to offer a token revocation wrapper. Should we assume that users are intended
to
> manage that through chrome and remove the option to sign out from the app?
> Or should we copy/move the manual token revocation from oauth2.js to
> identity.js? (Revoking any one of the issued tokens seems to revoke all tokens
> and require the app to ask permission again, so it should work as a sign-out
> mechanism).

Token revocation is much less critical in this case, because we'll no longer be
sharing a token between client & host.

Based on previous discussions w/ identity API folks I think a token revocation
API would make sense to propose, although it may be necessary to limit the
revocation to tokens previously issued via getAuthToken in some way.

[Wez, 2013-01-07 23:54:20]

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity.js
File remoting/webapp/identity.js (right):

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity....
remoting/webapp/identity.js:66: /** @type {remoting.Identity} */
On 2013/01/07 23:52:20, Jamie wrote:
> On 2013/01/05 05:21:11, rmsousa wrote:
> > Did you mean to remove these from oauth2.js? (If you do so, you'll need to
> > change the non-appsv2 code, as it relies on calling these functions on the
> > remoting.OAuth2 instance in remoting.identity).
> > 
> > Keeping them in either one of the files is OK, but I don't like having them
> > duplicated (but I can live with just a big TODO() in oauth2.js to delete it
> once
> > we switch to using identity.js)
> 
> oauth2.js will be deleted completely once we're a v2 app. I've added a comment
> to that effect.

How will we fetch a refresh token to configure the use with?

[Jamie, 2013-01-08 00:01:12]

On 2013/01/07 23:54:20, Wez wrote:
>
https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity.js
> File remoting/webapp/identity.js (right):
> 
>
https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity....
> remoting/webapp/identity.js:66: /** @type {remoting.Identity} */
> On 2013/01/07 23:52:20, Jamie wrote:
> > On 2013/01/05 05:21:11, rmsousa wrote:
> > > Did you mean to remove these from oauth2.js? (If you do so, you'll need to
> > > change the non-appsv2 code, as it relies on calling these functions on the
> > > remoting.OAuth2 instance in remoting.identity).
> > > 
> > > Keeping them in either one of the files is OK, but I don't like having
them
> > > duplicated (but I can live with just a big TODO() in oauth2.js to delete
it
> > once
> > > we switch to using identity.js)
> > 
> > oauth2.js will be deleted completely once we're a v2 app. I've added a
comment
> > to that effect.
> 
> How will we fetch a refresh token to configure the use with?

My preference is that the web-app will never deal with refresh tokens--the
native host config apps will do that. Failing that, I think we'll need to use
<webview>.

[Wez, 2013-01-08 02:37:27]

On 2013/01/08 00:01:12, Jamie wrote:
> On 2013/01/07 23:54:20, Wez wrote:
> >
>
https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity.js
> > File remoting/webapp/identity.js (right):
> > 
> >
>
https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity....
> > remoting/webapp/identity.js:66: /** @type {remoting.Identity} */
> > On 2013/01/07 23:52:20, Jamie wrote:
> > > On 2013/01/05 05:21:11, rmsousa wrote:
> > > > Did you mean to remove these from oauth2.js? (If you do so, you'll need
to
> > > > change the non-appsv2 code, as it relies on calling these functions on
the
> > > > remoting.OAuth2 instance in remoting.identity).
> > > > 
> > > > Keeping them in either one of the files is OK, but I don't like having
> them
> > > > duplicated (but I can live with just a big TODO() in oauth2.js to delete
> it
> > > once
> > > > we switch to using identity.js)
> > > 
> > > oauth2.js will be deleted completely once we're a v2 app. I've added a
> comment
> > > to that effect.
> > 
> > How will we fetch a refresh token to configure the use with?
> 
> My preference is that the web-app will never deal with refresh tokens--the
> native host config apps will do that. Failing that, I think we'll need to use
> <webview>.

Let's discuss that off-CL! :)

[Wez, 2013-01-08 18:35:37]

On 2013/01/07 23:52:44, Wez wrote:
> https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.js
> File remoting/webapp/identity.js (right):
> 
>
https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
> remoting/webapp/identity.js:92: // TODO(ajwong): Update to new v2 API.
> On 2013/01/05 01:32:54, Jamie wrote:
> > On 2013/01/05 00:04:24, Wez wrote:
> > > Does this mean use a new v2 Google API, or use an Apps v2 API equivalent?
> > 
> > There's an endpoint, https://www.googleapis.com/oauth2/v2/userinfo, that
> returns
> > JSON.
> 
> OK, great - can you create a bug for migrating to that, or replace the TODO
with
> a non-TODO just stating there's a newer API we could use?
> 
> https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/remoting.js
> File remoting/webapp/remoting.js (right):
> 
>
https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/remoting.j...
> remoting/webapp/remoting.js:122: if (!remoting.oauth2.isAuthenticated()) {
> On 2013/01/05 05:21:11, rmsousa wrote:
> > On 2013/01/05 01:32:54, Jamie wrote:
> > > On 2013/01/05 00:04:24, Wez wrote:
> > > > Is there an equivalent to isAuthenticated() for the identity API?
> > > 
> > > I don't think so.
> > 
> > I'm not sure we need to do this here at all. For appsv2, we call several
APIs
> on
> > startup (getEmail on init(), hostlist refresh here) - any one of them will
> cause
> > a callWithToken flow to run (which will eventually will show the auth dialog
> > with the correct button callback if it needs to be shown).
> > 
> > For non-appsv2, now that we don't have a separate unauthenticated mode, we
> don't
> > need to do this isAuthenticated check here, oauth2 could take the same
> > constructor parameter as identity (consentRequired), and, while it's being
> > initialized, check if it's authenticated and call
> > consentRequired(doAuthRedirect) if not. That should have the same effect as
> this
> > code (show the auth dialog if needed), but make the two implementations more
> > similar.
> 
> The one distinction between the identity and OAuth2 flows is that the OAuth2
> authorization flow navigates the tab away to the OAuth pages, so we lose any
> application state.  I don't think that should be a problem so long as we
> getEmail() early on in the startup process.
> 
>
https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity.js
> File remoting/webapp/identity.js (right):
> 
>
https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity....
> remoting/webapp/identity.js:26: *     which must be called from a "click"
event
> handler.
> nit: Is that the actual requirement, or will any ser-interaction suffice?
> 
>
https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity....
> remoting/webapp/identity.js:48: this.pendingCallbacks_.push(new
> remoting.Identity.Callbacks(onOk, onError));
> On 2013/01/05 05:21:11, rmsousa wrote:
> > Do we really want to do mint a new token on every API call? (I tried this
out,
> > and it does go to the OAuth endpoint and issue a new, distinct token for
every
> > getAuthToken call).
> > I can see why we may not want to save these tokens to persistent storage,
but
> > saving them to a member variable while the app is running (and minting a new
> one
> > on start up) should be fine. This seems to be what the gdocs sample does.
> > 
> > If there's a reason why we're doing it this way (e.g. to avoid having to
deal
> > with token expiration/revocation), please add a comment explaining the
> > rationale.
> 
> Access tokens last about an hour, I think, so during a long session they might
> expire - we really want to trap and only request a new access token in that
> case.
> 
> Jamie, does the identity API take care of caching tokens under-the-hood, do
you
> know?
> 
>
https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/remoting.js
> File remoting/webapp/remoting.js (right):
> 
>
https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/remoting....
> remoting/webapp/remoting.js:32: * Show the consent UI and register a one-shot
> event handler to continue
> nit: consent -> authorization consent
> 
>
https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/remoting....
> remoting/webapp/remoting.js:63: // TODO(jamiewalch): Reinstate this when we
> migrate to apps v2.
> nit: Reference the v2 app bug.
> 
>
https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/remoting....
> remoting/webapp/remoting.js:218: remoting.oauth2.clear();
> On 2013/01/05 05:21:11, rmsousa wrote:
> > What are we planning to do here for appsv2? experimental.identity doesn't
seem
> > to offer a token revocation wrapper. Should we assume that users are
intended
> to
> > manage that through chrome and remove the option to sign out from the app?
> > Or should we copy/move the manual token revocation from oauth2.js to
> > identity.js? (Revoking any one of the issued tokens seems to revoke all
tokens
> > and require the app to ask permission again, so it should work as a sign-out
> > mechanism).
> 
> Token revocation is much less critical in this case, because we'll no longer
be
> sharing a token between client & host.
> 
> Based on previous discussions w/ identity API folks I think a token revocation
> API would make sense to propose, although it may be necessary to limit the
> revocation to tokens previously issued via getAuthToken in some way.

Ping.

[Jamie, 2013-01-08 22:09:10]

ptal

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.js
File remoting/webapp/identity.js (right):

https://codereview.chromium.org/11769002/diff/2001/remoting/webapp/identity.j...
remoting/webapp/identity.js:92: // TODO(ajwong): Update to new v2 API.
On 2013/01/07 23:52:44, Wez wrote:
> On 2013/01/05 01:32:54, Jamie wrote:
> > On 2013/01/05 00:04:24, Wez wrote:
> > > Does this mean use a new v2 Google API, or use an Apps v2 API equivalent?
> > 
> > There's an endpoint, https://www.googleapis.com/oauth2/v2/userinfo, that
> returns
> > JSON.
> 
> OK, great - can you create a bug for migrating to that, or replace the TODO
with
> a non-TODO just stating there's a newer API we could use?

I think Renato is in the process of fixing it as we speak, so I'll hold of
committing this until he does and rebase.

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity.js
File remoting/webapp/identity.js (right):

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity....
remoting/webapp/identity.js:26: *     which must be called from a "click" event
handler.
On 2013/01/07 23:52:44, Wez wrote:
> nit: Is that the actual requirement, or will any ser-interaction suffice?

I've only tested it with a "click" handler, but I suspect that any interactive
event will work. I've updated the comment.

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/identity....
remoting/webapp/identity.js:48: this.pendingCallbacks_.push(new
remoting.Identity.Callbacks(onOk, onError));
On 2013/01/07 23:52:44, Wez wrote:
> On 2013/01/05 05:21:11, rmsousa wrote:
> > Do we really want to do mint a new token on every API call? (I tried this
out,
> > and it does go to the OAuth endpoint and issue a new, distinct token for
every
> > getAuthToken call).
> > I can see why we may not want to save these tokens to persistent storage,
but
> > saving them to a member variable while the app is running (and minting a new
> one
> > on start up) should be fine. This seems to be what the gdocs sample does.
> > 
> > If there's a reason why we're doing it this way (e.g. to avoid having to
deal
> > with token expiration/revocation), please add a comment explaining the
> > rationale.
> 
> Access tokens last about an hour, I think, so during a long session they might
> expire - we really want to trap and only request a new access token in that
> case.
> 
> Jamie, does the identity API take care of caching tokens under-the-hood, do
you
> know?

I don't think it does--that's what Renato's comment was about. However, I think
it should, and I've contacted Munjal with some feedback to that effect. We can't
implement our own caching because we don't know the token lifetime. We could use
the gettokeninfo endpoint to find it out, but that doesn't seem like the right
approach. Or we could modify our code to be able to handle an invalid token and
request a new one if so. That's probably the right thing to do if caching is not
an option, but it's much too large a change for this CL.

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/remoting.js
File remoting/webapp/remoting.js (right):

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/remoting....
remoting/webapp/remoting.js:32: * Show the consent UI and register a one-shot
event handler to continue
On 2013/01/07 23:52:44, Wez wrote:
> nit: consent -> authorization consent

Done.

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/remoting....
remoting/webapp/remoting.js:63: // TODO(jamiewalch): Reinstate this when we
migrate to apps v2.
On 2013/01/07 23:52:44, Wez wrote:
> nit: Reference the v2 app bug.

Done, here and elsewhere.

https://codereview.chromium.org/11769002/diff/11002/remoting/webapp/remoting....
remoting/webapp/remoting.js:218: remoting.oauth2.clear();
On 2013/01/05 05:21:11, rmsousa wrote:
> What are we planning to do here for appsv2? experimental.identity doesn't seem
> to offer a token revocation wrapper. Should we assume that users are intended
to
> manage that through chrome and remove the option to sign out from the app?
> Or should we copy/move the manual token revocation from oauth2.js to
> identity.js? (Revoking any one of the issued tokens seems to revoke all tokens
> and require the app to ask permission again, so it should work as a sign-out
> mechanism).

I think we'll eventually remove the option from the app altogether.

[Wez, 2013-01-09 23:47:24]

lgtm

https://chromiumcodereview.appspot.com/11769002/diff/13002/remoting/webapp/id...
File remoting/webapp/identity.js (right):

https://chromiumcodereview.appspot.com/11769002/diff/13002/remoting/webapp/id...
remoting/webapp/identity.js:43: * getAuthToken starts doing so itself.
Can you create a bug for resolving this one way or the other?