Fix permission checking for negotiate auth on Android

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 bbc0583d84685c3802eac52ba880fc12649e969b..a965196633f169c1e4b001e07b9484400a2c2061 100644
--- a/net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java
+++ b/net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java
@@ -4,6 +4,7 @@
 
 package org.chromium.net;
 
+import android.Manifest;
 import android.accounts.Account;
 import android.accounts.AccountManager;
 import android.accounts.AccountManagerCallback;
@@ -16,6 +17,7 @@
 import android.content.Intent;
 import android.content.IntentFilter;
 import android.content.pm.PackageManager;
+import android.os.Build;
 import android.os.Bundle;
 import android.os.Handler;
 import android.os.Process;
@@ -33,6 +35,26 @@
 /**
  * Class to get Auth Tokens for HTTP Negotiate authentication (typically used for Kerberos) An
  * instance of this class is created for each separate negotiation.
+ *
+ * Please keep the documentation from the chromium.org page (https://goo.gl/46hmKx) in sync.
+ * ================================================================================================
+ * In addition to the error codes that can be forwarded from the authenticator app, the following
+ * errors can be displayed when trying to authenticate a request:
+ *
+ *  - ERR_UNEXPECTED: An unexpected error happened and the request has been terminated.
+ *
+ *  - ERR_MISSING_AUTH_CREDENTIALS: The account information is not usable. It can be raised for
+ *    example if the user did not log in to the authenticator app and no eligible account is found,
+ *    if the account information can't be obtained because the current app does not have the
+ *    required permissions, or if there is more than one eligible account and we can't obtain a
+ *    selection from the user.
+ *
+ *  - ERR_MISCONFIGURED_AUTH_ENVIRONMENT: The authentication can't be completed because of some
+ *    issues in the configuration of the app. Some permissions may be missing.
+ *
+ * Please search for the "cr_net_auth" tag in logcat to have more information about the cause of
+ * these errors.
+ * ================================================================================================
  */
 @JNINamespace("net::android")
 public class HttpNegotiateAuthenticator {
@@ -78,22 +100,37 @@ public void run(AccountManagerFuture<Account[]> future) {
             try {
                 accounts = future.getResult();
             } catch (OperationCanceledException | AuthenticatorException | IOException e) {
-                Log.w(TAG, "Unable to retrieve the results for the getAccounts call.", e);
+                Log.w(TAG, "ERR_UNEXPECTED: Error while attempting to retrieve accounts.", e);
                 nativeSetResult(mRequestData.nativeResultObject, NetError.ERR_UNEXPECTED, null);
                 return;
             }
 
-            if (accounts.length != 1) {
-                Log.w(TAG, "Unable to obtain a unique eligible account for negotiate auth.");
+            if (accounts.length == 0) {
+                Log.w(TAG, "ERR_MISSING_AUTH_CREDENTIALS: No account provided for the kerberos "
+                                + "authentication. Please verify the configuration policies and "
+                                + "that the CONTACTS runtime permission is granted. ");
                 nativeSetResult(mRequestData.nativeResultObject,
-                        NetError.ERR_INVALID_AUTH_CREDENTIALS, null);
+                        NetError.ERR_MISSING_AUTH_CREDENTIALS, null);

[dgn, 2016/01/06 21:13:24]

Changed from INVALID to MISSING since we don't have anything to use.

                 return;
             }
-            if (!hasPermission(ContextUtils.getApplicationContext(),
-                        "android.permission.USE_CREDENTIALS")) {
+
+            if (accounts.length > 1) {
+                Log.w(TAG, "ERR_MISSING_AUTH_CREDENTIALS: Found %d accounts eligible for the "
+                                + "kerberos authentication. Please fix the configuration by "
+                                + "providing a single account.",
+                        accounts.length);
+                nativeSetResult(mRequestData.nativeResultObject,
+                        NetError.ERR_MISSING_AUTH_CREDENTIALS, null);

[dgn, 2016/01/06 21:13:24]

Changed from INVALID to MISSING since the credentials have not been denied, we
just don't know what to use.

+                return;
+            }
+
+            if (lacksPermission(ContextUtils.getApplicationContext(),
+                        "android.permission.USE_CREDENTIALS", true)) {
+                // Protecting the AccountManager#getAuthToken call.
                 // API  < 23 Requires the USE_CREDENTIALS permission or throws an exception.
-                // API >= 23 USE_CREDENTIALS is auto-granted without having to be declared.
-                Log.e(TAG, "USE_CREDENTIALS permission not granted. Aborting authentication.");
+                // API >= 23 USE_CREDENTIALS permission is removed
+                Log.e(TAG, "ERR_MISCONFIGURED_AUTH_ENVIRONMENT: USE_CREDENTIALS permission not "
+                                + "granted. Aborting authentication.");
                 nativeSetResult(mRequestData.nativeResultObject,
                         NetError.ERR_MISCONFIGURED_AUTH_ENVIRONMENT, null);
                 return;
@@ -119,7 +156,7 @@ public void run(AccountManagerFuture<Bundle> future) {
             try {
                 result = future.getResult();
             } catch (OperationCanceledException | AuthenticatorException | IOException e) {
-                Log.w(TAG, "Operation did not complete", e);
+                Log.w(TAG, "ERR_UNEXPECTED: Error while attempting to obtain a token.", e);
                 nativeSetResult(mRequestData.nativeResultObject, NetError.ERR_UNEXPECTED, null);
                 return;
             }
@@ -197,41 +234,12 @@ void getNextAuthToken(final long nativeResultObject, final String principal, Str
                     HttpNegotiateConstants.KEY_SPNEGO_CONTEXT, mSpnegoContext);
         }
         requestData.options.putBoolean(HttpNegotiateConstants.KEY_CAN_DELEGATE, canDelegate);
-        Handler uiThreadHandler = new Handler(ThreadUtils.getUiThreadLooper());
 
         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.
-            if (!hasPermission(applicationContext, android.Manifest.permission.GET_ACCOUNTS)) {
-                // API  < 23 Requires the GET_ACCOUNTS permission or throws an exception.
-                // API >= 23 Requires the GET_ACCOUNTS permission or returns only the accounts
-                //           whose authenticator has a signature matches our app.
-                Log.e(TAG, "GET_ACCOUNTS permission not granted. Aborting authentication.");
-                nativeSetResult(requestData.nativeResultObject,
-                        NetError.ERR_MISCONFIGURED_AUTH_ENVIRONMENT, null);
-                return;
-            }
-            requestData.accountManager.getAccountsByTypeAndFeatures(
-                    mAccountType, features, new GetAccountsCallback(requestData), uiThreadHandler);
+            requestTokenWithoutActivity(applicationContext, requestData, features);
         } else {
-            // If there is more than one account, it will show the an account picker dialog for
-            // each query (e.g. html file, then favicon...)
-            // Same if the credentials need to be confirmed.
-            // If there is a failure, it will retry automatically.
-            if (!hasPermission(applicationContext, "android.permission.MANAGE_ACCOUNTS")) {
-                // API  < 23 Requires the MANAGE_ACCOUNTS permission.
-                // API >= 23 Requires something in the CONTACTS permission group to behave properly.
-                //           MANAGE_ACCOUNTS is auto-granted on install without having be declared.
-                Log.e(TAG, "MANAGE_ACCOUNTS permission not granted. Aborting authentication.");
-                nativeSetResult(requestData.nativeResultObject,
-                        NetError.ERR_MISCONFIGURED_AUTH_ENVIRONMENT, null);
-                return;
-            }
-            requestData.accountManager.getAuthTokenByFeatures(mAccountType,
-                    requestData.authTokenType, features, activity, null, requestData.options,
-                    new GetTokenCallback(requestData), uiThreadHandler);
+            requestTokenWithActivity(applicationContext, activity, requestData, features);
         }
     }
 
@@ -281,11 +289,86 @@ private void processResult(Bundle result, RequestData requestData) {
                 result.getString(AccountManager.KEY_AUTHTOKEN));
     }
 
+    /**
+     * Requests an authentication token. If the account is not properly setup, it will result in
+     * a failure.
+     *
+     * @param ctx The application context
+     * @param requestData The object holding the data related to the current request
+     * @param features An array of the account features to require, may be null or empty
+     */
+    private void requestTokenWithoutActivity(
+            Context ctx, RequestData requestData, String[] features) {
+        if (lacksPermission(ctx, Manifest.permission.GET_ACCOUNTS, true /* onlyPreM */)) {
+            // Protecting the AccountManager#getAccountsByTypeAndFeatures call.
+            // API  < 23 Requires the GET_ACCOUNTS permission or throws an exception.
+            // API >= 23 Requires the GET_ACCOUNTS permission (CONTACTS permission group) or
+            //           returns only the accounts whose authenticator has a signature that
+            //           matches our app. Working with this restriction and not requesting
+            //           the permission is a valid use case in the context of WebView, so we
+            //           don't require it on M+
+            Log.e(TAG, "ERR_MISCONFIGURED_AUTH_ENVIRONMENT: GET_ACCOUNTS permission not "
+                            + "granted. Aborting authentication.");
+            nativeSetResult(requestData.nativeResultObject,
+                    NetError.ERR_MISCONFIGURED_AUTH_ENVIRONMENT, null);
+            return;
+        }
+        requestData.accountManager.getAccountsByTypeAndFeatures(mAccountType, features,
+                new GetAccountsCallback(requestData), new Handler(ThreadUtils.getUiThreadLooper()));
+    }
+
+    /**
+     * Requests an authentication token. Handles the account selection/creation and the credentials
+     * confirmation if that is needed.
+     * If there is more than one account, it will show an account picker dialog for
+     * each query (e.g. html file, then favicon...)
+     * Same if the credentials need to be confirmed.
+     *
+     * @param ctx The application context
+     * @param activity The Activity context to use for launching new sub-Activities to prompt to
+     *                 add an account, select an account, and/or enter a password, as necessary;
+     *                 used only to call startActivity(); should not be null
+     * @param requestData The object holding the data related to the current request
+     * @param features An array of the account features to require, may be null or empty
+     */
+    private void requestTokenWithActivity(
+            Context ctx, Activity activity, RequestData requestData, String[] features) {
+        boolean isPreM = Build.VERSION.SDK_INT < Build.VERSION_CODES.M;
+        String permission = isPreM
+                ? "android.permission.MANAGE_ACCOUNTS"
+                : Manifest.permission.GET_ACCOUNTS;
+
+        // Check if the AccountManager#getAuthTokenByFeatures call can be made.
+        // API  < 23 Requires the MANAGE_ACCOUNTS permission.
+        // API >= 23 Requires the GET_ACCOUNTS permission to behave properly. When it's not granted,
+        //           accounts not managed by the current application can't be retrieved. Depending
+        //           on the authenticator implementation, it might prompt to create an account, but
+        //           that won't be saved. This would be a bad user experience, so we also consider
+        //           it a failure case.
+        if (lacksPermission(ctx, permission, isPreM)) {
+            Log.e(TAG, "ERR_MISCONFIGURED_AUTH_ENVIRONMENT: %s permission not granted. "
+                       + "Aborting authentication", permission);
+            nativeSetResult(requestData.nativeResultObject,
+                    NetError.ERR_MISCONFIGURED_AUTH_ENVIRONMENT, null);
+            return;
+        }
+
+        requestData.accountManager.getAuthTokenByFeatures(mAccountType, requestData.authTokenType,
+                features, activity, null, requestData.options, new GetTokenCallback(requestData),
+                new Handler(ThreadUtils.getUiThreadLooper()));
+    }
+
+    /**
+     * Returns whether the current context lacks a given permission. Skips the check on M+ systems
+     * if {@code onlyPreM} is {@code true}, and just returns {@code false}.
+     */
     @VisibleForTesting
-    boolean hasPermission(Context context, String permission) {
+    boolean lacksPermission(Context context, String permission, boolean onlyPreM) {
+        if (onlyPreM && Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) return false;
+
         int permissionResult =
                 context.checkPermission(permission, Process.myPid(), Process.myUid());
-        return permissionResult == PackageManager.PERMISSION_GRANTED;
+        return permissionResult != PackageManager.PERMISSION_GRANTED;
     }
 
     @VisibleForTesting