CB-14260: (android) captureImage permission denial on android 8.1

[mcelotti]

captureImage: permission denial on android 8.1

Platforms affected

Android

What does this PR do?

It fixes this issue: https://issues.apache.org/jira/browse/CB-14260

What testing has been done on this change?

Real device and emulator

Checklist

• Reported an issue in the JIRA database
• Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
• Added automated test coverage as appropriate for this change.

[mcelotti]

Hi, who's taking care of this? Is it possible to have a feedback?

[janpio]

What exactly does this PR do? The JIRA ticket talks about a permission problem, but the PR seems to a) fix a bug with a wrong permission and b) add some image rotating for some reason? Did I misunderstand? Is this all related?

[mcelotti]

Hi @janpio
my fault, I pushed two commits for two different PRs, now I cleaned up and you can see that this PR is only about referenced issue (permissions on android 8).
If this PR is going to be accepted I could create a new PR for the other issue https://issues.apache.org/jira/browse/CB-13683
Thank you!

[janpio]

Thanks, now it makes sense ;)

What was Manifest.permission.READ_EXTERNAL_STORAGE before is now Manifest.permission.WRITE_EXTERNAL_STORAGE. Was this a bug before? Is this a new permission? Was this somehow changed with Android 8.1 that you mention?

PS: Go for it with the other PR. The sooner it is there, the sooner it can be merged.

[mcelotti]

Well, I noticed that the "android.provider.MediaStore.ACTION_IMAGE_CAPTURE" intent uses the EXTRA_OUTPUT extra (https://developer.android.com/reference/android/provider/MediaStore#EXTRA_OUTPUT) and therefore WRITE_EXTERNAL_STORAGE is required to save the image.
I suppose this is related to multiple image capture feature, because there's a ContentResolver used to reference all the pictures.

[mcelotti]

As you suggested I created another PR (#100) for the image rotation issue.
Thank you

[janpio]

I don't really know Android, could you explain EXTRA_OUTPUT a bit more and maybe provide a link to where the plugin uses it?

I suppose this is related to multiple image capture feature, because there's a ContentResolver used to reference all the pictures.

Is this a new feature added to the plugin, and this code wasn't updated to keep pace?

[mcelotti]

The EXTRA_OUTPUT was already there:

https://github.com/mcelotti/cordova-plugin-media-capture/blob/3157ddc995ec509d2f3d63eb7c4651fb890f5b4b/src/android/Capture.java#L279

The "multiple images" feature is part of the plugin, I did not change anything about it.

[janpio]

Yes, I understand that.
I am trying to understand why this problem is present in this plugin at all to understand if your solution has any side effects.

[mcelotti]

Here we have the answer: https://developer.android.com/about/versions/oreo/android-8.0-changes#rmp
Looks like WRITE_EXTERNAL_STORAGE was granted automatically since it belongs to the same group of READ_EXTERNAL_STORAGE

[janpio]

Ok, that makes sense.

Let me try to recap what is happening here:
The plugin was always requesting READ_EXTERNAL_STORAGE although it needs to write to that storage. Before Android 8 that was fine as requesting READ_EXTERNAL_STORAGE also gave you WRITE_EXTERNAL_STORAGE automatically, now with Android 8 you would also get the permission automatically but only if you requested it explicitly. As we don't do that, we should ask for it in the first place and be done with it.

Correct?

[mcelotti]

Yes

[janpio]

Have my approval then.

[ffMathy]

Can we merge this? It fixes a critical issue, and it has been quite a few days since it was approved.

[janpio]

I won't as I don't know enough about possible side effects. If you tested the PR and can confirm that it works, please also approve the PR. Each "vote" helps a future maintainer to decide if to merge or not.

[ffMathy]

Works like a charm, tested on brand new Samsung Galaxy S9.

[mnill]

I have tested on xiaomi redmi 4a, works like a charm

[ffMathy]

@janpio so both @mnill and I tested, and this is insanely critical - the plugin is not working at all for specific target SDKs. Furthermore, per default, it is not working in Ionic because of this.

How many tests and approvals will it need before we merge it? I can get a few friends to help as well maybe.

[janpio]

We still need a Cordova maintainer to merge it and cut a release. Commenting here won't do a thing about that. I requested reviews from 2 people that maybe could help, but they don't seem to have the time right now. As we are all volunteers, this is total understandable.

Alternatively you can just install this branch of this PR or fork the plugin yourself and merge this branch into it.

[jcesarmobile]

This doesn't need a certain number of "tests and approvals", it just needs to be verified by some core contributor that can merge, so instead of getting friends saying "it works", what you can do to accelerate the process is to provide a sample app where the bug can be easily reproduced so it's easier for whoever test this.

[jcesarmobile]

I'm pretty sure the audio recording still needs the read permission, so I think it might be safer to ask for both, at least in captureAudio, or just don't change it there and change only for captureImage.
Couldn't really test because my 8.1 device doesn't include an audio capture app, and tried using a few and only one from sony allows the intent, but then cordova-android CordovaResourceApi is not able to handle the content url returned.

[ffMathy]

@mcelotti do you have any idea of why the tests are failing? I have not been able to find the reason.

[janpio]

Probably unrelated to this specific PR as it is happening with other PRs and commits as well @ffMathy.

[mcelotti]

@ffMathy @janpio I agree, it seems that the failed test (only safari is failing: https://travis-ci.org/apache/cordova-plugin-media-capture/builds/413520949) is not directly related to this PR.

[ffMathy]

@mcelotti can you fix @jcesarmobile's latest comments so it's ready again?

[ffMathy]

Oh, and one more question... The two reviewers remaining are very(!) rarely active on GitHub. Are they both required to approve and/or merge?

[janpio]

Besides this being not true, none of the requested reviewers need to approve the PR to be merged. It just needs any Apache Cordova maintainer to pull the trigger.
As currently tests are failing, even unrelated, we will probably want to have those fixed as well before adding code to master btw.

[ffMathy]

Should we create a new issue for that then? Because or else no one will probably fix it, and no PRs will ever be merged in that state.

[janpio]

Of course, feel free to create an issue, investigate, create a PR for it. Anything that moves stuff forward is helpful.

[ffMathy]

I created #105 for fixing the build issue. @mcelotti do you have any idea of how to fix it?

Apparently to get this out of the door, we need to fix the build too 😞

[mcelotti]

@ffMathy captureAudio has now READ_EXTERNAL_STORAGE b2edbd7

[jcesarmobile]

I'm going to merge despite the test errors as it's not related

[ffMathy]

Thank you so much for being pragmatic about it!

[ffMathy]

How frequently are releases performed?

[jcesarmobile]

Last one was in April, so not very frequent

[ffMathy]

Who is responsible for the release? Can we speed it up somehow?