Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[image_picker] Change storage location for camera captures to internal cache on Android, to comply with new Google Play storage requirements. #3956

Merged
merged 20 commits into from
Jun 1, 2021
Merged

[image_picker] Change storage location for camera captures to internal cache on Android, to comply with new Google Play storage requirements. #3956

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
7be1c7c
Switch to app-specific folder for writing camera captures.
BeMacized May 21, 2021
d79bac3
Merge branch 'master' into issue/80502
BeMacized May 21, 2021
df306e2
Update documentation
BeMacized May 21, 2021
467bb5c
Update targetSdkVersion for example. Update tests.
BeMacized May 21, 2021
b2e902e
Updated changelog and pubspec version
BeMacized May 21, 2021
84a5ad1
Update packages/image_picker/image_picker/android/src/main/AndroidMan…
BeMacized May 21, 2021
46df81f
Update packages/image_picker/image_picker/example/android/app/src/tes…
BeMacized May 21, 2021
15d841d
Update packages/image_picker/image_picker/android/src/main/res/xml/fl…
BeMacized May 21, 2021
e1cb24e
Implemented PR feedback
BeMacized May 21, 2021
eafa023
Add test to check if file is created in cache directory
BeMacized May 26, 2021
aa3c24d
Upgrade PR to breaking change
BeMacized May 26, 2021
a3c737c
Update packages/image_picker/image_picker/CHANGELOG.md
BeMacized May 26, 2021
bc3e4d5
Update CHANGELOG.md
BeMacized May 26, 2021
121a7ca
Merge branch 'master' into issue/80502
BeMacized May 26, 2021
1143108
Removed redundant robolectric.properties
BeMacized May 26, 2021
bfb2b6a
Fix merge issue
BeMacized May 26, 2021
e973353
Removed WRITE_EXTERNAL_STORAGE permission completely
BeMacized Jun 1, 2021
ca846ba
Remove READ_EXTERNAL_STORAGE permission checks
BeMacized Jun 1, 2021
fa3c577
Ran format and removed redundant unit tests
BeMacized Jun 1, 2021
80906ca
Merge branch 'master' into issue/80502
BeMacized Jun 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/image_picker/image_picker/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## 0.7.6

* Changed storage location for captured images and videos to internal cache on Android,
to comply with new Google Play storage requirements.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if this should be considered as a breaking change. As the API itself doesn't change, but the locations where images from the camera are stored is changed to scoped storage instead of the Environment.DIRECTORY_PICTURES external file location.
Meaning it will be more difficult for other Apps to reach the images. @stuartmorgan or @cyanglaz can you possibly advice on this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably better to err on the side of making it a breaking change since we don't know if people are relying on the location, even if it's not something that's guaranteed by the API.

@blasten any preference on this?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"If you require your picked image to be stored permanently, it is your responsibility to move it to a more permanent location."

I agree the plugin never said that this was the case in the first place, but I'm inclined to think that given the nature of the issue, making it a major release sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changelog and pubspec have been updated to show this as a breaking change.

* Updated Mockito to fix Android tests.

## 0.7.5+3
* Localize `UIAlertController` strings.

Expand Down
6 changes: 3 additions & 3 deletions packages/image_picker/image_picker/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ Add the following keys to your _Info.plist_ file, located in `<project root>/ios

### Android

#### API < 29
No configuration required - the plugin should work out of the box.

#### API 29+
It is no longer required to add `android:requestLegacyExternalStorage="true"` as an attribute to the `<application>` tag in AndroidManifest.xml, as `image_picker` has been updated to make use of scoped storage.

Add `android:requestLegacyExternalStorage="true"` as an attribute to the `<application>` tag in AndroidManifest.xml. The [attribute](https://developer.android.com/training/data-storage/compatibility) is `false` by default on apps targeting Android Q.
**Note:** Images and videos picked using the camera are saved to your application's local cache, and should therefore be expected to only be around temporarily.
If you require your picked image to be stored permanently, it is your responsibility to move it to a more permanent location.

### Example

Expand Down
13 changes: 8 additions & 5 deletions packages/image_picker/image_picker/android/src/main/AndroidManifest.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="io.flutter.plugins.imagepicker">
<uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE"/>
<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/>
package="io.flutter.plugins.imagepicker">

<uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" />
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can someone point me towards documentation that talks about why we have either of these permissions declared? As mentioned here (see the first row in the table), no permissions are needed for accessing the cache directory. I think these permissions are over-reaching and it'd be nice to drop them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the same goes for any API level of course, not just 29+. WRITE_EXTERNAL_STORAGE should not be needed at all anymore.

I cannot actually find any evidence that the READ_EXTERNAL_STORAGE permission is required for Intent.ACTION_GET_CONTENT, but it seems like a logical permission to request as we are reading images written by other apps, from external storage.
I have tried removing the permission, but it ends up throwing PlatformException(photo_access_denied, The user did not allow photo access., null, null), without prompting the user.

Therefore I would propose just removing the WRITE_EXTERNAL_STORAGE permission entirely and leaving READ_EXTERNAL_STORAGE as is.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tell me more about your issue with that platform exception? I put together this sample to prove you don't need that permission (in the sample I use the get content action successfully without it). If I can reproduce the issue it may help me understand why it's needed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @BeMacized I just had another look at the code. You're running into that Exception because you likely removed the permission declaration from the manifest but didn't remove the request for it from the code, which isn't allowed. If you remove it in both places you should be able to fetch the image without issue.

Copy link
Contributor Author

@BeMacized BeMacized Jun 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see! that makes things a lot more clear, thanks for the hint.
I've made the changes so that the read permission is no longer required either.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I'm probably being a pest @BeMacized but it's worth noting the plugin also doesn't need to request the camera permission because it's using the ACTION_VIDEO_CAPTURE and ACTION_IMAGE_CAPTURE intent actions. You can see that explained here. I figure since this is now a major release and the PR addresses permissions, it may be nice to do a sweep and remove them all?

FWIW, the reason I'm calling these all out is because it's nice for client apps to not request these permissions needlessly. It helps build trust with our users, so I'd love to see these go away if possible :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we could drop that one too that would indeed be excellent! I'll definitely take a look at it, although I can't guarantee it will also land together with this PR.

<uses-permission
android:name="android.permission.WRITE_EXTERNAL_STORAGE"
android:maxSdkVersion="28" />

<application>
<provider
Expand All @@ -11,7 +14,7 @@
android:grantUriPermissions="true">
<meta-data
android:name="android.support.FILE_PROVIDER_PATHS"
android:resource="@xml/flutter_image_picker_file_paths"/>
android:resource="@xml/flutter_image_picker_file_paths" />
</provider>
</application>
</manifest>
</manifest>
2 changes: 1 addition & 1 deletion ...mage_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public class ImagePickerDelegate
@VisibleForTesting final String fileProviderName;

private final Activity activity;
private final File externalFilesDirectory;
@VisibleForTesting final File externalFilesDirectory;
private final ImageResizer imageResizer;
private final ImagePickerCache cache;
private final PermissionManager permissionManager;
Expand Down
7 changes: 3 additions & 4 deletions .../image_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import android.app.Activity;
import android.app.Application;
import android.os.Bundle;
import android.os.Environment;
import android.os.Handler;
import android.os.Looper;
import androidx.annotation.NonNull;
Expand Down Expand Up @@ -216,11 +215,11 @@ private void tearDown() {
application = null;
}

private final ImagePickerDelegate constructDelegate(final Activity setupActivity) {
@VisibleForTesting
final ImagePickerDelegate constructDelegate(final Activity setupActivity) {
final ImagePickerCache cache = new ImagePickerCache(setupActivity);

final File externalFilesDirectory =
setupActivity.getExternalFilesDir(Environment.DIRECTORY_PICTURES);
final File externalFilesDirectory = setupActivity.getCacheDir();
final ExifDataCopier exifDataCopier = new ExifDataCopier();
final ImageResizer imageResizer = new ImageResizer(externalFilesDirectory, exifDataCopier);
return new ImagePickerDelegate(setupActivity, externalFilesDirectory, imageResizer, cache);
Expand Down
4 changes: 2 additions & 2 deletions ...es/image_picker/image_picker/android/src/main/res/xml/flutter_image_picker_file_paths.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<paths>
<external-path name="external_files" path="."/>
</paths>
<cache-path name="cached_files" path="."/>
</paths>
2 changes: 1 addition & 1 deletion packages/image_picker/image_picker/example/android/app/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ flutter {

dependencies {
testImplementation 'junit:junit:4.12'
testImplementation 'org.mockito:mockito-core:2.17.0'
testImplementation 'org.mockito:mockito-core:2.28.2'
androidTestImplementation 'androidx.test:runner:1.1.1'
androidTestImplementation 'androidx.test.espresso:espresso-core:3.1.1'
testImplementation 'androidx.test:core:1.2.0'
Expand Down
18 changes: 18 additions & 0 deletions ...ample/android/app/src/test/java/io/flutter/plugins/imagepicker/ImagePickerPluginTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@

package io.flutter.plugins.imagepicker;

import static org.hamcrest.core.IsEqual.equalTo;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyZeroInteractions;
import static org.mockito.Mockito.when;
Expand All @@ -15,6 +18,7 @@
import android.app.Application;
import io.flutter.plugin.common.MethodCall;
import io.flutter.plugin.common.MethodChannel;
import java.io.File;
import java.util.HashMap;
import java.util.Map;
import org.junit.Before;
Expand Down Expand Up @@ -149,6 +153,20 @@ public void onConstructor_WhenContextTypeIsActivity_ShouldNotCrash() {
"No exception thrown when ImagePickerPlugin() ran with context instanceof Activity", true);
}

@Test
public void constructDelegate_ShouldUseInternalCacheDirectory() {
File mockDirectory = new File("/mockpath");
when(mockActivity.getCacheDir()).thenReturn(mockDirectory);

ImagePickerDelegate delegate = plugin.constructDelegate(mockActivity);

verify(mockActivity, times(1)).getCacheDir();
assertThat(
"Delegate uses cache directory for storing camera captures",
delegate.externalFilesDirectory,
equalTo(mockDirectory));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if we also check (in another unit test) that calling delegate.takeImageWithCamera writes a file to the mock directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point! A test for this has been added.

}

private MethodCall buildMethodCall(String method, final int source) {
final Map<String, Object> arguments = new HashMap<>();
arguments.put("source", source);
Expand Down
1 change: 1 addition & 0 deletions ...s/image_picker/image_picker/example/android/app/src/test/resources/robolectric.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
sdk=29
2 changes: 1 addition & 1 deletion packages/image_picker/image_picker/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: image_picker
description: Flutter plugin for selecting images from the Android and iOS image
library, and taking new pictures with the camera.
homepage: https://github.com/flutter/plugins/tree/master/packages/image_picker/image_picker
version: 0.7.5+3
version: 0.7.6

flutter:
plugin:
Expand Down