Arlodotexe / OwlCore.Storage.FluentFTP

An implementation of OwlCore.Storage that works with FTP.
MIT License
1 stars 0 forks source link

Read only file/folder implementation #2

Closed itsWindows11 closed 3 weeks ago

itsWindows11 commented 3 months ago

Currently we only have an implementation fully compatible with read/write FTP files/folders. We will have to check the item permissions to see if it's read only.

On my own IIS-powered local FTP server, permission checks do not seem to work properly, a read-only file doesn't give any permissions (None), same for read-write files.

One workaround is to try and see if we can append to the file, but the extra roundtrips needed to do this will slow down performance especially on slow networks.

Arlodotexe commented 3 months ago

I did some digging through the FluentFTP repo and couldn't find any obvious documentation or code locations that would specify a connection is "read-only". The closest thing seems to be the idea of anonymous connections, but this is only one way that FTP might be read-only, with individual directory permissions being the other.

Generally, this is analogous to having both individual and root/drive level access permissions in the inbox SystemFile. Since we haven't explicitly addressed and documented how to handle this scenario, let's do that now.

Gathering existing established conventions for this library from past discussions:

From https://github.com/Arlodotexe/OwlCore.Storage/issues/37#issuecomment-1658828466:

Our API dictates the resource should exist with reasonable confidence on construction so that when you try to access the file and use the object, it doesn't throw.

From https://github.com/Arlodotexe/OwlCore.Storage/pull/56#pullrequestreview-2131480054:

A file or folder resource should exist before it's accessed through our abstraction. [...] Instances that are created internally can pre-validate this and skip the consumer-facing validation/guard.

From this discussion in the UWP Community Discord:

Permissions and authentication should always be settled by the underlying API before (when async, like StorageFile/UAC/FTP/OneDrive/etc) or during (when synchronous, like zip passwords) creation of a folder or file instance.

For authenticated resources, you'll have specific exceptions thrown from the underlying library if the permissions are unexpectedly revoked. If that happens, just catch the exception at the application-side callsite to the storage abstraction, refresh permissions, and rerun.

Remember that recursive operations are implemented by your applications and not this library.

I believe these comments have everything we need to pick a direction here.

Carefully leaving recursive enumeration to the consuming application means that the responsibility of authentication falls to the consumer prior to construction, following the same rules we've established for other authenticated resources.

This means we shouldn't need to change SystemFile or SystemFolder to account for these scenarios. The next step is to figure out how these translate to the FTP protocol, and how FluentFTP handles those things.

itsWindows11 commented 3 months ago

couldn't find any obvious documentation or code locations that would specify a connection is "read-only". The closest thing seems to be the idea of anonymous connections, but this is only one way that FTP might be read-only, with individual directory permissions being the other.

Anonymous connections can also have R/W permission, mostly in the case of having a test server so this isn't a reliable way to check for it.

Permissions and authentication should always be settled by the underlying API before (when async, like StorageFile/UAC/FTP/OneDrive/etc) or during (when synchronous, like zip passwords) creation of a folder or file instance.

Main problem here is you have to distinguish between R/O & R/W folders & files during enumeration. I think for other operations i.e. copying and moving this approach is okay and we don't have to do anything on this part.

There don't seem to be any issues reported regarding permission checks so no idea if it's just an issue on my end.

Arlodotexe commented 3 months ago

Anonymous connections can also have R/W permission, mostly in the case of having a test server so this isn't a reliable way to check for it.

Good point, noted.

Regarding this, I think the analysis above holds everything we need to clarify how to handle it:

Main problem here is you have to distinguish between R/O & R/W folders & files during enumeration. I think for other operations i.e. copying and moving this approach is okay and we don't have to do anything on this part.

When we say that

Our API dictates the resource should exist with reasonable confidence on construction so that when you try to access the file and use the object, it doesn't throw.

and

Instances that are created internally can pre-validate this and skip the consumer-facing validation/guard.

We're also implying that distinguishing between "read-only" and "modifiable" can just be the responsibility of the consumer, it doesn't need to happen dynamically at runtime. They should know whether they have the ability to modify or not, and like validation, that assumption can be trickled down to child instances.

This is what we mean by highlighting:

Permissions and authentication should always be settled by the underlying API before (when async, like StorageFile/UAC/FTP/OneDrive/etc) or during (when synchronous, like zip passwords) creation of a folder or file instance.

For authenticated resources, you'll have specific exceptions thrown from the underlying library if the permissions are unexpectedly revoked. If that happens, just catch the exception at the application-side callsite to the storage abstraction, refresh permissions, and rerun.

Remember that recursive operations are implemented by your applications and not this library.

It's important to think about this because SystemFile and SystemFolder are simple and rather fitting analogies where this hasn't been fully explored. Any problems we solve or improvements we make in this space for FTP will translate to all other implementations which have a permissions system.

It seems like we're fully covered on the basics, though. If you run into a permission issue that can't be resolved during interaction with the storage API, you can fall back to a read-only implementation or (as with SystemFile since there's no ReadOnlySystemFile) just avoid calling those methods entirely.

itsWindows11 commented 3 weeks ago

To some degree this issue/concern is resolved. It'll be the consumer's responsibility to check if the filesystem is read only or not.

Another set of tests & env variables will be added for read-only filesystems specifically where folders & files are assumed to exist.