Use GoogleAuthUtil's getToken instead of AccountManager

Use GoogleAuthUtil's getToken instead of AccountManager.

GoogleAuthUtil gives us better error handling capability and gets rid of the AccountManagerFuture interface that is difficult to Mock in tests.

We can do this since we are only handling Google accounts anyway.

BUG=555495, 547048
Committed: https://crrev.com/4a3edbe6c7515cfdb2d001ddc8fb4e830425a5a5
Cr-Commit-Position: refs/heads/master@{#362676}

[Bernhard Bauer, 2015-11-18 17:12:29]

bauerb@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2015-11-18 17:12:30]

https://codereview.chromium.org/1440363002/diff/40001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java
(right):

https://codereview.chromium.org/1440363002/diff/40001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:30: *
@param account The {@link Account} for which the authToken is requested.
Nit: "auth token".

https://codereview.chromium.org/1440363002/diff/40001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:32: *
@return If the auth token was fetched successfully it will be present in the
bundle at key
Hm... returning a bundle that has some combination of fields set doesn't strike
me like the best interface. I have to admit, I kind of like returning a string
and throwing an exception in error cases; maybe we could do that? Although we
might not want to use the GMSCore exception classes directly so we don't leak
implementation details.

https://codereview.chromium.org/1440363002/diff/40001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:37:
Bundle getAuthToken(Account account, String authTokenScope);
You should probably mention that this method should be called on a background
thread.

Also, do we want to name this method to reflect that it will only work for
Google accounts?

https://codereview.chromium.org/1440363002/diff/40001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java
(right):

https://codereview.chromium.org/1440363002/diff/40001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:375:
mApplicationContext.startActivity(recoveryIntent);
We don't want to automatically start the recovery intent.

https://codereview.chromium.org/1440363002/diff/40001/sync/test/android/javat...
File
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
(left):

https://codereview.chromium.org/1440363002/diff/40001/sync/test/android/javat...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:389:
private class AccountManagerTask<T> extends FutureTask<T> implements
AccountManagerFuture<T> {
Yay, removing code! 😃

https://codereview.chromium.org/1440363002/diff/40001/sync/test/android/javat...
File
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/1440363002/diff/40001/sync/test/android/javat...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:185:
Intent intent = newGrantCredentialsPermissionIntent(account, authTokenScope);
I think you are conflating two different things here: The recovery intent in
case of an error, and the authorization prompt that is shown by GMSCore (or its
effective mock in this case). The latter is shown automatically, and the request
returns when authorization is granted, whereas the former should not be shown
automatically, and requests should just fail straight away. And as we've
previously discussed offline, this intent really only seems to be used for
testing test code, so it doesn't seem to be necessary for a mock implementation
of the AccountManagerDelegate.

[knn, 2015-11-19 11:08:38]

PTAL. Thanks!

https://codereview.chromium.org/1440363002/diff/40001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java
(right):

https://codereview.chromium.org/1440363002/diff/40001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:30: *
@param account The {@link Account} for which the authToken is requested.
On 2015/11/18 17:12:30, Bernhard Bauer wrote:
> Nit: "auth token".

Done.

https://codereview.chromium.org/1440363002/diff/40001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:32: *
@return If the auth token was fetched successfully it will be present in the
bundle at key
On 2015/11/18 17:12:30, Bernhard Bauer wrote:
> Hm... returning a bundle that has some combination of fields set doesn't
strike
> me like the best interface. I have to admit, I kind of like returning a string
> and throwing an exception in error cases; maybe we could do that? Although we
> might not want to use the GMSCore exception classes directly so we don't leak
> implementation details.

I agree exceptions are a much better interface here. I was skeptical that my
reviewers won't like custom exceptions. Glad that atleast you aren't :)

https://codereview.chromium.org/1440363002/diff/40001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:37:
Bundle getAuthToken(Account account, String authTokenScope);
On 2015/11/18 17:12:30, Bernhard Bauer wrote:
> You should probably mention that this method should be called on a background
> thread.
> 
> Also, do we want to name this method to reflect that it will only work for
> Google accounts?

About that, this entire package is only used for Google accounts I am wondering
whether we should rename the class itself and remove non-google specific stuff?

https://codereview.chromium.org/1440363002/diff/40001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java
(right):

https://codereview.chromium.org/1440363002/diff/40001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:375:
mApplicationContext.startActivity(recoveryIntent);
On 2015/11/18 17:12:30, Bernhard Bauer wrote:
> We don't want to automatically start the recovery intent.

My bad, test code.

https://codereview.chromium.org/1440363002/diff/40001/sync/test/android/javat...
File
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/1440363002/diff/40001/sync/test/android/javat...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:185:
Intent intent = newGrantCredentialsPermissionIntent(account, authTokenScope);
On 2015/11/18 17:12:30, Bernhard Bauer wrote:
> I think you are conflating two different things here: The recovery intent in
> case of an error, and the authorization prompt that is shown by GMSCore (or
its
> effective mock in this case). The latter is shown automatically, and the
request
> returns when authorization is granted, whereas the former should not be shown
> automatically, and requests should just fail straight away. And as we've
> previously discussed offline, this intent really only seems to be used for
> testing test code, so it doesn't seem to be necessary for a mock
implementation
> of the AccountManagerDelegate.

So at least from GmsCore's perspective both are user recoverable exceptions.
Perhaps because the AccountManager API did not have a way to return the intent
other than hidden behind a key in the Bundle, it was fired automatically. The
correct behaviour from a Mock's perspective is thus returning this intent and
not auto firing it. But I am removing it now as it doesn't seem to really test
anything.

[knn, 2015-11-19 11:12:56]

Description was changed from

==========
remove AccountManagerFuture based getAuthToken
==========

to

==========
Use GoogleAuthUtil's getToken instead of AccountManager.

GoogleAuthUtil gives us better error handling capability and gets rid of the
AccountManagerFuture interface that is difficult to Mock in tests.

We can do this since we are only handling Google accounts anyway.

BUG=555495, 547048
==========

[Bernhard Bauer, 2015-11-19 15:23:34]

https://codereview.chromium.org/1440363002/diff/100001/chrome/android/javates...
File chrome/android/javatests/AndroidManifest.xml (left):

https://codereview.chromium.org/1440363002/diff/100001/chrome/android/javates...
chrome/android/javatests/AndroidManifest.xml:34: android:exported="true">
Ooh... this probably shouldn't have been exported in the first place. Nice
cleanup!

https://codereview.chromium.org/1440363002/diff/100001/sync/android/java/src/...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java
(right):

https://codereview.chromium.org/1440363002/diff/100001/sync/android/java/src/...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:360:
isTransientError.set(ex.isTransientError());
Can you add a TODO here to handle the recovery intent?

https://codereview.chromium.org/1440363002/diff/100001/sync/android/java/src/...
File sync/android/java/src/org/chromium/sync/signin/AuthException.java (right):

https://codereview.chromium.org/1440363002/diff/100001/sync/android/java/src/...
sync/android/java/src/org/chromium/sync/signin/AuthException.java:23: * before
retry.
Can you check this invariant on construction?

https://codereview.chromium.org/1440363002/diff/100001/sync/android/java/src/...
File
sync/android/java/src/org/chromium/sync/signin/SystemAccountManagerDelegate.java
(right):

https://codereview.chromium.org/1440363002/diff/100001/sync/android/java/src/...
sync/android/java/src/org/chromium/sync/signin/SystemAccountManagerDelegate.java:82:
throw new AuthException(false /* isTransientError */, ex.getIntent());
Could you add the caught exception to the AuthException, then log when it you're
handling that one?

https://codereview.chromium.org/1440363002/diff/100001/sync/test/android/java...
File
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/1440363002/diff/100001/sync/test/android/java...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:137:
public void waitForGetAccountsTask() throws InterruptedException {
Where is this called?

https://codereview.chromium.org/1440363002/diff/100001/sync/test/android/java...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:295:
mLock.wait(WAIT_TIMEOUT_MS);
Wait, you wait on the lock while in a critical section (which even happens to be
guarded by the same lock)? o_O That doesn't really work, does it?

I think you probably want CountdownLatch here -- that one requires you to set
the countdown value in advance, but I don't quite understand anyway why we need
to wait for an arbitrary number of finished tasks here anyway, and the fact that
the method to do so doesn't appear to be called doesn't help.

[maxbogue, 2015-11-19 15:49:45]

maxbogue@chromium.org changed reviewers:
+ maxbogue@chromium.org

[maxbogue, 2015-11-19 15:49:46]

Drive-by response to comments on my code. This change looks awesome, btw! \o/

https://codereview.chromium.org/1440363002/diff/100001/sync/test/android/java...
File
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/1440363002/diff/100001/sync/test/android/java...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:137:
public void waitForGetAccountsTask() throws InterruptedException {
On 2015/11/19 15:23:34, Bernhard Bauer wrote:
> Where is this called?

I don't think it's in code search yet. See http://crrev.com/1434053002.

https://codereview.chromium.org/1440363002/diff/100001/sync/test/android/java...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:295:
mLock.wait(WAIT_TIMEOUT_MS);
On 2015/11/19 15:23:34, Bernhard Bauer wrote:
> Wait, you wait on the lock while in a critical section (which even happens to
be
> guarded by the same lock)? o_O That doesn't really work, does it?

That's how it works in Java. You have to hold an object's lock when you call
wait on it; the lock is released until you return from the wait. It literally
will not compile otherwise.

> I think you probably want CountdownLatch here -- that one requires you to set
> the countdown value in advance, but I don't quite understand anyway why we
need
> to wait for an arbitrary number of finished tasks here anyway, and the fact
that
> the method to do so doesn't appear to be called doesn't help.

I don't know the value in advance. I'm using this to be able to wait until all
the GetAccounts tasks are done, i.e. the count of in-progress tasks is zero.
CountdownLatch does not do what I want, and also cannot be reused once it has
been triggered. Java has lots of concurrency tools that are weirdly specific and
not many that are usefully generic.

[Bernhard Bauer, 2015-11-19 15:58:53]

https://codereview.chromium.org/1440363002/diff/100001/sync/test/android/java...
File
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/1440363002/diff/100001/sync/test/android/java...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:295:
mLock.wait(WAIT_TIMEOUT_MS);
On 2015/11/19 15:49:46, maxbogue wrote:
> On 2015/11/19 15:23:34, Bernhard Bauer wrote:
> > Wait, you wait on the lock while in a critical section (which even happens
to
> be
> > guarded by the same lock)? o_O That doesn't really work, does it?
> 
> That's how it works in Java. You have to hold an object's lock when you call
> wait on it; the lock is released until you return from the wait. It literally
> will not compile otherwise.
> 
> > I think you probably want CountdownLatch here -- that one requires you to
set
> > the countdown value in advance, but I don't quite understand anyway why we
> need
> > to wait for an arbitrary number of finished tasks here anyway, and the fact
> that
> > the method to do so doesn't appear to be called doesn't help.
> 
> I don't know the value in advance. I'm using this to be able to wait until all
> the GetAccounts tasks are done, i.e. the count of in-progress tasks is zero.
> CountdownLatch does not do what I want, and also cannot be reused once it has
> been triggered. Java has lots of concurrency tools that are weirdly specific
and
> not many that are usefully generic.

Oh, oops... turns out I was a) too dumb to check whether this was actually part
of the CL, and b) had no idea how Object.wait() worked 😳 Thanks for the
explanation!

[knn, 2015-11-19 16:09:25]

Thanks for jumping in Max!

Can you also review my cl?

Bernhard, PTAL.

https://codereview.chromium.org/1440363002/diff/100001/sync/android/java/src/...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java
(right):

https://codereview.chromium.org/1440363002/diff/100001/sync/android/java/src/...
sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:360:
isTransientError.set(ex.isTransientError());
On 2015/11/19 15:23:33, Bernhard Bauer wrote:
> Can you add a TODO here to handle the recovery intent?

Done.

https://codereview.chromium.org/1440363002/diff/100001/sync/android/java/src/...
File sync/android/java/src/org/chromium/sync/signin/AuthException.java (right):

https://codereview.chromium.org/1440363002/diff/100001/sync/android/java/src/...
sync/android/java/src/org/chromium/sync/signin/AuthException.java:23: * before
retry.
On 2015/11/19 15:23:33, Bernhard Bauer wrote:
> Can you check this invariant on construction?

Done.

https://codereview.chromium.org/1440363002/diff/100001/sync/android/java/src/...
File
sync/android/java/src/org/chromium/sync/signin/SystemAccountManagerDelegate.java
(right):

https://codereview.chromium.org/1440363002/diff/100001/sync/android/java/src/...
sync/android/java/src/org/chromium/sync/signin/SystemAccountManagerDelegate.java:82:
throw new AuthException(false /* isTransientError */, ex.getIntent());
On 2015/11/19 15:23:34, Bernhard Bauer wrote:
> Could you add the caught exception to the AuthException, then log when it
you're
> handling that one?

Done.

[maxbogue, 2015-11-19 18:15:59]

https://codereview.chromium.org/1440363002/diff/120001/sync/test/android/java...
File
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/1440363002/diff/120001/sync/test/android/java...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:187:
assert ah.hasBeenAccepted(authTokenScope);
I'm less familiar with all the auth stuff than you two; can you explain why we
now assert this when before we handled it not being accepted? Even looking at
AccountHolder I'm not entirely sure what accepted means...

https://codereview.chromium.org/1440363002/diff/120001/sync/test/android/java...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:194:
Log.d(TAG, "Created new auth token for " + ah.getAccount() + ": autTokenType = "
s/autTokenType/authTokenType

[knn, 2015-11-20 13:04:18]

https://codereview.chromium.org/1440363002/diff/120001/sync/test/android/java...
File
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/1440363002/diff/120001/sync/test/android/java...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:187:
assert ah.hasBeenAccepted(authTokenScope);
On 2015/11/19 18:15:59, maxbogue wrote:
> I'm less familiar with all the auth stuff than you two; can you explain why we
> now assert this when before we handled it not being accepted? Even looking at
> AccountHolder I'm not entirely sure what accepted means...

TLDR: The assert is because the other case is not used at all.
Previously we fired a MockGrantCredentialsPermissionActivity which broadcasted
an Intent and returned. In the meanwhile, if somebody called
prepare{Allow/Deny}AppPermission, that was used to fake an accept/deny and
finally generate the auth token. (See applyPreparedPermission and generateResult
methods.)
However as far as I can see this is not used anywhere. Hence I am asserting to
be on the safe side.

https://codereview.chromium.org/1440363002/diff/120001/sync/test/android/java...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:194:
Log.d(TAG, "Created new auth token for " + ah.getAccount() + ": autTokenType = "
On 2015/11/19 18:15:59, maxbogue wrote:
> s/autTokenType/authTokenType

Done.

[maxbogue, 2015-11-20 16:28:47]

LGTM!

[Bernhard Bauer, 2015-11-20 18:55:42]

LGTM w/ nits:

https://codereview.chromium.org/1440363002/diff/140001/sync/android/java/src/...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java
(right):

https://codereview.chromium.org/1440363002/diff/140001/sync/android/java/src/...
sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:31: *
@return The auth token, if it was fetched successfully, or {@code null}.
Remove the "or null" part, and maybe even "if it was fetched successfully", and
instead document the thrown exception.

https://codereview.chromium.org/1440363002/diff/140001/sync/android/java/src/...
File sync/android/java/src/org/chromium/sync/signin/AuthException.java (right):

https://codereview.chromium.org/1440363002/diff/140001/sync/android/java/src/...
sync/android/java/src/org/chromium/sync/signin/AuthException.java:31:
mInnerException = exception;
You could pass this one to the super constructor instead.

[knn, 2015-11-23 11:35:33]

Thanks for the review!

https://codereview.chromium.org/1440363002/diff/140001/sync/android/java/src/...
File sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java
(right):

https://codereview.chromium.org/1440363002/diff/140001/sync/android/java/src/...
sync/android/java/src/org/chromium/sync/signin/AccountManagerDelegate.java:31: *
@return The auth token, if it was fetched successfully, or {@code null}.
On 2015/11/20 18:55:42, Bernhard -- OOO until Dec 1 wrote:
> Remove the "or null" part, and maybe even "if it was fetched successfully",
and
> instead document the thrown exception.

Done.

https://codereview.chromium.org/1440363002/diff/140001/sync/android/java/src/...
File sync/android/java/src/org/chromium/sync/signin/AuthException.java (right):

https://codereview.chromium.org/1440363002/diff/140001/sync/android/java/src/...
sync/android/java/src/org/chromium/sync/signin/AuthException.java:31:
mInnerException = exception;
On 2015/11/20 18:55:42, Bernhard -- OOO until Dec 1 wrote:
> You could pass this one to the super constructor instead.

Done.

[knn, 2015-11-23 11:36:57]

The CQ bit was checked by knn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-23 11:37:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440363002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440363002/160001

[commit-bot: I haz the power, 2015-11-23 12:15:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-23 12:15:55]

Dry run: This issue passed the CQ dry run.

[knn, 2015-12-01 19:50:01]

The CQ bit was checked by knn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-01 19:50:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440363002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440363002/200001

[commit-bot: I haz the power, 2015-12-01 21:00:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-01 21:00:45]

Dry run: This issue passed the CQ dry run.

[knn, 2015-12-02 10:13:41]

The CQ bit was checked by knn@chromium.org

[knn, 2015-12-02 10:13:42]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
maxbogue@chromium.org
Link to the patchset: https://codereview.chromium.org/1440363002/#ps200001
(title: "multidex")

[commit-bot: I haz the power, 2015-12-02 10:14:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440363002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440363002/200001

[commit-bot: I haz the power, 2015-12-02 10:18:54]

Description was changed from

==========
Use GoogleAuthUtil's getToken instead of AccountManager.

GoogleAuthUtil gives us better error handling capability and gets rid of the
AccountManagerFuture interface that is difficult to Mock in tests.

We can do this since we are only handling Google accounts anyway.

BUG=555495, 547048
==========

to

==========
Use GoogleAuthUtil's getToken instead of AccountManager.

GoogleAuthUtil gives us better error handling capability and gets rid of the
AccountManagerFuture interface that is difficult to Mock in tests.

We can do this since we are only handling Google accounts anyway.

BUG=555495, 547048
==========

[commit-bot: I haz the power, 2015-12-02 10:18:55]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-12-02 10:19:57]

Description was changed from

==========
Use GoogleAuthUtil's getToken instead of AccountManager.

GoogleAuthUtil gives us better error handling capability and gets rid of the
AccountManagerFuture interface that is difficult to Mock in tests.

We can do this since we are only handling Google accounts anyway.

BUG=555495, 547048
==========

to

==========
Use GoogleAuthUtil's getToken instead of AccountManager.

GoogleAuthUtil gives us better error handling capability and gets rid of the
AccountManagerFuture interface that is difficult to Mock in tests.

We can do this since we are only handling Google accounts anyway.

BUG=555495, 547048

Committed: https://crrev.com/4a3edbe6c7515cfdb2d001ddc8fb4e830425a5a5
Cr-Commit-Position: refs/heads/master@{#362676}
==========

[commit-bot: I haz the power, 2015-12-02 10:19:58]

Patchset 11 (id:??) landed as
https://crrev.com/4a3edbe6c7515cfdb2d001ddc8fb4e830425a5a5
Cr-Commit-Position: refs/heads/master@{#362676}