Skip to content

Commit

Permalink
Adjustment after review
Browse files Browse the repository at this point in the history 
- 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
  • Loading branch information
@Wadeck
Wadeck committed Sep 2, 2020
1 parent 18bcb49 commit fcb41b7
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 54 deletions.
41 changes: 13 additions & 28 deletions src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -70,15 +70,14 @@
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;
import javax.servlet.ServletException;

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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down
32 changes: 17 additions & 15 deletions ...rg/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.jelly
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@
<j:choose>
<j:when test="${metadata.isEmpty()}">
<td colspan="9">
<i title="${%emptyMetadata_tooltip}">${%emptyMetadata}</i>
<i class="info-cursor">
${%emptyMetadata}
</i>
</td>
</j:when>
<j:otherwise>
Expand All @@ -61,9 +63,9 @@
</div>
</j:when>
<j:otherwise>
<span title="${%noScriptCodeSinceMetadata_tooltip}" class="info-cursor">
<i title="${%noScriptCodeSinceMetadata_tooltip}" class="info-cursor">
${%noScriptCodeSinceMetadata}
</span>
</i>
</j:otherwise>
</j:choose>
</td>
Expand All @@ -75,9 +77,9 @@
<a href="${rootURL}/user/${user}" class="model-link">${user}</a>
</j:when>
<j:otherwise>
<span title="${%noRequesterSinceMetadata_tooltip}" class="info-cursor">
<i title="${%noRequesterSinceMetadata_tooltip}" class="info-cursor">
${%noRequesterSinceMetadata}
</span>
</i>
</j:otherwise>
</j:choose>
</td>
Expand All @@ -88,9 +90,9 @@
<a href="${rootURL}/${contextItem.url}" class="model-link">${contextItem.fullDisplayName}</a>
</j:when>
<j:otherwise>
<span title="${%noContextItemSinceMetadata_tooltip}" class="info-cursor">
<i title="${%noContextItemSinceMetadata_tooltip}" class="info-cursor">
${%noContextItemSinceMetadata}
</span>
</i>
</j:otherwise>
</j:choose>
</td>
Expand All @@ -101,9 +103,9 @@
<span title="${%usageCount_tooltip}" class="info-cursor">${usageCount}</span>
</j:when>
<j:otherwise>
<span title="${%noUsageCountSinceMetadata_tooltip}" class="info-cursor">
<i title="${%noUsageCountSinceMetadata_tooltip}" class="info-cursor">
${%noUsageCountSinceMetadata}
</span>
</i>
</j:otherwise>
</j:choose>
</td>
Expand All @@ -114,9 +116,9 @@
<i:formatDate value="${lastTimeUsed}" type="both" dateStyle="medium" timeStyle="medium"/>
</j:when>
<j:otherwise>
<span title="${%notUsedSinceMetadata_tooltip}" class="info-cursor">
<i title="${%notUsedSinceMetadata_tooltip}" class="info-cursor">
${%notUsedSinceMetadata}
</span>
</i>
</j:otherwise>
</j:choose>
</td>
Expand All @@ -134,9 +136,9 @@
<i:formatDate value="${lastApprovalTime}" type="both" dateStyle="medium" timeStyle="medium" />
</j:when>
<j:otherwise>
<span title="${%notApprovedSinceMetadata_tooltip}" class="info-cursor">
<i title="${%notApprovedSinceMetadata_tooltip}" class="info-cursor">
${%notApprovedSinceMetadata}
</span>
</i>
</j:otherwise>
</j:choose>
</td>
Expand All @@ -147,9 +149,9 @@
<a href="${rootURL}/user/${lastKnownApproverLogin}" class="model-link">${lastKnownApproverLogin}</a>
</j:when>
<j:otherwise>
<span title="${%noKnownApproverSinceMetadata_tooltip}" class="info-cursor">
<i title="${%noKnownApproverSinceMetadata_tooltip}" class="info-cursor">
${%noKnownApproverSinceMetadata}
</span>
</i>
</j:otherwise>
</j:choose>
</td>
Expand Down
13 changes: 6 additions & 7 deletions ...nkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand All @@ -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
Expand Down
8 changes: 4 additions & 4 deletions ...org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_pending.jelly
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@
<a href="${rootURL}/user/${user}" class="model-link">${user}</a>
</j:when>
<j:otherwise>
<span title="${%noRequesterSinceMetadata_tooltip}" class="info-cursor">
<i title="${%noRequesterSinceMetadata_tooltip}" class="info-cursor">
${%noRequesterSinceMetadata}
</span>
</i>
</j:otherwise>
</j:choose>
</td>
Expand All @@ -54,9 +54,9 @@
<a href="${rootURL}/${contextItem.url}" class="model-link">${contextItem.fullDisplayName}</a>
</j:when>
<j:otherwise>
<span title="${%noContextItemSinceMetadata_tooltip}" class="info-cursor">
<i title="${%noContextItemSinceMetadata_tooltip}" class="info-cursor">
${%noContextItemSinceMetadata}
</span>
</i>
</j:otherwise>
</j:choose>
</td>
Expand Down

0 comments on commit fcb41b7

Please sign in to comment.