Skip to content
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

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

Wadeck
Copy link
Contributor

@Wadeck Wadeck commented May 25, 2020

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 #300 🔥)

Screenshots

Full script pending approvals

script-security-approval

Full script pending approvals (with annotations)

Screenshot_2020-05-29_164350_001 - Copie

Full script already approved

script-security-approved

Full script already approved (with annotations)

Screenshot_2020-05-29_164418_001 - Copie

Other tabs kept the same

Screenshot_2020-05-29_165406_001


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

Copy link
Member

@timja timja left a 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

@jeffret-b
Copy link

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
Copy link

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
Copy link
Member

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 👍

Wadeck and others added 11 commits May 29, 2020 15:15
…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]>
@jglick
Copy link
Member

jglick commented May 29, 2020

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. 🤷‍♂️

@Wadeck
Copy link
Contributor Author

Wadeck commented May 29, 2020

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
Copy link
Contributor Author

Wadeck commented May 29, 2020

Added some annotated screenshots and put more highlight on that section

@jglick
Copy link
Member

jglick commented May 29, 2020

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 Wadeck requested a review from fqueiruga August 18, 2020 13:55
@Wadeck
Copy link
Contributor Author

Wadeck commented Aug 24, 2020

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
Copy link
Member

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.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both proposals seem to pollute the space due to the size of the sentence.

Proof

After

Screenshot_2020-09-02_083609_001

Before (with the icon for tooltip discoverability)

Screenshot_2020-09-02_083719_001

Before (with just the cursor: help)

Screenshot_2020-09-02_084251_001

Going with no modification. This could be improved in a follow up PR.

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. \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tooltip lacks discoverability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried with a help icon but it will require to have newer version of core to have the latest help icon, so keeping it simple for the moment with cursor: help

Cursor

Screenshot_2020-09-02_084251_001

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When are these shown?

Copy link

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.

Copy link
Contributor Author

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.

@daniel-beck
Copy link
Member

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
Copy link
Contributor Author

Wadeck commented Aug 24, 2020

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
Copy link
Member

Looking at something like

Screenshot

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.

Screenshot 2020-08-24 at 20 27 01

(I'm not proposing to remove the pre-approval checkmark, but Preview.app just isn't a good drawing program!)

@jglick
Copy link
Member

jglick commented Aug 24, 2020

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.

- 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
@Wadeck
Copy link
Contributor Author

Wadeck commented Sep 2, 2020

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

Screenshot_2020-09-02_085343_001


Screenshot_2020-09-02_085350_001


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 :(

// Probably need not add to pendingScripts, since generally that would have happened already in configuring.

^- 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
Copy link
Member

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
Copy link
Contributor Author

Wadeck commented Sep 2, 2020

@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
Copy link

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
Copy link
Contributor Author

Wadeck commented Sep 14, 2020

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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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).

Comment on lines +37 to +43
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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

@fqueiruga
Copy link

How can we resurrect this?

@Wadeck
Copy link
Contributor Author

Wadeck commented Nov 27, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants