Add application pre-authorization.

Currently, any invocation of getToken() may result in consent UI being shown, which makes it hard to predict what UI changes may result when a token is requested. This CL simplifies the flow:

* Pre-authorize the app before calling the start() method.
* Only that first authorization will result in any in-app UI being shown.
* Subsequent calls to getToken() may cause the chrome.identity consent UI to be shown, but this should only happen in exceptional circumstances and since no DOM changes are involved, it's a more predictable flow.

Committed: https://crrev.com/fc25fc74bd0ba18099ce64a21d3daaa657f5967b
Cr-Commit-Position: refs/heads/master@{#319536}

[Jamie, 2015-03-05 01:35:54]

jamiewalch@chromium.org changed reviewers:
+ courage@chromium.org, garykac@chromium.org, kelvinp@chromium.org

[Jamie, 2015-03-05 01:35:56]

garykac: PTAL
courage: Please give an opinion on how I'm detecting cancel vs. error.
kelvinp: FYI

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/app_remoting...
File remoting/webapp/app_remoting/js/app_remoting.js (left):

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/app_remoting...
remoting/webapp/app_remoting/js/app_remoting.js:81: remoting.onUserInfoAvailable
= function(email, fullName) {
This was needed when fullName and email had synchronous getCached* accessors.
It's not needed any more.

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/app_remoting...
remoting/webapp/app_remoting/js/app_remoting.js:93:
remoting.initIdentity(remoting.onUserInfoAvailable);
This initialization was shared between the App- and DesktopRemoting delegate
classes, so I moved it to the Application class.

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/base/js/auth...
File remoting/webapp/base/js/auth_init.js (right):

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/base/js/auth...
remoting/webapp/base/js/auth_init.js:18: // to allow this behaviour to be
customized.
For example, AppStreaming should not use an in-DOM consent dialog; it should be
a top-level window.

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/ident...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:191: remoting.Error.CANCELLED :
remoting.Error.NOT_AUTHENTICATED;
courage@, is this the recommended way of determining whether the user declined
the permissions (ie, not an error--should probably just close the app) or
something went wrong (ie, should show an error message)? It seems fragile to do
a string comparison against a human-readable error message, rather than, for
example, an enumerated constant.

[Michael Courage, 2015-03-05 01:43:55]

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/ident...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:191: remoting.Error.CANCELLED :
remoting.Error.NOT_AUTHENTICATED;
On 2015/03/05 01:35:56, Jamie wrote:
> courage@, is this the recommended way of determining whether the user declined
> the permissions (ie, not an error--should probably just close the app) or
> something went wrong (ie, should show an error message)? It seems fragile to
do
> a string comparison against a human-readable error message, rather than, for
> example, an enumerated constant.

Sorry, that's still the only way until there is an overhaul of the apps error
handling system. Such an overhaul is not on the horizon as far as I know :(

[kelvinp, 2015-03-05 02:12:14]

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

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/app_remoting...
remoting/webapp/app_remoting/js/app_remoting.js:115:
remoting.AppRemoting.prototype.start = function(connector, token) {
onAuthenticated or onInitialized would be better name as this function is in
response to the completion of the start call.

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/app_remoting...
remoting/webapp/app_remoting/js/app_remoting.js:115:
remoting.AppRemoting.prototype.start = function(connector, token) {
Since the delegate should not cache this token, nor is it currently used in
AppRemoting.start() or DesktopRemoting.start().  Maybe we should remove this
token from the function signature to discourage the use of it.

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/ident...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:189: console.error(error_message);
Should we set this.interactive_ to false here? Or else, once the dialog is
shown, it will consider all future attempts as interactive.

[Jamie, 2015-03-05 02:31:27]

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

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/app_remoting...
remoting/webapp/app_remoting/js/app_remoting.js:115:
remoting.AppRemoting.prototype.start = function(connector, token) {
On 2015/03/05 02:12:14, kelvinp wrote:
> Since the delegate should not cache this token, nor is it currently used in
> AppRemoting.start() or DesktopRemoting.start().  Maybe we should remove this
> token from the function signature to discourage the use of it.

We could, but the semantics are no different from any other auth token, so I
think this is fine. The alternative is to call getToken and then throw away the
token and have start() request a new one if it needs it. There's no harm in that
(it will just grab one from the cache), but it seems a bit perverse :) I also
like the fact that it makes it clear what sort of things are expected in init
(stuff that can be done before authentication) and what should wait for start.

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/app_remoting...
remoting/webapp/app_remoting/js/app_remoting.js:115:
remoting.AppRemoting.prototype.start = function(connector, token) {
On 2015/03/05 02:12:14, kelvinp wrote:
> onAuthenticated or onInitialized would be better name as this function is in
> response to the completion of the start call.

I want the name to reflect the fact that this is an entry point of sorts, and I
think start() does that quite well. It's also a reasonable counterpoint to
init().

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/ident...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:189: console.error(error_message);
On 2015/03/05 02:12:14, kelvinp wrote:
> Should we set this.interactive_ to false here? Or else, once the dialog is
> shown, it will consider all future attempts as interactive.

That's intentional. Part of the reason for this change is that we only
(potentially) show in-DOM UI for the first call to getToken(). This class was
originally designed under the assumption that you could only set {interactive:
true} from a UIEvent, but since that's not the case I prefer that we let
chrome.identity show whatever UI it deems appropriate in the rare case that a
token is revoked--it's better UX than popping up something in-DOM which might
not get cleaned up correctly.

[garykac, 2015-03-05 02:38:26]

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

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/app_remoting...
remoting/webapp/app_remoting/js/app_remoting.js:200:
chrome.app.window.current().close();
We also call this in onDisconnected, but it also clears the ping timer while
this code makes sure the loading window is closed.

I think that we should add Application.Delegate.closeApp() and have it do all of
these cleanup/shutdown tasks in one place.

Then signInFailed could be moved into Application since the code would be
identical between DesktopRemoting and AppRemoting.

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/deskt...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_remoting.js:153:
chrome.app.window.current().close();
if AppsV2?

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_remoting.js:250: };
if AppsV2, then chrome.app.window.current().close?

[Jamie, 2015-03-05 19:01:37]

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

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/app_remoting...
remoting/webapp/app_remoting/js/app_remoting.js:200:
chrome.app.window.current().close();
On 2015/03/05 02:38:25, garykac wrote:
> We also call this in onDisconnected, but it also clears the ping timer while
> this code makes sure the loading window is closed.
> 
> I think that we should add Application.Delegate.closeApp() and have it do all
of
> these cleanup/shutdown tasks in one place.
> 
> Then signInFailed could be moved into Application since the code would be
> identical between DesktopRemoting and AppRemoting.

That's a good idea. I think I'll make it a follow-up CL because we'd also want
to call that when the user closes the app explicitly (and maybe in some other
cases) and I don't want this CL to grow too large.

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/deskt...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_remoting.js:153:
chrome.app.window.current().close();
On 2015/03/05 02:38:25, garykac wrote:
> if AppsV2?

I'm assuming that there is only apps v2 for this change. Once we update the
webstore entry, there's no going back.

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_remoting.js:250: };
On 2015/03/05 02:38:26, garykac wrote:
> if AppsV2, then chrome.app.window.current().close?

No, we don't want to close the widow on disconnect for desktop-remoting.

[Jamie, 2015-03-06 20:45:23]

Ping

[garykac, 2015-03-06 21:16:25]

lgtm

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

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/app_remoting...
remoting/webapp/app_remoting/js/app_remoting.js:200:
chrome.app.window.current().close();
On 2015/03/05 19:01:36, Jamie wrote:
> On 2015/03/05 02:38:25, garykac wrote:
> > We also call this in onDisconnected, but it also clears the ping timer while
> > this code makes sure the loading window is closed.
> > 
> > I think that we should add Application.Delegate.closeApp() and have it do
all
> of
> > these cleanup/shutdown tasks in one place.
> > 
> > Then signInFailed could be moved into Application since the code would be
> > identical between DesktopRemoting and AppRemoting.
> 
> That's a good idea. I think I'll make it a follow-up CL because we'd also want
> to call that when the user closes the app explicitly (and maybe in some other
> cases) and I don't want this CL to grow too large.

sgtm

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/deskt...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_remoting.js:153:
chrome.app.window.current().close();
On 2015/03/05 19:01:36, Jamie wrote:
> On 2015/03/05 02:38:25, garykac wrote:
> > if AppsV2?
> 
> I'm assuming that there is only apps v2 for this change. Once we update the
> webstore entry, there's no going back.

Acknowledged.

https://codereview.chromium.org/981723003/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_remoting.js:250: };
On 2015/03/05 19:01:36, Jamie wrote:
> On 2015/03/05 02:38:26, garykac wrote:
> > if AppsV2, then chrome.app.window.current().close?
> 
> No, we don't want to close the widow on disconnect for desktop-remoting.

Acknowledged.

[Jamie, 2015-03-06 21:29:19]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2015-03-06 21:30:26]

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

[commit-bot: I haz the power, 2015-03-07 00:19:13]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-03-07 00:19:57]

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