From d9b9ba8d3e476e80a40c3d99f51dfe903d465dc3 Mon Sep 17 00:00:00 2001 From: csturiale Date: Fri, 10 Apr 2026 15:05:45 +0200 Subject: [PATCH] Fix: Handle disabled or uninstalled permissions gracefully This change improves robustness when dealing with permissions that are disabled or belong to uninstalled plugins. Previously, the plugin would fail to start if a configured permission was not available (e.g., from a plugin that was uninstalled). Now, it logs a warning and continues, ignoring the invalid permission. This prevents a broken configuration from taking down a Jenkins instance. Specifically, this commit: - Modifies `PermissionWrapper` to tolerate unknown or disabled permission IDs instead of throwing an exception. - Introduces `PermissionWrapper.isValid()` to check if a permission can be used for access control. - Filters out disabled permissions from being granted in the UI and from being applied to ACLs, improving compliance with SECURITY-3062. - Adds null checks in the `FolderBasedAuthorizationStrategy` constructor to improve compatibility with Jenkins Configuration as Code (JCasC). - Adds `csturiale` as a developer in `pom.xml`. --- pom.xml | 10 +++- ...erAuthorizationStrategyManagementLink.java | 2 + .../FolderBasedAuthorizationStrategy.java | 12 +++-- .../folderauth/acls/GlobalAclImpl.java | 5 +- .../folderauth/misc/PermissionWrapper.java | 53 ++++++++++++++++--- .../misc/PermissionWrapperTest.java | 16 ++++-- 6 files changed, 82 insertions(+), 16 deletions(-) diff --git a/pom.xml b/pom.xml index c019d42..0243e95 100644 --- a/pom.xml +++ b/pom.xml @@ -1,12 +1,13 @@ - + 4.0.0 org.jenkins-ci.plugins plugin 5.3 - + io.jenkins.plugins @@ -23,6 +24,11 @@ Abhyudaya Sharma sharmaabhyudaya@gmail.com + + + csturiale + + diff --git a/src/main/java/io/jenkins/plugins/folderauth/FolderAuthorizationStrategyManagementLink.java b/src/main/java/io/jenkins/plugins/folderauth/FolderAuthorizationStrategyManagementLink.java index 1f585be..10db359 100644 --- a/src/main/java/io/jenkins/plugins/folderauth/FolderAuthorizationStrategyManagementLink.java +++ b/src/main/java/io/jenkins/plugins/folderauth/FolderAuthorizationStrategyManagementLink.java @@ -386,6 +386,8 @@ public void doDeleteAgentRole(@QueryParameter(required = true) String roleName) static Set getSafePermissions(Set groups) { TreeSet safePermissions = new TreeSet<>(Permission.ID_COMPARATOR); groups.stream().map(PermissionGroup::getPermissions).forEach(safePermissions::addAll); + // SECURITY-3062: remove disabled permissions so they cannot be selected in the UI + safePermissions.removeIf(p -> !p.enabled); safePermissions.removeAll(PermissionWrapper.DANGEROUS_PERMISSIONS); return safePermissions; } diff --git a/src/main/java/io/jenkins/plugins/folderauth/FolderBasedAuthorizationStrategy.java b/src/main/java/io/jenkins/plugins/folderauth/FolderBasedAuthorizationStrategy.java index 56369d7..2501687 100644 --- a/src/main/java/io/jenkins/plugins/folderauth/FolderBasedAuthorizationStrategy.java +++ b/src/main/java/io/jenkins/plugins/folderauth/FolderBasedAuthorizationStrategy.java @@ -81,9 +81,10 @@ public class FolderBasedAuthorizationStrategy extends AuthorizationStrategy { @DataBoundConstructor public FolderBasedAuthorizationStrategy(Set globalRoles, Set folderRoles, Set agentRoles) { - this.agentRoles = new HashSet<>(agentRoles); - this.globalRoles = new HashSet<>(globalRoles); - this.folderRoles = new HashSet<>(folderRoles); + // Guard against null — JCasC passes null when a parameter is omitted from YAML + this.agentRoles = agentRoles != null ? new HashSet<>(agentRoles) : new HashSet<>(); + this.globalRoles = globalRoles != null ? new HashSet<>(globalRoles) : new HashSet<>(); + this.folderRoles = folderRoles != null ? new HashSet<>(folderRoles) : new HashSet<>(); // the sets above should NOT be modified. They are not Collections.unmodifiableSet() // because that complicates the serialized XML and add unnecessary nesting. @@ -270,7 +271,10 @@ private void updateGenericAcl(String fullName, ConcurrentHashMap globalRoles) { for (GlobalRole role : globalRoles) { Set impliedPermissions = ConcurrentHashMap.newKeySet(); - role.getPermissionsUnsorted().parallelStream().map(PermissionWrapper::getPermission).forEach(impliedPermissions::add); + role.getPermissionsUnsorted().parallelStream() + .filter(PermissionWrapper::isValid) + .map(PermissionWrapper::getPermission) + .forEach(impliedPermissions::add); role.getSids().parallelStream().forEach(sid -> { Set permissionsForSid = permissionList.get(sid); diff --git a/src/main/java/io/jenkins/plugins/folderauth/misc/PermissionWrapper.java b/src/main/java/io/jenkins/plugins/folderauth/misc/PermissionWrapper.java index 8f06fa9..77ecc0f 100644 --- a/src/main/java/io/jenkins/plugins/folderauth/misc/PermissionWrapper.java +++ b/src/main/java/io/jenkins/plugins/folderauth/misc/PermissionWrapper.java @@ -10,6 +10,8 @@ import java.util.Collections; import java.util.HashSet; import java.util.Set; +import java.util.logging.Level; +import java.util.logging.Logger; import java.util.stream.Collectors; import java.util.stream.Stream; import javax.annotation.ParametersAreNonnullByDefault; @@ -24,6 +26,7 @@ */ @ParametersAreNonnullByDefault public final class PermissionWrapper implements Comparable { + private static final Logger LOGGER = Logger.getLogger(PermissionWrapper.class.getName()); // should've been final but needs to be setup when the // object is deserialized from the XML config private transient Permission permission; @@ -49,6 +52,10 @@ public PermissionWrapper(String id) { } public String getId() { + // If the permission could not be resolved (unknown plugin/disabled), fall back to the raw stored id. + if (permission == null) { + return id; + } return String.format("%s/%s", permission.group.getId(), permission.name); } @@ -66,15 +73,22 @@ private Object readResolve() { } /** - * Get the permission corresponding to this {@link PermissionWrapper} - * - * @return the permission corresponding to this {@link PermissionWrapper} + * Get the permission corresponding to this {@link PermissionWrapper}. + * May return {@code null} when the permission ID references a plugin that is not installed + * or a permission that could not be resolved. Callers must handle the {@code null} case. */ - @NonNull public Permission getPermission() { return permission; } + /** + * Returns {@code true} if this wrapper holds a valid, enabled permission. + * Wrappers for which this returns {@code false} should not be used for access-control decisions. + */ + public boolean isValid() { + return permission != null && permission.enabled; + } + @Override public boolean equals(Object o) { @@ -92,13 +106,36 @@ public int hashCode() { /** * Checks if the permission for this {@link PermissionWrapper} is valid. * - * @throws IllegalArgumentException when the permission did not exist, was null or was dangerous. + *

Behaviour by case: + *

    + *
  • Dangerous permissions ({@link #DANGEROUS_PERMISSIONS}) – always rejected with an + * {@link IllegalArgumentException} regardless of context.
    • + *
  • Unknown permissions ({@code permission == null}) – a WARNING is logged and the + * wrapper is kept with a {@code null} internal permission. This allows Jenkins to start up + * even when a referenced permission belongs to a plugin that is not currently installed.
    • + *
  • Disabled permissions ({@code !permission.enabled}) – a WARNING is logged. + * SECURITY-3062 compliance is preserved via two mechanisms: the UI filters disabled + * permissions out ({@code getSafePermissions}) and Jenkins core refuses to honour them + * at access-check time ({@code AbstractACL.hasPermission}).
    • + *
*/ private void checkPermission() { if (permission == null) { - throw new IllegalArgumentException(Messages.PermissionWrapper_UnknownPermission(id)); + // Permission from an uninstalled/unavailable plugin – log and continue rather than crashing. + LOGGER.log(Level.WARNING, + "Permission ''{0}'' is unknown in this Jenkins installation (plugin may not be installed) " + + "and will have no effect.", id); } else if (DANGEROUS_PERMISSIONS.contains(permission)) { throw new IllegalArgumentException(Messages.PermissionWrapper_NoDangerousPermissions()); + } else if (!permission.enabled) { + // SECURITY-3062: disabled permissions cannot be granted via the UI (getSafePermissions filters + // them out) and Jenkins core itself refuses to honour them at access-check time. We log a + // WARNING here instead of throwing so that existing configurations that reference a permission + // which is disabled in the current environment (e.g. Credentials/UseItem, Job/WipeOut) do not + // prevent Jenkins from starting up. + LOGGER.log(Level.WARNING, + "Permission ''{0}'' is disabled in this Jenkins installation and will have no effect. " + + "Consider removing it from the folder-auth configuration.", id); } } @@ -134,6 +171,10 @@ private static Set _wrapPermissions(Stream stream @Override public int compareTo(@NonNull PermissionWrapper other) { + // Fall back to string comparison when either permission could not be resolved. + if (this.permission == null || other.permission == null) { + return this.id.compareTo(other.id); + } return Permission.ID_COMPARATOR.compare(this.permission, other.permission); } } diff --git a/src/test/java/io/jenkins/plugins/folderauth/misc/PermissionWrapperTest.java b/src/test/java/io/jenkins/plugins/folderauth/misc/PermissionWrapperTest.java index 89946ec..16bca0d 100644 --- a/src/test/java/io/jenkins/plugins/folderauth/misc/PermissionWrapperTest.java +++ b/src/test/java/io/jenkins/plugins/folderauth/misc/PermissionWrapperTest.java @@ -5,6 +5,9 @@ import org.junit.Test; import org.jvnet.hudson.test.JenkinsRule; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; + public class PermissionWrapperTest { @ClassRule public static JenkinsRule j = new JenkinsRule(); @@ -14,8 +17,15 @@ public void shouldNotAllowDangerousPermissions() { new PermissionWrapper(Jenkins.RUN_SCRIPTS.getId()); } - @Test(expected = IllegalArgumentException.class) - public void shouldNotAllowNullPermissions() { - new PermissionWrapper("this is not a permission id"); + /** + * Unknown permission IDs (e.g. from an uninstalled plugin) no longer throw. + * Instead the wrapper is created with a null internal permission and {@link PermissionWrapper#isValid()} + * returns {@code false}. Jenkins will not grant any access for such wrappers. + */ + @Test + public void unknownPermissionsAreAcceptedWithWarning() { + PermissionWrapper wrapper = new PermissionWrapper("this is not a permission id"); + assertNull("permission should be null for an unknown id", wrapper.getPermission()); + assertFalse("wrapper with unknown permission must not be valid", wrapper.isValid()); } }