fix(android): Removed read and write media permissions to allow to use Android photo picker #866

[ravi-yk]

Platforms affected

• Android

Motivation and Context

Google recommends using the Android photo picker in apps where it's not frequent to access these files instead of the READ_MEDIA_IMAGES and READ_MEDIA_VIDEO permissions.

This should fix #866

Description

Removed the READ_MEDIA_IMAGES and READ_MEDIA_VIDEO permissions and WRITE_EXTERNAL_STORAGE storage permission maxSdkVersion is set to 30.

Testing

Tested these changes on Android 10,11,12,13 and 14 devices.

Checklist

• I've run the tests to see all new and existing tests pass
• I added automated test coverage as appropriate for this change
• Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
• If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
• I've updated the documentation if necessary

[breautek]

As it stands, this PR will break picking content from the gallery.

The PR simply removes the READ_MEDIA_* permissions and reduces the WRITE_EXTERNAL_STORAGE permission to max 30.

I understand that the photo picker requires API 30, so starting with API 30 the photo picker could be implemented and API 29 and lower can do what the plugin currently does. Is the intent to have another PR implement the photo picker to cover the gallery use case?

[breautek]

Also #890 is merged so if you rebase, it will get the new CI config for the android tests.

[ravi-yk]

As per information present in the below page (shared in #866 by Fantikor),

https://medium.com/androiddevelopers/permissionless-is-the-future-of-storage-on-android-3fbceeb3d70a

This plugin is using ACTION_GET_CONTENT intent, so this new photo picker is actually used under the hood. So I modified this plugin and removed the code that requests permissions (for READ_MEDIA_IMAGES and READ_MEDIA_VIDEO) as they were previously only required for Android API levels 33 and above and for the versions 29 and below the plugin would retain the same behavior as before. If I am not wrong the gallery use case is also working as expected and no other changes are required.

Let me know if you see something is missing or If I misunderstood it.

[breautek]

This plugin is using ACTION_GET_CONTENT intent, so this new photo picker is actually used under the hood. So I modified this plugin

Ok this is good... I thought we were using some other intent and didn't actually realise ACTION_GET_CONTENT will actually use the photo picker. Android docs doesn't explicitly state this but if we restrict the media type (which in most cases we do) then android will choose the best picker/application for the requested type of content.

The bad news is, simply using the picker and removing the permissions isn't the complete solution. Bear with me this will be quite a lengthy post... but this will explain the situation.

To be clear, this is not the fault of this PR or anything, just things that Cordova has done in the past because it seemed like a good idea at the time and never has updated or introduced the breaking changes required to move to newer concepts and now Cordova is getting a bite of those consequences.

First, I've done all my testing using the FILE_URI mode, and only using images. I'm assuming videos has identical behaviour.

Cordova takes primarily two different paths when using the camera API:

1. Sourcing new content from the camera hardware
2. Sourcing existing content from the gallery

Sourcing new content from the camera does work, regardless of permissions or API level. This works because the native receives a content:// uses the content resolver and reads the content and writes it to an app-specific cache directory and then returns the app-specific internal file:// path, which is usable by the filesystem APIs. While this works... it is kinda problematic for a reason I'll get into later.

Sourcing existing content from the gallery however doesn't do file copy. It instead simply resolves the content:// to the external file system path and returns that. Both READ_EXTERNAL_STORAGE and READ_MEDIA_* permission requirements comes from the fact that the API shares these external URIs. They are required if you use the Filesystem APIs instead of the MediaStore APIs. In API 30, Android has a concept of FUSE filesystem that maps external FS kernel calls to the media store to enforce access but this does not grant the application temporary access that the content:// urls does when using the media store. Scope storage exists in API 29 which also lacks this FUSE filesystem so I believe this is currently completely broken on API 29 devices.

Solution 1

While this works... it is kinda problematic for a reason I'll get into later.

So the quick solution which will fix API 29, and should also allow us to eliminate the storage permissions is to do what we do when sourcing content from the camera. Use the content resolver to obtain the content and write it to app-specific cache and return those uris instead of external paths. This isn't a breaking change, we aren't changing the public API. But the reason why this approach is problematic is, copying large files obviously will take time. So picking videos will likely not give a good user experience, depending on the size of the video. Image content is usually manageable in terms of file size.

Here are some code references:

• https://github.com/apache/cordova-plugin-camera/blob/master/src/android/CameraLauncher.java#L576 (if you follow the method calls, it will eventually return a temporary file location to write to)
• https://github.com/apache/cordova-plugin-camera/blob/master/src/android/CameraLauncher.java#L583 (here, if you follow into the method, we use the content resolver to get the content output stream) and copy via a 4096 byte buffer to the temporary cache location
• https://github.com/apache/cordova-plugin-camera/blob/master/src/android/CameraLauncher.java#L586 (here, we return the tempoary file location uri back to the webview)

The problematic returns are likely:

• https://github.com/apache/cordova-plugin-camera/blob/master/src/android/CameraLauncher.java#L574 (which I think returns the uri from gallery if it already exists in the gallery, probably yielding an external or content URI)
• https://github.com/apache/cordova-plugin-camera/blob/master/src/android/CameraLauncher.java#L733 (which is the main handler method for processing the result from gallery)

Other Thoughts

I've also explored passing content:// uris directly to the webview, which poses several issues:

1. The webview will not accept content:// uris (despite even using setAllowContent(true) on the webview settings). Android docs doesn't state anything on why this might be the case but blocking content uris might be intentional based since Android KitKat on https://issuetracker.google.com/issues/36985138 (aka when Android moves to a chromium based webview).
2. Apache Cordova doesn't have a media store interface, so the user can't programmatically interact with the content
3. content:// uris give temporary access to the app, which makes the uris not persistent. Once the app activity closes, access to the content:// content is expired. So if the user wants to persistently access that resource, they must be able to programmatically interact/copy the data making issue 2 a big deal.

cordova-android platform could solve the first issue by mapping a path inside the WebViewAssetLoader that can resolve to the content:// uri and make use of the content resolver and return that content. But the other 2 issues will be prevalent without a media store API. There are third-party plugins available for a media store API but I don't think Apache Cordova should build the core plugins to rely on third-party plugins.

Being able to use content:// uris (even indirectly) is probably much better, but will require a lot of other moving parts to occur first. Additionally, requiring the use of a media store plugin for android also isn't ideal when iOS will need to continue to use the file plugin APIs. Cordova code is meant to be shared across platform, so we can't have a plugin API that requires using the result in two different ways depending on the platform. Therefore Cordova ideally needs to figure out how it can incorporate content:// uris in the file plugin APIs so that usage can be consistent cross platform.

Final Thoughts

Due to time constraints, I don't think the content:// solution is viable, so I think we are stuck with the first Solution. I know this is a lot, I'm not sure if this is still something you are willing to work on. If not, then that's okay too, we can probably still make use of what you've done in this PR and implement the rest in another PR.

[breautek]

I've done a bit more testing using content urls and wanted to follow up with some more details.

Using content urls directly simply doesn't work and I still don't know why.

Using resolveLocalFileSystemURL on a content url does work and you can then use toURL() on the entry object to obtain an asset loader will yield a "valid" https url. This however still doesn't work and is likely a bug inside cordova-android core platform.

Filesystem does have some support for using file APIs against content urls, though permissions may limit the actions you can take on content uri sources, but I've tested the following:

• Using the FileReader to obtain a data uri and/or blob will allow you to use the asset in the DOM.
• You can also use copyTo (with a caveat) to copy the content from the content filesystem into the local app-data for more persistent access.

First caveat is content urls have a filename that contains a : character which apache file apis deems illegal, so a new file name must explicitly be supplied when using copyTo.
The second caveat is content urls give temporary access. The url cannot be saved to be used later. Once the app exits, the read grants provided to the app is expired, and attempting to read it again will result in a security exception.

But this might actually make the content path usage viable, especially if we can fix the asset loader in cordova-android.

[ravi-yk]

Thank you for the clear explanation, didn't reliaze that the selecting photos from Gallery (external file system) is broken for API levels 29 and below. I was under the assumption that functionality should still work on devices with 29 and below as READ_MEDIA_IMAGES and READ_MEDIA_VIDEO permissions were previously only added from 33 and above in the plugin and for 29 and below it didn't require these permissions. Probably then this is an existing issue in the plugin that selecting the files from the external system didn't work. I, myself didn't check this behavior, will check once.
Will wait on #629 to be merged then

[lorenzodallavecchia]

Will wait on #629 to be merged then

I don't get the reference to that PR. How is #629 related to fixing the permissions problem?

[breautek]

Will wait on #629 to be merged then

I don't get the reference to that PR. How is #629 related to fixing the permissions problem?

The actual link is apache/cordova-plugin-file#629 (it's a different repo)

And the reason that relates is because this PR will cause content:// urls to be returned instead of file:// urls which Cordova had support for, but it was something infrequently used and it hasn't actually worked properly in quite some time.

apache/cordova-plugin-file#629 makes the content uris consumable by the file plugin allowing you to use toURL() to get a DOM-usable url or any other file APIs to read and/or copy the file content.

We are going to have to add some documentation I think, there are significant differences and applications will have to adapt their usage. For example access to the content:// is temporary and will expire once the app's activity has been closed. Therefore those urls cannot be saved. If you need permanent access then a copy will need to be made. A hypothetical scenario would be letting the user choose a profile pic, you'll want to be able to access that content again in the future, so the image should be copied to the app's internal data storage.

[lorenzodallavecchia]

Thanks for the info @breautek. Yeah, I was tricked by the wrong auto-link.

As long as the final URIs work from the WebView (like cdvfile) it totally makes sense to me. I eas already treating the URIs to picked images as "ephemeral" and copying the bytes somewhere else, usually sending to a REST API or storing in a local database.

[breautek]

I believe the changes are good and helps us get more aligned to what Google wants app developers to do.

Tests passes (except for browser/chrome tests, which is unrelated to this PR).

Manual testing shows that no behaviour changes occur when sourcing an image from the camera hardware (e.g. if the user snaps a new picture).

There is 2 breaking changes when sourcing an image/video resource from the gallery.

1. Old behaviour returned a file path (not even a URI, just simply a /path/... without the file:// protocol). New behaviour returns a content:// path.

2. The content:// paths gives temporary read access to the file. Read access expires if the app activity is closes. If users previously stored the return file path, the file path will not work and will return a security exception if attempted to be read unless if the user goes through the file picker UI and selects that file again (regranting the temporary read access).

App developers who sources images or videos via the gallery may have to adapt their code. For example, if an app is using the gallery to source an image for a profile picture, the app will want to copy the file while it has the read access to an app data directory and store the app data file path to the resource instead where it will have long-term permanent access to that file.

Other Notes:

This PR does break using the picked resources in the DOM. This is because a FileEntry using a file:// path can be resolved via .toURL() method to a local web asset loader url and the asset can used in the DOM, using <img src="..." /> for example.

content:// paths are currently not supported. This is however isn't an issue within the camera plugin and this issue is being addressed in the file plugin: apache/cordova-plugin-file#629

[seamlink-aalves]

The deadline set by Google for this change is Oct 31. Is there anything missing for this PR to be merged?

[dpogue]

I don't have any objections if this has been tested and confirmed working, but I've never used the camera plugin in an app so my vote shouldn't count for much here 😛

Does this need to be a major version bump for the plugin, since it changes the returned file paths?
Does that change affect older versions of Android as well? (I'm a bit unclear since we're actually lowering the version checks in the code rather than raising them)

[breautek]

Does this need to be a major version bump for the plugin, since it changes the returned file paths?

It will be included in a 8.0 major release.

Does that change affect older versions of Android as well? (I'm a bit unclear since we're actually lowering the version checks in the code rather than raising them)

The READ_MEDIA_* permissions were API 33+ permissions which I think is the key point.

But tbh I think this is something I want to re-test. Causing looking back now it does seem weird.

[t-var-s]

Just to get a cordova-wide view of this issue, plugin-file has already merged Content FS support, plugin-media-capture has an open issue with a PR removing the permissions and plugin-camera is here doing the same, right?

[breautek]

plugin-media-capture has an open issue with a PR removing the permissions and plugin-camera is here doing the same, right?

I haven't really kept a close eye on media-capture plugin so I don't know if it's "in-sync" with what the camera plugin is doing. I know there was previous proposals to make the media-capture plugin copy the assets from the content:// store to the app's internal temp directory to avoid permission problems or requiring the READ_MEDIA_* permissions but I think that fell through. This isn't so much a problem for images, but for videos copying the files can be an expensive and lengthy process too.

With the Content FS changes in file plugin (which is currently in the release process and I'm hoping to make a release this week if I can get enough binding votes) it will give the app developers a choice of immediate playback directly from content:// or if they need more persistent access they can choose to copy themselves, depending on the application use cases & needs. I'm not sure if the media-capture plugin has any PR that just simply exposes the content:// path, but that is likely where we want to go for it.

[breautek]

@ravi-yk I think I'm going to fork your PR to extend what you've done even further. This way you're contributions are still credited in the form of a commit.

The reason is I think we can go much further, I've been re-testing things and I think with some more changes we can completely remove all permissions, including the READ/WRITE external storage permission. It would simplify and cleanup a lot of the codebase. I have a working PoC testing API 24 atm.

Is this OK?

EDIT:

Actually scratch that we have the votes already to merge, so I'll merge and do an addon PR instead of replacing your PR. Thank you for your work here!