feat(android)!: support API 33+ permissions #262

ochakov · 2022-10-13T19:14:26Z

Platforms affected

Android

Motivation and Context

Allow building applications using Android SDK 33, support for Android 13

Description

Remove deprecated READ_EXTERNAL_STORAGE and WRITE_EXTERNAL_STORAGE permissions and replace with new READ_MEDIA_IMAGES, READ_MEDIA_VIDEO and READ_MEDIA_AUDIO

Testing

Capture video and audio

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

Replace READ_EXTERNAL_STORAGE permission with READ_MEDIA_AUDIO, READ_MEDIA_VIDEO and READ_MEDIA_IMAGES

Do not request new permissions on older SDKs as they do not support

breautek

Codewise I think is okay. I haven't ran these changes.

I did find the changes hard to follow, we may want to refactor this at some point in the future for readability, but I don't consider this a blocker for a merge.

breautek · 2023-06-15T13:36:25Z

src/android/Capture.java

+    private static String[] storagePermissions;
+    static {
+        if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
+            storagePermissions = new String[]{


Originally thought this was a mistake, however reading through the PR I see the list is added onto on demand.

Not a merge blocker but perhaps add a comment to make this clear.

it dont seem to work i have added all changes but on android 13 it gives permission denied error

FrancescoPaiola · 2023-06-26T12:53:05Z

any update on this?

erisu

LGTM

PabbleDabble · 2023-07-25T21:22:21Z

Anyone else getting Android manifest merger errors with this? Our app uses a bunch of different plugins and it looks like these two items are being placed in the manifest from other plugins...

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

...and these these items appear to be coming from this (cordova-plugin-media-capture)

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

It looked like I might be able to do something in my app's config.xml in order to replace them ( https://stackoverflow.com/questions/25981156/tools-replace-not-replacing-in-android-manifest ) but I wasn't fully understanding it / created more errors in Android studio.

Apologies if I'm off base, I'm not blaming this change, this looks like the better way to specify it, just not sure if there's anything else that needs done? Or maybe it's something on my end and need to find out what other plugins might be adding this and get them upgraded as well?

Sorry if this is off topic.

EDIT/UPDATE: I had to fork this plugin locally and remove the android:maxSdkVersion="32" portion of each of those permission requests because we use a VM to build and editing the AndroidManifest manually before packaging was not an option. If I get a better solution to this I may update.

ath0mas · 2023-07-30T20:48:26Z

About @PabbleDabble feedback, please review PR #211 again and merge it? to fix Issue #209.

apache#262 (comment)

jmelvin · 2023-09-10T15:04:35Z

@PabbleDabble I also ran into this exact scenario. We include cordova-plugin-camera which requests WRITE_EXTERNAL_STORAGE permissions... without the maxSdkVersion qualifier. AndroidManifest.xml includes the following entries after plugin installs...

cordova-plugin-camera

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

cordova-plugin-media-capture

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

The maxSdkVersion qualifier foils the manifest merge process and the build throws up an error...

Task :app:processReleaseMainManifest FAILED
/Users/jmelvin/dev/sizzlescene/repos/mobile/platforms/android/app/src/main/AndroidManifest.xml:47:5-108 Error:
Element uses-permission#android.permission.WRITE_EXTERNAL_STORAGE at AndroidManifest.xml:47:5-108 duplicated with element declared at AndroidManifest.xml:26:5-81

From a pending PR, the temporary workaround is to manually remove the maxSdkVersion qualifier before the cordova build. See SO post I made before seeing this comment...

https://stackoverflow.com/questions/77074389/duplicate-write-external-storage-permissions-in-androidmanifest-using-cordova-12

ath0mas · 2023-09-10T15:19:54Z

Except from fixing the merge process, a proper solution may be to have this maxSdk attribute identically set on both sides? And so to have a PR for the camera plugin and to make it be released soon.

jmelvin · 2023-09-10T16:14:55Z

@ath0mas Indeed, adding the maxSdk qualifier to the camera plugin entry would resolve the merge problem. However, is that the correct end result for READ/WRITE_EXTERNAL_STORAGE permission? I'm not an expert in the permissions minefield.

Or, does API 33+ just safely ignore the permissions and the maxSdk qualifier is not required?

I can temporarily script either solution before the build.

ath0mas · 2023-09-10T17:25:59Z

In API 33 these two permissions are ineffective when required from the code, so plugins are moving to conditionally calling these and thus the use of maxSdkVersion is a follow-up for the Manifest part.

Better to add it into all applicable plugins, and make the corresponding code changes, even if not having it at all would still work but be possibly reported as incorrect.

breautek · 2023-09-11T00:03:46Z

@PabbleDabble I also ran into this exact scenario. We include cordova-plugin-camera which requests WRITE_EXTERNAL_STORAGE permissions... without the maxSdkVersion qualifier. AndroidManifest.xml includes the following entries after plugin installs...

cordova-plugin-camera
<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
cordova-plugin-media-capture
<uses-permission android:maxSdkVersion="32" android:name="android.permission.READ_EXTERNAL_STORAGE" />
<uses-permission android:maxSdkVersion="32" android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
The maxSdkVersion qualifier foils the manifest merge process and the build throws up an error...

Task :app:processReleaseMainManifest FAILED
/Users/jmelvin/dev/sizzlescene/repos/mobile/platforms/android/app/src/main/AndroidManifest.xml:47:5-108 Error:
Element uses-permission#android.permission.WRITE_EXTERNAL_STORAGE at AndroidManifest.xml:47:5-108 duplicated with element declared at AndroidManifest.xml:26:5-81

From a pending PR, the temporary workaround is to manually remove the maxSdkVersion qualifier before the cordova build. See SO post I made before seeing this comment...

https://stackoverflow.com/questions/77074389/duplicate-write-external-storage-permissions-in-androidmanifest-using-cordova-12

I posted an answer on SO, but cross-posting here, I have a gist that will clean extra WRITE_EXTERNAL_STORAGE permissions. It can be used as a after_prepare hook:

https://gist.github.com/breautek/bd157b8598f9a816f2ec0d45e3d932c8

A more detailed answer on that topic can be found at https://stackoverflow.com/a/77078295/4685664

* Replace READ_EXTERNAL_STORAGE permission with READ_MEDIA_AUDIO, READ_MEDIA_VIDEO and READ_MEDIA_IMAGES * Fix READ_MEDIA_* permission on older SDKs * Do not request new permissions on older SDKs as they do not support

* feat(android)!: support API 33+ permissions (apache#262) * Replace READ_EXTERNAL_STORAGE permission with READ_MEDIA_AUDIO, READ_MEDIA_VIDEO and READ_MEDIA_IMAGES * Fix READ_MEDIA_* permission on older SDKs * Do not request new permissions on older SDKs as they do not support * Bump version --------- Co-authored-by: ochakov <[email protected]>

ochakov added 2 commits October 13, 2022 12:37

Android 13 support

83e5791

Replace READ_EXTERNAL_STORAGE permission with READ_MEDIA_AUDIO, READ_MEDIA_VIDEO and READ_MEDIA_IMAGES

Fix READ_MEDIA_* permission on older SDKs

d77d171

Do not request new permissions on older SDKs as they do not support

breautek approved these changes Jun 15, 2023

View reviewed changes

erisu changed the title ~~Android 13 support~~ feat(android)!: support Android 13 permissions Jul 12, 2023

erisu changed the title ~~feat(android)!: support Android 13 permissions~~ feat(android)!: support API 33+ permissions Jul 12, 2023

erisu approved these changes Jul 20, 2023

View reviewed changes

erisu merged commit f21e0a0 into apache:master Jul 20, 2023

kemp pushed a commit to kemp/cordova-plugin-media-capture that referenced this pull request Aug 22, 2023

Remove conflicting permissions

649a5a2

apache#262 (comment)

joaolourencoge mentioned this pull request Sep 28, 2023

Version 5.0.0 requires Android 12 and Node 16 #282

Closed

bmarsaud mentioned this pull request Jun 5, 2024

fix(android): remove broad media permissions #295

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(android)!: support API 33+ permissions #262

feat(android)!: support API 33+ permissions #262

ochakov commented Oct 13, 2022

breautek left a comment

breautek Jun 15, 2023

adeel399 Oct 17, 2023

FrancescoPaiola commented Jun 26, 2023

erisu left a comment

PabbleDabble commented Jul 25, 2023 •

edited

Loading

ath0mas commented Jul 30, 2023

jmelvin commented Sep 10, 2023

ath0mas commented Sep 10, 2023

jmelvin commented Sep 10, 2023

ath0mas commented Sep 10, 2023 •

edited

Loading

breautek commented Sep 11, 2023

feat(android)!: support API 33+ permissions #262

feat(android)!: support API 33+ permissions #262

Conversation

ochakov commented Oct 13, 2022

Platforms affected

Motivation and Context

Description

Testing

Checklist

breautek left a comment

Choose a reason for hiding this comment

breautek Jun 15, 2023

Choose a reason for hiding this comment

adeel399 Oct 17, 2023

Choose a reason for hiding this comment

FrancescoPaiola commented Jun 26, 2023

erisu left a comment

Choose a reason for hiding this comment

PabbleDabble commented Jul 25, 2023 • edited Loading

ath0mas commented Jul 30, 2023

jmelvin commented Sep 10, 2023

ath0mas commented Sep 10, 2023

jmelvin commented Sep 10, 2023

ath0mas commented Sep 10, 2023 • edited Loading

breautek commented Sep 11, 2023

PabbleDabble commented Jul 25, 2023 •

edited

Loading

ath0mas commented Sep 10, 2023 •

edited

Loading