Skip to content

Commit

Permalink
Bypass non-existent permission check (#12766)
Browse files Browse the repository at this point in the history 
In Android 13+ android.permission.WRITE_EXTERNAL_STORAGE no longer exists, and requesting it always returns `Denied`.
AFAIK it is not needed for the media capture in this Android version either, though I may be wrong.
  • Loading branch information
@Ghostbird
Ghostbird authored Jan 26, 2023
1 parent 762e07e commit 197f203
Showing 1 changed file with 4 additions and 1 deletion.
5 changes: 4 additions & 1 deletion src/Essentials/src/MediaPicker/MediaPicker.android.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,10 @@ public async Task<FileResult> CaptureAsync(MediaPickerOptions options, bool phot
throw new FeatureNotSupportedException();

await Permissions.EnsureGrantedAsync<Permissions.Camera>();
await Permissions.EnsureGrantedAsync<Permissions.StorageWrite>();
// StorageWrite no longer exists starting from Android API 33
if (!OperatingSystem.IsAndroidVersionAtLeast(33))
await Permissions.EnsureGrantedAsync<Permissions.StorageWrite>();


var capturePhotoIntent = new Intent(photo ? MediaStore.ActionImageCapture : MediaStore.ActionVideoCapture);

Expand Down

4 comments on commit 197f203

@cjrvdev
Copy link

Choose a reason for hiding this comment

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

@Ghostbird this is wrong/outdated anymore. If the app targets API >30 this permission is no longer in effect. Can you please open a new PR and remove this requierement?
https://developer.android.com/reference/android/Manifest.permission#WRITE_EXTERNAL_STORAGE

@Ghostbird
Copy link
Contributor Author

@Ghostbird Ghostbird commented on 197f203 Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Your lordship, can you explain what you mean? I think this code is still good.

Did you misunderstand my change? This change did not add a requirement. It removes a requirement if the API level is higher than 33. This is because as you said, this permission was no longer in effect in API 30+ but in API 33+ it immediately returns Denied instead.

The original code:

  • Requests storage write permissions correctly on API levels 29 and lower
  • Requests storage write permission needlessly on API levels 30, 31 and 32
  • Requests storage write permission erroneously, was denied, and stopped working on API levels 33 and higher.

My change made it so that the code now:

  • Requested storage write permissions correctly on API levels 29 and lower
  • Requested storage write permission needlessly on API levels 30, 31 and 32
  • Does not request storage write permission API levels 33 and higher.

Arguably I could have removed the requirement for API levels 30 and higher1, however that would have meant a change that impacted API levels 30, 31 and 32, which already were confirmed working. Now my change only affected API 33 and (at the time of writing hypothetical) higher levels. API 33 was already confirmed broken due to the change in Android.

Does this explanation clear things up. Or did I miss something and is it really broken in API 35+? I've confirmed this working in API 33 and API 34, but I've only installed API 35 on my device since two weeks and haven't actually hit this scenario yet.

Footnotes

  1. https://github.com/dotnet/maui/pull/12766#issuecomment-1437294625 ↩

@cjrvdev
Copy link

Choose a reason for hiding this comment

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

You made my day, never expected anyone to read the pronouns 😆

Thing is it was alright back in the day I think, but at some point (not sure when, not gonna lie) they changed into not necessary. As per documentation this permission is no longer required if you target android 30+. The condition should be "if the target sdk is >=30" rather than "if current operating system is 33 or lower".

Thats what they say at least! I came across this when removing no longer necessary permissions from manifest and saw Essentials throwing this exception.

From the docs:
Note: If your app targets Build.VERSION_CODES.R or higher, this permission has no effect.

Thanks buddy!

@Ghostbird
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Glad I could make you chuckle. Github shows a brief user blurb including pronouns when you hover a username on Desktop, which I did to know who I was replying to. When I read your tooltip I had to check your profile, because I seriously considered for a moment that Github might support titles of nobility. Then I realised it was just the pronoun setting. Brilliant!

I've checked the code and I think you're right. This commit can be reverted because 4 months ago @jfversluis fixed this fundamentally in 31ed71f. Before that, calling await Permissions.EnsureGrantedAsync<Permissions.StorageWrite>(); on Android 33+ would throw IIRC.

Please sign in to comment.