Hi Christoph, in general your change looks good to me. I need some more time to think about the implications mentioned by Alan and Chris but for the time being please find some comments regarding your implementation below:
ZipFileAttributeView.java - can you please throw an AssertionError() for the default branch in the switch expression in the "ZipFileAttributeView.name()". ZipFileSystem.java - please also put @Override annotations on the method implementations from ZipFileAttributes (e.g. "compressedSize()", "crc()", etc...) and a comment at the place where the implementation of the "PosixFileAttributes" methods starts. ZipUtils.java - just a question: isn't it possible to reuse sun.nio.fs.UnixFileModeAttribute.toUnixMode() and the corresponding constants from sun.nio.fs.UnixConstants instead of redefining them here? You could export them from java.base to jdk.zipfs but the problem is probably that at least sun.nio.fs.UnixConstants is a generated class which only gets generated on Unix systems, right ? ZipFileAttributes.java - it seems a little odd that you've added "setPermissions()" to ZipFileAttributes. All the attribute classes (i.e. BasicFileAttributes, PosixFileAttributes) have no setters. Isn't it possible to implement "ZipFileAttributeView.setPermissions()" by calling "path.setPermissions()" (as this is done in "ZipFileAttributeView.setTimes()") and handle everything in ZipFileSyste (as this is done with "setTimes()"). Thanks, Volker On Mon, Nov 5, 2018 at 8:32 AM Langer, Christoph <christoph.lan...@sap.com> wrote: > > Hi, > > > > Ping. > > > > May I get reviews/substantial feedback on this zipfs enhancement? > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8213031 > > CSR: https://bugs.openjdk.java.net/browse/JDK-8213082 > > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.2/ > > > > Thanks > > Christoph > > > > From: Langer, Christoph > Sent: Montag, 29. Oktober 2018 15:55 > To: 'Alan Bateman' <alan.bate...@oracle.com>; core-libs-dev > <core-libs-dev@openjdk.java.net>; security-...@openjdk.java.net; Xueming Shen > <xueming.s...@oracle.com> > Cc: Volker Simonis <volker.simo...@gmail.com>; nio-dev > <nio-...@openjdk.java.net> > Subject: RE: RFR 8213031: (zipfs) Add support for POSIX file permissions > (was: Enhance jdk.nio.zipfs to support Posix File Permissions) > > > > Hi Alan, security guys, > > > > I’ve proposed a CSR for this change now: > https://bugs.openjdk.java.net/browse/JDK-8213082. > > > > I also updated the webrev, simplifying jdk.nio.zipfs.ZipUtils.permsFromFlags > and eliminating the WeakHashMap: > http://cr.openjdk.java.net/~clanger/webrevs/8213031.2/ > > > > Since I’ve decoupled the changes to java.util.zip and jartool from those in > jdk.zipfs, we’re discussing only jdk.zipfs here. > > > > The implication of my change is that when working with files backed by the > nio FileSystemProvider (java.nio.file.spi.FileSystemProvider) named “jar”, > which is the alias for zipfs, the files will support attributes of type > java.nio.file.attribute.PosixFilePermissions (“posix:permissions”). > > > > It basically means that some methods of java.nio.file.Files that would return > null or UnsupportedOperationException before would find an implementation now. > > > > Examples: > > https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/Files.html#getPosixFilePermissions(java.nio.file.Path,java.nio.file.LinkOption...) > > https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/Files.html#setPosixFilePermissions(java.nio.file.Path,java.util.Set) > > https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/Files.html#readAttributes(java.nio.file.Path,java.lang.Class,java.nio.file.LinkOption...) > > With class > https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/attribute/PosixFileAttributes.html > > https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/Files.html#getFileAttributeView(java.nio.file.Path,java.lang.Class,java.nio.file.LinkOption...) > > With class > https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/attribute/PosixFileAttributeView.html > > > > Thanks in advance for reviewing. > > > > Best regards > > Christoph > > > > > > From: Alan Bateman <alan.bate...@oracle.com> > Sent: Montag, 29. Oktober 2018 13:06 > To: Langer, Christoph <christoph.lan...@sap.com>; core-libs-dev > <core-libs-dev@openjdk.java.net>; security-...@openjdk.java.net; Xueming Shen > <xueming.s...@oracle.com> > Cc: Volker Simonis <volker.simo...@gmail.com>; Andrew Luo > <andrewluotechnolog...@outlook.com>; nio-dev <nio-...@openjdk.java.net> > Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions > (was: Enhance jdk.nio.zipfs to support Posix File Permissions) > > > > On 29/10/2018 09:26, Langer, Christoph wrote: > > : > > > > As per request from Alan, I’m adding security-dev to get a review from > security perspective. > > > > For security-dev then I think it would be better to write-up a summary of the > overall proposal and the implications for applications/libraries that use the > APIs and the jar tool. The security discussion points all relate to capture > and propagation of file permissions. > > -Alan
