[Bug][Android] File Picker exception when using third-party file managers

[dimonovdd]

Description

An exception occurs when using third-party file managers.

Selecting files on WhatsApp app using this and other third-party file managers on phones that I tested works without errors.

Exception StackTrace

Steps to Reproduce

1. Install File Manager+
2. Select File Manager+

Basic Information

• Affected Devices: tested on several Androids 10

Screenshots

[vividos]

I think the bug is that TakePersistableUriPermission() is called without checking if the content provider granted permissions:
https://developer.android.com/reference/android/content/ContentResolver#takePersistableUriPermission(android.net.Uri,%20int)

The code should probably look like this:

if ((data.Flags & Intent.GrantReadUriPermission) != 0)
    Platform.AppContext.ContentResolver.TakePersistableUriPermission(contentUri, ActivityFlags.GrantReadUriPermission);

(or some variant of it).

[dimonovdd]

@vividos
No, When I use a standard file Manager (Files on the screenshot), there are no errors

[vividos]

I meant the case when FileManager+ is used. Could you try if the if in front of the call to TakePersistableUriPermission helps? Thanks!

[dimonovdd]

@vividos Your example is incorrect

I tried it. There is no error and I get the file.

if (intent.Flags == ActivityFlags.GrantReadUriPermission)
{
   Platform.AppContext.ContentResolver.TakePersistableUriPermission(
   contentUri,
   ActivityFlags.GrantReadUriPermission);
}

But the result of this if is always false, because of this:
intent.AddFlags(ActivityFlags.GrantPersistableUriPermission);

[vividos]

Let me check that when I'm at home. My guess is that the FileManager doesn't grant the permission, and then the FilePicker can't take the permission.

[dimonovdd]

@vividos I tested this pull request on 5 devices with Android 10, and it works well

[dimonovdd]

@mattleibow @jamesmontemagno @Redth
What do you think?

[vividos]

if (intent.Flags == ActivityFlags.GrantReadUriPermission)

I meant checking the result intent if the flag has been set, not the original intent:

if (result.Flags.HasFlag(ActivityFlags.GrantReadUriPermission))

Unfortunately, while reading about TakePersistableUriPermission I found another possible issue calling that method: One can only take 128 persistable permissions before running into a limit: https://commonsware.com/blog/2020/06/13/count-your-saf-uri-permission-grants.html

[vividos]

I tried out file picking using an Android 10 emulator and the File Manager Plus app installed. The app returns a result intent that doesn't have set the ActivityFlags.GrantPersistableUriPermission flag, whereas the "Files" picker does. So the correct if statement to wrap the TakePersistableUriPermission would be:

if (result.Flags.HasFlag(ActivityFlags.GrantPersistableUriPermission))

I'm also interested what the Essentials devs think!

[dimonovdd]

@vividos
I think that if is not the best solution for this issue
The developer will not be notified in any way that the behaviour of the method has changed.

I think it's better to delete this code at all

Platform.AppContext.ContentResolver.TakePersistableUriPermission(contentUri, ActivityFlags.GrantReadUriPermission);

[vividos]

Yes, maybe it's best, but then the Uri is not persisted between reboots and that may also be expected of the user of the library. And when you read the commonsware.com link, we would get another problem if more than 128 Uris are persisted per app, which may be unusual, but possible.

[dimonovdd]

but then the Uri is not persisted between reboots

But this will be the same for different file managers

128 Uris are persisted per app

How can we clear them?

[vividos]

Please read the mentioned article, it's all in there.

[dimonovdd]

I expressed myself wrong
How to clear the saved Uris on Android via the Essentials Api?
After all, then the implementation of Android will be very different from other platforms

[dimonovdd]

@jamesmontemagno @Redth @mattleibow @vividos
I think it's better:

1. delete this code at all

Platform.AppContext.ContentResolver.TakePersistableUriPermission(contentUri, ActivityFlags.GrantReadUriPermission);

2. dotnt use MANAGE_DOCUMENTS permission

3. add this info to the documentation
   On Android a Uri may not be saved between reboots

[vividos]

See this PR on the original FilePicker on why the call to TakePersistableUriPermission() was added in the first place:
jfversluis/FilePicker-Plugin-for-Xamarin-and-Windows#178

[mattleibow]

HI folks, I am looking at this and I am a bit unsure of exactly what needs to be done. I see we have set the ActivityFlags.GrantPersistableUriPermission flag, but do we need to? This is just for the time the FileResult lives, and it can't really be used in shared code with the content://. I am not even sure why we expose that version.

But, basically I see two sides:

• use the flag and get a persistent uri, but we have no way to clear it
• don't use the flag, but nobody can save the content:// uri

However, since the content:// uri is very much dependent on the device state - for example if the image comes from an app, but then the user uninstalls that app - so maybe we don't need the persist flag?

[vividos]

In the past, in the FilePicker plugin, we had developers that grabbed the content Uri and stored it somewhere, expecting to be some kind of path that they can access indefinitely. So this is mostly a documentation issue. I would suggest to remove the persistent Uri stuff and add documentation that the FileResult is temporary and that files need to be copied to the app data folder or read immediately. I even would show some code using Stream.CopyToAsync on how to do it.

[dimonovdd]

I think this is the right way
#1444 (comment)

You should also not forget the cleaning when using the ActivityFlags.GrantReadUriPermission
#1444 (comment)