45Drives / cockpit-file-sharing

A Cockpit plugin to easily manage samba and NFS file sharing.
GNU General Public License v3.0
556 stars 30 forks source link

Adding a second Windows ACLs option on samba shares #59

Closed baileyallison closed 1 year ago

baileyallison commented 2 years ago

Feature Request

Describe the request Currently when selecting the Windows ACLs option on a samba share it adds the following values:

map acl inherit = yes
acl_xattr:ignore system acls = yes
vfs objects = acl_xattr

Which is perfect, however with these options it is designed for solely Windows clients, not a mixed Windows/Linux/Possibly MacOS? clients due to the way it applies the ACLs

If we're to apply the following values:

map acl inherit = yes
vfs objects = acl_xattr

The Windows ACLs are able to be processed by both Windows and non-Windows clients, and is technically how we were setting up the shares prior to the typo within acl_xattr: ignore system acls = yes being fixed.

(Prior when the share would actually list the options as:

map acl inherit = yes
acl_xattr:ignore system acl = yes
vfs objects = acl_xattr

It would actually ignore the middle value as it seems samba stops checking for error handling and just makes the value null if there is an option with a : that has a junk value after it, for example read only:garbage = yes gets parsed correctly by testparm but does not actually change anything on samba, so we were essentially configuring it as if there were only the map acl inherit = yes, and vfs objects = acl_xattr options previously)

Both configurations are completely valid, and have their own use-cases, they're just 2 different ways to setup the same thing both with their own reasons to use.

TL;DR --

Would it be possible to leave the current Windows ACLs option as-is, and add a second option which only adds the following to the share and call it something like "Windows ACLs with Linux support" or something?

map acl inherit = yes
vfs objects = acl_xattr

Let me know if you need any more info

joshuaboud commented 1 year ago

just implemented, releasing shortly