osu-framework icon indicating copy to clipboard operation
osu-framework copied to clipboard

File/folder picker does not handle init folder and root folder access denied properly on Android 10 and lower

Open honguyenminh opened this issue 3 years ago • 9 comments

The file/folder picker on Android is a bit broken. Everything is tested with osu!~lazer~ on real unrooted devices with no SD card plugged in, internal storage only.

On open, picker will drop user into /sdcard as init directory, which is denied on Android 10 and lower, even with Storage permission allowed. This works fine on Android 11 though, no problem is encountered.

The current code on directory change will try to traverse up the tree of the directory until it reaches a readable directory. On Android 10 and lower, it will deny access to /sdcard, so the picker goes up one directory, which is root. Root is (partially) readable, we can see some content in here, but we can't go anywhere listed, access denied. So, after the picker load, user will be dropped into root folder and got stuck here, while the path shown is still kept at /sdcard (this is another bug, but this can be fixed quite easily).

On Android 11 (12 not tested because I don't have money) I can get into /sdcard and everything works fine. But if I went to root or "Computer" then same thing happens, I got stuck there, and /sdcard is not inside root somehow, so I'm forced to close the picker then open it again to get back to /sdcard. In this case a back button mechanism should kind of "fix" this. Somehow I can access this folder without even giving the Storage permission. Weird.

honguyenminh avatar Nov 15 '21 15:11 honguyenminh

known issue.

https://github.com/ppy/osu/pull/15248 has a flow for requesting permissions properly, but is broken because of android losing egl context on pause. i have a path forward how to resolve that but it may take a while more until that's ready.

bdach avatar Nov 15 '21 16:11 bdach

Does the solution address user getting into inaccessible path (like root) though, because going up the tree until a readable folder is found is not a good idea, especially on *nix and Android.

Just stay in the current path and don't change path at all is better imo.

honguyenminh avatar Nov 15 '21 17:11 honguyenminh

that assumes that the original directory is still accessible which is not necessarily true either. example:

dir1
└─ dir 2
   └─ dir 3

now assume you're in dir 3, then delete dir 2, but then try to navigate to dir 2 after that deletion. neither the target directory, nor the original directory, exist anymore. this would previously lead to crashes.

just needs a better sane fallback directory for android, is all.

bdach avatar Nov 15 '21 17:11 bdach

Yeah, we need a better Computer directory for Android.

The Computer directory is generated by this code: https://github.com/ppy/osu-framework/blob/c4a6dc5bc3e3b8c7121b334408eae6d9b48ab5bd/osu.Framework/Graphics/UserInterface/DirectorySelector.cs#L130-L138

This generates a bunch of nonsensical "drives" on android (one of which is /storage/emulated/0/data so it's actually possible to return to a sane folder).

We should move this drive-creation process to a platform specific thing in GameHost, probably make it an IEnumerable<DirectoryInfo>, similar to UserStoragePaths.

Needs a bit more consideration for display names (eg. /storage/emulated/0Internal storage).

Or maybe we want Computer to map to /storage/emulated/0?

(Throughout this post, when I say /storage/emulated/0 I mean the user ID-adjusted /storage/emulated/<n> folder.


Made a quick and dirty PoC for a custom Computer directory, and it works as expected.

if (newDirectory == null)
{
    string[] customDirs =
    {
        "/sdcard",
        "/storage/emulated/0",
    };

    var dirs = customDirs.Select(s => new DirectoryInfo(s));

    foreach (var dir in dirs)
        directoryFlow.Add(CreateDirectoryItem(dir, dir.FullName));

    return;
}

image

Susko3 avatar Nov 15 '21 19:11 Susko3

So, should "Computer" represent the emulated user storage (/storage/emulated/) or list all the actual storage devices connected (/storage/sdcard)?

https://android.stackexchange.com/a/39546

Not sure how to actual solve this though. Making "Computer" one of the above will prevent rooted Android user from accessing these path, but will bring better experience for unrooted user. By binding "Computer" root folder won't even be shown to the user.

And about deleted dir handling for bdach's example, we can

  • On error opening a directory, check if current directory is still accessible. If it's still ok, stay there and flash error as usual.
  • If current dir is not accessible, try to go to a fallback or default directory that we're pretty sure is accessible.
  • If still inaccessible, try to traverse the tree like what we're currently doing.
  • Worst case, if all else fail, show error then close picker or similar panic behavior.

honguyenminh avatar Nov 16 '21 06:11 honguyenminh

So, should "Computer" represent the emulated user storage (/storage/emulated/) or list all the actual storage devices connected (/storage/sdcard)?

Since Android has gotten really restrictive with storage in 12, we definitely want Computer (should probably be renamed, keeping localisation support in mind) to link to or be /storage/emulated/<user id>.

Making "Computer" one of the above will prevent rooted Android user from accessing these path, but will bring better experience for unrooted user. By binding "Computer" root folder won't even be shown to the user.

Computer is not the root folder, it's the null folder, which, in our code, is the "parent" for /. So if you want to access / on a rooted device, you could open /storage/emulated/0 and then tap on the / breadcrumb. If we can read that folder, we'll stay on it and show the folder/file list, if not, we'll just fall back to it's "parent", Computer.

We don't want to include / in Computer, as that will confuse normal/unrooted people.

And for your latter points, since Computer is the "true parent" of all directories, we'll always have a sane default to fall back to.

Susko3 avatar Nov 16 '21 06:11 Susko3

So "Computer" is only a logical place we created to hold the directory content?

So, from what I understand, when user click on "Computer", it will redirect to /storage/emulated/<user-id> and change breadcrumb to
Computer > / > storage > emulated > id

And breadcrumb with only Computer is, as a result, not possible on Android, right?
Alright that makes sense.

Now we just need the permission PR to be finished.

honguyenminh avatar Nov 16 '21 06:11 honguyenminh

Or maybe we want Computer to map to /storage/emulated/0?

When I say this, I mean Computer just listing every directory in /storage/emulated/0. I can see this being confusing, so we probably won't do that.

Susko3 avatar Nov 16 '21 07:11 Susko3

Yeah, just use the "Computer" as a redirect is best IMO.

Actually, I'm thinking about setting "Computer" to be just root on *nix OSes, since what they're currently doing is list all mounted devices (this means all the virtual mounts like root(?), sys, proc,...) and is kinda useless, you already have access to these from root. It's better to just make it root and make it /storage/emulated/0 on Android only. Windows is the only OS that need a "fake" Computer to hold the drives.

honguyenminh avatar Nov 16 '21 08:11 honguyenminh