Android permission requests with one-time permissions in mind

A change of the meaning of shared preferences starting with
"HasRequestedAndroidPermission::"

  Without one-time permissions it was enough to remember *if* the user
  was asked for an Android permission. But now, we need to remember
  user's response. A permission that at first was granted can be later
  denied without any user action.

This change is tested using the newest methods provided by Robolectric
and androidx.test

  Robolectric.buildActivity is a low-level tool for managing Activity
  lifetime and Robolectric.setupActivity is deprecated in favor of
  androidx.test.core.app.ActivityScenario

  For the ActivityScenario to work, the Activity used in test must be
  declared in the AndroidManifest

Change-Id: I2f809e43dd4790a09cdac08cc0e0803a6fed086e
Bug: 1096085
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2285470
Commit-Queue: who/bttk <bttk@chromium.org>
Reviewed-by: Patrick Noland <pnoland@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790189}

ui/android/BUILD.gn

@@ -370,6 +370,7 @@ android_library("ui_java_test_support") {
     "javatests/src/org/chromium/ui/test/util/UiRestriction.java",
     "javatests/src/org/chromium/ui/test/util/UiRestrictionSkipCheck.java",
     "javatests/src/org/chromium/ui/test/util/modelutil/FakeViewProvider.java",
+    "junit/src/org/chromium/ui/base/TestActivity.java",
   ]
   deps = [
     ":ui_java",
@@ -383,19 +384,20 @@ android_library("ui_java_test_support") {
 }
 
 android_resources("ui_javatest_resources") {
-  custom_package = "org.chromium.test.ui"
+  testonly = true
   sources = [
     "junit/res/layout/inflated_view.xml",
     "junit/res/layout/layout_view_builder_test.xml",
     "junit/res/layout/main_view.xml",
   ]
+  android_manifest = "junit/AndroidManifest.xml"
 }
 
 junit_binary("ui_junit_tests") {
-  package_name = "org.chromium.test.ui"
   sources = [
     "junit/src/org/chromium/ui/AsyncViewProviderTest.java",
     "junit/src/org/chromium/ui/AsyncViewStubTest.java",
+    "junit/src/org/chromium/ui/base/ActivityAndroidPermissionDelegateTest.java",
     "junit/src/org/chromium/ui/base/ApplicationViewportInsetSupplierTest.java",
     "junit/src/org/chromium/ui/base/ClipboardTest.java",
     "junit/src/org/chromium/ui/base/EventOffsetHandlerTest.java",
@@ -431,11 +433,16 @@ junit_binary("ui_junit_tests") {
     "//base:base_junit_test_support",
     "//base/test:test_support_java",
     "//third_party/android_deps:androidx_annotation_annotation_java",
+    "//third_party/android_deps:androidx_appcompat_appcompat_java",
     "//third_party/android_deps:androidx_appcompat_appcompat_resources_java",
     "//third_party/android_deps:androidx_asynclayoutinflater_asynclayoutinflater_java",
+    "//third_party/android_deps:androidx_test_core_java",
+    "//third_party/android_deps:androidx_test_ext_junit_java",
     "//third_party/android_deps:androidx_test_runner_java",
     "//third_party/hamcrest:hamcrest_java",
+    "//third_party/mockito:mockito_java",
   ]
+  android_manifest = "junit/AndroidManifest.xml"
 }
 
 test("ui_android_unittests") {

ui/android/java/src/org/chromium/ui/base/AndroidPermissionDelegate.java

@@ -10,6 +10,7 @@ package org.chromium.ui.base;
 public interface AndroidPermissionDelegate {
     /**
      * Determine whether access to a particular permission is granted.
+     *
      * @param permission The permission whose access is to be checked.
      * @return Whether access to the permission is granted.
      */
@@ -18,13 +19,14 @@ public interface AndroidPermissionDelegate {
     /**
      * Determine whether the specified permission can be requested.
      * <p>
-     * A permission can be requested in the following states:
-     * 1.) Default un-granted state, permission can be requested
-     * 2.) Permission previously requested but denied by the user, but the user did not select
-     *     "Never ask again".
+     * A permission can not be requested in the following states:
+     * <ul>
+     *   <li>Permission is denied by policy.
+     *   <li>Permission previously denied and the user selected "Never ask again".
+     * </ul>
      *
      * @param permission The permission name.
-     * @return Whether the requesting the permission is allowed.
+     * @return Whether the permission can be requested.
      */
     boolean canRequestPermission(String permission);
 

ui/android/java/src/org/chromium/ui/base/AndroidPermissionDelegateWithRequester.java

@@ -30,7 +30,18 @@ public abstract class AndroidPermissionDelegateWithRequester implements AndroidP
     private static final int REQUEST_CODE_PREFIX = 1000;
     private static final int REQUEST_CODE_RANGE_SIZE = 100;
 
-    private static final String PERMISSION_QUERIED_KEY_PREFIX = "HasRequestedAndroidPermission::";
+    /**
+     * Shared preference key prefix for remembering Android permissions denied by the user.
+     * <p>
+     * <b>NOTE:</b> As of M86 the semantics of shared prefs using this key prefix has changed:
+     * <ul>
+     *   <li>Previously: {@code true} if the user was ever asked for a permission, otherwise absent.
+     *   <li>M86+: {@code true} if the user most recently has denied permission access,
+     *     otherwise absent.
+     * </ul>
+     */
+    private static final String PERMISSION_WAS_DENIED_KEY_PREFIX =
+            "HasRequestedAndroidPermission::";
 
     public AndroidPermissionDelegateWithRequester() {
         mHandler = new Handler();
@@ -39,9 +50,27 @@ public abstract class AndroidPermissionDelegateWithRequester implements AndroidP
 
     @Override
     public final boolean hasPermission(String permission) {
-        return ApiCompatibilityUtils.checkPermission(ContextUtils.getApplicationContext(),
-                       permission, Process.myPid(), Process.myUid())
+        boolean isGranted =
+                ApiCompatibilityUtils.checkPermission(ContextUtils.getApplicationContext(),
+                        permission, Process.myPid(), Process.myUid())
                 == PackageManager.PERMISSION_GRANTED;
+        if (isGranted) {
+            clearPermissionWasDenied(permission);
+        }
+        return isGranted;
+    }
+
+    /**
+     * Clear the shared pref indicating that {@code permission} was denied by the user.
+     */
+    private void clearPermissionWasDenied(String permission) {
+        String key = getPermissionWasDeniedKey(permission);
+        SharedPreferences prefs = ContextUtils.getAppSharedPreferences();
+        if (!prefs.contains(key)) return;
+
+        SharedPreferences.Editor editor = prefs.edit();
+        editor.remove(key);
+        editor.apply();
     }
 
     /** @see Activity.shouldShowRequestPermissionRationale */
@@ -51,22 +80,30 @@ public abstract class AndroidPermissionDelegateWithRequester implements AndroidP
     public final boolean canRequestPermission(String permission) {
         if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) return false;
 
+        if (hasPermission(permission)) {
+            // There is no need to call clearPermissionWasDenied - hasPermission already cleared
+            // the shared pref if needed.
+            return true;
+        }
+
         if (isPermissionRevokedByPolicy(permission)) {
             return false;
         }
 
         if (shouldShowRequestPermissionRationale(permission)) {
+            // This information from Android suggests we should not assume the user will always deny
+            // the permission.
+            clearPermissionWasDenied(permission);
             return true;
         }
 
-        // Check whether we have ever asked for this permission by checking whether we saved
+        // Check whether we have been denied this permission by checking whether we saved
         // a preference associated with it before.
-        String permissionQueriedKey = getHasRequestedPermissionKey(permission);
         SharedPreferences prefs = ContextUtils.getAppSharedPreferences();
-        return !prefs.getBoolean(permissionQueriedKey, false);
+        return !prefs.getBoolean(getPermissionWasDeniedKey(permission), false);
     }
 
-    /** @see PackageManager.isPermissionRevokedByPolicy */
+    /** @see PackageManager#isPermissionRevokedByPolicy(String, String) */
     protected abstract boolean isPermissionRevokedByPolicyInternal(String permission);
 
     @Override
@@ -102,7 +139,11 @@ public abstract class AndroidPermissionDelegateWithRequester implements AndroidP
             int requestCode, String[] permissions, int[] grantResults) {
         SharedPreferences.Editor editor = ContextUtils.getAppSharedPreferences().edit();
         for (int i = 0; i < permissions.length; i++) {
-            editor.putBoolean(getHasRequestedPermissionKey(permissions[i]), true);
+            if (grantResults[i] == PackageManager.PERMISSION_GRANTED) {
+                editor.remove(getPermissionWasDeniedKey(permissions[i]));
+            } else {
+                editor.putBoolean(getPermissionWasDeniedKey(permissions[i]), true);
+            }
         }
         editor.apply();
 
@@ -133,8 +174,7 @@ public abstract class AndroidPermissionDelegateWithRequester implements AndroidP
         return true;
     }
 
-    private String getHasRequestedPermissionKey(String permission) {
-        String permissionQueriedKey = permission;
+    private String normalizePermissionName(String permission) {
         // Prior to O, permissions were granted at the group level.  Post O, each permission is
         // granted individually.
         if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) {
@@ -151,13 +191,21 @@ public abstract class AndroidPermissionDelegateWithRequester implements AndroidP
                                 permission, PackageManager.GET_META_DATA);
 
                 if (!TextUtils.isEmpty(permissionInfo.group)) {
-                    permissionQueriedKey = permissionInfo.group;
+                    return permissionInfo.group;
                 }
             } catch (NameNotFoundException e) {
                 // Unknown permission.  Default back to the permission name instead of the group.
             }
         }
 
-        return PERMISSION_QUERIED_KEY_PREFIX + permissionQueriedKey;
+        return permission;
+    }
+
+    /**
+     * Returns the name of a shared preferences key used to store whether Chrome was denied
+     * {@code permission}.
+     */
+    private String getPermissionWasDeniedKey(String permission) {
+        return PERMISSION_WAS_DENIED_KEY_PREFIX + normalizePermissionName(permission);
     }
 }

ui/android/junit/AndroidManifest.xml

+<?xml version="1.0" encoding="utf-8"?>
+<!-- Copyright 2020 The Chromium Authors. All rights reserved.
+     Use of this source code is governed by a BSD-style license that can be
+     found in the LICENSE file.
+-->
+
+<!-- This manifest is required for tests using
+     androidx.test.core.app.ActivityScenario to work.
+-->
+<manifest xmlns:android="http://schemas.android.com/apk/res/android"
+    package="org.chromium.test.ui">
+
+  <!-- Value used by ActivityAndroidPermissionDelegateTest -->
+  <uses-permission android:name="android.permission.INTERNET"/>
+
+  <application android:theme="@style/Theme.AppCompat.Light">
+    <activity android:name="org.chromium.ui.base.TestActivity"/>
+  </application>
+</manifest>

ui/android/junit/src/org/chromium/ui/base/ActivityAndroidPermissionDelegateTest.java

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.ui.base;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+
+import android.content.pm.PackageManager;
+
+import androidx.test.core.app.ActivityScenario;
+import androidx.test.ext.junit.rules.ActivityScenarioRule;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mockito;
+import org.robolectric.Shadows;
+import org.robolectric.shadows.ShadowActivity;
+import org.robolectric.shadows.ShadowActivity.PermissionsRequest;
+
+import org.chromium.base.test.BaseRobolectricTestRunner;
+
+import java.lang.ref.WeakReference;
+
+/**
+ * Robolectric unit tests for {@link ActivityAndroidPermissionDelegate} and
+ * {@link AndroidPermissionDelegateWithRequester}.
+ */
+@RunWith(BaseRobolectricTestRunner.class)
+public class ActivityAndroidPermissionDelegateTest {
+    /**
+     * Rule managing the lifecycle of activity in each {@code @Test}.
+     * <p>
+     * To access the activity and run code on its main thread, use
+     * {@link ActivityScenario#onActivity}:
+     * <pre>
+     * mActivityScenarios.getScenario().onActivity(activity -> {
+     *     // Your test code using the activity here.
+     * });
+     * </pre>
+     */
+    @Rule
+    public ActivityScenarioRule<TestActivity> mActivityScenarios =
+            new ActivityScenarioRule<>(TestActivity.class);
+
+    @Test
+    public void testHasPermissionDenied() {
+        mActivityScenarios.getScenario().onActivity(activity -> {
+            AndroidPermissionDelegate permissionDelegate =
+                    new ActivityAndroidPermissionDelegate(new WeakReference(activity));
+
+            boolean hasPermission =
+                    permissionDelegate.hasPermission(android.Manifest.permission.INTERNET);
+
+            assertFalse("The default result of hasPermission should be false", hasPermission);
+        });
+    }
+
+    @Test
+    public void testHasPermissionGranted() {
+        mActivityScenarios.getScenario().onActivity(activity -> {
+            Shadows.shadowOf(activity).grantPermissions(android.Manifest.permission.INTERNET);
+            AndroidPermissionDelegate permissionDelegate =
+                    new ActivityAndroidPermissionDelegate(new WeakReference(activity));
+
+            boolean hasPermission =
+                    permissionDelegate.hasPermission(android.Manifest.permission.INTERNET);
+
+            assertTrue("hasPermission should return true if permission is granted", hasPermission);
+        });
+    }
+
+    @Test
+    public void testCanRequestPermissionInitial() {
+        mActivityScenarios.getScenario().onActivity(activity -> {
+            AndroidPermissionDelegate permissionDelegate =
+                    new ActivityAndroidPermissionDelegate(new WeakReference(activity));
+
+            boolean canRequest =
+                    permissionDelegate.canRequestPermission(android.Manifest.permission.INTERNET);
+
+            assertTrue("The default result of canRequestPermission should be true", canRequest);
+        });
+    }
+
+    @Test
+    public void testRequestPermissionsGranted() {
+        mActivityScenarios.getScenario().onActivity(activity -> {
+            AndroidPermissionDelegate permissionDelegate =
+                    new ActivityAndroidPermissionDelegate(new WeakReference(activity));
+            PermissionCallback callback = Mockito.mock(PermissionCallback.class);
+
+            // Request permission
+            permissionDelegate.requestPermissions(
+                    new String[] {android.Manifest.permission.INTERNET}, callback);
+
+            PermissionsRequest lastRequest =
+                    Shadows.shadowOf(activity).getLastRequestedPermission();
+            assertEquals(new String[] {android.Manifest.permission.INTERNET},
+                    lastRequest.requestedPermissions);
+            verify(callback,
+                    never().description("PermissionCallback should not receive results before "
+                            + "handlePermissionResult is invoked"))
+                    .onRequestPermissionsResult(any(), any());
+
+            // Respond to the request
+            int[] grantResults = new int[] {PackageManager.PERMISSION_GRANTED};
+            permissionDelegate.handlePermissionResult(
+                    lastRequest.requestCode, lastRequest.requestedPermissions, grantResults);
+
+            verify(callback,
+                    Mockito.description(
+                            "handlePermissionResult should invoke the PermissionCallback"))
+                    .onRequestPermissionsResult(lastRequest.requestedPermissions, grantResults);
+        });
+    }
+
+    @Test
+    public void testRequestPermissionsDenied() {
+        mActivityScenarios.getScenario().onActivity(activity -> {
+            AndroidPermissionDelegate permissionDelegate =
+                    new ActivityAndroidPermissionDelegate(new WeakReference(activity));
+            PermissionCallback callback = Mockito.mock(PermissionCallback.class);
+
+            // Request permission
+            permissionDelegate.requestPermissions(
+                    new String[] {android.Manifest.permission.INTERNET}, callback);
+
+            PermissionsRequest lastRequest =
+                    Shadows.shadowOf(activity).getLastRequestedPermission();
+            assertEquals(new String[] {android.Manifest.permission.INTERNET},
+                    lastRequest.requestedPermissions);
+            verify(callback,
+                    never().description("PermissionCallback should not receive results before "
+                            + "handlePermissionResult is invoked"))
+                    .onRequestPermissionsResult(any(), any());
+
+            // Respond to the request
+            int[] grantResults = new int[] {PackageManager.PERMISSION_DENIED};
+            permissionDelegate.handlePermissionResult(
+                    lastRequest.requestCode, lastRequest.requestedPermissions, grantResults);
+
+            verify(callback,
+                    Mockito.description(
+                            "handlePermissionResult should invoke the PermissionCallback"))
+                    .onRequestPermissionsResult(lastRequest.requestedPermissions, grantResults);
+        });
+    }
+
+    @Test
+    public void testCanRequestPermissionAfterRequestGranted() {
+        mActivityScenarios.getScenario().onActivity(activity -> {
+            AndroidPermissionDelegate permissionDelegate =
+                    new ActivityAndroidPermissionDelegate(new WeakReference(activity));
+            performRequestPermission(permissionDelegate, Shadows.shadowOf(activity),
+                    android.Manifest.permission.INTERNET, PackageManager.PERMISSION_GRANTED);
+
+            boolean canRequest =
+                    permissionDelegate.canRequestPermission(android.Manifest.permission.INTERNET);
+
+            assertTrue("After a granted permission request canRequestPermission should return true",
+                    canRequest);
+        });
+    }
+
+    @Test
+    public void testCanRequestPermissionAfterRequestDenied() {
+        mActivityScenarios.getScenario().onActivity(activity -> {
+            AndroidPermissionDelegate permissionDelegate =
+                    new ActivityAndroidPermissionDelegate(new WeakReference(activity));
+            performRequestPermission(permissionDelegate, Shadows.shadowOf(activity),
+                    android.Manifest.permission.INTERNET, PackageManager.PERMISSION_DENIED);
+
+            boolean canRequest =
+                    permissionDelegate.canRequestPermission(android.Manifest.permission.INTERNET);
+
+            assertFalse(
+                    "After a denied permission request canRequestPermission should return false",
+                    canRequest);
+        });
+    }
+
+    @Test
+    public void testCanRequestPermissionWithShowRequestRationale() {
+        mActivityScenarios.getScenario().onActivity(activity -> {
+            AndroidPermissionDelegate permissionDelegate =
+                    new ActivityAndroidPermissionDelegate(new WeakReference(activity));
+            performRequestPermission(permissionDelegate, Shadows.shadowOf(activity),
+                    android.Manifest.permission.INTERNET, PackageManager.PERMISSION_DENIED);
+            Shadows.shadowOf(activity.getPackageManager())
+                    .setShouldShowRequestPermissionRationale(
+                            android.Manifest.permission.INTERNET, true);
+
+            boolean canRequest =
+                    permissionDelegate.canRequestPermission(android.Manifest.permission.INTERNET);
+
+            assertTrue("When shouldShowRequestPermissionRationale is true "
+                            + "canRequestPermission should return true",
+                    canRequest);
+        });
+    }
+
+    @Test
+    public void testCanRequestPermissionAfterHasPermissionGranted() {
+        mActivityScenarios.getScenario().onActivity(activity -> {
+            AndroidPermissionDelegate permissionDelegate =
+                    new ActivityAndroidPermissionDelegate(new WeakReference(activity));
+            performRequestPermission(permissionDelegate, Shadows.shadowOf(activity),
+                    android.Manifest.permission.INTERNET, PackageManager.PERMISSION_DENIED);
+
+            Shadows.shadowOf(activity).grantPermissions(android.Manifest.permission.INTERNET);
+            permissionDelegate.hasPermission(android.Manifest.permission.INTERNET);
+            Shadows.shadowOf(activity).denyPermissions(android.Manifest.permission.INTERNET);
+
+            boolean canRequest =
+                    permissionDelegate.canRequestPermission(android.Manifest.permission.INTERNET);
+
+            assertTrue("After hasPermission sees that a permission is granted "
+                            + "canRequestPermission should return true",
+                    canRequest);
+        });
+    }
+
+    @Test
+    public void testCanRequestPermissionWhileGranted() {
+        mActivityScenarios.getScenario().onActivity(activity -> {
+            AndroidPermissionDelegate permissionDelegate =
+                    new ActivityAndroidPermissionDelegate(new WeakReference(activity));
+            performRequestPermission(permissionDelegate, Shadows.shadowOf(activity),
+                    android.Manifest.permission.INTERNET, PackageManager.PERMISSION_DENIED);
+            Shadows.shadowOf(activity).grantPermissions(android.Manifest.permission.INTERNET);
+
+            boolean canRequest =
+                    permissionDelegate.canRequestPermission(android.Manifest.permission.INTERNET);
+
+            assertTrue(
+                    "If a permission is currently granted canRequestPermission should return true",
+                    canRequest);
+        });
+    }
+
+    /**
+     * Calls {@link AndroidPermissionDelegate#requestPermissions} and {@link
+     * AndroidPermissionDelegate#handlePermissionResult} for a single permission.
+     */
+    private void performRequestPermission(AndroidPermissionDelegate permissionDelegate,
+            ShadowActivity shadowActivity, String permission, int grantResult) {
+        permissionDelegate.requestPermissions(
+                new String[] {permission}, Mockito.mock(PermissionCallback.class));
+        PermissionsRequest lastRequest = shadowActivity.getLastRequestedPermission();
+        permissionDelegate.handlePermissionResult(
+                lastRequest.requestCode, lastRequest.requestedPermissions, new int[] {grantResult});
+    }
+}

ui/android/junit/src/org/chromium/ui/base/TestActivity.java

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.ui.base;
+
+import androidx.appcompat.app.AppCompatActivity;
+
+/**
+ * Activity used in {@code ui/base} tests.
+ * <p>
+ * This activity is declared in {@code ui/android/junit/AndroidManifest.xml}.
+ */
+public class TestActivity extends AppCompatActivity {}