RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi Alan,

I now found time to get back to the POSIX file permissions in zipfs. The goal would be to get it done for JDK13 ��

I went through your mail in details, please see my comments:

> 1. By default, a zip file system will have no support for the "posix"
> and "owner" views of file attributes, this means the following will fail
> with UOE:
> 
>    PosixFileAttributes attrs = Files.readAttributes(file,
> PosixFileAttributes.class);
> 
> and the zip file system's supportedFileAttributView method will not
> include "posix".

Correct.

> 2. Creating a zip file system with configuration parameter XXX set to
> "true" will create the zip file system that supports
> PosixFileAttributeView (and FileOwnerAttributeView). The current patch
> names the configuration parameter "posix". That name might be misleading
> as a configuration parameter as it conveys more than it actually does.
> Something like "enablePosixFileAttributes" clear conveys that it enables
> support for POSIX file attribute but might be a better wordy.

OK. I've changed to "enablePosixFileAttributes" in my new webrev.

> The value in the configuration map is documented as Boolean but I think
> we are allowing it to be the string representation of a Boolean, meaning
> it can be "true" or "false" like we have with the "create" parameter.

I fixed the doc.

> 3. The map of configurations parameters can optionally include the
> parameters "defaultOwner", "defaultGroup", and "defaultPermissions" with
> the default owner/group/permissions to return. These names seem okay to
> me.
> 
> For the owner/group, the parameter type can be
> UserPrincipal/GroupPrincipal or strings. If the value is a String then
> we need to decide if there is any validation. If I read the current
> patch correctly then it doesn't do any validation - that might be okay
> but minimally maybe we should disallow the empty string.

I added checking for the empty string in my new webrev.

> If the defaultXXX parameters aren't present then the zip file
> owner/group would be a sensible default. If the zip file is on a file
> store that supports FileOwnerAttributeView then we've got the owner. If
> the file store supports PosixFileAttributeView then we have a group
> owner. If PosixFileAttributeView is not supported then using the owner
> as the group is okay. I see the current patch as a fallback to fallback
> to "<zipfs_default>" but I'd prefer not have that because it will be
> very confusing to diagnose if there are issues.

Ok, but what shall we do then in the case of cases? That is, if the file store does not support the FileOwnerAttributeView or security permissions don't allow us to access them? Shall we fail, e.g. throw an IOException upon attempting to create a zipfs?

> For defaultPermissions, the value is a Set<PosixFilePermissions> or the
> string representation. In the implementation, initPermissions can be
> changed to use PosixFilePermissions.fromString as it does the
> translation that we need.

I don't see where it still needs to be changed. It's using PosixFilePermissions.fromString already.

> 4. As an alternative to the type safe access that PosixFileAttributeView
> provides, the proposal is to document the "zip" view of file attributes.
> Initially, it will admit to one file attribute for the permissions. The
> current patch uses the name "storedPermissions" but it might be simpler
> to use "permissions" so that "zip:permissions" and "posix:permissions"
> are the same when the zip entry has permissions.

There's both, "storedPermissions" and "permissions. "storedPermissions" was chosen deliberately and has a different semantical meaning than "permissions". The former one will reflect the actual permission value on a zip entry, e.g. it can be null. The latter one will use the default, in case no permission information is associated with the zip entry.

> With this support you can write code like this:
>      Set<PosixFilePermission> perms = (Set<PosixFilePermission>)
> Files.getAttribute(file, "zip:permissions");
> 
> The cast is ugly of course but that's the consequence of not exposing
> ZipFileAttributes in the API so there is no type token to specify on the
> RHS.

That should work with the proposed patch.

> Documenting the "zip" view brings up a few issues, e.g. will
> Files.readAttributes(file, "zip:*") reveal more than "permissions". For
> this API it is okay for the map to attributes beyond the specified list.

I guess we should reveal all attributes of a zip view in the documentation.

> 5. There will be no support for specifying as POSIX FileAttribute to
> createFile or createDirectory to atomically set the permissions when
> creating a new file/directory, UOE will thrown as is is now.

Nope. It will work with the current state of the patch.

Here is an updated webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.8/

What's next?

Thanks
Christoph