Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ravi-yk
Copy link

@ravi-yk ravi-yk commented Jul 12, 2024

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
Copy link
Contributor

breautek commented Jul 16, 2024

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
Copy link
Contributor

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

@ravi-yk
Copy link
Author

ravi-yk commented Jul 17, 2024

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

fragment

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
Copy link
Contributor

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:

The problematic returns are likely:

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
Copy link
Contributor

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
Copy link
Author

ravi-yk commented Jul 23, 2024

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

@breautek breautek added this to the 8.0.0 milestone Jul 26, 2024
@lorenzodallavecchia
Copy link

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
Copy link
Contributor

breautek commented Aug 1, 2024

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
Copy link

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.

Copy link
Contributor

@breautek breautek left a comment

Choose a reason for hiding this comment

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

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 an issue within the camera plugin and this issue is being addressed in the file plugin: apache/cordova-plugin-file#629

@seamlink-aalves
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow using Android photo picker instead of READ_MEDIA_IMAGES and READ_MEDIA_VIDEO
4 participants