Merge "Fix for legacy storage app op" into main

7 files changed, 155 insertions, 3 deletions

diff --git a/src/com/android/providers/media/LocalCallingIdentity.java b/src/com/android/providers/media/LocalCallingIdentity.java
index 8074656bd..2a5e452ce 100644
--- a/src/com/android/providers/media/LocalCallingIdentity.java
+++ b/src/com/android/providers/media/LocalCallingIdentity.java
@@ -29,8 +29,8 @@ import static com.android.providers.media.util.PermissionUtils.checkPermissionIn
 import static com.android.providers.media.util.PermissionUtils.checkPermissionManager;
 import static com.android.providers.media.util.PermissionUtils.checkPermissionQueryAllPackages;
 import static com.android.providers.media.util.PermissionUtils.checkPermissionReadAudio;
+import static com.android.providers.media.util.PermissionUtils.checkPermissionReadForLegacyStorage;
 import static com.android.providers.media.util.PermissionUtils.checkPermissionReadImages;
-import static com.android.providers.media.util.PermissionUtils.checkPermissionReadStorage;
 import static com.android.providers.media.util.PermissionUtils.checkPermissionReadVideo;
 import static com.android.providers.media.util.PermissionUtils.checkPermissionReadVisualUserSelected;
 import static com.android.providers.media.util.PermissionUtils.checkPermissionSelf;
@@ -576,8 +576,14 @@ public class LocalCallingIdentity {
     }
 
     private boolean isLegacyReadInternal() {
-        return hasPermission(PERMISSION_IS_LEGACY_GRANTED)
-                && checkPermissionReadStorage(context, pid, uid, getPackageName(), attributionTag);
+        boolean isLegacyStorageGranted = hasPermission(PERMISSION_IS_LEGACY_GRANTED);
+        if (!isLegacyStorageGranted) {
+            return false;
+        }
+
+        boolean isTargetSdkAtleastT = getTargetSdkVersion() >= Build.VERSION_CODES.TIRAMISU;
+        return checkPermissionReadForLegacyStorage(context, pid, uid, getPackageName(),
+                attributionTag, isTargetSdkAtleastT);
     }
 
     /** System internals or callers holding permission have no redaction */
diff --git a/src/com/android/providers/media/util/PermissionUtils.java b/src/com/android/providers/media/util/PermissionUtils.java
index 2458785e2..7dbfd23da 100644
--- a/src/com/android/providers/media/util/PermissionUtils.java
+++ b/src/com/android/providers/media/util/PermissionUtils.java
@@ -149,6 +149,41 @@ public class PermissionUtils {
     }
 
     /**
+     * Check for read permission when legacy storage is granted.
+     * There is a bug in AppOpsManager that keeps legacy storage granted even
+     * when an app updates its targetSdkVersion from value <30 to >=30.
+     * If an app upgrades from targetSdk 29 to targetSdk 33, legacy storage
+     * remains granted and in targetSdk 33, app are required to replace R_E_S
+     * with R_M_*. If an app updates its manifest with R_M_*, permission check
+     * in MediaProvider will look for R_E_S and will not grant read access as
+     * the app would be still treated as legacy. Ensure that legacy app either has
+     * R_E_S or all of R_M_* to get read permission. Since this is a fix for legacy
+     * app op bug, we are avoiding granular permission checks based on media type.
+     */
+    public static boolean checkPermissionReadForLegacyStorage(@NonNull Context context,
+            int pid, int uid, @NonNull String packageName, @Nullable String attributionTag,
+            boolean isTargetSdkAtleastT) {
+        if (isTargetSdkAtleastT) {
+            return checkPermissionForDataDelivery(context, READ_EXTERNAL_STORAGE, pid, uid,
+                    packageName, attributionTag,
+                    generateAppOpMessage(packageName, sOpDescription.get())) || (
+                    checkPermissionForDataDelivery(context, READ_MEDIA_IMAGES, pid, uid,
+                            packageName, attributionTag,
+                            generateAppOpMessage(packageName, sOpDescription.get()))
+                            && checkPermissionForDataDelivery(context, READ_MEDIA_VIDEO, pid, uid,
+                            packageName, attributionTag,
+                            generateAppOpMessage(packageName, sOpDescription.get()))
+                            && checkPermissionForDataDelivery(context, READ_MEDIA_AUDIO, pid, uid,
+                            packageName, attributionTag,
+                            generateAppOpMessage(packageName, sOpDescription.get())));
+        } else {
+            return checkPermissionForDataDelivery(context, READ_EXTERNAL_STORAGE, pid, uid,
+                    packageName, attributionTag,
+                    generateAppOpMessage(packageName, sOpDescription.get()));
+        }
+    }
+
+    /**
      * Check if the given package has been granted the
      * android.Manifest.permission#ACCESS_MEDIA_LOCATION permission.
      */
diff --git a/tests/Android.bp b/tests/Android.bp
index cdb44b814..3de35a077 100644
--- a/tests/Android.bp
+++ b/tests/Android.bp
@@ -155,6 +155,25 @@ android_test_helper_app {
     ],
 }
 
+android_test_helper_app {
+    name: "LegacyMediaProviderTestAppFor33",
+    manifest: "test_app/LegacyTestAppWithTargetSdk33.xml",
+    srcs: [
+        "test_app/src/**/*.java",
+        "src/com/android/providers/media/util/TestUtils.java",
+    ],
+    static_libs: [
+        "cts-install-lib",
+    ],
+    sdk_version: "test_current",
+    target_sdk_version: "33",
+    min_sdk_version: "30",
+    test_suites: [
+        "general-tests",
+        "mts-mediaprovider",
+    ],
+}
+
 // This looks a bit awkward, but we need our tests to run against either
 // MediaProvider or MediaProviderGoogle, and we don't know which one is
 // on the device being tested, so we can't sign our tests with a key that
@@ -250,6 +269,7 @@ android_test {
 
     data: [
         ":LegacyMediaProviderTestApp",
+        ":LegacyMediaProviderTestAppFor33",
         ":LegacyMediaProviderTestAppFor35",
         ":MediaProviderTestAppForPermissionActivity",
         ":MediaProviderTestAppForPermissionActivity33",
diff --git a/tests/AndroidManifest.xml b/tests/AndroidManifest.xml
index 0917ac388..2e7d9348c 100644
--- a/tests/AndroidManifest.xml
+++ b/tests/AndroidManifest.xml
@@ -13,6 +13,7 @@
         <package android:name="com.android.providers.media.testapp.withuserselectedperms" />
         <package android:name="com.android.providers.media.testapp.legacy" />
         <package android:name="com.android.providers.media.testapp.legacywithtargetsdk35" />
+        <package android:name="com.android.providers.media.testapp.legacywithtargetsdk33" />
     </queries>
 
     <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" />
diff --git a/tests/AndroidTest.xml b/tests/AndroidTest.xml
index 31d9e1535..44fb27930 100644
--- a/tests/AndroidTest.xml
+++ b/tests/AndroidTest.xml
@@ -30,6 +30,7 @@
         <option name="test-file-name" value="MediaProviderTestAppWithUserSelectedPerms.apk" />
         <option name="test-file-name" value="MediaProviderTestAppWithoutPerms.apk" />
         <option name="test-file-name" value="LegacyMediaProviderTestApp.apk" />
+        <option name="test-file-name" value="LegacyMediaProviderTestAppFor33.apk" />
         <option name="test-file-name" value="LegacyMediaProviderTestAppFor35.apk" />
         <option name="install-arg" value="-g" />
     </target_preparer>
diff --git a/tests/src/com/android/providers/media/util/PermissionUtilsTest.java b/tests/src/com/android/providers/media/util/PermissionUtilsTest.java
index f58f1bbf6..7eb6c667b 100644
--- a/tests/src/com/android/providers/media/util/PermissionUtilsTest.java
+++ b/tests/src/com/android/providers/media/util/PermissionUtilsTest.java
@@ -21,6 +21,7 @@ import static android.Manifest.permission.MANAGE_EXTERNAL_STORAGE;
 import static android.Manifest.permission.MANAGE_MEDIA;
 import static android.Manifest.permission.UPDATE_APP_OPS_STATS;
 import static android.app.AppOpsManager.OPSTR_ACCESS_MEDIA_LOCATION;
+import static android.app.AppOpsManager.OPSTR_LEGACY_STORAGE;
 import static android.app.AppOpsManager.OPSTR_NO_ISOLATED_STORAGE;
 import static android.app.AppOpsManager.OPSTR_READ_MEDIA_AUDIO;
 import static android.app.AppOpsManager.OPSTR_READ_MEDIA_IMAGES;
@@ -44,6 +45,7 @@ import static com.android.providers.media.util.PermissionUtils.checkPermissionIn
 import static com.android.providers.media.util.PermissionUtils.checkPermissionManageMedia;
 import static com.android.providers.media.util.PermissionUtils.checkPermissionManager;
 import static com.android.providers.media.util.PermissionUtils.checkPermissionReadAudio;
+import static com.android.providers.media.util.PermissionUtils.checkPermissionReadForLegacyStorage;
 import static com.android.providers.media.util.PermissionUtils.checkPermissionReadImages;
 import static com.android.providers.media.util.PermissionUtils.checkPermissionReadStorage;
 import static com.android.providers.media.util.PermissionUtils.checkPermissionReadVideo;
@@ -105,6 +107,10 @@ public class PermissionUtilsTest {
             "com.android.providers.media.testapp.legacy", 1, false,
             "LegacyMediaProviderTestApp.apk");
 
+    private static final TestApp LEGACY_TEST_APP_33 = new TestApp("LegacyTestAppWithTargetSdk33",
+            "com.android.providers.media.testapp.legacywithtargetsdk33", 1, false,
+            "LegacyMediaProviderTestAppFor33.apk");
+
     private static final TestApp LEGACY_TEST_APP_35 = new TestApp("LegacyTestAppWithTargetSdk35",
             "com.android.providers.media.testapp.legacywithtargetsdk35", 1, false,
             "LegacyMediaProviderTestAppFor35.apk");
@@ -251,6 +257,44 @@ public class PermissionUtilsTest {
                     getContext(), TEST_APP_PID, testAppUid, packageName, null)).isFalse();
             assertThat(checkPermissionReadStorage(
                     getContext(), TEST_APP_PID, testAppUid, packageName, null)).isTrue();
+            assertThat(checkPermissionReadForLegacyStorage(
+                    getContext(), TEST_APP_PID, testAppUid, packageName,
+                    null, /* isTargetSdkAtLeastT */ true)).isTrue();
+        } finally {
+            dropShellPermission();
+        }
+    }
+
+    @Test
+    @SdkSuppress(minSdkVersion = Build.VERSION_CODES.TIRAMISU, codeName = "Tiramisu")
+    public void testDefaultPermissionsOnLegacyTestAppWithTargetSdk33() throws Exception {
+        String packageName = LEGACY_TEST_APP_33.getPackageName();
+        int testAppUid = getContext().getPackageManager().getPackageUid(packageName, 0);
+        adoptShellPermission(UPDATE_APP_OPS_STATS, MANAGE_APP_OPS_MODES);
+
+        try {
+            assertThat(checkPermissionSelf(getContext(), TEST_APP_PID, testAppUid)).isFalse();
+            assertThat(checkPermissionShell(testAppUid)).isFalse();
+            assertThat(checkPermissionInstallPackages(
+                    getContext(), TEST_APP_PID, testAppUid, packageName, null)).isFalse();
+            assertThat(checkPermissionAccessMtp(
+                    getContext(), TEST_APP_PID, testAppUid, packageName, null)).isFalse();
+            assertThat(checkPermissionWriteStorage(
+                    getContext(), TEST_APP_PID, testAppUid, packageName, null)).isFalse();
+
+            modifyAppOp(testAppUid, OPSTR_READ_MEDIA_IMAGES, AppOpsManager.MODE_ALLOWED);
+            modifyAppOp(testAppUid, OPSTR_READ_MEDIA_VIDEO, AppOpsManager.MODE_ALLOWED);
+            modifyAppOp(testAppUid, OPSTR_READ_MEDIA_AUDIO, AppOpsManager.MODE_ALLOWED);
+            modifyAppOp(testAppUid, OPSTR_LEGACY_STORAGE, AppOpsManager.MODE_ALLOWED);
+
+            assertThat(checkIsLegacyStorageGranted(getContext(), testAppUid,
+                    packageName,  /* isTargetSdkAtLeastV */ false)).isTrue();
+            // Since R_E_S is not granted, this is should return false
+            assertThat(checkPermissionReadStorage(
+                    getContext(), TEST_APP_PID, testAppUid, packageName, null)).isFalse();
+            assertThat(checkPermissionReadForLegacyStorage(
+                    getContext(), TEST_APP_PID, testAppUid, packageName,
+                    null, /* isTargetSdkAtLeastT */ true)).isTrue();
         } finally {
             dropShellPermission();
         }
@@ -330,6 +374,11 @@ public class PermissionUtilsTest {
                     checkIsLegacyStorageGranted(getContext(), testAppUid, packageName,
                             /* isTargetSdkAtLeastS */ false)).isTrue();
             assertThat(
+                    checkPermissionReadForLegacyStorage(getContext(), TEST_APP_PID,
+                            testAppUid, packageName,
+                            null, /* isTargetSdkAtLeastT */ false)).isTrue();
+
+            assertThat(
                     checkPermissionInstallPackages(getContext(), TEST_APP_PID, testAppUid,
                             packageName, null)).isFalse();
             assertThat(
diff --git a/tests/test_app/LegacyTestAppWithTargetSdk33.xml b/tests/test_app/LegacyTestAppWithTargetSdk33.xml
new file mode 100644
index 000000000..541e5abcf
--- /dev/null
+++ b/tests/test_app/LegacyTestAppWithTargetSdk33.xml
@@ -0,0 +1,40 @@
+<?xml version="1.0" encoding="utf-8"?>
+<!--
+  ~ Copyright (C) 2025 The Android Open Source Project
+  ~
+  ~ Licensed under the Apache License, Version 2.0 (the "License");
+  ~ you may not use this file except in compliance with the License.
+  ~ You may obtain a copy of the License at
+  ~
+  ~      http://www.apache.org/licenses/LICENSE-2.0
+  ~
+  ~ Unless required by applicable law or agreed to in writing, software
+  ~ distributed under the License is distributed on an "AS IS" BASIS,
+  ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  ~ See the License for the specific language governing permissions and
+  ~ limitations under the License.
+  -->
+
+<manifest xmlns:android="http://schemas.android.com/apk/res/android"
+          package="com.android.providers.media.testapp.legacywithtargetsdk33"
+          android:versionCode="1"
+          android:versionName="1.0">
+
+    <uses-sdk android:minSdkVersion="30" android:targetSdkVersion="33" />
+
+    <uses-permission android:name="android.permission.READ_MEDIA_IMAGES" />
+    <uses-permission android:name="android.permission.READ_MEDIA_VIDEO" />
+    <uses-permission android:name="android.permission.READ_MEDIA_AUDIO" />
+
+    <application android:label="LegacyTestAppWithTargetSdk33"
+                 android:requestLegacyExternalStorage="true">
+        <activity android:name="com.android.providers.media.util.TestAppActivity"
+                  android:exported="true">
+            <intent-filter>
+                <action android:name="android.intent.action.MAIN"/>
+                <category android:name="android.intent.category.DEFAULT"/>
+                <category android:name="android.intent.category.LAUNCHER"/>
+            </intent-filter>
+        </activity>
+    </application>
+</manifest>