Make Android HttpNegotiateAuthenticator work without an activity

net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java

Index: net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java
diff --git a/net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java b/net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java
index 917f174c50e8208af9144518460d0ec5a6015adc..6a27537d4f0360b91b084356965301154c101ddc 100644
--- a/net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java
+++ b/net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java
@@ -4,16 +4,22 @@
 
 package org.chromium.net;
 
+import android.accounts.Account;
 import android.accounts.AccountManager;
 import android.accounts.AccountManagerCallback;
 import android.accounts.AccountManagerFuture;
 import android.accounts.AuthenticatorException;
 import android.accounts.OperationCanceledException;
 import android.app.Activity;
+import android.content.BroadcastReceiver;
+import android.content.Context;
+import android.content.Intent;
+import android.content.IntentFilter;
 import android.os.Bundle;
 import android.os.Handler;
 
 import org.chromium.base.ApplicationStatus;
+import org.chromium.base.Log;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
@@ -27,11 +33,116 @@
  */
 @JNINamespace("net::android")
 public class HttpNegotiateAuthenticator {
+    private static final String TAG = "net_auth";
     private Bundle mSpnegoContext = null;
     private final String mAccountType;
-    private AccountManagerFuture<Bundle> mFuture;
 
-    private HttpNegotiateAuthenticator(String accountType) {
+    /**
+     * Structure designed to hold the data related to a specific request across the various
+     * callbacks needed to complete it.
+     */
+    static class RequestData {
+        /** Native object to post the result to. */
+        public long nativeResultObject;
+
+        /** Reference to the account manager to use for the various requests. */
+        public AccountManager accountManager;
+
+        /** Authenticator-specific options for the request, used for AccountManager#getAuthToken. */
+        public Bundle options;
+
+        /** Desired token type, used for AccountManager#getAuthToken. */
+        public String authTokenType;
+
+        /** Account to fetch an auth token for. */
+        public Account account;
+    }
+
+    /**
+     * Expects to receive a single account as result, and uses that account to request a token
+     * from the {@link AccountManager} provided via the {@link RequestData}
+     */
+    @VisibleForTesting
+    class GetAccountsCallback implements AccountManagerCallback<Account[]> {
+        private final RequestData mRequestData;
+        public GetAccountsCallback(RequestData requestData) {
+            mRequestData = requestData;
+        }
+
+        @Override
+        public void run(AccountManagerFuture<Account[]> future) {
+            Account[] accounts;
+            try {
+                accounts = future.getResult();
+            } catch (OperationCanceledException | AuthenticatorException | IOException e) {
+                notifyFailure("Unable to retrieve the results for the getAccounts call.", e,
+                        mRequestData);
+                return;
+            }
+
+            if (accounts.length != 1) {

[asanka, 2015/11/03 17:45:51]

Is there no way to select an account in this case?

Also, is there some UI to indicate that a particular identity is going to be
used for authentication? (Not having one would still be consistent with how
desktop works, but I'm curious).

[dgn, 2015/11/05 14:39:14]

There isn't any. Showing any kind of UI requires having access to an activity,
and in this case we don't have one. When we have one, we use the old code path
that allows selecting an account.

Going to settings > accounts shows a list of accounts and there should be one
installed by the device/network administrator to authenticate users. Aside from
that, we don't have anything to display it.

+                // TODO(dgn) Maybe should fail with an error like ERR_INVALID_AUTH_CREDENTIALS
+                // instead of UNEXPECTED?

[asanka, 2015/11/03 17:45:51]

Yeah. ERR_INVALID_AUTH_CREDENTIALS would be better.

[dgn, 2015/11/05 14:39:14]

Done.

+                notifyFailure("Unable to obtain a unique eligible account for negotiate auth.",
+                        null, mRequestData);
+                return;
+            }
+
+            mRequestData.account = accounts[0];
+            mRequestData.accountManager.getAuthToken(mRequestData.account,
+                    mRequestData.authTokenType, mRequestData.options, true /* notifyAuthFailure */,
+                    new GetTokenCallback(mRequestData),
+                    new Handler(ThreadUtils.getUiThreadLooper()));
+        }
+    }
+
+    @VisibleForTesting
+    class GetTokenCallback implements AccountManagerCallback<Bundle> {
+        private final RequestData mRequestData;
+        public GetTokenCallback(RequestData requestData) {
+            mRequestData = requestData;
+        }
+
+        @Override
+        public void run(AccountManagerFuture<Bundle> future) {
+            Bundle result;
+            try {
+                result = future.getResult();
+            } catch (OperationCanceledException | AuthenticatorException | IOException e) {
+                notifyFailure("Operation did not complete", e, mRequestData);
+                return;
+            }
+
+            if (result.containsKey(AccountManager.KEY_INTENT)) {
+                final Context appContext = ApplicationStatus.getApplicationContext();

[dgn, 2015/10/30 12:38:56]

Note: torne@ is adding a replacement for this, I'll switch once the CL is live.

[aberent, 2015/10/30 16:38:10]

nit: Maybe add a TODO if this is landing first.

[dgn, 2015/10/30 17:04:40]

I'll wait. It's broken otherwise, even though the code is unreachable until my
webview patch lands

+
+                // We wait for a broadcast that should be sent once the user is done interacting
+                // with the notification
+                // TODO(dgn) We currently hang around if the notification is swiped away, until
+                // a LOGIN_ACCOUNTS_CHANGED_ACTION filter is received. It might be for something
+                // unrelated then we would wait again here. Maybe we should limit the number of
+                // retries in some way?
+                BroadcastReceiver broadcastReceiver = new BroadcastReceiver() {
+
+                    @Override
+                    public void onReceive(Context context, Intent intent) {
+                        appContext.unregisterReceiver(this);
+                        mRequestData.accountManager.getAuthToken(mRequestData.account,
+                                mRequestData.authTokenType, mRequestData.options,
+                                true /* notifyAuthFailure */, new GetTokenCallback(mRequestData),
+                                null);
+                    }
+
+                };
+                appContext.registerReceiver(broadcastReceiver,
+                        new IntentFilter(AccountManager.LOGIN_ACCOUNTS_CHANGED_ACTION));
+            } else {
+                setResult(result, mRequestData);
+            }
+        }
+    }
+
+    protected HttpNegotiateAuthenticator(String accountType) {
         assert !android.text.TextUtils.isEmpty(accountType);
         mAccountType = accountType;
     }
@@ -57,80 +168,89 @@ static HttpNegotiateAuthenticator create(String accountType) {
     void getNextAuthToken(final long nativeResultObject, final String principal, String authToken,
             boolean canDelegate) {
         assert principal != null;
-        String authTokenType = HttpNegotiateConstants.SPNEGO_TOKEN_TYPE_BASE + principal;
-        Activity activity = ApplicationStatus.getLastTrackedFocusedActivity();
-        if (activity == null) {
-            nativeSetResult(nativeResultObject, NetError.ERR_UNEXPECTED, null);
-            return;
-        }
-        AccountManager am = AccountManager.get(activity);
-        String features[] = {HttpNegotiateConstants.SPNEGO_FEATURE};
 
-        Bundle options = new Bundle();
+        RequestData requestData = new RequestData();
+        requestData.authTokenType = HttpNegotiateConstants.SPNEGO_TOKEN_TYPE_BASE + principal;
+        requestData.accountManager = AccountManager.get(ApplicationStatus.getApplicationContext());
+        requestData.nativeResultObject = nativeResultObject;
+        String features[] = {HttpNegotiateConstants.SPNEGO_FEATURE};
 
+        requestData.options = new Bundle();
         if (authToken != null) {
-            options.putString(HttpNegotiateConstants.KEY_INCOMING_AUTH_TOKEN, authToken);
+            requestData.options.putString(
+                    HttpNegotiateConstants.KEY_INCOMING_AUTH_TOKEN, authToken);
         }
         if (mSpnegoContext != null) {
-            options.putBundle(HttpNegotiateConstants.KEY_SPNEGO_CONTEXT, mSpnegoContext);
+            requestData.options.putBundle(
+                    HttpNegotiateConstants.KEY_SPNEGO_CONTEXT, mSpnegoContext);
         }
-        options.putBoolean(HttpNegotiateConstants.KEY_CAN_DELEGATE, canDelegate);
+        requestData.options.putBoolean(HttpNegotiateConstants.KEY_CAN_DELEGATE, canDelegate);
+        Handler uiThreadHandler = new Handler(ThreadUtils.getUiThreadLooper());
 
-        mFuture = am.getAuthTokenByFeatures(mAccountType, authTokenType, features, activity, null,
-                options, new AccountManagerCallback<Bundle>() {
+        Activity activity = ApplicationStatus.getLastTrackedFocusedActivity();
+        if (activity == null) {
+            // This code path is not as able to get user input as the one that has an activity. It
+            // doesn't handle 0 or multiple accounts and will just result in a failure in those
+            // cases.
+            requestData.accountManager.getAccountsByTypeAndFeatures(
+                    mAccountType, features, new GetAccountsCallback(requestData), uiThreadHandler);
+        } else {
+            // Behavior in Chrome
+            // If there is more than one account, it will show the disambig for each query

[asanka, 2015/11/03 17:45:51]

expn disambig

[dgn, 2015/11/06 16:51:48]

Done.

+            // (e.g. main page, then favicon ...)
+            // Same if the credentials need to be confirmed/
+            // If there is a failure, it will retry automatically.
+            requestData.accountManager.getAuthTokenByFeatures(mAccountType,
+                    requestData.authTokenType, features, activity, null, requestData.options,
+                    new GetTokenCallback(requestData), uiThreadHandler);
+        }
+    }
 
-                    @Override
-                    public void run(AccountManagerFuture<Bundle> future) {
-                        try {
-                            Bundle result = future.getResult();
-                            mSpnegoContext =
-                                    result.getBundle(HttpNegotiateConstants.KEY_SPNEGO_CONTEXT);
-                            int status;
-                            switch (result.getInt(HttpNegotiateConstants.KEY_SPNEGO_RESULT,
-                                    HttpNegotiateConstants.ERR_UNEXPECTED)) {
-                                case HttpNegotiateConstants.OK:
-                                    status = 0;
-                                    break;
-                                case HttpNegotiateConstants.ERR_UNEXPECTED:
-                                    status = NetError.ERR_UNEXPECTED;
-                                    break;
-                                case HttpNegotiateConstants.ERR_ABORTED:
-                                    status = NetError.ERR_ABORTED;
-                                    break;
-                                case HttpNegotiateConstants.ERR_UNEXPECTED_SECURITY_LIBRARY_STATUS:
-                                    status = NetError.ERR_UNEXPECTED_SECURITY_LIBRARY_STATUS;
-                                    break;
-                                case HttpNegotiateConstants.ERR_INVALID_RESPONSE:
-                                    status = NetError.ERR_INVALID_RESPONSE;
-                                    break;
-                                case HttpNegotiateConstants.ERR_INVALID_AUTH_CREDENTIALS:
-                                    status = NetError.ERR_INVALID_AUTH_CREDENTIALS;
-                                    break;
-                                case HttpNegotiateConstants.ERR_UNSUPPORTED_AUTH_SCHEME:
-                                    status = NetError.ERR_UNSUPPORTED_AUTH_SCHEME;
-                                    break;
-                                case HttpNegotiateConstants.ERR_MISSING_AUTH_CREDENTIALS:
-                                    status = NetError.ERR_MISSING_AUTH_CREDENTIALS;
-                                    break;
-                                case HttpNegotiateConstants
-                                        .ERR_UNDOCUMENTED_SECURITY_LIBRARY_STATUS:
-                                    status = NetError.ERR_UNDOCUMENTED_SECURITY_LIBRARY_STATUS;
-                                    break;
-                                case HttpNegotiateConstants.ERR_MALFORMED_IDENTITY:
-                                    status = NetError.ERR_MALFORMED_IDENTITY;
-                                    break;
-                                default:
-                                    status = NetError.ERR_UNEXPECTED;
-                            }
-                            nativeSetResult(nativeResultObject, status,
-                                    result.getString(AccountManager.KEY_AUTHTOKEN));
-                        } catch (OperationCanceledException | AuthenticatorException
-                                | IOException e) {
-                            nativeSetResult(nativeResultObject, NetError.ERR_ABORTED, null);
-                        }
-                    }
+    private void notifyFailure(String message, Exception e, RequestData requestData) {
+        Log.w(TAG, message, e);
+        nativeSetResult(requestData.nativeResultObject, NetError.ERR_UNEXPECTED, null);
+    }
 
-                }, new Handler(ThreadUtils.getUiThreadLooper()));
+    private void setResult(Bundle result, RequestData requestData) {
+        mSpnegoContext = result.getBundle(HttpNegotiateConstants.KEY_SPNEGO_CONTEXT);
+        int status;
+        switch (result.getInt(
+                HttpNegotiateConstants.KEY_SPNEGO_RESULT, HttpNegotiateConstants.ERR_UNEXPECTED)) {
+            case HttpNegotiateConstants.OK:

[asanka, 2015/11/03 17:45:51]

How permanent are the error codes in HttpNegotiateConstants.java?

The error codes in //net are quite organic and were added as they were needed.
For a more concrete API, we'd probably want to invest some time into figuring
out what signals we want to get from the authenticator and have a few values for
those rather than exposing the //net error codes.

[dgn, 2015/11/05 14:39:14]

They are supposed to not change as 3p devs will use these values in their
authenticators. We explicitely direct them to the HttpNegotiateConstants source
in
https://www.chromium.org/developers/design-documents/http-authentication/writ...

I'm not sure about changing them now. aberent@, what do you think?

[asanka, 2015/11/05 17:41:32]

To give some background, the net::Error values were constructed based on the
fact that Chrome is responsible for logging and reporting error conditions in
addition to handling them. //net explicitly invoked known and tested libraries
(where applicable) or //net itself implemented support for the underlying
protocols, and //chrome was responsible for prompting the user. These
assumptions, I believe, no longer hold when we are invoking an unknown
authenticator determined at runtime. So we kind of have to abstract out our HTTP
auth state machine and figure out which high-level signals we want from this
library.

For example, the result of invoking the authenticator might be one of the
following:

* Success. Put this token in a Authorization/Proxy-Authorization header and send
the request out again.
* Failure:
  * Don't invoke me again for this origin. (w/ TTL or some other signal?)
  * Don't invoke me again for any origin (i.e. permanent error). (w/ TTL or some
other signal?)

[dgn, 2015/11/06 16:51:48]

Moved this discussion to http://crbug.com/552414. I agree that simplifying the
exposed codes seems to be the right thing to do. But if we make a change, it
should be a different CL. I would prefer to keep the previously existing status
codes in this one

+                status = 0;
+                break;
+            case HttpNegotiateConstants.ERR_UNEXPECTED:
+                status = NetError.ERR_UNEXPECTED;
+                break;
+            case HttpNegotiateConstants.ERR_ABORTED:
+                status = NetError.ERR_ABORTED;
+                break;
+            case HttpNegotiateConstants.ERR_UNEXPECTED_SECURITY_LIBRARY_STATUS:
+                status = NetError.ERR_UNEXPECTED_SECURITY_LIBRARY_STATUS;
+                break;
+            case HttpNegotiateConstants.ERR_INVALID_RESPONSE:
+                status = NetError.ERR_INVALID_RESPONSE;
+                break;
+            case HttpNegotiateConstants.ERR_INVALID_AUTH_CREDENTIALS:
+                status = NetError.ERR_INVALID_AUTH_CREDENTIALS;
+                break;
+            case HttpNegotiateConstants.ERR_UNSUPPORTED_AUTH_SCHEME:
+                status = NetError.ERR_UNSUPPORTED_AUTH_SCHEME;
+                break;
+            case HttpNegotiateConstants.ERR_MISSING_AUTH_CREDENTIALS:
+                status = NetError.ERR_MISSING_AUTH_CREDENTIALS;
+                break;
+            case HttpNegotiateConstants.ERR_UNDOCUMENTED_SECURITY_LIBRARY_STATUS:
+                status = NetError.ERR_UNDOCUMENTED_SECURITY_LIBRARY_STATUS;
+                break;
+            case HttpNegotiateConstants.ERR_MALFORMED_IDENTITY:
+                status = NetError.ERR_MALFORMED_IDENTITY;
+                break;
+            default:
+                status = NetError.ERR_UNEXPECTED;
+        }
+        nativeSetResult(requestData.nativeResultObject, status,
+                result.getString(AccountManager.KEY_AUTHTOKEN));
     }
 
     @VisibleForTesting