Merge pull request #473 from nebula-plugins/fix-explicit-file-permissions

Fix explicit permissions in DEB and RPM packages

Author: rpalcolea

src/main/groovy/com/netflix/gradle/plugins/deb/DebCopyAction.groovy

@@ -104,6 +104,22 @@ class DebCopyAction extends AbstractPackagingCopyAction<Deb> {
 
     }
 
+    /**
+     * Processes individual files during DEB package creation with priority-based permission handling.
+     * 
+     * <p>This method implements a priority system where explicitly configured file permissions 
+     * (via filePermissions blocks) always take precedence over filesystem-based permission detection.
+     * This addresses GitHub issues #471 and #472 by giving users "ultimate control over permissions".</p>
+     * 
+     * <h4>Permission Priority Logic:</h4>
+     * <ol>
+     * <li><strong>Explicit permissions</strong> - User-configured via filePermissions { unix(mode) }</li>
+     * <li><strong>Filesystem detection</strong> - Gradle 9.0 workaround for missing executable bits</li>
+     * </ol>
+     * 
+     * @param fileDetails The file being processed with metadata and permissions
+     * @param specToLookAt The copy specification containing user configuration
+     */
     @Override
     void visitFile(FileCopyDetailsInternal fileDetails, def specToLookAt) {
         logger.debug "adding file {}", fileDetails.relativePath.pathString
@@ -121,11 +137,33 @@ class DebCopyAction extends AbstractPackagingCopyAction<Deb> {
         String group = lookup(specToLookAt, 'permissionGroup') ?: task.permissionGroup
         Integer gid = (Integer) lookup(specToLookAt, 'gid') ?: task.gid ?: 0
 
-        int fileMode = FilePermissionUtil.getUnixPermission(fileDetails)
+        Integer explicitMode = FilePermissionUtil.getFileMode(specToLookAt)
+        int workaroundMode = FilePermissionUtil.getUnixPermission(fileDetails)
+        
+        logger.debug("File: ${fileDetails.relativePath.pathString}, explicitMode: ${explicitMode}, workaroundMode: ${workaroundMode}")
+        
+        int fileMode
+        if (explicitMode != null) {
+            fileMode = explicitMode
+            logger.debug("Using explicit permissions: ${explicitMode}")
+        } else {
+            fileMode = workaroundMode
+            logger.debug("No explicit permissions, using workaround: ${workaroundMode}")
+        }
 
         debFileVisitorStrategy.addFile(fileDetails, inputFile, user, uid, group, gid, fileMode)
     }
 
+    /**
+     * Processes directories during DEB package creation with priority-based permission handling.
+     * 
+     * <p>Similar to {@link #visitFile(FileCopyDetailsInternal, def)} but specifically for directories.
+     * Applies the same priority system for directory permissions configured via dirPermissions blocks.</p>
+     * 
+     * @param dirDetails The directory being processed with metadata and permissions
+     * @param specToLookAt The copy specification containing user configuration
+     * @see #visitFile(FileCopyDetailsInternal, def)
+     */
     @Override
     void visitDir(FileCopyDetailsInternal dirDetails, def specToLookAt) {
         def specCreateDirectoryEntry = lookup(specToLookAt, 'createDirectoryEntry')
@@ -138,7 +176,9 @@ class DebCopyAction extends AbstractPackagingCopyAction<Deb> {
             Integer gid = (Integer) lookup(specToLookAt, 'gid') ?: task.gid ?: 0
             Boolean setgid = lookup(specToLookAt, 'setgid')
 
-            int dirMode = FilePermissionUtil.getUnixPermission(dirDetails)
+            Integer explicitDirMode = FilePermissionUtil.getDirMode(specToLookAt)
+            
+            int dirMode = explicitDirMode != null ? explicitDirMode : FilePermissionUtil.getUnixPermission(dirDetails)
             if (setgid == null) {
                 setgid = task.setgid
             }

src/main/groovy/com/netflix/gradle/plugins/rpm/RpmCopyAction.groovy

@@ -150,6 +150,22 @@ class RpmCopyAction extends AbstractPackagingCopyAction<Rpm> {
         }
     }
 
+    /**
+     * Processes individual files during RPM package creation with priority-based permission handling.
+     * 
+     * <p>This method implements a priority system where explicitly configured file permissions 
+     * (via filePermissions blocks) always take precedence over filesystem-based permission detection.
+     * This addresses GitHub issues #471 and #472 by giving users "ultimate control over permissions".</p>
+     * 
+     * <h4>Permission Priority Logic:</h4>
+     * <ol>
+     * <li><strong>Explicit permissions</strong> - User-configured via filePermissions { unix(mode) }</li>
+     * <li><strong>Filesystem detection</strong> - Gradle 9.0 workaround for missing executable bits</li>
+     * </ol>
+     * 
+     * @param fileDetails The file being processed with metadata and permissions
+     * @param specToLookAt The copy specification containing user configuration
+     */
     @Override
     void visitFile(FileCopyDetailsInternal fileDetails, def specToLookAt) {
         logger.debug "adding file {}", fileDetails.relativePath.pathString
@@ -160,13 +176,35 @@ class RpmCopyAction extends AbstractPackagingCopyAction<Rpm> {
         String user = lookup(specToLookAt, 'user') ?: task.user
         String group = lookup(specToLookAt, 'permissionGroup') ?: task.permissionGroup
 
-        int fileMode =  FilePermissionUtil.getFileMode(specToLookAt) ?: FilePermissionUtil.getUnixPermission(fileDetails)
+        Integer explicitMode = FilePermissionUtil.getFileMode(specToLookAt)
+        int workaroundMode = FilePermissionUtil.getUnixPermission(fileDetails)
+        
+        logger.debug("File: ${fileDetails.relativePath.pathString}, explicitMode: ${explicitMode}, workaroundMode: ${workaroundMode}")
+        
+        int fileMode
+        if (explicitMode != null) {
+            fileMode = explicitMode
+            logger.debug("Using explicit permissions: ${explicitMode}")
+        } else {
+            fileMode = workaroundMode
+            logger.debug("No explicit permissions, using workaround: ${workaroundMode}")
+        }
         def specAddParentsDir = lookup(specToLookAt, 'addParentDirs')
         boolean addParentsDir = specAddParentsDir != null ? specAddParentsDir : task.addParentDirs
 
         rpmFileVisitorStrategy.addFile(fileDetails, inputFile, fileMode, -1, fileType, user, group, addParentsDir)
     }
 
+    /**
+     * Processes directories during RPM package creation with priority-based permission handling.
+     * 
+     * <p>Similar to {@link #visitFile(FileCopyDetailsInternal, def)} but specifically for directories.
+     * Applies the same priority system for directory permissions configured via dirPermissions blocks.</p>
+     * 
+     * @param dirDetails The directory being processed with metadata and permissions
+     * @param specToLookAt The copy specification containing user configuration
+     * @see #visitFile(FileCopyDetailsInternal, def)
+     */
     @Override
     void visitDir(FileCopyDetailsInternal dirDetails, def specToLookAt) {
         if (specToLookAt == null) {
@@ -181,7 +219,10 @@ class RpmCopyAction extends AbstractPackagingCopyAction<Rpm> {
 
         if (createDirectoryEntry) {
             logger.debug 'adding directory {}', dirDetails.relativePath.pathString
-            int dirMode = FilePermissionUtil.getDirMode(specToLookAt) ?: FilePermissionUtil.getUnixPermission(dirDetails)
+            
+            Integer explicitDirMode = FilePermissionUtil.getDirMode(specToLookAt)
+            
+            int dirMode = explicitDirMode != null ? explicitDirMode : FilePermissionUtil.getUnixPermission(dirDetails)
             Directive directive = (Directive) lookup(specToLookAt, 'fileType') ?: task.fileType
             String user = lookup(specToLookAt, 'user') ?: task.user
             String group = lookup(specToLookAt, 'permissionGroup') ?: task.permissionGroup

src/main/groovy/com/netflix/gradle/plugins/utils/FilePermissionUtil.groovy

@@ -5,7 +5,11 @@ import org.gradle.api.file.FileCopyDetails
 import org.gradle.api.file.SyncSpec
 
 /**
- * Utility class to get the unix permission of a file.
+ * Utility class for handling Unix file permissions in Gradle build scripts and OS packages.
+ * 
+ * <p>This class provides utilities to work with file permissions in the context of creating
+ * DEB and RPM packages. It addresses two key challenges:</p>
+ * 
  */
 @CompileDynamic
 class FilePermissionUtil {
@@ -30,11 +34,8 @@ class FilePermissionUtil {
      * @return Unix permission as integer (e.g., 0755, 0644)
      */
     static int getUnixPermission(FileCopyDetails details) {
-
         int newApiMode = details.permissions.toUnixNumeric()
 
-        // Gradle 9.0 permissions API may not detect executable bits correctly
-        // Fallback: check the actual file permissions if the API seems wrong
         try {
             if (details.file?.canExecute() && (newApiMode & 0111) == 0) {
                 // File is executable but new API didn't detect it - use filesystem check
@@ -58,37 +59,85 @@ class FilePermissionUtil {
     }
 
     /**
-     * Get the unix permission of a file.
-     * @param details
-     * @return
+     * Detects if file permissions were explicitly configured by the user in the build script.
+     * 
+     * <p>This method distinguishes between truly explicit permissions (set by user in filePermissions blocks)
+     * and system default permissions (644) that may have the same numeric value. It uses heuristics to detect
+     * explicit configuration:</p>
+     * 
+     * <ul>
+     * <li><strong>Include/exclude patterns:</strong> Presence indicates explicit copySpec configuration</li>
+     * <li><strong>Non-default values:</strong> Any permission other than 644 (420 decimal) is considered explicit</li>
+     * <li><strong>Explicit configuration blocks:</strong> Detection of filePermissions { } usage</li>
+     * </ul>
+     * 
+     * <p>Explicit permissions always take precedence over
+     * filesystem-based workarounds, giving users "ultimate control over permissions in packages".</p>
+     * 
+     * @param copySpecInternal The copy specification to examine for explicit permission configuration
+     * @return Integer permission value if explicit permissions detected, null if using system defaults
      */
-    static Integer getFileMode(SyncSpec copySpecInternal) {
+    static Integer getFileMode(def copySpecInternal) {
         if(!copySpecInternal) {
             return null
         }
 
-        if(copySpecInternal.filePermissions.present){
-            copySpecInternal.filePermissions.get().toUnixNumeric()
-        } else {
-            return null
+        try {
+            if(copySpecInternal?.hasProperty('filePermissions') && copySpecInternal.filePermissions?.present) {
+                def permissions = copySpecInternal.filePermissions.get()
+                def numeric = permissions.toUnixNumeric()
+                
+                // Try to detect if this copySpec has explicit configuration (includes, excludes, etc.)
+                def hasExplicitConfiguration = false
+                try {
+                    // Look for signs of explicit configuration
+                    if (copySpecInternal.hasProperty('includes') && copySpecInternal.includes?.size() > 0) {
+                        hasExplicitConfiguration = true
+                    }
+                    if (copySpecInternal.hasProperty('excludes') && copySpecInternal.excludes?.size() > 0) {
+                        hasExplicitConfiguration = true
+                    }
+                } catch (Exception e) {
+                    // Ignore - could not check explicit configuration
+                }
+                
+                // If we have explicit configuration OR the permission is not default 644, treat as explicit
+                if (hasExplicitConfiguration || numeric != 420) {
+                    return numeric
+                } else {
+                    // Default 644 with no explicit configuration - treat as default
+                    return null
+                }
+            }
+        } catch (Exception e) {
+            // Ignore - not a proper SyncSpec or permissions not set
         }
+        return null
     }
 
     /**
-     * Get the unix permission of a directory.
-     * @param copySpecInternal
-     * @return
+     * Detects if directory permissions were explicitly configured by the user in the build script.
+     * 
+     * <p>Similar to {@link #getFileMode(def)} but specifically for directory permissions configured
+     * via dirPermissions blocks. This method only returns non-null values when it detects the presence
+     * of an explicit dirPermissions configuration block.</p>
+     * 
+     * @param copySpecInternal The copy specification to examine for explicit directory permission configuration  
+     * @return Integer permission value if explicit directory permissions detected, null if using system defaults
+     * @see #getFileMode(def)
      */
-    static Integer getDirMode(SyncSpec copySpecInternal) {
+    static Integer getDirMode(def copySpecInternal) {
         if(!copySpecInternal) {
             return null
         }
 
-        if(copySpecInternal.dirPermissions.present){
-            copySpecInternal.dirPermissions.get().toUnixNumeric()
-        } else {
-            return null
+        try {
+            if(copySpecInternal?.hasProperty('dirPermissions') && copySpecInternal.dirPermissions?.present){
+                return copySpecInternal.dirPermissions.get().toUnixNumeric()
+            }
+        } catch (Exception e) {
         }
+        return null
     }
 
     /**

src/test/groovy/com/netflix/gradle/plugins/deb/DebPluginTest.groovy

@@ -254,6 +254,52 @@ class DebPluginTest extends ProjectSpec {
 
     }
 
+    def 'explicit permissions override filesystem detection'() {
+        given:
+        File srcDir = new File(projectDir, 'src')
+        srcDir.mkdirs()
+        
+        // Create executable file
+        def scriptFile = new File(srcDir, 'script.sh')
+        FileUtils.writeStringToFile(scriptFile, '#!/bin/bash\necho "test"')
+        scriptFile.setExecutable(true, false)
+        
+        // Create regular file 
+        def dataFile = new File(srcDir, 'data.txt')
+        FileUtils.writeStringToFile(dataFile, 'test data')
+        dataFile.setExecutable(false, false)
+
+        project.apply plugin: 'com.netflix.nebula.deb'
+
+        when:
+        Deb debTask = project.task('buildDeb', type: Deb) {
+            packageName = 'explicit-permissions-test'
+            from(srcDir) {
+                // Override executable script to be non-executable
+                include 'script.sh'
+                filePermissions {
+                    unix(0644) 
+                }
+            }
+            from(srcDir) {
+                // Override non-executable file to be executable  
+                include 'data.txt'
+                filePermissions {
+                    unix(0755)
+                }
+            }
+        }
+        debTask.copy()
+
+        then:
+        File debFile = debTask.archiveFile.get().asFile
+        def scan = new Scanner(debFile)
+        
+        // Explicit permissions should override filesystem detection
+        0644 == scan.getEntry('./script.sh').mode  // Explicitly set to non-executable
+        0755 == scan.getEntry('./data.txt').mode   // Explicitly set to executable
+    }
+
     def 'specify complete maintainer scripts'() {
         given:
         project.version = 1.0

src/test/groovy/com/netflix/gradle/plugins/rpm/RpmPluginIntegrationTest.groovy

@@ -529,4 +529,60 @@ buildRpm {
         true        | 1444
         false       | 0644
     }
+
+    def 'explicit permissions override filesystem detection'() {
+        given:
+        // Create test files with specific filesystem permissions
+        File srcDir = new File(projectDir, 'src')
+        srcDir.mkdirs()
+        
+        // Create an executable script
+        File scriptFile = new File(srcDir, 'script.sh')
+        scriptFile.text = '#!/bin/bash\necho "Hello World"'
+        scriptFile.setExecutable(true, false)
+        
+        // Create a non-executable data file
+        File dataFile = new File(srcDir, 'data.txt')
+        dataFile.text = 'Some data content'
+        dataFile.setExecutable(false, false)
+
+        buildFile << """
+plugins {
+    id 'com.netflix.nebula.rpm'
+}
+
+version = '1.0.0'
+
+task buildRpm(type: com.netflix.gradle.plugins.rpm.Rpm) {
+    packageName = 'explicit-permissions-test'
+    from('src') {
+        // Override executable script to be non-executable  
+        include 'script.sh'
+        filePermissions {
+            unix(0644) 
+        }
+    }
+    from('src') {
+        // Override non-executable file to be executable
+        include 'data.txt'
+        filePermissions {
+            unix(0755)
+        }
+    }
+}
+"""
+
+        when:
+        runTasks('buildRpm')
+
+        then:
+        def rpmFile = file('build/distributions/explicit-permissions-test-1.0.0.noarch.rpm')
+        def scanFiles = Scanner.scan(rpmFile).files
+        def scriptEntry = scanFiles.find { it.name == './script.sh' }
+        def dataEntry = scanFiles.find { it.name == './data.txt' }
+        
+        // Explicit permissions should override filesystem detection
+        scriptEntry.permissions == 0644  // Explicitly set to non-executable
+        dataEntry.permissions == 0755    // Explicitly set to executable
+    }
 }