From fcb41b7e540a6e0a53f89098e1fb6cc5210195a3 Mon Sep 17 00:00:00 2001 From: Wadeck Follonier Date: Wed, 2 Sep 2020 09:44:03 +0200 Subject: [PATCH] Adjustment after review - Wording / tooltips - Removal of badges when there is no needed action - Stop using deprecated ACL.impersonate - Stop using SecurityContextHolder when Jenkins is sufficient - Remove all occurrences of acegi usage in the plugin --- .../scripts/ScriptApproval.java | 41 ++++++------------- .../tabs/fullScript_approved.jelly | 32 ++++++++------- .../tabs/fullScript_approved.properties | 13 +++--- .../tabs/fullScript_pending.jelly | 8 ++-- 4 files changed, 40 insertions(+), 54 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java index 689d11f44..c2bbd8f58 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -25,12 +25,12 @@ package org.jenkinsci.plugins.scriptsecurity.scripts; import com.google.common.annotations.VisibleForTesting; +import hudson.security.ACLContext; import hudson.util.HttpResponses; import jenkins.model.GlobalConfiguration; import jenkins.model.GlobalConfigurationCategory; import net.sf.json.JSONArray; import net.sf.json.JSONObject; -import org.acegisecurity.Authentication; import org.apache.commons.lang.StringUtils; import org.jenkinsci.Symbol; import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.AclAwareWhitelist; @@ -70,6 +70,7 @@ import java.util.function.Consumer; import java.util.logging.Level; import java.util.logging.Logger; +import java.util.stream.Stream; import javax.annotation.CheckForNull; import javax.annotation.Nonnull; import javax.annotation.concurrent.GuardedBy; @@ -77,8 +78,6 @@ import jenkins.model.Jenkins; import net.sf.json.JSON; -import org.acegisecurity.context.SecurityContext; -import org.acegisecurity.context.SecurityContextHolder; import org.jenkinsci.plugins.scriptsecurity.scripts.languages.LanguageHelper; import org.jenkinsci.plugins.scriptsecurity.scripts.metadata.FullScriptMetadata; import org.jenkinsci.plugins.scriptsecurity.scripts.metadata.HashAndFullScriptMetadata; @@ -235,18 +234,12 @@ public void doIndex(StaplerRequest req, StaplerResponse rsp, @QueryParameter("ta tabInfos[FULL_SCRIPT_PENDING_INDEX].numOfNotification = pendingScripts.size(); tabInfos[FULL_SCRIPT_PENDING_INDEX].primaryColor = true; - tabInfos[FULL_SCRIPT_APPROVED_INDEX].numOfNotification = approvedScriptHashes.size(); - tabInfos[SIGNATURE_PENDING_INDEX].numOfNotification = pendingSignatures.size(); tabInfos[SIGNATURE_PENDING_INDEX].primaryColor = true; - tabInfos[SIGNATURE_APPROVED_INDEX].numOfNotification = approvedSignatures.size(); - tabInfos[CLASS_PATH_PENDING_INDEX].numOfNotification = pendingClasspathEntries.size(); tabInfos[CLASS_PATH_PENDING_INDEX].primaryColor = true; - tabInfos[CLASS_PATH_APPROVED_INDEX].numOfNotification = approvedClasspathEntries.size(); - // will be injected as a variable inside Jelly view req.setAttribute("activeTab", activeTab); req.setAttribute("tabInfos", tabInfos); @@ -971,13 +964,11 @@ public void approveScript(String hash) { save(); } - SecurityContext orig = ACL.impersonate(ACL.SYSTEM); - try { + + try (ACLContext unused = ACL.as(ACL.SYSTEM)) { for (ApprovalListener listener : ExtensionList.lookup(ApprovalListener.class)) { listener.onApproved(hash); } - } finally { - SecurityContextHolder.setContext(orig); } } @@ -1048,15 +1039,13 @@ public void doApprovePendingScripts(@JsonBody AllSelectedHashesModel content) { save(); } - SecurityContext orig = ACL.impersonate(ACL.SYSTEM); - try { + + try (ACLContext unused = ACL.as(ACL.SYSTEM)) { for (ApprovalListener listener : ExtensionList.lookup(ApprovalListener.class)) { for (String hash : content.hashes) { listener.onApproved(hash); } } - } finally { - SecurityContextHolder.setContext(orig); } } @@ -1249,13 +1238,10 @@ public JSON approveClasspathEntry(String hash) throws IOException { } } if (url != null) { - SecurityContext orig = ACL.impersonate(ACL.SYSTEM); - try { + try (ACLContext unused = ACL.as(ACL.SYSTEM)) { for (ApprovalListener listener : ExtensionList.lookup(ApprovalListener.class)) { listener.onApprovedClasspathEntry(hash, url); } - } finally { - SecurityContextHolder.setContext(orig); } } return getClasspathRenderInfo(); @@ -1296,13 +1282,12 @@ public synchronized JSON clearApprovedClasspathEntries() throws IOException { * To have an effectively final variable */ private @CheckForNull String getCurrentUserLogin() { - String userLogin = null; - Authentication auth = SecurityContextHolder.getContext().getAuthentication(); - if (auth != null) { - userLogin = auth.getName(); - } - - return userLogin; + // used to remove completely the dependency on Acegi Security + return Stream.of(Jenkins.getAuthentication()) + .filter(auth -> !auth.equals(Jenkins.ANONYMOUS)) + .findFirst() + .map(auth -> auth.getName()) + .orElse(null); } @Restricted(NoExternalUse.class) // for jelly only diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.jelly b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.jelly index bdd30f3a5..dd46eea02 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.jelly +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.jelly @@ -45,7 +45,9 @@ - ${%emptyMetadata} + + ${%emptyMetadata} + @@ -61,9 +63,9 @@ - + ${%noScriptCodeSinceMetadata} - + @@ -75,9 +77,9 @@ ${user} - + ${%noRequesterSinceMetadata} - + @@ -88,9 +90,9 @@ ${contextItem.fullDisplayName} - + ${%noContextItemSinceMetadata} - + @@ -101,9 +103,9 @@ ${usageCount} - + ${%noUsageCountSinceMetadata} - + @@ -114,9 +116,9 @@ - + ${%notUsedSinceMetadata} - + @@ -134,9 +136,9 @@ - + ${%notApprovedSinceMetadata} - + @@ -147,9 +149,9 @@ ${lastKnownApproverLogin} - + ${%noKnownApproverSinceMetadata} - + diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.properties b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.properties index 569f20ebf..5fa49e954 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.properties +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.properties @@ -9,7 +9,7 @@ metadataGatheringDisabled=Metadata gathering disabled. \ approvedExpandCode=Code approvedLanguage=Language usageCount=# of uses -usageCount_tooltip=Uses since the introduction of metadata +usageCount_tooltip=How often this script was executed. Only executions since Script Security plugin was updated to version 1.75. lastTimeUsed=Date of last use wasPreapproved_header_tooltip=The presence of a green tick means the script was approved during the configuration. \ Otherwise it was created by a user without the permission to run scripts and was then approved using this page. @@ -21,12 +21,10 @@ lastKnownApproverLogin=Last approver noRequesterSinceMetadata=N/A noRequesterSinceMetadata_tooltip=The requester was not recorded. \ It could be because the request was created before the metadata was introduced or the code calling this script did not provide user information. -noRequestApprovalTimeSinceMetadata=N/A -noRequestApprovalTimeSinceMetadata_tooltip=It was requested before the introduction of metadata. noContextItemSinceMetadata=N/A noContextItemSinceMetadata_tooltip=The context item was not provided by the code asking for approval. -emptyMetadata=There is no metadata for this approval +emptyMetadata=There is no metadata for this approval. emptyMetadata_tooltip=If the script is used after the metadata introduction, some metadata will be displayed here. \ It means that an unused approval will not have metadata and could be reasonably removed after a certain period of time. noScriptCodeSinceMetadata=N/A @@ -45,9 +43,10 @@ noKnownApproverSinceMetadata=N/A noKnownApproverSinceMetadata_tooltip=It was not approved since the introduction of metadata. revokeApproval=Remove selected -revokeApproval_tooltip=Remove an approved script can have an impact on the jobs using it. \ - The next time an attempt to execute the script is made, a new approval will be required. -revokeApproval_confirm=Really delete all selected approvals? Any existing scripts will need to be requeued and reapproved. +revokeApproval_tooltip=Removing a script from this list will revoke approval for its execution. \ + The next attempt to execute the script will fail, and the script will be added to the list of scripts requiring approval. +revokeApproval_confirm=Really revoke approvals for all selected scripts? \ + All scripts not using the sandbox will need to be approved again before they can be executed successfully, and will fail until they are. approvedNoSelectedItemError=Please select at least one line. approvedExpand=show code diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_pending.jelly b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_pending.jelly index d8282c27b..649dd9fe5 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_pending.jelly +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_pending.jelly @@ -41,9 +41,9 @@ ${user} - + ${%noRequesterSinceMetadata} - + @@ -54,9 +54,9 @@ ${contextItem.fullDisplayName} - + ${%noContextItemSinceMetadata} - +