Update database for files renamed by apps that bypass restrictions.
Previously, for apps that bypassed scoped storage restrictions, we
deleted database entry for old path but didn't add new entry for new
path. This left the database in inconsistent state. In the current flow,
we update database entries on rename. App that bypasses scoped storage
restrictions skips all checks while updating database entries.
This doesn't add support for system gallery apps to rename directories
with media/non-media files.
Modified tests to check database updates after rename.
Test:atest FuseDaemonHostTest#testSystemGalleryCanRenameImagesAndVideos
Test atest
FuseDaemonHostTest#testManageExternalStorageCanRenameOtherAppsContents
Test: atest FuseDaemonHostTest
Bug: 150147690
Change-Id: I0472a9547ae3640b985cbafb6fafbe54d8f4f4df
diff --git a/src/com/android/providers/media/MediaProvider.java b/src/com/android/providers/media/MediaProvider.java
index a2cad24..e48eea1 100644
--- a/src/com/android/providers/media/MediaProvider.java
+++ b/src/com/android/providers/media/MediaProvider.java
@@ -1195,6 +1195,9 @@
boolean retryUpdateWithReplace = false;
try {
+ // TODO(b/146777893): System gallery apps can rename a media directory containing
+ // non-media files. This update doesn't support updating non-media files that are not
+ // owned by system gallery app.
count = qbForUpdate.update(helper, values, selection, new String[]{oldPath});
} catch (SQLiteConstraintException e) {
Log.w(TAG, "Database update failed while renaming " + oldPath, e);
@@ -1241,6 +1244,27 @@
}
/**
+ * Gets all files in the given {@code path} and subdirectories of the given {@code path}.
+ */
+ private ArrayList<String> getAllFilesForRenameDirectory(String oldPath) {
+ final String selection = MediaColumns.RELATIVE_PATH + " REGEXP '^" +
+ extractRelativePathForDirectory(oldPath) + "/?.*' and mime_type not like 'null'";
+ ArrayList<String> fileList = new ArrayList<>();
+
+ final LocalCallingIdentity token = clearLocalCallingIdentity();
+ try (final Cursor c = query(Files.getContentUriForPath(oldPath),
+ new String[] {MediaColumns.DATA}, selection, null, null)) {
+ while (c.moveToNext()) {
+ final String filePath = c.getString(0).replaceFirst("^" + oldPath + "/(.*)", "$1");
+ fileList.add(filePath);
+ }
+ } finally {
+ restoreLocalCallingIdentity(token);
+ }
+ return fileList;
+ }
+
+ /**
* Gets files in the given {@code path} and subdirectories of the given {@code path} for which
* calling package has write permissions.
*
@@ -1248,7 +1272,8 @@
* files for which calling package doesn't have write permission or if file type is not
* supported in {@code newPath}
*/
- private ArrayList<String> getWritableFilesForRenameDirectory(String oldPath, String newPath) {
+ private ArrayList<String> getWritableFilesForRenameDirectory(String oldPath, String newPath)
+ throws IllegalArgumentException {
final int countAllFilesInDirectory;
final String selection = MediaColumns.RELATIVE_PATH + " REGEXP '^" +
extractRelativePathForDirectory(oldPath) + "/?.*' and mime_type not like 'null'";
@@ -1330,16 +1355,21 @@
* write permission.
* This method can also return errno returned from {@code Os.rename} function.
*/
- private int renameDirectoryForFuse(String oldPath, String newPath) {
+ private int renameDirectoryCheckedForFuse(String oldPath, String newPath) {
final ArrayList<String> fileList;
try {
fileList = getWritableFilesForRenameDirectory(oldPath, newPath);
- } catch(IllegalArgumentException e) {
+ } catch (IllegalArgumentException e) {
final String errorMessage = "Rename " + oldPath + " to " + newPath + " failed. ";
Log.e(TAG, errorMessage, e);
return OsConstants.EPERM;
}
+ return renameDirectoryUncheckedForFuse(oldPath, newPath, fileList);
+ }
+
+ private int renameDirectoryUncheckedForFuse(String oldPath, String newPath,
+ ArrayList<String> fileList) {
final DatabaseHelper helper;
try {
helper = getDatabaseForUri(Files.getContentUriForPath(oldPath));
@@ -1390,13 +1420,17 @@
* {@code oldPath} or {@code newPath}, or file type is not supported by {@code newPath}.
* This method can also return errno returned from {@code Os.rename} function.
*/
- private int renameFileForFuse(String oldPath, String newPath) {
+ private int renameFileCheckedForFuse(String oldPath, String newPath) {
// Check if new mime type is supported in new path.
final String newMimeType = MimeUtils.resolveMimeType(new File(newPath));
if (!isMimeTypeSupportedInPath(newPath, newMimeType)) {
return OsConstants.EPERM;
}
+ return renameFileUncheckedForFuse(oldPath, newPath);
+ }
+ private int renameFileUncheckedForFuse(String oldPath, String newPath) {
+ final String newMimeType = MimeUtils.resolveMimeType(new File(newPath));
final DatabaseHelper helper;
try {
helper = getDatabaseForUri(Files.getContentUriForPath(oldPath));
@@ -1428,19 +1462,18 @@
}
/**
- * Renames file or directory in lower file system and deletes {@code oldPath} from database.
+ * Rename file/directory without imposing any restrictions.
+ *
+ * We don't impose any rename restrictions for apps that bypass scoped storage restrictions.
+ * However, we update database entries for renamed files to keep the database consistent.
*/
private int renameUncheckedForFuse(String oldPath, String newPath) {
- final int result = renameInLowerFs(oldPath, newPath);
- if (result == 0) {
- LocalCallingIdentity token = clearLocalCallingIdentity();
- try {
- scanFile(new File(oldPath), REASON_DEMAND);
- } finally {
- restoreLocalCallingIdentity(token);
- }
+ if (new File(oldPath).isFile()) {
+ return renameFileUncheckedForFuse(oldPath, newPath);
+ } else {
+ return renameDirectoryUncheckedForFuse(oldPath, newPath,
+ getAllFilesForRenameDirectory(oldPath));
}
- return result;
}
/**
@@ -1537,9 +1570,9 @@
// Continue renaming files/directories if rename of oldPath to newPath is allowed.
if (new File(oldPath).isFile()) {
- return renameFileForFuse(oldPath, newPath);
+ return renameFileCheckedForFuse(oldPath, newPath);
} else {
- return renameDirectoryForFuse(oldPath, newPath);
+ return renameDirectoryCheckedForFuse(oldPath, newPath);
}
} finally {
restoreLocalCallingIdentity(token);
diff --git a/tests/jni/FuseDaemonTest/host/src/com/android/tests/fused/host/LegacyAccessHostTest.java b/tests/jni/FuseDaemonTest/host/src/com/android/tests/fused/host/LegacyAccessHostTest.java
index 1f1b8c7..0502aed 100644
--- a/tests/jni/FuseDaemonTest/host/src/com/android/tests/fused/host/LegacyAccessHostTest.java
+++ b/tests/jni/FuseDaemonTest/host/src/com/android/tests/fused/host/LegacyAccessHostTest.java
@@ -161,7 +161,12 @@
@Test
public void testCantRename_hasR() throws Exception {
revokePermissions("android.permission.WRITE_EXTERNAL_STORAGE");
- runDeviceTest("testCantRename_hasR");
+ createFileAsShell(SHELL_FILE, /*bypassFuse*/ true);
+ try {
+ runDeviceTest("testCantRename_hasR");
+ } finally {
+ executeShellCommand("rm " + SHELL_FILE);
+ }
}
diff --git a/tests/jni/FuseDaemonTest/legacy/src/com/android/tests/fused/legacy/LegacyFileAccessTest.java b/tests/jni/FuseDaemonTest/legacy/src/com/android/tests/fused/legacy/LegacyFileAccessTest.java
index 49609d2..8e0a0d9 100644
--- a/tests/jni/FuseDaemonTest/legacy/src/com/android/tests/fused/legacy/LegacyFileAccessTest.java
+++ b/tests/jni/FuseDaemonTest/legacy/src/com/android/tests/fused/legacy/LegacyFileAccessTest.java
@@ -16,6 +16,9 @@
package com.android.tests.fused.legacy;
+import static com.android.tests.fused.lib.TestUtils.assertCanRenameFile;
+import static com.android.tests.fused.lib.TestUtils.assertCanRenameDirectory;
+import static com.android.tests.fused.lib.TestUtils.assertCantRenameFile;
import static com.android.tests.fused.lib.TestUtils.createFileAs;
@@ -294,16 +297,16 @@
try {
// can rename a file to root directory.
assertThat(musicFile1.createNewFile()).isTrue();
- assertCanRename(musicFile1, musicFile2);
+ assertCanRenameFile(musicFile1, musicFile2);
// can rename a music file to Movies directory.
- assertCanRename(musicFile2, musicFile3);
+ assertCanRenameFile(musicFile2, musicFile3);
assertThat(nonMediaDir1.mkdir()).isTrue();
assertThat(pdfFile1.createNewFile()).isTrue();
// can rename directory to root directory.
- assertCanRename(nonMediaDir1, nonMediaDir2);
- assertThat(pdfFile2.exists()).isTrue();
+ assertCanRenameDirectory(nonMediaDir1, nonMediaDir2, new File[]{pdfFile1},
+ new File[]{pdfFile2});
} finally {
musicFile1.delete();
musicFile2.delete();
@@ -335,13 +338,14 @@
getExternalMediaDirs()[0], "LegacyFileAccessTest2");
try {
// app can't rename shell file.
- assertThat(shellFile1.renameTo(shellFile2)).isFalse();
+ assertCantRenameFile(shellFile1, shellFile2);
// app can't move shell file to its media directory.
- assertThat(mediaFile1.renameTo(shellFile1)).isFalse();
+ assertCantRenameFile(shellFile1, mediaFile1);
// However, even without permissions, app can rename files in its own external media
// directory.
assertThat(mediaFile1.createNewFile()).isTrue();
- assertCanRename(mediaFile1, mediaFile2);
+ assertThat(mediaFile1.renameTo(mediaFile2)).isTrue();
+ assertThat(mediaFile2.exists()).isTrue();
} finally {
mediaFile1.delete();
mediaFile2.delete();
@@ -367,13 +371,14 @@
getExternalMediaDirs()[0], "LegacyFileAccessTest2");
try {
// app can't rename shell file.
- assertThat(shellFile1.renameTo(shellFile2)).isFalse();
+ assertCantRenameFile(shellFile1, shellFile2);
// app can't move shell file to its media directory.
- assertThat(mediaFile1.renameTo(shellFile1)).isFalse();
+ assertCantRenameFile(shellFile1, mediaFile1);
// However, even without permissions, app can rename files in its own external media
// directory.
assertThat(mediaFile1.createNewFile()).isTrue();
- assertCanRename(mediaFile1, mediaFile2);
+ assertThat(mediaFile1.renameTo(mediaFile2)).isTrue();
+ assertThat(mediaFile2.exists()).isTrue();
} finally {
mediaFile1.delete();
mediaFile2.delete();
@@ -480,10 +485,4 @@
dir.delete();
}
}
-
- private static void assertCanRename(File oldPath, File newPath) {
- assertThat(oldPath.renameTo(newPath)).isTrue();
- assertThat(oldPath.exists()).isFalse();
- assertThat(newPath.exists()).isTrue();
- }
}
diff --git a/tests/jni/FuseDaemonTest/libs/FuseDaemonTestLib/src/com/android/tests/fused/lib/TestUtils.java b/tests/jni/FuseDaemonTest/libs/FuseDaemonTestLib/src/com/android/tests/fused/lib/TestUtils.java
index eb34939..89863ed 100644
--- a/tests/jni/FuseDaemonTest/libs/FuseDaemonTestLib/src/com/android/tests/fused/lib/TestUtils.java
+++ b/tests/jni/FuseDaemonTest/libs/FuseDaemonTestLib/src/com/android/tests/fused/lib/TestUtils.java
@@ -404,6 +404,46 @@
return path.delete();
}
+ public static void assertCanRenameFile(File oldFile, File newFile) {
+ assertThat(oldFile.renameTo(newFile)).isTrue();
+ assertThat(oldFile.exists()).isFalse();
+ assertThat(newFile.exists()).isTrue();
+ assertThat(getFileRowIdFromDatabase(oldFile)).isEqualTo(-1);
+ assertThat(getFileRowIdFromDatabase(newFile)).isNotEqualTo(-1);
+ }
+
+ public static void assertCantRenameFile(File oldFile, File newFile) {
+ final int rowId = getFileRowIdFromDatabase(oldFile);
+ assertThat(oldFile.renameTo(newFile)).isFalse();
+ assertThat(oldFile.exists()).isTrue();
+ assertThat(getFileRowIdFromDatabase(oldFile)).isEqualTo(rowId);
+ }
+
+ public static void assertCanRenameDirectory(File oldDirectory, File newDirectory,
+ @Nullable File[] oldFilesList, @Nullable File[] newFilesList) {
+ assertThat(oldDirectory.renameTo(newDirectory)).isTrue();
+ assertThat(oldDirectory.exists()).isFalse();
+ assertThat(newDirectory.exists()).isTrue();
+ for (File file : oldFilesList != null ? oldFilesList : new File[0]) {
+ assertThat(file.exists()).isFalse();
+ assertThat(getFileRowIdFromDatabase(file)).isEqualTo(-1);
+ }
+ for (File file : newFilesList != null ? newFilesList : new File[0]) {
+ assertThat(file.exists()).isTrue();
+ assertThat(getFileRowIdFromDatabase(file)).isNotEqualTo(-1);
+ };
+ }
+
+ public static void assertCantRenameDirectory(File oldDirectory, File newDirectory,
+ @Nullable File[] oldFilesList) {
+ assertThat(oldDirectory.renameTo(newDirectory)).isFalse();
+ assertThat(oldDirectory.exists()).isTrue();
+ for (File file : oldFilesList != null ? oldFilesList : new File[0]) {
+ assertThat(file.exists()).isTrue();
+ assertThat(getFileRowIdFromDatabase(file)).isNotEqualTo(-1);
+ }
+ }
+
public static void pollForExternalStorageState() throws Exception {
for (int i = 0; i < POLLING_TIMEOUT_MILLIS / POLLING_SLEEP_MILLIS; i++) {
if(Environment.getExternalStorageState(Environment.getExternalStorageDirectory())
diff --git a/tests/jni/FuseDaemonTest/src/com/android/tests/fused/FilePathAccessTest.java b/tests/jni/FuseDaemonTest/src/com/android/tests/fused/FilePathAccessTest.java
index 8f47edd..97daf94 100644
--- a/tests/jni/FuseDaemonTest/src/com/android/tests/fused/FilePathAccessTest.java
+++ b/tests/jni/FuseDaemonTest/src/com/android/tests/fused/FilePathAccessTest.java
@@ -25,8 +25,12 @@
import static com.android.tests.fused.lib.RedactionTestHelper.assertExifMetadataMismatch;
import static com.android.tests.fused.lib.RedactionTestHelper.getExifMetadata;
import static com.android.tests.fused.lib.RedactionTestHelper.getExifMetadataFromRawResource;
+import static com.android.tests.fused.lib.TestUtils.assertCanRenameFile;
+import static com.android.tests.fused.lib.TestUtils.assertCanRenameDirectory;
import static com.android.tests.fused.lib.TestUtils.adoptShellPermissionIdentity;
import static com.android.tests.fused.lib.TestUtils.allowAppOpsToUid;
+import static com.android.tests.fused.lib.TestUtils.assertCantRenameDirectory;
+import static com.android.tests.fused.lib.TestUtils.assertCantRenameFile;
import static com.android.tests.fused.lib.TestUtils.assertThrows;
import static com.android.tests.fused.lib.TestUtils.createFileAs;
import static com.android.tests.fused.lib.TestUtils.deleteFileAs;
@@ -989,21 +993,19 @@
assertFileContent(otherAppVideoFile, BYTES_DATA1);
// Assert we can rename the file and ensure the file has the same content
- assertThat(otherAppVideoFile.renameTo(videoFile)).isTrue();
- assertThat(otherAppVideoFile.exists()).isFalse();
+ assertCanRenameFile(otherAppVideoFile, videoFile);
assertFileContent(videoFile, BYTES_DATA1);
// We can even move it to the top level directory
- assertThat(videoFile.renameTo(topLevelVideoFile)).isTrue();
- assertThat(videoFile.exists()).isFalse();
+ assertCanRenameFile(videoFile, topLevelVideoFile);
assertFileContent(topLevelVideoFile, BYTES_DATA1);
// And we can even convert it into an image file, because why not?
- assertThat(topLevelVideoFile.renameTo(imageFile)).isTrue();
- assertThat(topLevelVideoFile.exists()).isFalse();
+ assertCanRenameFile(topLevelVideoFile, imageFile);
assertFileContent(imageFile, BYTES_DATA1);
- // However, we can't convert it to a music file, because system gallery has full access
- // to images and video only
- assertThat(imageFile.renameTo(musicFile)).isFalse();
+ // We can convert it to a music file, but we won't have access to music file after
+ // renaming.
+ assertThat(imageFile.renameTo(musicFile)).isTrue();
+ assertThat(getFileRowIdFromDatabase(musicFile)).isEqualTo(-1);
} finally {
deleteFileAsNoThrow(TEST_APP_A, otherAppVideoFile.getAbsolutePath());
uninstallApp(TEST_APP_A);
@@ -1352,24 +1354,19 @@
assertFileContent(otherAppPdf, BYTES_DATA1);
// Assert we can rename the file and ensure the file has the same content
- assertThat(otherAppPdf.renameTo(pdf)).isTrue();
- assertThat(otherAppPdf.exists()).isFalse();
+ assertCanRenameFile(otherAppPdf, pdf);
assertFileContent(pdf, BYTES_DATA1);
// We can even move it to the top level directory
- assertThat(pdf.renameTo(topLevelPdf)).isTrue();
- assertThat(pdf.exists()).isFalse();
+ assertCanRenameFile(pdf, topLevelPdf);
assertFileContent(topLevelPdf, BYTES_DATA1);
// And even rename to a place where PDFs don't belong, because we're an omnipotent
// external storage manager
- assertThat(topLevelPdf.renameTo(pdfInObviouslyWrongPlace)).isTrue();
- assertThat(topLevelPdf.exists()).isFalse();
+ assertCanRenameFile(topLevelPdf, pdfInObviouslyWrongPlace);
assertFileContent(pdfInObviouslyWrongPlace, BYTES_DATA1);
// And we can even convert it into a music file, because why not?
- assertThat(pdfInObviouslyWrongPlace.renameTo(musicFile)).isTrue();
- assertThat(pdfInObviouslyWrongPlace.exists()).isFalse();
+ assertCanRenameFile(pdfInObviouslyWrongPlace, musicFile);
assertFileContent(musicFile, BYTES_DATA1);
-
} finally {
pdf.delete();
pdfInObviouslyWrongPlace.delete();
@@ -1542,46 +1539,6 @@
}
}
- private static void assertCanRenameFile(File oldFile, File newFile) {
- assertThat(oldFile.renameTo(newFile)).isTrue();
- assertThat(oldFile.exists()).isFalse();
- assertThat(newFile.exists()).isTrue();
- assertThat(getFileRowIdFromDatabase(oldFile)).isEqualTo(-1);
- assertThat(getFileRowIdFromDatabase(newFile)).isNotEqualTo(-1);
- }
-
- private static void assertCantRenameFile(File oldFile, File newFile) {
- final int rowId = getFileRowIdFromDatabase(oldFile);
- assertThat(oldFile.renameTo(newFile)).isFalse();
- assertThat(oldFile.exists()).isTrue();
- assertThat(getFileRowIdFromDatabase(oldFile)).isEqualTo(rowId);
- }
-
- private static void assertCanRenameDirectory(File oldDirectory, File newDirectory,
- @Nullable File[] oldFilesList, @Nullable File[] newFilesList) {
- assertThat(oldDirectory.renameTo(newDirectory)).isTrue();
- assertThat(oldDirectory.exists()).isFalse();
- assertThat(newDirectory.exists()).isTrue();
- for (File file : oldFilesList != null ? oldFilesList : new File[0]) {
- assertThat(file.exists()).isFalse();
- assertThat(getFileRowIdFromDatabase(file)).isEqualTo(-1);
- }
- for (File file : newFilesList != null ? newFilesList : new File[0]) {
- assertThat(file.exists()).isTrue();
- assertThat(getFileRowIdFromDatabase(file)).isNotEqualTo(-1);
- };
- }
-
- private static void assertCantRenameDirectory(File oldDirectory, File newDirectory,
- @Nullable File[] oldFilesList) {
- assertThat(oldDirectory.renameTo(newDirectory)).isFalse();
- assertThat(oldDirectory.exists()).isTrue();
- for (File file : oldFilesList != null ? oldFilesList : new File[0]) {
- assertThat(file.exists()).isTrue();
- assertThat(getFileRowIdFromDatabase(file)).isNotEqualTo(-1);
- }
- }
-
/**
* Asserts the entire content of the file equals exactly {@code expectedContent}.
*/