[JENKINS-62448] Enhance information displayed in approval page

[Wadeck]

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 for new 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 https://github.com/jenkinsci/script-security-plugin/pull/300/#issuecomment-675491523 🔥)

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

[jeffret-b]

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:

1. Create a job a with a script that needs to be approved. For example I just used the "Hello World" example script.
2. Create a second job b with the exact same script that needs to be approved.
3. Login as admin to approve scripts.
4. Only the first version shows as needing approval via the job a context.
5. Check job b. It no longer shows that the script needs approval.
6. Run job b.
7. Check the "Script - Approved" tab. It still shows the context as job a even though it has never run.

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.

[jeffret-b]

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.

[oleg-nenashev]

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 👍

[jglick]

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. :man_shrugging:

[Wadeck]

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.

[Wadeck]

Added some annotated screenshots and put more highlight on that section

[jglick]

recommended approach

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 RUN_SCRIPTS, now ADMINISTER but not MANAGE), who can freely save scripts without them appearing on this page.

[Wadeck]

who can freely save scripts without them appearing on this page.

Are you sure about this part? From my limited testing, when an admin configures a script / job / pipeline, the script is automatically approved, but the hash is still stored. (It was the reason why I introduced the green tick about "preapproved" scripts, to differentiate the two situations)

[jglick]

the script is automatically approved, but the hash is still stored

Right, the script may appear on this page, I just meant that in this scenario there is no reason to ever visit this page.

[jsoref]

Fwiw, it'd be nice if i could sort by code, language, requester, request date - (the others are probably meh, but...)

approve all selected / deny all selected -> please drop all, it's superfluous.

I'm not a fan of expand code / minimize code -> show code / hide code?

[fqueiruga]

Approving the CSS code (it's namespaced) and JS code (was only moved). I've got a few comments regarding potential XSS vectors. Other than that looks solid

[fqueiruga]

One more question. Any chance you can use the same tab templates as provided by Jenkins core? Maybe you could even extend them if you need it. Their colours and borders will probably change in the following weeks and it'd be a shame if this plugin gets left behind. It can always be adapted then but it would be double the work.

[Wadeck]

Was this intented?

Yes, the code that was just split/moved is not modified.

Any chance you can use the same tab templates as provided by Jenkins core? Maybe you could even extend them if you need it

Due to the lack of extensibility of tags in general, I was forced to copy-paste the stuff over to get the information badge in the tab headers that are providing values about what requires a look. If this change is done in core, this plugin could use that, but in the meantime, the plugin should use a "fork" otherwise that information is missing. If you know a way to extend the existing without rewriting, let me know!

XSS risk here. Is it protected?

We are not aware of any XSS risk by using <st:bind value="${it}"/>;, but be careful as the code was just moved, if it was a real XSS, it means that you disclosed it in this public PR. The only XSS that was discovered during this work was corrected as SECURITY-1866.

For your information, the result of that call gives (with some random values):

var mgr = makeStaplerProxy('/jenkins/$stapler/bound/b4496557-xxxx-xxxx-xxxx-ee4d8b251c51','xxxxdea5xxxxe0fd15b23b28e66913e476e7bff91f0e936e307a0002f12c6bcf',['denyApprovedClasspathEntry','aclApproveSignature','denyClasspathEntry','clearDangerousApprovedSignatures','approveClasspathEntry','clearApprovedClasspathEntries','clearApprovedSignatures','clearApprovedScripts','getClasspathRenderInfo','approveSignature','denySignature']);

.pane-frame related CSS issue

Unfortunately it's very complicated to have a code that is compatible with versions before the table revamp and after it. So I moved "full speed" to be compatible with the version after table revamp. Meaning: removal of the titles and pane-frame (and my custom one) to have table in full size on the panel, like in update-center.

Screenshots

Before table revamp

Scripts pending

Scripts approved

Signatures pending

After table revamp

Scripts pending

Scripts approved

Signatures pending

[Wadeck]

Fwiw, it'd be nice if i could sort by code, language, requester, request date

Follow-up PR welcome :)

approve all selected / deny all selected -> please drop all, it's superfluous. I'm not a fan of expand code / minimize code -> show code / hide code?

Adjusted

[Wadeck]

From Daniel:

1. As 'user', create a Pipeline, write a script that hasn't been used before, disable sandbox, save.
2. As 'user', build the job, it fails
3. As 'admin', approve the script.
4. As 'user' build the job, it works
5. As 'admin', delete from approvals
6. As 'user', build the job, it fails

Expected: Script is now pending approval Actual: It's neither approved nor pending

[daniel-beck]

Some UI feedback:

• I don't think it's useful to show badges with the number of already approved entries. I associated badges with "unread messages", while these tabs don't have content that needs attention. Badges should be limited to content requiring action.
• I wonder whether it makes sense to hide tabs for empty lists, so that the most common UI would only show "approved scripts" and "approved signatures", and classpath-related content to only show rarely. This could probably go either way.

[daniel-beck]

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?

[Wadeck]

I don't think it's useful to show badges with the number of already approved entries. I associated badges with "unread messages", while these tabs don't have content that needs attention. Badges should be limited to content requiring action.

Great idea, I will change this.

I wonder whether it makes sense to hide tabs for empty lists, so that the most common UI would only show "approved scripts" and "approved signatures", and classpath-related content to only show rarely. This could probably go either way.

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.

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?

I am just adding metadata. If you go back, the metadata will not be used any longer. The original XML file is not touched.

[daniel-beck]

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!)

[jglick]

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.

[Wadeck]

I wonder whether it makes sense to hide tabs for empty lists, so that the most common UI would only show "approved scripts" and "approved signatures", and classpath-related content to only show rarely. This could probably go either way.

New version

---

---

From Daniel: As 'user', [...] Expected: Script is now pending approval Actual: It's neither approved nor pending

It's a bug already present before this PR :(

https://github.com/jenkinsci/script-security-plugin/blob/fcb41b7e540a6e0a53f89098e1fb6cc5210195a3/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java#L637

^- 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)

[daniel-beck]

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.

[Wadeck]

@dwnusbaum @jglick any thoughts on the recent comments? (the bug about approval => revocation => not possible to put back in pending)

Modifying the using method could have terrible side effects depending on the plugin calling it. I am not confident enough to propose such bug correction in this PR.

[jeffret-b]

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 strongly agree with this.

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.

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.

[Wadeck]

Due to the bug discovered by Daniel, this PR is currently blocked by JENKINS-63668.

☁️ 🐝 internal tickets: JENSEC-863 -> JENSEC-1243

[fqueiruga]

How can we resurrect this?

[Wadeck]

@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 :)