From 8d7668ae0d7f382700a1090d4079576e3a782ba7 Mon Sep 17 00:00:00 2001 From: Sudheer Shanka Date: Wed, 8 May 2019 18:56:22 -0700 Subject: Update behavior of setDestinationInExternalPublicDir(). Allow apps to use DownloadManager for downloading files into known public dirs other than "Download" but don't include them as part of Downloads collection. Bug: 132136431 Test: manual Test: atest DownloadProviderTests Test: atest cts/tests/app/src/android/app/cts/DownloadManagerTest.java Test: atest cts/tests/app/DownloadManagerLegacyTest/src/android/app/cts/DownloadManagerLegacyTest.java Test: atest cts/tests/app/DownloadManagerApi28Test/src/android/app/cts/DownloadManagerApi28Test.java Change-Id: I62d4f810c71c9231e2b6d8e276a0a1e326382e14 --- .../providers/downloads/DownloadProvider.java | 123 +++++++-------------- .../providers/downloads/DownloadReceiver.java | 45 ++++---- src/com/android/providers/downloads/Helpers.java | 80 +++++++++++++- .../android/providers/downloads/HelpersTest.java | 15 +++ 4 files changed, 157 insertions(+), 106 deletions(-) diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java index f393333b..a2f8532e 100644 --- a/src/com/android/providers/downloads/DownloadProvider.java +++ b/src/com/android/providers/downloads/DownloadProvider.java @@ -75,6 +75,7 @@ import android.util.ArrayMap; import android.util.Log; import android.util.LongArray; import android.util.LongSparseArray; +import android.util.SparseArray; import com.android.internal.util.ArrayUtils; import com.android.internal.util.IndentingPrintWriter; @@ -409,7 +410,7 @@ public final class DownloadProvider extends ContentProvider { Log.e(Constants.TAG, "Error getting media content values from " + info, e); continue; } - final Uri mediaStoreUri = updateMediaProvider(client, null, mediaValues); + final Uri mediaStoreUri = updateMediaProvider(client, mediaValues); if (mediaStoreUri != null) { updateValues.clear(); updateValues.put(Downloads.Impl.COLUMN_MEDIASTORE_URI, @@ -564,38 +565,20 @@ public final class DownloadProvider extends ContentProvider { private void reconcileRemovedUidEntries() { // Grant access permissions for all known downloads to the owning apps - final SQLiteDatabase db = mOpenHelper.getReadableDatabase(); - final Cursor cursor = db.query(DB_TABLE, - new String[] { Downloads.Impl._ID, Constants.UID, COLUMN_DESTINATION, _DATA }, - null, null, null, null, null); final ArrayList idsToDelete = new ArrayList<>(); final ArrayList idsToOrphan = new ArrayList<>(); - final String downloadsDir = Environment.getExternalStoragePublicDirectory( - Environment.DIRECTORY_DOWNLOADS).getAbsolutePath(); - try { - while (cursor.moveToNext()) { - final long downloadId = cursor.getLong(0); - final int uid = cursor.getInt(1); - - final String ownerPackage = getPackageForUid(uid); - if (ownerPackage == null) { - final int destination = cursor.getInt(2); - final String filePath = cursor.getString(3); - - if ((destination == DESTINATION_EXTERNAL - || destination == DESTINATION_FILE_URI - || destination == DESTINATION_NON_DOWNLOADMANAGER_DOWNLOAD) - && FileUtils.contains(downloadsDir, filePath)) { - idsToOrphan.add(downloadId); - } else { - idsToDelete.add(downloadId); - } - } else { - grantAllDownloadsPermission(ownerPackage, downloadId); - } - } - } finally { - cursor.close(); + final LongSparseArray idsToGrantPermission = new LongSparseArray<>(); + final SQLiteDatabase db = mOpenHelper.getReadableDatabase(); + try (Cursor cursor = db.query(DB_TABLE, + new String[] { Downloads.Impl._ID, Constants.UID, COLUMN_DESTINATION, _DATA }, + Constants.UID + " IS NOT NULL", null, null, null, null)) { + Helpers.handleRemovedUidEntries(getContext(), cursor, + idsToDelete, idsToOrphan, idsToGrantPermission); + } + for (int i = 0; i < idsToGrantPermission.size(); ++i) { + final long downloadId = idsToGrantPermission.keyAt(i); + final String ownerPackageName = idsToGrantPermission.valueAt(i); + grantAllDownloadsPermission(ownerPackageName, downloadId); } if (idsToOrphan.size() > 0) { Log.i(Constants.TAG, "Orphaning downloads with ids " @@ -603,23 +586,14 @@ public final class DownloadProvider extends ContentProvider { final ContentValues values = new ContentValues(); values.putNull(Constants.UID); update(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI, values, - buildQueryWithIds(idsToOrphan), null); + Helpers.buildQueryWithIds(idsToOrphan), null); } if (idsToDelete.size() > 0) { Log.i(Constants.TAG, "Deleting downloads with ids " + Arrays.toString(idsToDelete.toArray()) + " as owner package is missing"); - delete(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI, buildQueryWithIds(idsToDelete), null); - } - } - - private String buildQueryWithIds(ArrayList downloadIds) { - final StringBuilder queryBuilder = new StringBuilder(Downloads.Impl._ID + " in ("); - final int size = downloadIds.size(); - for (int i = 0; i < size; i++) { - queryBuilder.append(downloadIds.get(i)); - queryBuilder.append((i == size - 1) ? ")" : ","); + delete(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI, + Helpers.buildQueryWithIds(idsToDelete), null); } - return queryBuilder.toString(); } /** @@ -862,7 +836,7 @@ public final class DownloadProvider extends ContentProvider { final CallingIdentity token = clearCallingIdentity(); try (ContentProviderClient client = getContext().getContentResolver() .acquireContentProviderClient(MediaStore.AUTHORITY)) { - final Uri mediaStoreUri = updateMediaProvider(client, null, + final Uri mediaStoreUri = updateMediaProvider(client, convertToMediaProviderValues(filteredValues)); if (mediaStoreUri != null) { filteredValues.put(Downloads.Impl.COLUMN_MEDIASTORE_URI, @@ -893,7 +867,8 @@ public final class DownloadProvider extends ContentProvider { insertRequestHeaders(db, rowID, values); - final String callingPackage = getPackageForUid(Binder.getCallingUid()); + final String callingPackage = Helpers.getPackageForUid(getContext(), + Binder.getCallingUid()); if (callingPackage == null) { Log.e(Constants.TAG, "Package does not exist for calling uid"); return null; @@ -916,27 +891,20 @@ public final class DownloadProvider extends ContentProvider { * add it, otherwise update that entry with the given values. */ private Uri updateMediaProvider(@NonNull ContentProviderClient mediaProvider, - @Nullable String currentMediaStoreUri, @NonNull ContentValues mediaValues) { - Uri mediaStoreUri; final String filePath = mediaValues.getAsString(MediaStore.DownloadColumns.DATA); - if (currentMediaStoreUri == null) { - mediaStoreUri = getMediaStoreUri(mediaProvider, filePath); - } else { - mediaStoreUri = Uri.parse(currentMediaStoreUri); - } + Uri mediaStoreUri = getMediaStoreUri(mediaProvider, filePath); try { if (mediaStoreUri == null) { mediaStoreUri = mediaProvider.insert( - MediaStore.Downloads.getContentUriForPath(filePath), + MediaStore.Files.getContentUriForPath(filePath), mediaValues); if (mediaStoreUri == null) { Log.e(Constants.TAG, "Error inserting into mediaProvider: " + mediaValues); } return mediaStoreUri; } else { - removeInvalidColumnsForUpdate(mediaValues); if (mediaProvider.update(mediaStoreUri, mediaValues, null, null) != 1) { Log.e(Constants.TAG, "Error updating MediaProvider, uri: " + mediaStoreUri + ", values: " + mediaValues); @@ -949,21 +917,15 @@ public final class DownloadProvider extends ContentProvider { return null; } - /** - * Remove column values which are not valid for updating downloads in MediaProvider. - */ - private void removeInvalidColumnsForUpdate(@NonNull ContentValues mediaValues) { - mediaValues.remove(MediaStore.Downloads.SIZE); - } - private Uri getMediaStoreUri(@NonNull ContentProviderClient mediaProvider, @NonNull String filePath) { - final Uri downloadsUri = MediaStore.Downloads.getContentUriForPath(filePath); - try (Cursor cursor = mediaProvider.query(downloadsUri, - new String[] { MediaStore.DownloadColumns._ID }, - MediaStore.DownloadColumns.DATA + "=?", new String[] { filePath }, null, null)) { + final Uri filesUri = MediaStore.setIncludePending( + MediaStore.Files.getContentUriForPath(filePath)); + try (Cursor cursor = mediaProvider.query(filesUri, + new String[] { MediaStore.Files.FileColumns._ID }, + MediaStore.Files.FileColumns.DATA + "=?", new String[] { filePath }, null, null)) { if (cursor.moveToNext()) { - return ContentUris.withAppendedId(downloadsUri, cursor.getLong(0)); + return ContentUris.withAppendedId(filesUri, cursor.getLong(0)); } } catch (RemoteException e) { // Should not happen @@ -983,11 +945,12 @@ public final class DownloadProvider extends ContentProvider { mediaValues.put(MediaStore.Downloads.SIZE, info.mTotalBytes); mediaValues.put(MediaStore.Downloads.DOWNLOAD_URI, info.mUri); mediaValues.put(MediaStore.Downloads.REFERER_URI, info.mReferer); - mediaValues.put(MediaStore.Downloads.DESCRIPTION, info.mDescription); mediaValues.put(MediaStore.Downloads.MIME_TYPE, info.mMimeType); mediaValues.put(MediaStore.Downloads.IS_PENDING, Downloads.Impl.isStatusSuccess(info.mStatus) ? 0 : 1); - mediaValues.put(MediaStore.Downloads.OWNER_PACKAGE_NAME, getPackageForUid(info.mUid)); + mediaValues.put(MediaStore.Downloads.OWNER_PACKAGE_NAME, + Helpers.getPackageForUid(getContext(), info.mUid)); + mediaValues.put(MediaStore.Files.FileColumns.IS_DOWNLOAD, info.mIsVisibleInDownloadsUi); return mediaValues; } @@ -1007,15 +970,15 @@ public final class DownloadProvider extends ContentProvider { downloadValues.getAsString(Downloads.Impl.COLUMN_URI)); mediaValues.put(MediaStore.Downloads.REFERER_URI, downloadValues.getAsString(Downloads.Impl.COLUMN_REFERER)); - mediaValues.put(MediaStore.Downloads.DESCRIPTION, - downloadValues.getAsString(Downloads.Impl.COLUMN_DESCRIPTION)); mediaValues.put(MediaStore.Downloads.MIME_TYPE, downloadValues.getAsString(Downloads.Impl.COLUMN_MIME_TYPE)); final boolean isPending = downloadValues.getAsInteger(Downloads.Impl.COLUMN_STATUS) != Downloads.Impl.STATUS_SUCCESS; mediaValues.put(MediaStore.Downloads.IS_PENDING, isPending ? 1 : 0); mediaValues.put(MediaStore.Downloads.OWNER_PACKAGE_NAME, - getPackageForUid(downloadValues.getAsInteger(Constants.UID))); + Helpers.getPackageForUid(getContext(), downloadValues.getAsInteger(Constants.UID))); + mediaValues.put(MediaStore.Files.FileColumns.IS_DOWNLOAD, + downloadValues.getAsBoolean(COLUMN_IS_VISIBLE_IN_DOWNLOADS_UI)); return mediaValues; } @@ -1024,15 +987,6 @@ public final class DownloadProvider extends ContentProvider { return TextUtils.equals(uri.getScheme(), ContentResolver.SCHEME_FILE) ? uri : null; } - private String getPackageForUid(int uid) { - String[] packages = getContext().getPackageManager().getPackagesForUid(uid); - if (packages == null || packages.length == 0) { - return null; - } - // For permission related purposes, any package belonging to the given uid should work. - return packages[0]; - } - private void ensureDefaultColumns(ContentValues values) { final Integer dest = values.getAsInteger(COLUMN_DESTINATION); if (dest != null) { @@ -1057,9 +1011,12 @@ public final class DownloadProvider extends ContentProvider { if (Helpers.isFileInExternalAndroidDirs(file.getAbsolutePath())) { mediaScannable = MEDIA_NOT_SCANNABLE; visibleInDownloadsUi = false; - } else { + } else if (Helpers.isFilenameValidInPublicDownloadsDir(file)) { mediaScannable = MEDIA_NOT_SCANNED; visibleInDownloadsUi = true; + } else { + mediaScannable = MEDIA_NOT_SCANNED; + visibleInDownloadsUi = false; } } values.put(COLUMN_MEDIA_SCANNED, mediaScannable); @@ -1100,7 +1057,7 @@ public final class DownloadProvider extends ContentProvider { final int targetSdkVersion = getCallingPackageTargetSdkVersion(); if (Helpers.isFilenameValidInExternalPackage(getContext(), file, getCallingPackage()) - || Helpers.isFilenameValidInPublicDownloadsDir(file)) { + || Helpers.isFilenameValidInKnownPublicDir(file.getAbsolutePath())) { // No permissions required for paths belonging to calling package or // public downloads dir. return; @@ -1677,7 +1634,7 @@ public final class DownloadProvider extends ContentProvider { .DESTINATION_NON_DOWNLOADMANAGER_DOWNLOAD) && visibleToUser) { final Uri mediaStoreUri = updateMediaProvider(client, - info.mMediaStoreUri, convertToMediaProviderValues(info)); + convertToMediaProviderValues(info)); if (!TextUtils.equals(info.mMediaStoreUri, mediaStoreUri == null ? null : mediaStoreUri.toString())) { updateValues.clear(); diff --git a/src/com/android/providers/downloads/DownloadReceiver.java b/src/com/android/providers/downloads/DownloadReceiver.java index 6a0707bb..f0b9de71 100644 --- a/src/com/android/providers/downloads/DownloadReceiver.java +++ b/src/com/android/providers/downloads/DownloadReceiver.java @@ -18,6 +18,8 @@ package com.android.providers.downloads; import static android.app.DownloadManager.Request.VISIBILITY_VISIBLE_NOTIFY_COMPLETED; import static android.app.DownloadManager.Request.VISIBILITY_VISIBLE_NOTIFY_ONLY_COMPLETION; +import static android.provider.Downloads.Impl.COLUMN_DESTINATION; +import static android.provider.Downloads.Impl._DATA; import static com.android.providers.downloads.Constants.TAG; import static com.android.providers.downloads.Helpers.getAsyncHandler; @@ -43,6 +45,8 @@ import android.util.Log; import android.util.Slog; import android.widget.Toast; +import java.util.ArrayList; +import java.util.Arrays; import java.util.regex.Pattern; /** @@ -144,28 +148,27 @@ public class DownloadReceiver extends BroadcastReceiver { final ContentResolver resolver = context.getContentResolver(); final int uid = intent.getIntExtra(Intent.EXTRA_UID, -1); - // First, disown any downloads that live in shared storage - final ContentValues values = new ContentValues(); - values.putNull(Constants.UID); - - final StringBuilder queryString = new StringBuilder(Constants.UID + "=" + uid); - queryString.append(" AND ").append(Downloads.Impl.COLUMN_DESTINATION + " IN (" - + Downloads.Impl.DESTINATION_EXTERNAL + "," - + Downloads.Impl.DESTINATION_FILE_URI + "," - + Downloads.Impl.DESTINATION_NON_DOWNLOADMANAGER_DOWNLOAD + ")"); - queryString.append(" AND ").append(Downloads.Impl._DATA - + " REGEXP '" + MediaStore.Downloads.PATTERN_DOWNLOADS_FILE.pattern() + "'"); - - final int disowned = resolver.update(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI, values, - queryString.toString(), null); - - // Finally, delete any remaining downloads owned by UID - final int deleted = resolver.delete(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI, - Constants.UID + "=" + uid, null); + final ArrayList idsToDelete = new ArrayList<>(); + final ArrayList idsToOrphan = new ArrayList<>(); + try (Cursor cursor = resolver.query(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI, + new String[] { Downloads.Impl._ID, Constants.UID, COLUMN_DESTINATION, _DATA }, + Constants.UID + "=" + uid, null, null)) { + Helpers.handleRemovedUidEntries(context, cursor, idsToDelete, idsToOrphan, null); + } - if ((disowned + deleted) > 0) { - Slog.d(TAG, "Disowned " + disowned + " and deleted " + deleted - + " downloads owned by UID " + uid); + if (idsToOrphan.size() > 0) { + Log.i(Constants.TAG, "Orphaning downloads with ids " + + Arrays.toString(idsToOrphan.toArray()) + " as owner package is removed"); + final ContentValues values = new ContentValues(); + values.putNull(Constants.UID); + resolver.update(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI, values, + Helpers.buildQueryWithIds(idsToOrphan), null); + } + if (idsToDelete.size() > 0) { + Log.i(Constants.TAG, "Deleting downloads with ids " + + Arrays.toString(idsToDelete.toArray()) + " as owner package is removed"); + resolver.delete(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI, + Helpers.buildQueryWithIds(idsToDelete), null); } } diff --git a/src/com/android/providers/downloads/Helpers.java b/src/com/android/providers/downloads/Helpers.java index a3832872..408754a0 100644 --- a/src/com/android/providers/downloads/Helpers.java +++ b/src/com/android/providers/downloads/Helpers.java @@ -16,17 +16,20 @@ package com.android.providers.downloads; -import static android.os.Environment.buildExternalStorageAndroidDataDirs; -import static android.os.Environment.buildExternalStorageAppCacheDirs; import static android.os.Environment.buildExternalStorageAppDataDirs; import static android.os.Environment.buildExternalStorageAppMediaDirs; import static android.os.Environment.buildExternalStorageAppObbDirs; import static android.os.Environment.buildExternalStoragePublicDirs; +import static android.provider.Downloads.Impl.DESTINATION_EXTERNAL; +import static android.provider.Downloads.Impl.DESTINATION_FILE_URI; +import static android.provider.Downloads.Impl.DESTINATION_NON_DOWNLOADMANAGER_DOWNLOAD; import static android.provider.Downloads.Impl.FLAG_REQUIRES_CHARGING; import static android.provider.Downloads.Impl.FLAG_REQUIRES_DEVICE_IDLE; import static com.android.providers.downloads.Constants.TAG; +import android.annotation.NonNull; +import android.annotation.Nullable; import android.app.job.JobInfo; import android.app.job.JobScheduler; import android.content.ComponentName; @@ -45,12 +48,18 @@ import android.os.storage.StorageVolume; import android.provider.Downloads; import android.text.TextUtils; import android.util.Log; +import android.util.LongSparseArray; +import android.util.SparseArray; +import android.util.SparseBooleanArray; import android.webkit.MimeTypeMap; +import com.android.internal.util.ArrayUtils; + import com.google.common.annotations.VisibleForTesting; import java.io.File; import java.io.IOException; +import java.util.ArrayList; import java.util.Random; import java.util.Set; import java.util.regex.Matcher; @@ -69,6 +78,9 @@ public class Helpers { private static final Pattern PATTERN_ANDROID_DIRS = Pattern.compile("(?i)^/storage/[^/]+(?:/[0-9]+)?/Android/(?:data|obb|media)/.+"); + private static final Pattern PATTERN_PUBLIC_DIRS = + Pattern.compile("(?i)^/storage/[^/]+(?:/[0-9]+)?/([^/]+)/.+"); + private static final Object sUniqueLock = new Object(); private static HandlerThread sAsyncHandlerThread; @@ -524,6 +536,16 @@ public class Helpers { return false; } + @com.android.internal.annotations.VisibleForTesting + public static boolean isFilenameValidInKnownPublicDir(String filePath) { + final Matcher matcher = PATTERN_PUBLIC_DIRS.matcher(filePath); + if (matcher.matches()) { + final String publicDir = matcher.group(1); + return ArrayUtils.contains(Environment.STANDARD_DIRECTORIES, publicDir); + } + return false; + } + /** * Checks whether the filename looks legitimate for security purposes. This * prevents us from opening files that aren't actually downloads. @@ -601,6 +623,60 @@ public class Helpers { } } + public static void handleRemovedUidEntries(@NonNull Context context, @NonNull Cursor cursor, + @NonNull ArrayList idsToDelete, @NonNull ArrayList idsToOrphan, + @Nullable LongSparseArray idsToGrantPermission) { + final SparseArray knownUids = new SparseArray<>(); + while (cursor.moveToNext()) { + final long downloadId = cursor.getLong(0); + final int uid = cursor.getInt(1); + + final String ownerPackageName; + final int index = knownUids.indexOfKey(uid); + if (index >= 0) { + ownerPackageName = knownUids.valueAt(index); + } else { + ownerPackageName = getPackageForUid(context, uid); + knownUids.put(uid, ownerPackageName); + } + + if (ownerPackageName == null) { + final int destination = cursor.getInt(2); + final String filePath = cursor.getString(3); + + if ((destination == DESTINATION_EXTERNAL + || destination == DESTINATION_FILE_URI + || destination == DESTINATION_NON_DOWNLOADMANAGER_DOWNLOAD) + && isFilenameValidInKnownPublicDir(filePath)) { + idsToOrphan.add(downloadId); + } else { + idsToDelete.add(downloadId); + } + } else if (idsToGrantPermission != null) { + idsToGrantPermission.put(downloadId, ownerPackageName); + } + } + } + + public static String buildQueryWithIds(ArrayList downloadIds) { + final StringBuilder queryBuilder = new StringBuilder(Downloads.Impl._ID + " in ("); + final int size = downloadIds.size(); + for (int i = 0; i < size; i++) { + queryBuilder.append(downloadIds.get(i)); + queryBuilder.append((i == size - 1) ? ")" : ","); + } + return queryBuilder.toString(); + } + + public static String getPackageForUid(Context context, int uid) { + String[] packages = context.getPackageManager().getPackagesForUid(uid); + if (packages == null || packages.length == 0) { + return null; + } + // For permission related purposes, any package belonging to the given uid should work. + return packages[0]; + } + /** * Checks whether this looks like a legitimate selection parameter */ diff --git a/tests/src/com/android/providers/downloads/HelpersTest.java b/tests/src/com/android/providers/downloads/HelpersTest.java index 2778663c..65c5d368 100644 --- a/tests/src/com/android/providers/downloads/HelpersTest.java +++ b/tests/src/com/android/providers/downloads/HelpersTest.java @@ -102,4 +102,19 @@ public class HelpersTest extends AndroidTestCase { assertFalse(Helpers.isFileInExternalAndroidDirs( "/storage/AAAA-FFFF/Download/dir/bar.html")); } + + public void testIsFilenameValidinKnownPublicDir() throws Exception { + assertTrue(Helpers.isFilenameValidInKnownPublicDir( + "/storage/emulated/0/Download/dir/file.txt")); + assertTrue(Helpers.isFilenameValidInKnownPublicDir( + "/storage/emulated/0/Music/foo.mp4")); + assertTrue(Helpers.isFilenameValidInKnownPublicDir( + "/storage/emulated/0/DCIM/vacation/bar.jpg")); + assertFalse(Helpers.isFilenameValidInKnownPublicDir( + "/storage/emulated/0/Testing/foo.mp4")); + assertFalse(Helpers.isFilenameValidInKnownPublicDir( + "/storage/emulated/0/Misc/Download/bar.jpg")); + assertFalse(Helpers.isFilenameValidInKnownPublicDir( + "/storage/emulated/0/Android/data/com.example/bar.jpg")); + } } -- cgit v1.2.3