-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[JENKINS-62448] Enhance information displayed in approval page #300
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
One question on the hash column and whether it needs to be exposed to the user,
Haven't really looked at the code itself just the description and screenshot
...s/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.jelly
Outdated
Show resolved
Hide resolved
...ces/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/classPath_pending.jelly
Show resolved
Hide resolved
.../jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.properties
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/metadata/MetadataStorage.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/metadata/MetadataStorage.java
Outdated
Show resolved
Hide resolved
src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties
Outdated
Show resolved
Hide resolved
...g/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_pending.properties
Outdated
Show resolved
Hide resolved
...g/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_pending.properties
Outdated
Show resolved
Hide resolved
...g/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_pending.properties
Outdated
Show resolved
Hide resolved
...es/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/signature_approved.jelly
Show resolved
Hide resolved
...es/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/signature_approved.jelly
Outdated
Show resolved
Hide resolved
...ces/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/signature_pending.jelly
Outdated
Show resolved
Hide resolved
There are some weird, interesting situations with this if multiple jobs have the same script. Some of them probably exist even before this PR but some are exacerbated by the addition of tracking info after the script has been approved. Here's one example scenario:
I'm not sure this is really a problem, though. First, it's rather a corner case for the exact same script to be used in multiple different jobs. I can imagine some situations in which that might be valid, but they're kind of odd and I'm not sure we need to worry about them. Second, even the behavior is odd, I don't know that it is wrong or harmful. The gain from improving the UI / UX here probably outweighs these oddities in these corner cases. |
A very minor detail at this point, but it looks odd to me to have the tab named "Script - approved" but then to have the content title be "Approved scripts". The first tab for "Script - pending approvals" is more similar to "Scripts pending approval" but still a little disconcerting, especially the shift from singular to plural. |
Thank you for this contribution! It is much appreciated. Are you doing it as a part of the Jenkins UI/UX Hackfest? If so, please consider reporting this contribution here so that we could facilitate reviews and share the story 👍 |
...s/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.jelly
Outdated
Show resolved
Hide resolved
…iptApproval.java Co-authored-by: Josh Soref <[email protected]>
…iptApproval.java Co-authored-by: Josh Soref <[email protected]>
…adata/MetadataStorage.java Co-authored-by: Josh Soref <[email protected]>
…adata/MetadataStorage.java Co-authored-by: Josh Soref <[email protected]>
…s/Messages.properties Co-authored-by: Josh Soref <[email protected]>
…s/ScriptApproval/tab-custom.jelly Co-authored-by: Josh Soref <[email protected]>
…s/ScriptApproval/tabs/fullScript_pending.properties Co-authored-by: Josh Soref <[email protected]>
…s/ScriptApproval/tabs/signature_approved.jelly Co-authored-by: Josh Soref <[email protected]>
…s/ScriptApproval/tabs/fullScript_pending.properties Co-authored-by: Josh Soref <[email protected]>
…s/ScriptApproval/tabs/fullScript_pending.properties Co-authored-by: Josh Soref <[email protected]>
…s/ScriptApproval/tab-with-body.jelly Co-authored-by: Josh Soref <[email protected]>
...ces/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/signature_pending.jelly
Outdated
Show resolved
Hide resolved
Co-authored-by: Josh Soref <[email protected]>
It looks nice. I am just concerned that by making the whole-script approval system look nice, we are implicitly encouraging people to use it, when really this was a terrible idea that I should never have introduced to begin with and it would better be deleted. 🤷♂️ |
If you have a good wording / warning / recommended approach, or similar, please share it/them, I can add them ;) I was working on this topic because I got multiple feedbacks that the UX was not helpful and some users had 100s of approved scripts without knowing if they are using them or not. |
Added some annotated screenshots and put more highlight on that section |
Preferably use the sandbox. Where this is impractical, because your script is basically implementing plugin-like functionality, use a (trusted / global) Groovy library, in the case of Pipeline. For scripting in freestyle projects, if there is no other alternative, limit job configuration to Jenkins administrators (was |
.../jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.properties
Outdated
Show resolved
Hide resolved
...g/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_pending.properties
Outdated
Show resolved
Hide resolved
...enkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/legacyApproval/scriptApproval.xml
Outdated
Show resolved
Hide resolved
From Daniel:
Expected: Script is now pending approval |
Some UI feedback:
|
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emptyMetadata=There is no metadata for this approval | |
emptyMetadata=This script has been approved before Script Security 1.75 and no additional metadata has been recorded yet. Once this script is executed, additional metadata will be added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emptyMetadata=There is no metadata for this approval | |
emptyMetadata=This script was approved before Script Security 1.75 and no additional metadata has been recorded yet. Once this script is executed, additional metadata will be added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noContextItemSinceMetadata_tooltip=The context item was not provided by the code asking for 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. \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tooltip lacks discoverability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.../jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.properties
Outdated
Show resolved
Hide resolved
.../jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.properties
Outdated
Show resolved
Hide resolved
notApprovedSinceMetadata=N/A | ||
notApprovedSinceMetadata_tooltip=It was not approved since the introduction of metadata. | ||
noKnownApproverSinceMetadata=N/A | ||
noKnownApproverSinceMetadata_tooltip=It was not approved since the introduction of metadata. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When are these shown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw while I suggest changing a has been
elsewhere to was
, these should be has not been
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a script is approved before the metadata introduction, and then is used. We do not have the information on who approved it.
.../jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/tabs/fullScript_approved.properties
Outdated
Show resolved
Hide resolved
Haven't looked in-depth at the code yet, but this looks a lot like a change that necessitates a compatibility warning because downgrades will be difficult. Thoughts? |
Great idea, I will change this.
I am not fan of UI elements being hidden until required. This reduce the likelihood the users will know where to go when they need this. If they are "used" to have them shown, it's easier.
I am just adding metadata. If you go back, the metadata will not be used any longer. The original XML file is not touched. |
Looking at something like I wonder whether it would make sense to use a checksum prefix to identify the specific entry, both when there is, and when there is not, additional metadata. The checksum is all we have in some cases, but we always have that. (I'm not proposing to remove the pre-approval checkmark, but Preview.app just isn't a good drawing program!) |
Maybe we can merge something that is good enough? This entire PR touches on functionality which you should never be using to begin with, so unless the behavior is clearly wrong, it does not make sense to sink a lot of time into it IMO. |
src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java
Outdated
Show resolved
Hide resolved
- 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
It's a bug already present before this PR :( Line 637 in fcb41b7
^- in the "using" method. I will not modify this method as it could have some impacts I am not aware of (like having script being auto-approved or related stuff) |
AFAIUI this PR newly adds the capability to un-approve specific single scripts, so what wasn't a (big) problem before is now probably much more relevant. |
@dwnusbaum @jglick any thoughts on the recent comments? (the bug about approval => revocation => not possible to put back in pending) Modifying the |
I strongly agree with this.
This seems like a legitimate concern. Providing a new capability that introduces problems because of existing limitations, creates a valid concern. I'm just not sure how big of a concern it is. Does it override Jesse's earlier desire to "merge something that is good enough"? My tentative answer is that we can merge something that is good enough when it comes to some of the UI and layout, but the introduction of a new, potentially confusing or limiting capability needs correction. |
Due to the bug discovered by Daniel, this PR is currently blocked by JENKINS-63668. ☁️ 🐝 internal tickets: JENSEC-863 -> JENSEC-1243 |
noScriptCodeSinceMetadata_tooltip=The approval was given while the metadata was not gathered. | ||
approvedContextUser=Requester | ||
approvedContextItem=Context | ||
wasPreapproved_tooltip=The script content was approved directly during configuration, meaning it comes from a user with the permission to run scripts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasPreapproved_tooltip=The script content was approved directly during configuration, meaning it comes from a user with the permission to run scripts. | |
wasPreapproved_tooltip=The script content written by a user with permission to run scripts (implicit approval). |
noUsageCountSinceMetadata_tooltip=It was not used since the introduction of metadata. | ||
notUsedSinceMetadata=N/A | ||
notUsedSinceMetadata_tooltip=It was not used since the introduction of metadata. | ||
notApprovedSinceMetadata=N/A | ||
notApprovedSinceMetadata_tooltip=It was not approved since the introduction of metadata. | ||
noKnownApproverSinceMetadata=N/A | ||
noKnownApproverSinceMetadata_tooltip=It was not approved since the introduction of metadata. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noUsageCountSinceMetadata_tooltip=It was not used since the introduction of metadata. | |
notUsedSinceMetadata=N/A | |
notUsedSinceMetadata_tooltip=It was not used since the introduction of metadata. | |
notApprovedSinceMetadata=N/A | |
notApprovedSinceMetadata_tooltip=It was not approved since the introduction of metadata. | |
noKnownApproverSinceMetadata=N/A | |
noKnownApproverSinceMetadata_tooltip=It was not approved since the introduction of metadata. | |
noUsageCountSinceMetadata_tooltip=It has not been used since the introduction of metadata. | |
notUsedSinceMetadata=N/A | |
notUsedSinceMetadata_tooltip=It has not been used since the introduction of metadata. | |
notApprovedSinceMetadata=N/A | |
notApprovedSinceMetadata_tooltip=It has not been approved since the introduction of metadata. | |
noKnownApproverSinceMetadata=N/A | |
noKnownApproverSinceMetadata_tooltip=It has not been approved since the introduction of metadata. |
How can we resurrect this? |
@fqueiruga Either you convince the people (mainly Jeff, a bit Daniel) blocking this PR that the existing bug should not block this PR, or convince pipeline people the reported bug should be prioritized :) |
See JENKINS-62448.
With this PR, we are storing more information than before. The change is incremental, it means that if you have some hashes already approved, they will just not have any metadata until used. When used, the metadata will be saved.
It means, after some weeks / months, you will know easily which entries are approved but not used (or no longer used).
If you have troubles with this PR, you can disable this feature by setting the System Property:
org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval.metadataGathering
to false.There is no test addition at the moment, I want to have feedbacks on the approach before investing time on the testing part.
Testing notes
Having pipeline installed will ease the testing.
Also, if you have an instance with already some approved items, it could be useful. I did some manual testing of "migration" without any issues but not 100% sure to have covered all corner cases.
To generate a full script approval request
You can create a new pipeline with a user without RUN_SCRIPT permission (just Job/Configure). Configure the pipeline content locally ("Pipeline script"), and do not check the "Use Groovy Sandbox". A simple
echo 'hello from pipeline'
is sufficient.Then run the pipeline.
To generate a signature approval request
Same as full script but you check the Groovy Sandbox.
new File(".")
will trigger the signature fornew java.io.File java.lang.String
.To generate a class entry approval request
It's a bit more complicated (or at least I was not able to find a better way). You can install EnvInject plugin. Then inside a Freestyle project, you configure the "Build Environment", with "Inject environment variables to the build process". As Groovy Script, put anything and use "Additional classpath", you can use the regular Artifactory of Jenkins to provide links.
I was using: https://repo.jenkins-ci.org/javanet2-cache/org/jvnet/hudson/main/maven-plugin/1.301/maven-plugin-1.301.hpi.
(🔥 Updated screenshots in #300 🔥)
Screenshots
Full script pending approvals
Full script pending approvals (with annotations)
Full script already approved
Full script already approved (with annotations)
Other tabs kept the same
Scope
The objective of the PR is only to provide metadata and new features for the Full Script approval process as it's the most annoying part. The two other categories are let aside (at least for the moment). Their script parts is let unmodified, just split and moved.
Reviewers
@jsoref as you worked on #282
@josephbrueggen for the UI/UX
@fqueiruga for the JavaScript review
@dwnusbaum as maintainer of the plugin
@jeffret-b as you worked on this effort too