[tool] SpnegoAuth: add multi account, credential confirmation

[tool] SpnegoAuth: add multi account, credential confirmation

Add more features to the SpnegoAuthenticator:

 -  Set up up to 2 accounts.
 -  Account 1 will start authenticated.
 -  Account 2 will start unauthenticated. The first token request
    will require an additional confirmation step.
 -  Accounts can be added and removed from the Android account
    settings screen

BUG=534293
Committed: https://crrev.com/e4cefacb5bc08b92c284a8d7a3deaaaa2545a04d
Cr-Commit-Position: refs/heads/master@{#385165}

[dgn, 2015-10-22 13:54:54]

dgn@chromium.org changed reviewers:
+ aberent@chromium.org

[dgn, 2015-10-22 13:54:55]

PTAL. A preview of the README file is available here: go/dgn-md-tests/kerberos

[dgn, 2015-10-22 14:35:33]

dgn@chromium.org changed reviewers:
+ android-auth-team@google.com

[dgn, 2015-10-22 14:35:34]

android-auth-team@: As a follow up to the discussion in
g/android-auth-team/LF6HUW418q4, could you please have a look at this CL? I'm
implementing a custom account authenticator to test the changes made in this
other CL[1], and I'm not quite sure I understood the documentation correctly.

[1]: https://codereview.chromium.org/1422693002/

[aberent, 2015-10-26 15:24:12]

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
File
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticator.java
(right):

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticator.java:107:
// All our accounts have the SPNEGO feature.
Nit: ... and no others.

The check will fail if Chrome asked for any feature other than SPNEGO.

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticator.java:129:
public static Intent confirmCredentials(Context context, String accountName) {
Name overload with confirmCredentials(AccountAuthenticatorResponse, Account,
Bundle)  is possibly confusing; could you use a different name for this member?

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticator.java:189:
private static class AccountData {
Nit: we normally put nested classes before member functions.

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticator.java:198:
public AccountData(String name, Context context) {
Maybe replace this with static AccountData.create(...). Feels like a better pair
for AccountData.get(...).

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticator.java:225:
public boolean save(Context context) {
Do you ever use the result?

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticator.java:228:
if (mIsNew) {
Could you simply save in the constructor when isNew is true (or in the two
argument constructor, which is the only thing that sets isNew to true), and
avoid the need for mIsNew and the save call at line 154?

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
File
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticatorActivity.java
(right):

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticatorActivity.java:20:
public class SpnegoAuthenticatorActivity extends AccountAuthenticatorActivity {
There seems to be a lot of calls backward and forward between this and
SpnegoAuthenticator. Would this be simpler you made this a nested class inside
SpnegoAuthenticator? If you did so you could get rid of the static functions,
and, I think, make the code easier to understand.

[dgn, 2015-10-27 12:24:44]

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
File
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticator.java
(right):

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticator.java:107:
// All our accounts have the SPNEGO feature.
On 2015/10/26 15:24:11, aberent wrote:
> Nit: ... and no others.
> 
> The check will fail if Chrome asked for any feature other than SPNEGO.

Updated the comment.

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticator.java:189:
private static class AccountData {
On 2015/10/26 15:24:11, aberent wrote:
> Nit: we normally put nested classes before member functions.

Done.

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticator.java:198:
public AccountData(String name, Context context) {
On 2015/10/26 15:24:11, aberent wrote:
> Maybe replace this with static AccountData.create(...). Feels like a better
pair
> for AccountData.get(...).

Done.

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticator.java:225:
public boolean save(Context context) {
On 2015/10/26 15:24:11, aberent wrote:
> Do you ever use the result?

Nope. Removed

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticator.java:228:
if (mIsNew) {
On 2015/10/26 15:24:11, aberent wrote:
> Could you simply save in the constructor when isNew is true (or in the two
> argument constructor, which is the only thing that sets isNew to true), and
> avoid the need for mIsNew and the save call at line 154?

I like separating the save action, so that the AccountData object can be used
for more stuff without systematically saving. That being said,
addAccountExplicitly properly handles existing accounts, so I don't really need
isNew. Removed it.

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
File
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticatorActivity.java
(right):

https://codereview.chromium.org/1416443003/diff/1/tools/android/kerberos/Spne...
tools/android/kerberos/SpnegoAuthenticator/src/org/chromium/tools/spnegoauthenticator/SpnegoAuthenticatorActivity.java:20:
public class SpnegoAuthenticatorActivity extends AccountAuthenticatorActivity {
On 2015/10/26 15:24:11, aberent wrote:
> There seems to be a lot of calls backward and forward between this and
> SpnegoAuthenticator. Would this be simpler you made this a nested class inside
> SpnegoAuthenticator? If you did so you could get rid of the static functions,
> and, I think, make the code easier to understand.

I moved AccountData out and moved some of the statics as member methods there.
Should achieve the same with more clarity?

[aberent, 2015-11-03 11:22:27]

lgtm

[dgn, 2015-11-11 12:05:31]

dgn@chromium.org changed reviewers:
+ carlosvaldivia@google.com

[dgn, 2015-11-11 12:05:31]

https://codereview.chromium.org/1416443003/diff/20001/tools/android/kerberos/...
File tools/android/kerberos/SpnegoAuthenticator/AndroidManifest.xml (right):

https://codereview.chromium.org/1416443003/diff/20001/tools/android/kerberos/...
tools/android/kerberos/SpnegoAuthenticator/AndroidManifest.xml:32:
android:resource="@xml/spnego_authenticator" />
I tried to remove my code to show the notification and send the broadcast
intent, and add <meta-data
android:name="android.accounts.AccountAuthenticator.customTokens"
android:value="1" /> here instead, hoping that it would make the framwork take
care of it. That didn't work, I don't seem to get anything, it behaves the same
as without the flag.

According to the code at http://go/dgn-review-androidauth-1
it looks like I should have customTokens to false if I want that to work. Am I
looking at the wrong thing?

[dgn, 2016-04-05 13:29:23]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2016-04-05 13:29:39]

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

[commit-bot: I haz the power, 2016-04-05 14:07:53]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-05 14:08:49]

Description was changed from

==========
[tool] SpnegoAuth: add multi account, credential confirmation

Add more features to the SpnegoAuthenticator:

 -  Set up up to 2 accounts.
 -  Account 1 will start authenticated.
 -  Account 2 will start unauthenticated. The first token request
    will require an additional confirmation step.
 -  Accounts can be added and removed from the Android account
    settings screen

BUG=534293
==========

to

==========
[tool] SpnegoAuth: add multi account, credential confirmation

Add more features to the SpnegoAuthenticator:

 -  Set up up to 2 accounts.
 -  Account 1 will start authenticated.
 -  Account 2 will start unauthenticated. The first token request
    will require an additional confirmation step.
 -  Accounts can be added and removed from the Android account
    settings screen

BUG=534293

Committed: https://crrev.com/e4cefacb5bc08b92c284a8d7a3deaaaa2545a04d
Cr-Commit-Position: refs/heads/master@{#385165}
==========

[commit-bot: I haz the power, 2016-04-05 14:08:50]

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