Community
Participate
Working Groups
For unix-like files, I would like to see "file permissions" and "owner" properties added. This support should be added to: 1. Info Property page and Properties sheet: I should be able to view file permissions and change them (i.e. 'chmod' functionality). I should also be able to view the owner of the file and modify that property ('chown' functionality). 2. Remote System Details table view: Add new columns to display this functionality. 3. Access to these properties through APIs.
Some related bugs: Bug #165171 for exposing file permissions and ownership through API. Bug #180507 for a dialog for changing file permissions.
There are a few things to consider with this one. 1) First, whether or not all services are able to support this. Can FTP do this? If a service doesn't support permissions and owner properties, then we'll have to account for the differences in the UI. 2) For dstore, this will effect compatability with older releases since we would need to store different info in DataElements and we would to workout a scheme that support the older versions as well as the newer. 3) What commands should we use on the remote system to determine permissions and ownership. Will different unixes have different commands or formats? 4) Another thing to consider is whether this would effect performance - for dstore, we might gather this info as part of file classification, such that the info comes back asynchronously after the initial query.
Created attachment 86557 [details] potential service interfaces permissions and ownership I've attached a patch with potential service APIs that could be used for retrieving and modifying permissions and ownership. A service implementation, such as the DStoreFileService, could implement these interfaces to provide support for the permissions and ownership, while services that are unable to provide this support would not. Dave Dykstal, suggested that we provide a mechanism to adapt to the permissions and ownership interfaces and use that to determine (1) whether these features are supported, and if so (2) the underlying service implementations. Any thoughts, issues or questions about these interfaces?
I'd also prefer adapting to the new interfaces rather than implementing them. When you do that, you'll probably need to pass the original IFileService as a parameter into the methods, such that the adaptor knows what it should adapt to. The IHostFilePermissions interface seems like an overkill to me -- just aggregating 9 booleans can also be done with a single integer plus some bitmask constants, and is easier to extend in a binary compatible way like that. The typical UNIX ugo=rwxrwxrwx bitfield should be just fine. I think I'd like to see canSetFilePermissions() and canSetFileOwner() methods because often the service has a chance to determine whether the call has any chance to succeed -- like if the file is owned by somebody else. Corresponding UI actions should be disabled in that case, perhaps with a tooltip telling the reason why it's disabled.
(In reply to comment #4) > The IHostFilePermissions interface seems like an overkill to me -- just > aggregating 9 booleans can also be done with a single integer plus some bitmask > constants, and is easier to extend in a binary compatible way like that. The > typical UNIX ugo=rwxrwxrwx bitfield should be just fine. I thought about that originally but then I figured we would want the interface to be very clear and readable for easier consumption. The implementation could take case of bitfields as suggested but shouldn't the interface be as obvious as possible?
This API is for programmers. They should be capable of dealing with bitfields. If you want to simplify it, you could do something like in IFileInfo: /** * Set or reset all the permission bits from the given bitmask. * Example: setPermission(PERM_USER_WRITE | PERM_GROUP_WRITE, true); */ public void setPermission(int bitmask, boolean value); /** * Test if any of the permission bits from the bitmask are set. * Example: getPermission(PERM_USER_WRITE | PERM_GROUP_WRITE) */ public boolean getPermission(int bitmask); public int getPermissionBits(); public void setPermissionBits(int bits); The main advantage of such a kind of API is that if you add permission bits in the future (e.g. PERM_SETUID) you can do so without changing the interface signature just by adding the new constant. Since the new constant is inlined, clients which use the new API will still be binary compatible against old versions of the framework. But there are more advantages: * Simpler code for changing / reading multiple perm bits at once * Supports constants like PERM_ANY_WRITE, PERM_ANY_EXECUTE which check multiple bits at once If you really want to have a generic API that should also hold in the future for yet unknown kinds of permission schemes, you could also add a "Properties" like interface: public String getPermissionAttribute(String attributeName); public void setPermissionAttribute(String attributeName, String value); For WinNT ACL's this could be used like String allowedUsers = getPermissionAttribute("aclRead"); But I'm unsure if this is the right thing to do at the moment.
Adding Bug #165171 and bug #180507 as blocked by this one, since this bug is for UNIX-like file systems specifically while the others are for more generic cases. We'll see what to do with those other bugs once this one is resolved.
Created attachment 86578 [details] updated patch of potential service inferfaces permissions and ownership Here's an updated version of the permissions stuff, including an AbstractHostFilePermissions implementation. How does this look?
1.) Why do you expose set(), clear(), _permissions as "protected" API? This unnecessarily exposes implementation details. 2.) Why is AbstractHostFilePermissions abstract? It looks like a complete implementation to me. Could also be HostFilePermissions, perhaps with a constructor taking intial "int mask". 3.) The public static int constants are missing in the interface PERM_USER_... 4.) I'd like to see canSet... API.
Also, please update your Copyright template to use year 2008 for new files.
(In reply to comment #9) > 1.) Why do you expose set(), clear(), _permissions as "protected" API? > This unnecessarily exposes implementation details. > 2.) Why is AbstractHostFilePermissions abstract? It looks like a complete > implementation to me. Could also be HostFilePermissions, perhaps with > a constructor taking intial "int mask". I was anticipating that each service may implement this slightly differently. We do it for IHostFile so I thought it may be needed for the IHostFilePermissions too. For example, dstore may create a DStoreFilePermissions class with a DStoreFilePermissions(DataElement) constructor (the DataElement storing the bits as determined from the remote machine). That's why I exposed the methods as protected API. But perhaps, it's simple enough that we don't need to specialize it. > 3.) The public static int constants are missing in the interface PERM_USER_... I'll add the constants in my next patch update. > 4.) I'd like to see canSet... API. I'll add a canSet in my next patch update.
This interface and implementation are so simple that exposing protected API for overriding it partially makes no sense. If a Dstore implementation wants to implement the interface by holding a dataElement rather than the int bitmask, the methods and data fields that you're exposing as protected are of no use to it.
Created attachment 86584 [details] next patch iteraction of potential service apis In this patch, I've added the bit mask constants, taken out the abstract of the host file permissions implementation, added canSet APIs for the permissions service and the owner service and updated copyrights.
(In reply to comment #12) > This interface and implementation are so simple that exposing protected API for > overriding it partially makes no sense. > If a Dstore implementation wants to implement the interface by holding a > dataElement rather than the int bitmask, the methods and data fields that > you're exposing as protected are of no use to it. For dstore, the thought was to construct the bitmask constant from the element, but it's easy enough just to do that before constructing a concrete instance of HostFilePermissions.
1.) HostFilePermissions is Copyright (c) 20078 2.) The interface is small enough that you don't need @see with all the constants 3.) Please use those decimal values that typical UNIX machines also use: ugo=rwxrwxrwx PERM_OTHERS_EXECUTE = 1 << 0; PERM_OTHERS_WRITE = 1 << 1; PERM_OTHERS_READ = 1 << 2; ... This will make it easier on the commandline, because then you can do chmod 200 otherfile just taking the octal value for the permission bits
(In reply to comment #12) > This interface and implementation are so simple that exposing protected API for > overriding it partially makes no sense. > > If a Dstore implementation wants to implement the interface by holding a > dataElement rather than the int bitmask, the methods and data fields that > you're exposing as protected are of no use to it. > As a design note: I'm developing a taste for never using "protected". It is only of use when subclassing. Making data directly available to subclasses is almost never a good idea for a public API. I'd rather see protected used on a limited basis only for internal, controlled frameworks.
... which goes with the general advice of many OO gurus, that composition (aggregation) of classes is usually better than extending them (derivation).
Created attachment 86601 [details] updated patch Here's another update based on comment #15.
Created attachment 86697 [details] updated patch with dstore implementation I've updated the interfaces to include a canGet*() apis as well since the service may need to determine this at runtime (i.e. dstore would query the command descriptors to determine whether the server has this functionality). I've added a basic dstore implementation to the DStoreFileService and the UniversalFileSystemMiner.
Created attachment 86714 [details] updated patch with property page for exercising this I've changed the IFileOwnerService to have both a user owner and group owner. In this patch, I've also implemented a property page to exercise this function. Currently, it just worked via checking for the appropriate service interfaces on the IFileService. This will have to change to use something more adaptable but I wanted to put something out now to show the function.
Created attachment 86959 [details] patch with further evolution of the enhancement This includes the adapter factor for getting at the appropriate permissions service given an IRemoteFile. It also includes property sheet and table view column support for permissions, user owner and group. In order to avoid performance and locking issues when getting the property sheet values for these new fields, we schedule a job, store results in an IRemoteFile, and then fire an property update notification.
Created attachment 87427 [details] patch to consolidate permissions and initial get support for ftp and ssh
I've provided another patch which alters this stuff a bit: 1) Consolidation of permissions services The IFilePermissionsService and the IFileOwnerService are consolidated into one IFilePermissionsService. IFileService is expected to implement this if the service is supported. The group and owner are now stored as part of the IHostFilePermissions. 2) IHostFilePermissionsContainer I've added IHostFilePermissionsContainer for IHostFile to implement when caching of IHostFilePermissions is desired. This provides an API for getPermissions() and setPermissions(IHostFilePermissions) for an IHostFile. The reason I didn't add these apis directly to IHostFile is that some implementations, like local, will not have this support and I didn't want to force this on the services. 3) DStore impl I've changed the dstore implementation to the getting (and setting) of permissions, user and group in one shot, rather than as separate queries for performance purposes and because they logically belong together. 4) FTP impl I've added get support for the FTP service. The set support still needs to be implemented. 5) SSH impl I've added get support for the SSH service. Currently it's storing the uid and gid for the user and group. I'm not sure whether there's an easy way, via the SSH service to translate those into the proper strings. The set support also still needs to be implemented.
(In reply to comment #23) 1) I like the consolidation of permissions and ownership, since they really belong together. The one thing that I find problematic is canSetFilePermissions(IHostFile file) because on many systems, you may be able to change the permissions but not the ownership. That should be reflected in the UI by potentially disabling the controls for ownership change when this feature is not available. One way out of this dilemma might be an API like int IFileService#getCapabilities(IHostFile file) that returns a bitmask of constants regarding remote capabilities: FS_CAN_GET_OWNER FS_CAN_GET_GROUP FS_CAN_GET_PERMISSIONS FS_CAN_SET_OWNER FS_CAN_SET_GROUP FS_CAN_SET_PERMISSIONS that's easily extendable in a binary compatible way. When passing a "null" file, the API could return general capabilities of the service. 2) I do not like that the IHostFilePermissionsContainer allows setPermissions(). I really think that the IHostFile object should be immutable (such that clients cannot change it). The only way for a client to change the permissions on an IHostFile should be by changing it on the remote side and then re-querying the remote. If we allow changing the IHostFile locally we risk getting out of sync with what we really have on the remote. The remote side should be the only one who can modify the actual IHostFile. For the same reason, IHostFile#getPermissions() should be specified as API to always return a COPY of the internal IHostFilePermissions because the client is allowed to modify the IHostFilePermissions. One possible API could be interface IFilePermissionsService { IHostFile setPermissions(IHostFile file, IHostFilePermissions perms); }; returning the modified IHostFile in case of success, or throwing an exception (and leaving the original IHostFile unchanged) in case of an error. 3) I like the proposed implementations. I particularly like the ability to set and get permissions in ASCII form as a String, because such an API could easily account for different conventions of representing permission info on different OS's (note that specific permission queries can still be made through the associated bitmask API). In general, I think it's important that the Model (IHostFile) is kept separate from the Controller (IFileService) performing the actual operations. What I'm unsure about, is whether it's really necessary to keep IFilePermissionsService / IHostFilePermissionsContainer separate from the original IFileService / IHostFile. Given that we have abstract base classes for both, it might also be possible to have the new capabilities added to the Service interface.
Created attachment 87539 [details] updated patch with suggestions from martin I've updated the patch based on the suggestions: 1) I've changed the can* methods to use getcapabilities() 2) I've taken out the setPermissions (and leave that to the services to set pending permissions). Will look into checking a queue in the service layer to determine whether or not a new query is required. Currently there's a problem where the PendingHostFilePermissions is not set soon enough so duplicate queries are sent.
Since this is pretty much done I'm going to close this. Work still needs to be done if we wish to support modification of permissions for SSH and FTP files but I think each of those should be handled in separate bugs. For any other issues with this, I'd prefer to use a separate defect.
