Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug][Android] Picking photo from Photos app leads to app crash unless we explicitly request StorageRead permissions on Android 9 #2099

Open
MitchBomcanhao opened this issue Sep 5, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@MitchBomcanhao
Copy link

MitchBomcanhao commented Sep 5, 2023

Description

After updating our solution to the newest Android 13 supporting packages from Essentials and Forms, and adapting the code to the new expectations regarding permissions, I've found that I could make the photo picker crash with a UnauthorizedAccessException whenever I was on Android 9 and I picked a photo in a certain way.
I cannot reproduce this issue on devices running Android 10 and higher, which I assume is related to changes in permissions that also resulted in previous issues such as the following, which were identified on android 10+:

It is possible that the same issue already existed in older versions of essentials but as there were different expectations regarding permissions, ie we always requested Permissions.StorageRead when picking photos. Now the expectation is that you might not have to explicitly request storage permissions and it seems to still work, except on some older android versions.

Xamarin essentials actually checks for certain permissions before running some of its picking/camera tasks (and some of those checks are even api level dependent), so it is sort of implied that if essentials does not check for a permission, it is likely unnecessary? is there something in the documentation that says we have to request Permissions.StorageRead when picking photos, even though essentials doesn't check for it and it doesn't seem to be necessary for android 10+? is there an exception somewhere that says we have to still request it for android 9 and under?

Steps to Reproduce

  1. take at least one photo with the device or emulator.
  2. run the test application and trigger the media picker task
  3. when it opens the media picker interface, tap the hamburger menu on the top left and switch to the "Photos" app context at the bottom.
  4. Now select the relevant folder and pick a photo file.
  5. the code will attempt to access the file's contents;
  6. Observe the unexpected behaviour.
    1. the app will crash with a System.UnauthorizedAccessException

Expected Behavior

File is accessible after picking.

Actual Behavior

App will crash with an UnauthorizedAccessException when we attempt to access it.

**System.UnauthorizedAccessException:** 'Access to the path "/storage/emulated/0/DCIM/Camera/IMG_20230905_122553.jpg" is denied.'

[mono-rt] [ERROR] FATAL UNHANDLED EXCEPTION: System.UnauthorizedAccessException: Access to the path "/storage/emulated/0/DCIM/Camera/IMG_20230905_122612.jpg" is denied.
[mono-rt]   at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.Int32 bufferSize, System.Boolean anonymous, System.IO.FileOptions options) [0x001aa] in /Users/builder/jenkins/workspace/archive-mono/2020-02/android/release/mcs/class/corlib/System.IO/FileStream.cs:239 
[mono-rt]   at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00000] in /Users/builder/jenkins/workspace/archive-mono/2020-02/android/release/mcs/class/corlib/System.IO/FileStream.cs:91 
[mono-rt]   at (wrapper remoting-invoke-with-check) System.IO.FileStream..ctor(string,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare)
[mono-rt]   at System.IO.File.OpenRead (System.String path) [0x00000] in /Users/builder/jenkins/workspace/archive-mono/2020-02/android/release/external/corefx/src/System.IO.FileSystem/src/System/IO/File.cs:266 
[mono-rt]   at Xamarin.Essentials.FileBase.PlatformOpenReadAsync () [0x00000] in D:\a\_work\1\s\Xamarin.Essentials\FileSystem\FileSystem.android.cs:384 
[mono-rt]   at Xamarin.Essentials.FileBase.OpenReadAsync () [0x00000] in D:\a\_work\1\s\Xamarin.Essentials\FileSystem\FileSystem.shared.cs:172 
[mono-rt]   at AndroidPickerBug.MainPage.Button_Clicked (System.Object sender, System.EventArgs e) [0x000b2] in C:\Users\redacted\source\repos\AndroidPickerBug\AndroidPickerBug\MainPage.xaml.cs:25 
[mono-rt]   at System.Runtime.CompilerServices.AsyncMethodBuilderCore+<>c.<ThrowAsync>b__7_0 (System.Object state) [0x00000] in /Users/builder/jenkins/workspace/archive-mono/2020-02/android/release/mcs/class/referencesource/mscorlib/system/runtime/compilerservices/AsyncMethodBuilder.cs:1021 
[mono-rt]   at Android.App.SyncContext+<>c__DisplayClass2_0.<Post>b__0 () [0x00000] in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Android.App/SyncContext.cs:36 
[mono-rt]   at Java.Lang.Thread+RunnableImplementor.Run () [0x00008] in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Java.Lang/Thread.cs:36 
[mono-rt]   at Java.Lang.IRunnableInvoker.n_Run (System.IntPtr jnienv, System.IntPtr native__this) [0x00008] in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/obj/Release/monoandroid10/android-33/mcw/Java.Lang.IRunnable.cs:84 
[mono-rt]   at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PP_V (_JniMarshal_PP_V callback, System.IntPtr jnienv, System.IntPtr klazz) [0x00005] in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Android.Runtime/JNINativeWrapper.g.cs:22 
[mono-rt]   at (wrapper native-to-managed) Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PP_V(intptr,intptr)

Basic Information

  • Version with issue: 1.8.0
  • Last known good version: not sure if applicable as permissions changed a great deal since target api 33 has been mandatory.
  • IDE: VS2022 17.7.1
  • Platform Target Frameworks:
    • Android: 33

Reproduction Link

Android's AssemblyInfo.cs file declares:

[assembly: UsesPermission(MaxSdkVersion = 32, Name = Android.Manifest.Permission.WriteExternalStorage)]
[assembly: UsesPermission(MaxSdkVersion = 32, Name = Android.Manifest.Permission.ReadExternalStorage)]

I just have a plain xaml page with a button that runs this on its clicked event:

        private async void Button_Clicked(object sender, EventArgs e)
        {
            var result = await Xamarin.Essentials.MediaPicker.PickPhotoAsync();

            if (result is FileResult fr) 
            {
                using (var fileStream = await fr.OpenReadAsync())
                {
                    // this will blow up if file permission is not actually granted
                }
            }            
        }

The above code will fail on fr.OpenReadAsync() when you're on android 9 and you haven't got StorageRead permission. It'll work fine on Android 10+.

Workaround

Call await Xamarin.Essentials.Permissions.RequestAsync<Permissions.StorageRead>(); before running this code, only if your device is running an android version lower than 10.

@MitchBomcanhao MitchBomcanhao added the bug Something isn't working label Sep 5, 2023
@MitchBomcanhao MitchBomcanhao changed the title [Bug][Android] Picking photo from Photos app leads to app crash unless we explicitly request StorageRead permissions (Android 9) [Bug][Android] Picking photo from Photos app leads to app crash unless we explicitly request StorageRead permissions on Android 9 Sep 5, 2023
@yevgeny-sotnikov
Copy link

yevgeny-sotnikov commented Oct 17, 2023

IMHO it isn't an issue - you need by design of permissions API to request explicitly them each time when you're trying to get an access to functionality which requires them

@MitchBomcanhao
Copy link
Author

do you think it is normal for this to blow up on specific versions of android? it is essentials' job to make sure they have requested the necessary permissions in order to achieve its functionality. there's nothing in the documentation stating that you have to request this permission yourself before calling the method. Xamarin essentials does still check for permissions in other areas of the code, so why not here? from a consistency standpoint, this is broken. We shouldn't have to be checking android versions and checking specific permissions when it is the package's job to do that when necessary.

@yevgeny-sotnikov
Copy link

yevgeny-sotnikov commented Oct 17, 2023

do you think it is normal for this to blow up on specific versions of android?

yes and not only for android. It's requirement of android and iOS SDK - you have to explicitly ask user about permission (if it's disabled) right before access to permission-related code

But cuz your business logic can vary depends on to specific functionality - better to call explicitly Essentials.Permissions at your own code and handle results

We shouldn't have to be checking android versions and checking specific permissions when it is the package's job to do that when necessary

What about Android-specific permissions - most of them have analogue on other platforms and common handlers at Essentials.Permissions, so you can call them)

If you don't have handler for some specific permission - you can implement it for your specific case

@MitchBomcanhao
Copy link
Author

I know I can implement it - but that's not the point, is it? we can implement anything we want. but the essentials package is supposed to do these things for us, and indeed they do... but not on this particular case. so it is a bug because their permission checks aren't sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants