[JENKINS-63668] fix: auto re-request approval once dismissed #546

meiswjn · 2023-11-27T12:02:10Z

This PR resolves JENKINS-63668. If a job fails because a script is not approved, it automatically re-requests the approval.
It also deprecates the current #using method and adds a new flag ignoreAdmin to it. This is due to concerns that administrators could unknowingly approve a script for everyone by running the job.

Should unblock #300 (hopefully - motivation behind this PR).

Testing done

Added automated test.
Followed previous reproduction steps from issue.

Submitter checklist

Give feedback

Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Options
Successfully updated the issue's project

There was an error updating the issue's project
Ensure that the pull request title represents the desired changelog entry

Ensure that the pull request title represents the desired changelog entry
Options
Successfully updated the issue's project

There was an error updating the issue's project
Please describe what you did

Please describe what you did
Options
Successfully updated the issue's project

There was an error updating the issue's project
Link to relevant issues in GitHub or Jira

Link to relevant issues in GitHub or Jira
Options
Successfully updated the issue's project

There was an error updating the issue's project
Link to relevant pull requests, esp. upstream and downstream changes

Link to relevant pull requests, esp. upstream and downstream changes
Options
Successfully updated the issue's project

There was an error updating the issue's project
Ensure you have provided tests - that demonstrates feature works or fixes the issue

Ensure you have provided tests - that demonstrates feature works or fixes the issue
Options
Successfully updated the issue's project

There was an error updating the issue's project
Options

meiswjn · 2023-11-27T12:34:16Z

@Wadeck, you wrote in #300:

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)

@Wadeck I addressed your concern in 3a8f7ee. This should prevent any accidential auto-approvals by admins unknowingly running a script and approving it for everyone.

Wadeck · 2023-11-27T12:52:49Z

Great, thanks @meiswjn for the bug fix. I will not have the time to test it but I believe the maintainers of the plugin will be happy to move forward this PR and the previous one about the UI revamp :)

meiswjn · 2023-12-12T08:41:56Z

Hey @daniel-beck,
Did you have any time to look at this PR already?

Thanks!
Jan

meiswjn · 2024-01-09T09:07:30Z

Hey @daniel-beck, perhaps you have missed this. If you do not have time, perhaps @Wadeck has time?

Wadeck · 2024-01-09T09:15:51Z

👋 Hello Jan, currently we (Daniel and me) are both lacking time however I pinged the maintainers of this plugin to see if they have some time for this PR :)

jglick · 2024-01-09T14:05:51Z

Everything related to ScriptApproval should be considered basically deprecated (do not use this system), and adjusting the UX is certainly dangerous. If @daniel-beck or @Wadeck wants to review this and take responsibility for the behavior, go ahead (and ideally get added to the maintainer list).

rsandell · 2024-01-10T15:05:57Z

There are still plenty of usage of ScriptApproval outside of pipeline and where sandboxing isn't really possible. Email-ext templates for example is the first that comes to mind.

rsandell · 2024-01-10T15:10:01Z

src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java

        final ConversionCheckResult result = checkAndConvertApprovedScript(script, language);
        if (!result.approved) {
-            if (!Jenkins.get().isUseSecurity() || 
+            if (!Jenkins.get().isUseSecurity() ||


The boolean logic here has become too complex and unreadable, so it is very difficult to grok if the added logic is sound.

But why do you need the new parameter at all? From the description it seems like it should suffice to just set approveIfAdmin to false?

Hey @rsandell, thanks for the review!

I agree that it is too complex.
The reason for adding this can be found here.

Does this answer your question? If yes, I would improve the readbility of the boolean. If not, let's further discuss its need.

Sorry I couldn't find @Wadeck 's comment so I am still a bit lost as to why, though I trust that there is a legitimate reeason for it I just would like to understand that reason 😅

If you could clean up the logic to make it more readable that would be hugely helpful :) Maybe it is just enough to reformat the code?

No worries!
#300 (comment)
There you go :) Last sentence in that comment.

What ignoreAdmin does can be seen here. When the using() function is called from a job, it will not cause scripts to be approved automatically. If the admin did set up the script, it would have already been approved on saving the configuration.

It is supposed to prevent the scenario, where an admin starts a job, not knowing that it will approve a potentially dangerous script in it.

I am reformatting it in a sec

though I trust that there is a legitimate reeason for it I just would like to understand that reason 😅

Yes, please! I am new to this repository's source code. There is a chance that I am completely wrong with that new boolean and since this is security relevant, I totally support that you question it.

I tried to make it much more readable, even though not as "fancy" as before. Does this help? @rsandell

rsandell · 2024-01-11T16:40:28Z

Just as an FYI I remember explicitly doing this in email-ext at least for when a template is picked up from the workspace; it will call configuring followed by using every time, which might be annoying for some admins 😅

meiswjn · 2024-01-12T07:59:43Z

Just as an FYI I remember explicitly doing this in email-ext at least for when a template is picked up from the workspace; it will call configuring followed by using every time, which might be annoying for some admins 😅

What would be the outcome of this scenario? I think once the script is approved, the admin does not have to do anything unless approval is revoked :)

jglick · 2024-01-16T19:04:12Z

outside of pipeline and where sandboxing isn't really possible. Email-ext templates for example

Getting off topic, but FWIW many cases would be better handled by replacing that plugin with e.g.

mail subject: 'Failure', …, body: """
Build broke! $BUILD_URL
"""

meiswjn · 2024-02-06T08:31:44Z

Hello @rsandell,
Given that this PR is open for several months now, would you be able to provide a timeline or delegate the review?

Thanks! I appreciate your work!

rsandell · 2024-02-06T18:50:28Z

I am very sorry for the delay, our team is quite swamped at the moment and this keeps falling between the cracks. So thanks for the reminder!
I've set a reminder so that me or someone in my team doesn't forget about it again. So when there is a calm hour we'll take a look again.

meiswjn · 2024-04-03T08:27:56Z

I am very sorry for the delay, our team is quite swamped at the moment and this keeps falling between the cracks. So thanks for the reminder! I've set a reminder so that me or someone in my team doesn't forget about it again. So when there is a calm hour we'll take a look again.

Thanks, @rsandell! Would you have an ETA for me?

meiswjn · 2024-05-16T07:27:03Z

In the current state, this PR is not compatible with some of the recent implemented changes. Moving it to draft until I find some time.

meiswjn force-pushed the fix/JENKINS-63668 branch 3 times, most recently from ae1520e to 3a8f7ee Compare November 27, 2023 12:31

Wadeck changed the title ~~fix: auto re-request approval once dismissed [JENKINS-63668]~~ [JENKINS-63668] fix: auto re-request approval once dismissed [JENKINS-63668] Nov 27, 2023

Wadeck changed the title ~~[JENKINS-63668] fix: auto re-request approval once dismissed [JENKINS-63668]~~ [JENKINS-63668] fix: auto re-request approval once dismissed Nov 27, 2023

meiswjn marked this pull request as ready for review November 27, 2023 14:43

meiswjn requested a review from a team as a code owner November 27, 2023 14:43

jglick requested a review from daniel-beck November 27, 2023 17:40

jglick added the bug label Nov 27, 2023

rsandell reviewed Jan 10, 2024

View reviewed changes

meiswjn requested a review from rsandell January 22, 2024 08:16

meiswjn and others added 6 commits May 3, 2024 13:41

fix: auto re-request approval once dismissed [JENKINS-63668]

41b2621

prevent accidential admin auto-approval

5f2fe4d

prettify

59f62e8

Update ScriptApprovalTest.java

2e73aa7

prettify

8507a9c

feat: improve boolean reability

50f5c5c

meiswjn force-pushed the fix/JENKINS-63668 branch from 95a3489 to 50f5c5c Compare May 3, 2024 11:41

trigger ci

4ee5332

meiswjn marked this pull request as draft May 16, 2024 07:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-63668] fix: auto re-request approval once dismissed #546

[JENKINS-63668] fix: auto re-request approval once dismissed #546

meiswjn commented Nov 27, 2023 •

edited

Submitter checklist

meiswjn commented Nov 27, 2023

Wadeck commented Nov 27, 2023

meiswjn commented Dec 12, 2023

meiswjn commented Jan 9, 2024

Wadeck commented Jan 9, 2024

jglick commented Jan 9, 2024

rsandell commented Jan 10, 2024

rsandell Jan 10, 2024

meiswjn Jan 10, 2024

rsandell Jan 11, 2024

meiswjn Jan 12, 2024

meiswjn Jan 12, 2024

meiswjn Jan 12, 2024

rsandell commented Jan 11, 2024

meiswjn commented Jan 12, 2024

jglick commented Jan 16, 2024

meiswjn commented Feb 6, 2024

rsandell commented Feb 6, 2024

meiswjn commented Apr 3, 2024

meiswjn commented May 16, 2024

[JENKINS-63668] fix: auto re-request approval once dismissed #546

Are you sure you want to change the base?

[JENKINS-63668] fix: auto re-request approval once dismissed #546

Conversation

meiswjn commented Nov 27, 2023 • edited

Testing done

Submitter checklist

meiswjn commented Nov 27, 2023

Wadeck commented Nov 27, 2023

meiswjn commented Dec 12, 2023

meiswjn commented Jan 9, 2024

Wadeck commented Jan 9, 2024

jglick commented Jan 9, 2024

rsandell commented Jan 10, 2024

rsandell Jan 10, 2024

Choose a reason for hiding this comment

meiswjn Jan 10, 2024

Choose a reason for hiding this comment

rsandell Jan 11, 2024

Choose a reason for hiding this comment

meiswjn Jan 12, 2024

Choose a reason for hiding this comment

meiswjn Jan 12, 2024

Choose a reason for hiding this comment

meiswjn Jan 12, 2024

Choose a reason for hiding this comment

rsandell commented Jan 11, 2024

meiswjn commented Jan 12, 2024

jglick commented Jan 16, 2024

meiswjn commented Feb 6, 2024

rsandell commented Feb 6, 2024

meiswjn commented Apr 3, 2024

meiswjn commented May 16, 2024

meiswjn commented Nov 27, 2023 •

edited