From 5a4dc98f6962c1f742060d4294f49a82d9a1eeca Mon Sep 17 00:00:00 2001 From: rsandell Date: Tue, 17 Oct 2023 16:39:28 +0200 Subject: [PATCH] [SECURITY-2835] --- .../AutofilledNetworkConfiguration.java | 11 +++--- .../computeengine/ComputeEngineCloud.java | 36 ++++++++--------- .../computeengine/InstanceConfiguration.java | 39 +++++++------------ .../SharedVpcNetworkConfiguration.java | 7 ++-- .../computeengine/SshConfiguration.java | 8 +--- .../computeengine/WindowsConfiguration.java | 12 +----- .../SharedVpcNetworkConfigurationTest.java | 13 +++++-- 7 files changed, 54 insertions(+), 72 deletions(-) diff --git a/src/main/java/com/google/jenkins/plugins/computeengine/AutofilledNetworkConfiguration.java b/src/main/java/com/google/jenkins/plugins/computeengine/AutofilledNetworkConfiguration.java index b5e76d3d..b2040208 100644 --- a/src/main/java/com/google/jenkins/plugins/computeengine/AutofilledNetworkConfiguration.java +++ b/src/main/java/com/google/jenkins/plugins/computeengine/AutofilledNetworkConfiguration.java @@ -29,6 +29,7 @@ import java.util.logging.Level; import java.util.logging.Logger; import jenkins.model.Jenkins; +import org.apache.commons.lang.StringUtils; import org.kohsuke.stapler.AncestorInPath; import org.kohsuke.stapler.DataBoundConstructor; import org.kohsuke.stapler.QueryParameter; @@ -56,7 +57,7 @@ public ListBoxModel doFillNetworkItems( @AncestorInPath Jenkins context, @QueryParameter("projectId") @RelativePath("../..") final String projectId, @QueryParameter("credentialsId") @RelativePath("../..") final String credentialsId) { - checkPermissions(); + checkPermissions(Jenkins.get(), Jenkins.ADMINISTER); ListBoxModel items = new ListBoxModel(); items.add(""); @@ -78,8 +79,7 @@ public ListBoxModel doFillNetworkItems( } public FormValidation doCheckNetwork(@QueryParameter String value) { - checkPermissions(); - if (value.equals("")) { + if (StringUtils.isEmpty(value)) { return FormValidation.error("Please select a network..."); } return FormValidation.ok(); @@ -91,8 +91,8 @@ public ListBoxModel doFillSubnetworkItems( @QueryParameter("region") @RelativePath("..") final String region, @QueryParameter("projectId") @RelativePath("../..") final String projectId, @QueryParameter("credentialsId") @RelativePath("../..") final String credentialsId) { + checkPermissions(Jenkins.get(), Jenkins.ADMINISTER); ListBoxModel items = new ListBoxModel(); - checkPermissions(); if (Strings.isNullOrEmpty(region)) { return items; @@ -105,7 +105,7 @@ public ListBoxModel doFillSubnetworkItems( if (subnetworks.size() <= 1) { items.add(new ListBoxModel.Option("", "", false)); } - if (subnetworks.size() == 0) { + if (subnetworks.isEmpty()) { items.add(new ListBoxModel.Option("default", "default", true)); return items; } @@ -124,7 +124,6 @@ public ListBoxModel doFillSubnetworkItems( } public FormValidation doCheckSubnetwork(@QueryParameter String value) { - checkPermissions(); if (value.isEmpty()) { return FormValidation.error("Please select a subnetwork..."); } diff --git a/src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineCloud.java b/src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineCloud.java index 860d8854..e46187ff 100644 --- a/src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineCloud.java +++ b/src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineCloud.java @@ -30,15 +30,15 @@ import com.google.common.collect.ImmutableMap; import com.google.jenkins.plugins.computeengine.client.ClientUtil; import com.google.jenkins.plugins.credentials.oauth.GoogleOAuth2Credentials; +import edu.umd.cs.findbugs.annotations.NonNull; import hudson.Extension; import hudson.model.Computer; import hudson.model.Descriptor; -import hudson.model.Item; -import hudson.model.Job; import hudson.model.Label; import hudson.model.Node; import hudson.model.TaskListener; import hudson.security.ACL; +import hudson.security.AccessControlled; import hudson.security.Permission; import hudson.slaves.AbstractCloudImpl; import hudson.slaves.Cloud; @@ -423,7 +423,7 @@ public InstanceConfiguration getInstanceConfigurationByDescription(String descri @RequirePOST public HttpResponse doProvision(@QueryParameter String configuration) throws ServletException, IOException { - checkPermissions(PROVISION); + checkPermissions(this, PROVISION); if (configuration == null) { throw HttpResponses.error(SC_BAD_REQUEST, "The 'configuration' query parameter is missing"); } @@ -442,19 +442,21 @@ public HttpResponse doProvision(@QueryParameter String configuration) /** * Ensures the executing user has the specified permissions. * - * @param permissions The list of permissions to be checked. If empty, defaults to Job.CONFIGURE. + * @param context The context on which to check the permissions, if null defaults + * to {@link Jenkins} And if {@link Jenkins} is also null then no permission + * checks will be performed. + * @param permissions The list of permissions to be checked. If empty, defaults to Jenkins.ADMINISTER. * @throws AccessDeniedException If the user lacks the proper permissions. + * @see Jenkins#get() */ - static void checkPermissions(Permission... permissions) { - Jenkins jenkins = Jenkins.getInstanceOrNull(); - if (jenkins != null) { - if (permissions.length > 0) { - for (Permission permission : permissions) { - jenkins.checkPermission(permission); - } - } else { - jenkins.checkPermission(Job.CONFIGURE); + static void checkPermissions(@NonNull AccessControlled context, Permission... permissions) { + + if (permissions.length > 0) { + for (Permission permission : permissions) { + context.checkPermission(permission); } + } else { + context.checkPermission(Jenkins.ADMINISTER); } } @@ -468,7 +470,6 @@ public String getDisplayName() { } public FormValidation doCheckProjectId(@QueryParameter String value) { - checkPermissions(); if (value == null || value.isEmpty()) { return FormValidation.error("Project ID is required"); } @@ -477,10 +478,7 @@ public FormValidation doCheckProjectId(@QueryParameter String value) { public ListBoxModel doFillCredentialsIdItems( @AncestorInPath Jenkins context, @QueryParameter String value) { - checkPermissions(); - if (context == null || !context.hasPermission(Item.CONFIGURE)) { - return new StandardListBoxModel(); - } + checkPermissions(Jenkins.getInstanceOrNull(), Jenkins.ADMINISTER); List domainRequirements = new ArrayList(); return new StandardListBoxModel() @@ -496,7 +494,7 @@ public FormValidation doCheckCredentialsId( @AncestorInPath Jenkins context, @QueryParameter("projectId") String projectId, @QueryParameter String value) { - checkPermissions(); + checkPermissions(Jenkins.getInstanceOrNull(), Jenkins.ADMINISTER); if (value.isEmpty()) return FormValidation.error("No credential selected"); if (projectId.isEmpty()) diff --git a/src/main/java/com/google/jenkins/plugins/computeengine/InstanceConfiguration.java b/src/main/java/com/google/jenkins/plugins/computeengine/InstanceConfiguration.java index ea4d0d57..291bbc87 100644 --- a/src/main/java/com/google/jenkins/plugins/computeengine/InstanceConfiguration.java +++ b/src/main/java/com/google/jenkins/plugins/computeengine/InstanceConfiguration.java @@ -640,7 +640,6 @@ public String getHelpFile(String fieldName) { } public FormValidation doCheckNetworkTags(@QueryParameter String value) { - checkPermissions(); if (value == null || value.isEmpty()) { return FormValidation.ok(); } @@ -657,7 +656,6 @@ public FormValidation doCheckNetworkTags(@QueryParameter String value) { } public FormValidation doCheckNamePrefix(@QueryParameter String value) { - checkPermissions(); if (value == null || value.isEmpty()) { return FormValidation.error("A prefix is required"); } @@ -675,7 +673,6 @@ public FormValidation doCheckNamePrefix(@QueryParameter String value) { } public FormValidation doCheckDescription(@QueryParameter String value) { - checkPermissions(); if (value == null || value.isEmpty()) { return FormValidation.error("A description is required"); } @@ -686,7 +683,7 @@ public ListBoxModel doFillRegionItems( @AncestorInPath Jenkins context, @QueryParameter("projectId") @RelativePath("..") final String projectId, @QueryParameter("credentialsId") @RelativePath("..") final String credentialsId) { - checkPermissions(); + checkPermissions(Jenkins.get(), Jenkins.ADMINISTER); ListBoxModel items = new ListBoxModel(); items.add(""); try { @@ -708,7 +705,7 @@ public ListBoxModel doFillTemplateItems( @AncestorInPath Jenkins context, @QueryParameter("projectId") @RelativePath("..") final String projectId, @QueryParameter("credentialsId") @RelativePath("..") final String credentialsId) { - checkPermissions(); + checkPermissions(Jenkins.get(), Jenkins.ADMINISTER); ListBoxModel items = new ListBoxModel(); items.add(""); try { @@ -727,8 +724,7 @@ public ListBoxModel doFillTemplateItems( } public FormValidation doCheckRegion(@QueryParameter String value) { - checkPermissions(); - if (value.equals("")) { + if (StringUtils.isEmpty(value)) { return FormValidation.error("Please select a region..."); } return FormValidation.ok(); @@ -739,7 +735,7 @@ public ListBoxModel doFillZoneItems( @QueryParameter("projectId") @RelativePath("..") final String projectId, @QueryParameter("region") final String region, @QueryParameter("credentialsId") @RelativePath("..") final String credentialsId) { - checkPermissions(); + checkPermissions(Jenkins.get(), Jenkins.ADMINISTER); ListBoxModel items = new ListBoxModel(); items.add(""); try { @@ -761,8 +757,7 @@ public ListBoxModel doFillZoneItems( } public FormValidation doCheckZone(@QueryParameter String value) { - checkPermissions(); - if (value.equals("")) { + if (StringUtils.isEmpty(value)) { return FormValidation.error("Please select a zone..."); } return FormValidation.ok(); @@ -773,7 +768,7 @@ public ListBoxModel doFillMachineTypeItems( @QueryParameter("projectId") @RelativePath("..") final String projectId, @QueryParameter("zone") final String zone, @QueryParameter("credentialsId") @RelativePath("..") final String credentialsId) { - checkPermissions(); + checkPermissions(Jenkins.get(), Jenkins.ADMINISTER); ListBoxModel items = new ListBoxModel(); items.add(""); try { @@ -795,8 +790,7 @@ public ListBoxModel doFillMachineTypeItems( } public FormValidation doCheckMachineType(@QueryParameter String value) { - checkPermissions(); - if (value.equals("")) { + if (StringUtils.isEmpty(value)) { return FormValidation.error("Please select a machine type..."); } return FormValidation.ok(); @@ -807,7 +801,7 @@ public ListBoxModel doFillMinCpuPlatformItems( @QueryParameter("projectId") @RelativePath("..") final String projectId, @QueryParameter("zone") final String zone, @QueryParameter("credentialsId") @RelativePath("..") final String credentialsId) { - checkPermissions(); + checkPermissions(Jenkins.get(), Jenkins.ADMINISTER); ListBoxModel items = new ListBoxModel(); items.add(""); try { @@ -833,7 +827,7 @@ public ListBoxModel doFillBootDiskTypeItems( @QueryParameter("projectId") @RelativePath("..") final String projectId, @QueryParameter("zone") String zone, @QueryParameter("credentialsId") @RelativePath("..") final String credentialsId) { - checkPermissions(); + checkPermissions(Jenkins.get(), Jenkins.ADMINISTER); ListBoxModel items = new ListBoxModel(); try { ComputeClient compute = computeClient(context, credentialsId); @@ -856,7 +850,7 @@ public ListBoxModel doFillBootDiskTypeItems( public ListBoxModel doFillBootDiskSourceImageProjectItems( @AncestorInPath Jenkins context, @QueryParameter("projectId") @RelativePath("..") final String projectId) { - checkPermissions(); + checkPermissions(Jenkins.get(), Jenkins.ADMINISTER); ListBoxModel items = new ListBoxModel(); items.add(""); items.add(projectId); @@ -867,8 +861,7 @@ public ListBoxModel doFillBootDiskSourceImageProjectItems( } public FormValidation doCheckBootDiskSourceImageProject(@QueryParameter String value) { - checkPermissions(); - if (value.equals("")) { + if (StringUtils.isEmpty(value)) { return FormValidation.warning("Please select source image project..."); } return FormValidation.ok(); @@ -878,7 +871,7 @@ public ListBoxModel doFillBootDiskSourceImageNameItems( @AncestorInPath Jenkins context, @QueryParameter("bootDiskSourceImageProject") final String projectId, @QueryParameter("credentialsId") @RelativePath("..") final String credentialsId) { - checkPermissions(); + checkPermissions(Jenkins.get(), Jenkins.ADMINISTER); ListBoxModel items = new ListBoxModel(); items.add(""); try { @@ -899,8 +892,7 @@ public ListBoxModel doFillBootDiskSourceImageNameItems( } public FormValidation doCheckBootDiskSourceImageName(@QueryParameter String value) { - checkPermissions(); - if (value.equals("")) { + if (StringUtils.isEmpty(value)) { return FormValidation.warning("Please select source image..."); } return FormValidation.ok(); @@ -912,7 +904,7 @@ public FormValidation doCheckBootDiskSizeGbStr( @QueryParameter("bootDiskSourceImageProject") final String projectId, @QueryParameter("bootDiskSourceImageName") final String imageName, @QueryParameter("credentialsId") @RelativePath("..") final String credentialsId) { - checkPermissions(); + checkPermissions(Jenkins.get(), Jenkins.ADMINISTER); if (Strings.isNullOrEmpty(credentialsId) || Strings.isNullOrEmpty(projectId) || Strings.isNullOrEmpty(imageName)) return FormValidation.ok(); @@ -936,7 +928,6 @@ public FormValidation doCheckBootDiskSizeGbStr( public FormValidation doCheckLabelString( @QueryParameter String value, @QueryParameter Node.Mode mode) { - checkPermissions(); if (mode == Node.Mode.EXCLUSIVE && (value == null || value.trim().isEmpty())) { return FormValidation.warning( "You may want to assign labels to this node;" @@ -949,7 +940,6 @@ public FormValidation doCheckCreateSnapshot( @AncestorInPath Jenkins context, @QueryParameter boolean value, @QueryParameter("oneShot") boolean oneShot) { - checkPermissions(); if (!oneShot && value) { return FormValidation.error(Messages.InstanceConfiguration_SnapshotConfigError()); } @@ -960,7 +950,6 @@ public FormValidation doCheckNumExecutorsStr( @AncestorInPath Jenkins context, @QueryParameter String value, @QueryParameter("oneShot") boolean oneShot) { - checkPermissions(); int numExecutors = intOrDefault(value, DEFAULT_NUM_EXECUTORS); if (numExecutors < 1) { return FormValidation.error( diff --git a/src/main/java/com/google/jenkins/plugins/computeengine/SharedVpcNetworkConfiguration.java b/src/main/java/com/google/jenkins/plugins/computeengine/SharedVpcNetworkConfiguration.java index 337373a8..e833a382 100644 --- a/src/main/java/com/google/jenkins/plugins/computeengine/SharedVpcNetworkConfiguration.java +++ b/src/main/java/com/google/jenkins/plugins/computeengine/SharedVpcNetworkConfiguration.java @@ -20,6 +20,7 @@ import hudson.Extension; import hudson.RelativePath; import hudson.util.FormValidation; +import jenkins.model.Jenkins; import lombok.Getter; import org.kohsuke.stapler.DataBoundConstructor; import org.kohsuke.stapler.QueryParameter; @@ -47,7 +48,7 @@ public String getDisplayName() { } public FormValidation doCheckProjectId(@QueryParameter String value) { - checkPermissions(); + checkPermissions(Jenkins.get(), Jenkins.ADMINISTER); if (Strings.isNullOrEmpty(value)) { return FormValidation.error("Project ID required"); } @@ -55,7 +56,7 @@ public FormValidation doCheckProjectId(@QueryParameter String value) { } public FormValidation doCheckSubnetworkName(@QueryParameter String value) { - checkPermissions(); + checkPermissions(Jenkins.get(), Jenkins.ADMINISTER); if (Strings.isNullOrEmpty(value)) { return FormValidation.error("Subnetwork name required"); } @@ -69,7 +70,7 @@ public FormValidation doCheckSubnetworkName(@QueryParameter String value) { public FormValidation doCheckRegion( @QueryParameter String value, @QueryParameter("region") @RelativePath("..") final String region) { - checkPermissions(); + checkPermissions(Jenkins.get(), Jenkins.ADMINISTER); if (Strings.isNullOrEmpty(region) || Strings.isNullOrEmpty(value) || !region.endsWith(value)) { diff --git a/src/main/java/com/google/jenkins/plugins/computeengine/SshConfiguration.java b/src/main/java/com/google/jenkins/plugins/computeengine/SshConfiguration.java index 86d0695a..5f0718b4 100644 --- a/src/main/java/com/google/jenkins/plugins/computeengine/SshConfiguration.java +++ b/src/main/java/com/google/jenkins/plugins/computeengine/SshConfiguration.java @@ -26,7 +26,6 @@ import hudson.Extension; import hudson.model.Describable; import hudson.model.Descriptor; -import hudson.model.Item; import hudson.security.ACL; import hudson.util.FormValidation; import hudson.util.ListBoxModel; @@ -95,10 +94,7 @@ public static final class DescriptorImpl extends Descriptor { public ListBoxModel doFillCustomPrivateKeyCredentialsIdItems( @AncestorInPath Jenkins context, @QueryParameter String customPrivateKeyCredentialsId) { - checkPermissions(); - if (context == null || !context.hasPermission(Item.CONFIGURE)) { - return new StandardListBoxModel(); - } + checkPermissions(context, Jenkins.ADMINISTER); StandardListBoxModel result = new StandardListBoxModel(); @@ -116,7 +112,7 @@ public FormValidation doCheckCustomPrivateKeyCredentialsId( @QueryParameter String value, @QueryParameter("customPrivateKeyCredentialsId") String customPrivateKeyCredentialsId) throws IOException { - checkPermissions(); + checkPermissions(Jenkins.get(), Jenkins.ADMINISTER); if (Strings.isNullOrEmpty(value) && Strings.isNullOrEmpty(customPrivateKeyCredentialsId)) { return FormValidation.error("An SSH private key credential is required"); diff --git a/src/main/java/com/google/jenkins/plugins/computeengine/WindowsConfiguration.java b/src/main/java/com/google/jenkins/plugins/computeengine/WindowsConfiguration.java index d0bf694a..59ff50d4 100644 --- a/src/main/java/com/google/jenkins/plugins/computeengine/WindowsConfiguration.java +++ b/src/main/java/com/google/jenkins/plugins/computeengine/WindowsConfiguration.java @@ -28,7 +28,6 @@ import hudson.Extension; import hudson.model.Describable; import hudson.model.Descriptor; -import hudson.model.Item; import hudson.security.ACL; import hudson.util.FormValidation; import hudson.util.ListBoxModel; @@ -111,10 +110,7 @@ public Descriptor getDescriptor() { @Extension public static final class DescriptorImpl extends Descriptor { public ListBoxModel doFillPasswordCredentialsIdItems(@AncestorInPath Jenkins context) { - checkPermissions(); - if (context == null || !context.hasPermission(Item.CONFIGURE)) { - return new StandardListBoxModel(); - } + checkPermissions(context, Jenkins.ADMINISTER); return new StandardListBoxModel() .withEmptySelection() .withMatching( @@ -127,10 +123,7 @@ public ListBoxModel doFillPasswordCredentialsIdItems(@AncestorInPath Jenkins con } public ListBoxModel doFillPrivateKeyCredentialsIdItems(@AncestorInPath Jenkins context) { - checkPermissions(); - if (context == null || !context.hasPermission(Item.CONFIGURE)) { - return new StandardUsernameListBoxModel(); - } + checkPermissions(context, Jenkins.ADMINISTER); return new StandardUsernameListBoxModel() .withEmptySelection() .withMatching( @@ -142,7 +135,6 @@ public ListBoxModel doFillPrivateKeyCredentialsIdItems(@AncestorInPath Jenkins c public FormValidation doCheckPrivateKeyCredentialsId( @QueryParameter String value, @QueryParameter("passwordCredentialsId") String passwordCredentialsId) { - checkPermissions(); if (Strings.isNullOrEmpty(value) && Strings.isNullOrEmpty(passwordCredentialsId)) { return FormValidation.error("A password or private key credential is required"); } diff --git a/src/test/java/com/google/jenkins/plugins/computeengine/SharedVpcNetworkConfigurationTest.java b/src/test/java/com/google/jenkins/plugins/computeengine/SharedVpcNetworkConfigurationTest.java index 0744a4f9..2d9ecaca 100644 --- a/src/test/java/com/google/jenkins/plugins/computeengine/SharedVpcNetworkConfigurationTest.java +++ b/src/test/java/com/google/jenkins/plugins/computeengine/SharedVpcNetworkConfigurationTest.java @@ -20,8 +20,12 @@ import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertTrue; +import hudson.ExtensionList; import hudson.util.FormValidation; +import org.junit.Rule; import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.WithoutJenkins; public class SharedVpcNetworkConfigurationTest { public final String PROJECT_ID = "test-project"; @@ -29,7 +33,10 @@ public class SharedVpcNetworkConfigurationTest { public final String BAD_SUBNETWORK_NAME = "projects/project/subnetworks/test-subnetwork"; public final String REGION = "test-region"; + @Rule public JenkinsRule r = new JenkinsRule(); + @Test + @WithoutJenkins public void construction() { // Empty constructor SharedVpcNetworkConfiguration anc = @@ -44,7 +51,7 @@ public void construction() { @Test public void descriptorSubnetwork() { SharedVpcNetworkConfiguration.DescriptorImpl d = - new SharedVpcNetworkConfiguration.DescriptorImpl(); + ExtensionList.lookupSingleton(SharedVpcNetworkConfiguration.DescriptorImpl.class); // Subnetwork with slash returns an error FormValidation fv = d.doCheckSubnetworkName(BAD_SUBNETWORK_NAME); @@ -66,7 +73,7 @@ public void descriptorSubnetwork() { @Test public void descriptorProjectId() { SharedVpcNetworkConfiguration.DescriptorImpl d = - new SharedVpcNetworkConfiguration.DescriptorImpl(); + ExtensionList.lookupSingleton(SharedVpcNetworkConfiguration.DescriptorImpl.class); // Empty project returns an error FormValidation fv = d.doCheckProjectId(""); @@ -84,7 +91,7 @@ public void descriptorProjectId() { @Test public void descriptorRegion() { SharedVpcNetworkConfiguration.DescriptorImpl d = - new SharedVpcNetworkConfiguration.DescriptorImpl(); + ExtensionList.lookupSingleton(SharedVpcNetworkConfiguration.DescriptorImpl.class); // All empty params returns an error FormValidation fv = d.doCheckRegion("region-textbox", "");