feat(android)!: Android 13 support #814

ochakov · 2022-10-13T19:12:31Z

Platforms affected

Android

Motivation and Context

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

Closes #825

Description

Remove deprecated READ_EXTERNAL_STORAGE and WRITE_EXTERNAL_STORAGE permissions and replace with new READ_MEDIA_IMAGES

Testing

Taking pictures and accessing gallery, save pictures into album

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 and WRITE_EXTERNAL_STORAGE permissions with READ_MEDIA_IMAGES on SDK 33

Request storage permission even without saving to photo album enabled. Required by design.

felicienfrancois · 2022-11-21T10:04:46Z

@ochakov I tested your fork in my project and got 2 issues :
1°) At compilation time
Element uses-permission#android.permission.WRITE_EXTERNAL_STORAGE at AndroidManifest.xml:59:5-108 duplicated with element declared at AndroidManifest.xml:51:5-81
It seems that manifest merger fail to merge
<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" android:maxSdkVersion="32" />
with
<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" /> from my other plugins.

=> removing android:maxSdkVersion="32" solved the compilation issue.

2°) At runtime when trying to capture photo (either from camera or library), on Android 13, I always get permission refused.
After adding some logs in onRequestPermissionResult, I saw that the only permission requested is READ_MEDIA_IMAGES and the result is [-1] (PackageManager.PERMISSION_DENIED).

It seems that READ_MEDIA_IMAGES permission does not have to be requested.

=> Removing Manifest.permission.READ_MEDIA_IMAGES from requested permissions in CameraLauncher:129-130 solve the issue

breautek · 2022-11-22T14:59:04Z

This is a breaking change as it requires at least apps compiled with API 33 and I'm sure the current tests are failing due to that reason since cordova-android@11 by default compiles with API 32.

So I created a new 7.0.0 milestone for this PR to be added to so this can be looked at after cordova-android is updated for API 33 support.

breautek · 2022-11-22T17:31:48Z

°) At compilation time
Element uses-permission#android.permission.WRITE_EXTERNAL_STORAGE at AndroidManifest.xml:59:5-108 duplicated with element declared at AndroidManifest.xml:51:5-81
It seems that manifest merger fail to merge

with from my other plugins.

=> removing android:maxSdkVersion="32" solved the compilation issue.

Removing android:maxSdkVersion is probably not the solution. You're getting the issue because of a conflict with other plugins, and those plugins if they use WRITE_EXTERNAL_STORAGE will need to be refactored to use the newer permission model introduced in API 33, and only use WRITE_EXTERNAL_STORAGE on older API devices. Similar to how this PR is handling this.

In other words, I think this PR has the right path moving forward, and the other plugins requires updates.

2°) At runtime when trying to capture photo (either from camera or library), on Android 13, I always get permission refused.
After adding some logs in onRequestPermissionResult, I saw that the only permission requested is READ_MEDIA_IMAGES and the result is [-1] (PackageManager.PERMISSION_DENIED).

It seems that READ_MEDIA_IMAGES permission does not have to be requested.

=> Removing Manifest.permission.READ_MEDIA_IMAGES from requested permissions in CameraLauncher:129-130 solve the issue

The Andoid Docs states that READ_MEDIA_IMAGES is required when reading from the external storage. I would think this would include the gallery/library as they are typically stored inside external storage. So we may need to figure out the nuances surrounding READ_MEDIA_IMAGES if it isn't working for you so that we can understanding the context of where READ_MEDIA_IMAGES is truly required.

ochakov · 2022-11-22T17:39:21Z

It seems that READ_MEDIA_IMAGES permission does not have to be requested.

It does work for me, using Pixel 4 and 7 and it does not allow taking pictures if this permission is not granted. I think we must compare the parameters sent to the plugin when taking pictures. In my case, I am using destination type file without saving to album.
This permission might not be required when using destination type URI.

felicienfrancois · 2022-11-22T17:44:45Z

It seems that READ_MEDIA_IMAGES permission does not have to be requested.

It does work for me, using Pixel 4 and 7 and it does not allow taking pictures if this permission is not granted. I think we must compare the parameters sent to the plugin when taking pictures. In my case, I am using destination type file without saving to album. This permission might not be required when using destination type URI.

On my side, i'm using destination type data URI on pixel 5. When using your fork, the permission popup is not shown at all. i get immediate callback with permission refused.

felicienfrancois · 2022-11-22T18:05:53Z

Removing android:maxSdkVersion is probably not the solution. You're getting the issue because of a conflict with other plugins, and those plugins if they use WRITE_EXTERNAL_STORAGE will need to be refactored to use the newer permission model introduced in API 33, and only use WRITE_EXTERNAL_STORAGE on older API devices. Similar to how this PR is handling this.

In other words, I think this PR has the right path moving forward, and the other plugins requires updates.

according to Android docs , WRITE_EXTERNAL_STORAGE permission has no effect since Android 11 (API 30). As a result the exact max sdk to set would be 29 on this permission entry.
Setting it to 32 seems quite arbitrary and other plugins would probably never set it to 32.
Additionally, it does not break anything to leave it uncapped.
I don't expect other plugins to set max sdk anytime soon. So the correct way to fix this issue is to leave it uncapped.
https://developer.android.com/about/versions/11/privacy/storage#permissions-target-11

breautek · 2022-11-22T18:13:29Z

according to Android docs , WRITE_EXTERNAL_STORAGE permission has no effect since Android 11 (API 30). The right code would be to set max sdk to 29 on this permission entry ?

Yes, it's actually been obsolete since scoped storage, which enabled in API 29, but can be opt out back to API 28 behaviour via requestLegacyExternalStorage. Starting in API 30, Scoped storage is forcefully enforced which makes the WRITE_EXTERNAL_STORAGE obsolete since API 30. However changing the manifest may have caveats on how it's handled in the code. For example... requesting permission even if it's a no-op is OK as long as it's declared, but attempting to request permission while not being declared I think will throw an error. So for sake of simplicity, I think we may want to keep it as maxSdkVersion 32 so that we can use the API 33 check as the PR stands now.

felicienfrancois · 2022-11-22T18:23:21Z

Ok understood.
So I would not bet on other plugins making the exact same change with the exact same max sdk number.

jfoclpf · 2023-02-03T12:41:10Z

Any time period for the reintegration of this PR? Thanks

jfoclpf · 2023-02-03T12:45:18Z

This is a breaking change as it requires at least apps compiled with API 33 and I'm sure the current tests are failing due to that reason since cordova-android@11 by default compiles with API 32.

So I created a new 7.0.0 milestone for this PR to be added to so this can be looked at after cordova-android is updated for API 33 support.

indeed, it is strange that the Android 12 test suite fails, but I can't see the test logs anymore

@ochakov can you kindly amend your commits such that it is Android 12 (SDK v 32) compatible? I.e., it passes the Test suite for Android 12.

breautek · 2023-02-03T12:52:17Z

Any time period for the reintegration of this PR? Thanks

It depends on cordova-android 12 which is unreleased and still in development.

indeed, it is strange that the Android 12 test suite fails, but I can't see the test logs anymore

@ochakov can you kindly amend your commits such that it is Android 12 (SDK v 32) compatible? I.e., it passes the Test suite for Android 12.

There's nothing to be done at this time. The code should work fine, it just needs to be compiled with SDK 33. The test fails because it uses the latest cordova-android version, which by default targets with SDK 32.

This PR should be usable as is if you set the android-compileSdkVersion to 33. You may also need to install build tools 33 and set android-buildToolsVersion to the full version name (e.g. 33.0.0) if you want to test it yourself.

jfoclpf · 2023-02-03T13:31:03Z

This PR should be usable as is if you set the android-compileSdkVersion to 33.

is this achievable merely by adding to config.xml this?

  <platform name="android">
    <preference name="android-compileSdkVersion" value="33"/>
    <preference name="android-targetSdkVersion" value="33"/>

jfoclpf · 2023-02-03T13:32:06Z

There's nothing to be done at this time. The code should work fine, it just needs to be compiled with SDK 33. The test fails because it uses the latest cordova-android version, which by default targets with SDK 32.

got it, thanks

breautek · 2023-02-03T13:36:06Z

This PR should be usable as is if you set the android-compileSdkVersion to 33.

is this achievable merely by adding to config.xml this?
  <platform name="android">
    <preference name="android-compileSdkVersion" value="33"/>
    <preference name="android-targetSdkVersion" value="33"/>

Usually at cordova-android level, support just entails bumping build tools version to the next API level, and compiling for that target. Sometimes it may require more changes but in my experience that's usually all that is required.

android-targetSdkVersion can also be left at version 32 if you're not ready to deal with whatever changes API 33 brings. This PR main requirement is that android-compileSdkVersion is set to 33 or later.

cordova-android@12 will have these values set to 33 by default.

breautek · 2023-02-17T15:27:15Z

@ochakov

Have you tested this PR for taking videos? I noticed that right now we are only handling the READ_MEDIA_IMAGES permission, but do not handle the READ_MEDIA_VIDEOS permission which I suspect we do if we use the video API part of this plugin.

jfoclpf · 2023-02-19T10:56:00Z

plugin.xml

@@ -55,7 +55,7 @@
            </feature>
        </config-file>
        <config-file target="AndroidManifest.xml" parent="/*">
-            <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
+            <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" android:maxSdkVersion="32" />
        </config-file>


I guess you should add here also

<uses-permission android:name="android.permission.READ_MEDIA_IMAGES" android:minSdkVersion="33" />

based on suggestion from @felicienfrancois

@ochakov could you add that to this PR, such that I can use this PR in my project?
thanks

Made the following changes in this PR:

Video permission when taking video instead of image permission as requested by @breautek

Updated manifest as requested by @jfoclpf

Handle video permission Add permissions to manifest

…mera

Only require media storage permissions when taking picture when save to album is requested

Final fixes

jfoclpf · 2023-02-19T22:07:01Z

@ochakov thanks a lot for your amendments

I just tested now and WRITE_EXTERNAL_STORAGE with maxSdkVersion creates a huge list of conflicts with other (also official, such as file) plugins. Other plugins don't mention maxSdkVersion for WRITE_EXTERNAL_STORAGE which creates a build failure. Please do kindly remove maxSdkVersion for WRITE_EXTERNAL_STORAGE (at least for now, maybe later maxSdkVersion can be added at same time for all official plugins).

On the other hand READ_MEDIA_IMAGES and READ_MEDIA_VIDEO are totally new permissions introduced only in API 33, thus do kindly add android:minSdkVersion="33" to these

Do kindly thus amend to the following:

  <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
  <uses-permission android:name="android.permission.READ_MEDIA_IMAGES" android:minSdkVersion="33" />
  <uses-permission android:name="android.permission.READ_MEDIA_VIDEO" android:minSdkVersion="33" />

I tested now with these amendments on both Android 12 (API 32) and 13 (API 33) and it builds and works just fine. I guess with API 33 Android simply ignores WRITE_EXTERNAL_STORAGE thus no need to have maxSdkVersion.

ochakov · 2023-02-20T08:28:24Z

@jfoclpf
Is there any reason to put minSdkVersion for READ_MEDIA_IMAGES and READ_MEDIA_VIDEO permissions? I found it will compile for older targets without specifying minSdkVersion and I can't find any recommendation to use this tag.
Regarding maxSdkVersion for WRITE_EXTERNAL_STORAGE, this is a specific requirement from Android and it is listed in mandatory changes for apps targeting API 33. In our case, we had to change other plugins (BTW, I submitted a similar PR for cordova-plugin-media-capture containing maxSdkVersion) and include maxSdkVersion in all of them.
I suggest to hear other opinions in this thread if to leave maxSdkVersion as required or remove it for compatibility.

jfoclpf · 2023-02-20T08:44:49Z

Hi @ochakov

WRITE_EXTERNAL_STORAGE also compiles OK for API 33, (I just tested, @felicienfrancois and @keva91 also tested) it is simply ignored in API 33 and by avoiding maxSdk we avoid a lot of building conflicts.

By having minSdk we ensure these new permissions are only used with API 33 and above without any building conflicts, since these permissions are totally new with API 33.

@breautek do you have any opinion therefor?

felicienfrancois · 2023-02-20T09:03:35Z

Maybe we should also raise an issue on the manifest merger. The conflict issue is caused my manifest merger not being able to merge 2 permissions having a different maxSdkVersion / minSdkVersion.
I think it could be improved to avoid such conflicts (taking the min / max value).

jfoclpf · 2023-02-20T11:41:51Z

That's off topic but it's a very good point IMHO, to merge considering the most restrictive feature, for example if there are two different maxSdk select the lowest.

jkainth · 2023-03-17T19:32:34Z

Any updates to this PR?

breautek · 2023-03-17T19:38:56Z

Any updates to this PR?

Like the last few comments stated, this PR depends on cordova-android@12 which is in development.

This PR is blocked until cordova-android@12 is released, and only then this PR will resume (cordova-android@12 should fix the failing tests).

Locking to keep the discussion history clean. Will unlock once cordova-android@12 is released to allow for further comments.

breautek · 2023-05-25T12:20:20Z

Closing/reopening to restart tests.

Kohagan91 · 2023-06-09T13:13:33Z

src/android/CameraLauncher.java

+            else {
+                switch (mediaType) {
+                    case PICTURE:
+                        return new String[]{ Manifest.permission.CAMERA, Manifest.permission.CAMERA, Manifest.permission.READ_MEDIA_IMAGES };


Suggested change

return new String[]{ Manifest.permission.CAMERA, Manifest.permission.CAMERA, Manifest.permission.READ_MEDIA_IMAGES };

return new String[]{ Manifest.permission.CAMERA, Manifest.permission.READ_MEDIA_IMAGES };

Duplicate permission ?

epetre · 2023-06-10T17:26:18Z

This PR works well on my Android s20 FE.

The only issue remaining is the Gallery, for some reason, it just complains about permissions being denied and the dined permission is (READ_MEDIA_IMAGES) when trying to access the gallery. But that permission is never requested in the app.

Removing the check and request for permissions makes it all work for me (even gallery). Even though I clearly see that READ_MEDIA_IMAGES is not granted.

cordova-plugin-camera/src/android/CameraLauncher.java

Line 196 in 67b4e61

// FIXME: Stop always requesting the permission

So just doing this.getImage(this.srcType, destType);

I'm a bit confused because I have done a fresh install of the app on both my Android s20 FE and A Pixel 3 emulator on API 30... And both now NEVER request permissions for camera or gallery and both actually just work.

breautek · 2023-06-15T12:47:55Z

src/android/CameraLauncher.java

+            if (storageOnly) {
+                switch (mediaType) {
+                    case PICTURE:
+                        return new String[]{ Manifest.permission.READ_MEDIA_IMAGES };


We'll need to test this.

the CAMERA permission is not something normally required, but if the android manifest declares the permisison, then the app must have it granted (even if we the app doesn't use any camera APIs itself). So requesting the CAMERA permission before was required. I'm not sure that bug is still present in the Android SDK in API 33.

DaedalusDev · 2023-06-27T12:26:03Z

src/android/CameraLauncher.java

+        String[] storagePermissions = getPermissions(true, mediaType);
+        boolean saveAlbumPermission;
+        if (this.saveToPhotoAlbum) {
+            saveAlbumPermission = hasPermissions(storagePermissions);
+        } else {
+            saveAlbumPermission = true;
+        }


Will produce an issue with Android < 8.0. I've replace the code above with this and it works.

String[] storagePermissions = getPermissions(true, mediaType); boolean saveAlbumPermission = hasPermissions(storagePermissions);

Kaabi · 2023-06-29T11:05:01Z

is keep add duplicate writes to external into manifest I think it conflict with Android permissions plugin

> Task :app:processDebugMainManifest FAILED
~platforms\android\app\src\main\AndroidManifest.xml:26:5-108 Error:                                                                                                               
        Element uses-permission#android.permission.WRITE_EXTERNAL_STORAGE at AndroidManifest.xml:26:5-108 duplicated with element declared at AndroidManifest.xml:25:5-81
~platforms\android\app\src\main\AndroidManifest.xml Error:
        Validation failed, exiting

Use READ_MEDIA_IMAGES and READ_MEDIA_VIDEO permissions when plugin is used with Android 13 or newer. Based on apache#814 WR-Issue: #17645

ndrake · 2023-07-28T13:40:15Z

In our testing, this PR works for us with Cordova Android 12

* feat(android)!: Android 13 support * refactor(android): simplify getPermissions logic * feat(android)!: bump cordova-android requirement to >=12.0.0 * feat(android): update saveAlbumPermission to include Android 9 and below use case --------- Co-authored-by: ochakov <[email protected]>

ochakov added 2 commits October 13, 2022 13:40

Android 13 support

8df6f87

Replace READ_EXTERNAL_STORAGE and WRITE_EXTERNAL_STORAGE permissions with READ_MEDIA_IMAGES on SDK 33

Request storage permission

8d0f5eb

Request storage permission even without saving to photo album enabled. Required by design.

felicienfrancois mentioned this pull request Nov 21, 2022

Android 13 : Camera/Gallery doesn't open #797

Closed

breautek added this to the 7.0.0 milestone Nov 22, 2022

breautek added the platform: android label Nov 22, 2022

fidaktk approved these changes Nov 27, 2022

View reviewed changes

This was referenced Jan 11, 2023

Support for Android 13 and API 33? DeveloperImobzi/cordova-plugin-x-socialsharing#2

Open

Support Android 13/API 33 media storage permissions dpa99c/cordova-diagnostic-plugin#488

Closed

This comment was marked as off-topic.

Sign in to view

jfoclpf reviewed Feb 19, 2023

View reviewed changes

ochakov added 4 commits February 19, 2023 19:29

Android 13 permissions

5b869f2

Handle video permission Add permissions to manifest

Merge branch 'master' of https://github.com/ochakov/cordova-plugin-ca…

e2ab738

…mera

Android 13 permissions

4958eb7

Only require media storage permissions when taking picture when save to album is requested

Android 13 permissions

67b4e61

Final fixes

breautek mentioned this pull request Feb 28, 2023

Support for Android api 33 #825

Closed

This comment was marked as off-topic.

Sign in to view

breautek changed the title ~~Android 13 support~~ feat(android)!: Android 13 support Mar 2, 2023

apache locked as spam and limited conversation to collaborators Mar 17, 2023

breautek closed this May 25, 2023

breautek reopened this May 25, 2023

Emanuelmoves mentioned this pull request Jun 4, 2023

Cordova 12 is released (Android 13 support) #834

Closed

apache unlocked this conversation Jun 4, 2023

Kohagan91 reviewed Jun 9, 2023

View reviewed changes

epetre mentioned this pull request Jun 10, 2023

Error Code 20 on Android #826

Closed

3 tasks

breautek reviewed Jun 15, 2023

View reviewed changes

DaedalusDev reviewed Jun 27, 2023

View reviewed changes

erkenberg mentioned this pull request Jul 12, 2023

Merge current state of Android 13 support pull request from into mobilex-master mobilexag/cordova-plugin-camera#1

Merged

AlessandroSomma added a commit to webratio/phonegap-plugin-camera that referenced this pull request Jul 19, 2023

Add support for Android 13

b59e05e

Use READ_MEDIA_IMAGES and READ_MEDIA_VIDEO permissions when plugin is used with Android 13 or newer. Based on apache#814 WR-Issue: #17645

erisu mentioned this pull request Aug 17, 2023

feat(android)!: Android 13 support #844

Merged

5 tasks

erisu closed this in #844 Aug 26, 2023

breautek mentioned this pull request Feb 27, 2024

Fix: Duplicate Permission Error For WRITE_EXTERNAL_STORAGE #881

Closed

5 tasks

	return new String[]{ Manifest.permission.CAMERA, Manifest.permission.CAMERA, Manifest.permission.READ_MEDIA_IMAGES };
	return new String[]{ Manifest.permission.CAMERA, Manifest.permission.READ_MEDIA_IMAGES };

feat(android)!: Android 13 support #814

feat(android)!: Android 13 support #814

Conversation

ochakov commented Oct 13, 2022 • edited by breautek Loading

Platforms affected

Motivation and Context

Description

Testing

Checklist

felicienfrancois commented Nov 21, 2022

breautek commented Nov 22, 2022

breautek commented Nov 22, 2022

ochakov commented Nov 22, 2022

felicienfrancois commented Nov 22, 2022 • edited Loading

felicienfrancois commented Nov 22, 2022 • edited Loading

breautek commented Nov 22, 2022

felicienfrancois commented Nov 22, 2022

jfoclpf commented Feb 3, 2023

jfoclpf commented Feb 3, 2023

breautek commented Feb 3, 2023

jfoclpf commented Feb 3, 2023

jfoclpf commented Feb 3, 2023

breautek commented Feb 3, 2023

This comment was marked as off-topic.

breautek commented Feb 17, 2023

This comment was marked as off-topic.

jfoclpf Feb 19, 2023

Choose a reason for hiding this comment

jfoclpf Feb 19, 2023

Choose a reason for hiding this comment

ochakov Feb 19, 2023

Choose a reason for hiding this comment

jfoclpf commented Feb 19, 2023 • edited Loading

ochakov commented Feb 20, 2023 • edited Loading

jfoclpf commented Feb 20, 2023

felicienfrancois commented Feb 20, 2023

jfoclpf commented Feb 20, 2023 via email

This comment was marked as off-topic.

This comment was marked as off-topic.

jkainth commented Mar 17, 2023

breautek commented Mar 17, 2023

breautek commented May 25, 2023

Kohagan91 Jun 9, 2023

Choose a reason for hiding this comment

epetre commented Jun 10, 2023 • edited Loading

breautek Jun 15, 2023

Choose a reason for hiding this comment

DaedalusDev Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Kaabi commented Jun 29, 2023

ndrake commented Jul 28, 2023

ochakov commented Oct 13, 2022 •

edited by breautek

Loading

felicienfrancois commented Nov 22, 2022 •

edited

Loading

felicienfrancois commented Nov 22, 2022 •

edited

Loading

jfoclpf commented Feb 19, 2023 •

edited

Loading

ochakov commented Feb 20, 2023 •

edited

Loading

epetre commented Jun 10, 2023 •

edited

Loading

DaedalusDev Jun 27, 2023 •

edited

Loading