react-native-documents / document-picker

Document Picker for React Native
https://react-native-documents.github.io/
MIT License
1.35k stars 441 forks source link

Windows - Picking directory doesn't actually give access to that directory #511

Closed ghost closed 1 year ago

ghost commented 2 years ago

Bug report

Summary

When picking a directory for windows and then trying to save a file there using react-native-fs, I get 'access denied'. The UWP documentation mentions you need to give access to the folder to allow it to be used.

I did a test by adding the below code to the PickDirectory c# method in react-native-document-picker and it fixed my issue

  // Application now has read/write access to all contents in the picked folder
                    // (including other sub-folder contents)
                    Windows.Storage.AccessCache.StorageApplicationPermissions.
                          FutureAccessList.AddOrReplace("PickedFolderToken", folder);

Requesting this be added to the code (or I can create a PR if preferable)

Environment info

npx react-native info output:

System:
    OS: Windows 10 10.0.19043
    CPU: (8) x64 Intel(R) Core(TM) i7-10610U CPU @ 1.80GHz
    Memory: 3.00 GB / 15.61 GB
  Binaries:
    Node: 14.15.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.5 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
    npm: 6.14.8 - C:\Program Files\nodejs\npm.CMD
    Watchman: Not Found
  SDKs:
    Android SDK: Not Found
    Windows SDK:
      AllowDevelopmentWithoutDevLicense: Enabled
      AllowAllTrustedApps: Enabled
      Versions: 10.0.18362.0, 10.0.19041.0
  IDEs:
    Android Studio: Version  4.1.0.0 AI-201.8743.12.41.7199119
    Visual Studio: 16.10.31321.278 (Visual Studio Enterprise 2019)
  Languages:
    Java: javac 16
  npmPackages:
    @react-native-community/cli: Not Found
    react: 17.0.2 => 17.0.2
    react-native: 0.64.2 => 0.64.2
    react-native-windows: ^0.64.11 => 0.64.14
  npmGlobalPackages:
    *react-native*: Not Found

library version: 7.1.2

windows RN version: 0.64.11

vonovak commented 2 years ago

Hello, yes please, be so kind and create a PR.

There is some old folder picking logic there as well so feel free to clear it up if you want. I don't have windows so.. 🤷‍♂️😄 Thanks!

MacKenzieHnC commented 1 year ago

I'm happy to take this up. Just implementing it as-is is trivial. However, things get weird when you want access to specific files in Windows. As things are now, you can't read a file that's selected using this module if it's out-of-bounds.

EDIT: Looked into the file thing a little more, and there isn't an easy way to implement it. The way Windows handles file permissions is to create a string token which you need to store to reference the file later. So this repo and react-native-fs would both have to implement parts of it. Might be worth looking into since the Microsoft team has a PR up for a full overhaul over on that repo.

But for now, I'll just make the directory change a PR by itself

MacKenzieHnC commented 1 year ago

Okay, so it is slightly more complicated than that. The way directory permissions work in UWP (what React Native Windows compiles to) is that you have to add the folder to 1 of 2 locations: -There's a recently-used list, which stores 25 entries and automatically deletes the oldest entry when you add a 26th -And the Future Access List, which stores 1,000 entries and does not allow more entries once full without manually removing.

It seems to me that the smart way to handle this is to have 3 possibilities when opening a directory:

  1. Directory is not stored and permissions are left untouched.
  2. Directory is stored in recent access permissions.
  3. Directory is stored in Future Access List.

We should then also have options for removing entries from the lists. I'm not sure what the easiest way (for the React Native programmer who uses this repro) is, but just naively implementing the first suggestion here seems like a bad strategy. It would give the program the ability to retain more permission than it needs without any option for limiting that permission. Plus, the Future Access List does fill up eventually. It might be an extremely rare case, but it would be an extremely frustrating bug to pin down for whoever does run into it.

Perhaps we could just add a Windows-only flag to pick()? something like store-permissions: futureAccessList | recentlyUsed | undefined? Then, just add a function for removeLastPermission() and/or removeAllPermissions(). I have no idea how permissions work on Android or iOS though, so there could be a more unified way to handle this.

Also, it's worth noting that Windows makes a string token to refer to the folder or file when you add permissions. To remove an arbitrary entry in the list, you need pass it that token, so we would have to return it back to React. Likewise, to even access a file you saved permissions for (unless you already have permission for its containing directory), you need its token.

vonovak commented 1 year ago

@MacKenzieHnC thanks for your comments here and on other issues in this repo regarding windows support. As you probably noticed, the windows part of the repo is kinda unmaintained. I don't have the means to develop and test on windows so I'll be happy if you can contribute for windows! :)

Thank you

MacKenzieHnC commented 1 year ago

Definitely, I am offering to. I need some guidance on how you want me to proceed for the parts that would require js changes though. Unless you want me to just unilaterally make those decisions and post a PR. Which I can also do.

What I'm saying is that to make this work the way that makes the most sense would be a breaking change. It would require adding a handful of new js functions that would only have effects in Windows and changing the return object of the picker to store a Windows-specific string so you could reference that later.

In the short-term, I'll just make a PR that adds in the simple functionality from the first comment in this thread, but I think it's a bad idea to leave it like that.

EDIT: actually, not a breaking change, but to use it would absolutely require different code for the end-user.

MacKenzieHnC commented 1 year ago

@vonovak Okay, yeah, I'm happy to fully adopt the windows side of this project. It's a good excuse to get some actual expertise in UWP.

I'm accustomed to the new module template though (where you run examples with yarn example whatever), so I need a second to figure out how to work in the old one.

My only real question is if we're just fully committed to this having react-native-fs as a "dependency". Like, for the save picker issue, we could do that, but since this module doesn't actually do file work, it would need to be given a file from a module that does.

EDIT: any chance you would be okay with refactoring to the modern template?

EDIT 2: After messing with the repo a lot yesterday, I think there's a bunch of missing stuff. Like, the windows example folder is almost completely empty and I'm pretty sure example is supposed to have its own package.json in order to run. I did manage to to get android working with the new template, and I'm working on windows. I can't test ios, though, so all I can do is copy stuff over and hope it works.