fix(android): remove broad media permissions

[bmarsaud]

Platforms affected

Android

Motivation and Context

As discussed on #288, broad media permissions (READ_MEDIA_IMAGES, READ_MEDIA_VIDEO, READ_MEDIA_AUDIO) policy will be enforced on August 31, 2024, threatening the Google Play approval of apps using this plugin.

For a more privacy preserving experience for users, we’re introducing the Photo and Video Permissions policy to reduce the number of apps permitted to request broad photo/video permissions (READ_MEDIA_IMAGES and READ_MEDIA_VIDEO). Apps may only access photos and videos for purposes directly related to app functionality. Apps that have a one-time or infrequent need to access these files are requested to use a system picker, such as the Android photo picker.

Apps that request access to the READ_MEDIA_VIDEO or READ_MEDIA_IMAGES permission must successfully demonstrate a core use case that requires persistent or frequent need of photo/video access located in shared storage.

The purpose of this PR is to not use broad media permissions.

Description

My understanding is that, we don't need these permissions in the first place.
I would be interested to know why the changes of #262 were considered necessary at the time, am I missing something?

Here are the requirements that I've gathered:

Android Version	Requirement	Source
<= 9	READ_EXTERNAL_STORAGE and WRITE_EXTERNAL_STORAGE are needed to "access any media file"	https://developer.android.com/training/data-storage/shared/media?hl=en#extra-permissions
10	Same as Android 11+ (see below) BUT if the app has opted-out of scoped storage we still need the EXTERNAL_STORAGE permissions	https://developer.android.com/training/data-storage/use-cases?hl=en#running_on_android_10 https://developer.android.com/training/data-storage/use-cases?hl=en#opt-out-in-production-app
11+	There is no permission needed to perform a MediaCapture intent, save the media file to the MediaStore.Images collection and read it later because you "own" the file	https://developer.android.com/training/data-storage/shared/media?hl=en#storage-permission

For Android 10, I would suggest this plugin be resilient to handle apps that have opted-out of scoped storage by using the android:requestLegacyExternalStorage="true" configuration.

With these information in mind, here is what I've changed:

• READ_EXTERNAL_STORAGE and WRITE_EXTERNAL_STORAGE declared in the manifest and asked at runtime only for Android up to 10
• Removal of READ_MEDIA_IMAGES, READ_MEDIA_VIDEO, READ_MEDIA_AUDIO from the manifest and runtime request.

Testing

I have tested an image capture on Android 14, 13, 12L, 11, 10 and 9: the photo app opens, the file is well saved and can be read.

I there a documentation on how to run the unit-tests ?

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]

For Android 10, I would suggest this plugin be resilient to handle apps that have opted-out of scoped storage by using the android:requestLegacyExternalStorage="true" configuration.

The requestLegacyExternalStorage is just a request. It is always rejected if the target SDK is 30 or later (current google play requires API 33 as a target). If you're targeting API 29, the request is only honoured if your app exists in google play before hand. Newer apps created after API 29 can be rejected (forcing developers to use scoped storage).

In otherwords, scoped storage is always enforced today on API 29+ devices.

[breautek]

btw the android test are failing cause it's attempting to test against cordova-android@13 which requires JDK 17, but this repo still is configured to use JDK 11. So don't worry about that. But another PR will need to be created to update the CI, and then this PR will need to be rebased after the CI PR is merged.

[ath0mas]

I think the ArrayList is "expected" here, as an ordered collection, and so should not be replaced by List.

And if I am correct Android expects the permissions to be requested in order, like let's say READ before WRITE, or location before background/always location.

[bmarsaud]

Aren't every list in JAVA ordered? In the sense that the iteration is always deterministic.
Here, you give a list that is ordered as you wish it to be, we don't need to constrain to use a particular implementation of List : the reponsability of the order is for the caller.

In our very case, we use Arrays.asList that keeps the same order as the initial array.

[ath0mas]

You're right!
To update and clean this up a little more, maybe also do?

• ArrayList<String> missingPermissions = … ⇒ List<String> missingPermissions = …
  (+ a bonus space before : in String permission: permissions 😇)
• ArrayList<String> cameraPermissions = … ⇒ List<String> cameraPermissions = …

[bmarsaud]

Good catch, indeed! Just commited that.

[ath0mas]

No need to change the android:maxSdkVersion="32" nor the check on TIRAMISU for storagePermissions.

As said and set in PR #262, it's In API 33 these two permissions are ineffective that explains the 32, required to prevent breaking the build, and in between 29 and 32 yes it may depend or be useless but even if declared here or requested by the app, Android deals with it seamlessly for us.
And "all" other plugins with such <uses-permission on EXTERNAL_STORAGE and check on TIRAMISU did the same, not less: cordova-plugin-camera, cordova-diagnostic-plugin, cordova-plugin-file

[breautek]

For some context, WRITE_EXTERNAL_STORAGE permission didn't grant any more privileges over READ_EXTERNAL_STORAGE since API 29/scoped storage. But requesting the write permission still implicitly granted the READ_EXTERNAL_STORAGE permission which had meaning until API 33.

There might still be Cordova code relying on that behavour and that's why API 32 was chosen for the max SDK version. It probably shouldn't be changed, especially READ_EXTERNAL_STORAGE

I think in order to appease Google, we will have to do what we did for the file plugin, which is to remove all permissions and have them opt in if the app requires them. But that isn't a complete solution alone...

I think the main problem is this plugin relies on using the File APIs to read the content, which isn't part of the MediaStore scoped storage. That's why Cordova depends on the READ_MEDIA_* permissions right now. I'm pretty sure a proper native solution would use the content:// urls provided by the MediaStore APIs which is a special link to the file within the media containers with temporary read (and potentially write) grants to that specific media resource. So I'm pretty sure Cordova has to start sharing the content:// urls but I'm I think content:// urls have limited support for reading (or writing).

We have experimented doing a file copy to the temp/cache folder in the past which works well for images and audio since they are going to be rather small files, so the copy operation will be pretty quick, and then you can access the file from the cache copy with standard file APIs without additional permissions. This strategy doesn't work very well for videos since they tend to be much larger.

[bmarsaud]

Maybe I don't have the full picture here, but I'm pretty sure we don't need this permission for Android 29+ for medias as stated in the following documentation : https://developer.android.com/training/data-storage/shared/media?hl=en#storage-permission and regarding the tests that I have performed.
Whats bugs me is that it add a permission popup on the first media capture intent.

Anyway, do you want me to rollback to android:maxSdkVersion="32" ?

[ath0mas]

rollback to android:maxSdkVersion="32"? to me yes, and the check on TIRAMISU for storagePermissions too

About the "broad" permissions, I would warn about the default Android/Google apps behavior that mostly writes and return the captured files following like the best practices, and so with the official emulators and any Pixel device we often have no issue. While apps from Google Play and other manufacturers make things more difficult, writing in there own directories, not shared or not as "default" expected.
The point on "files that your app owns" is not always easy in this plugin scenario, delegating capture through an intent, and so which app really owns the file? I always considered we need these permissions for this case and so that your app could access the file written by the capture app on intent return (because owned by the capture app and not yours?).

[bmarsaud]

Understood, I force-pushed to remove the last commit that changed the maxSdkVersion for storage permissions instead of doing an unescessary revert commit.

I re-tested on Android 9, 10, 11, 12L, 13 and 14, everything is still fine.

[mirko77]

@bmarsaud did you test both audio and video recording? I will do it myself soon, however, I do not have an Android 12 device anymore.