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

[meiswjn]

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

Beta Give feedback

1. Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
2. Ensure that the pull request title represents the desired changelog entry
3. Please describe what you did
4. Link to relevant issues in GitHub or Jira
5. Link to relevant pull requests, esp. upstream and downstream changes
6. Ensure you have provided tests - that demonstrates feature works or fixes the issue

[meiswjn]

@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]

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]

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

Thanks!
Jan

[meiswjn]

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

[Wadeck]

👋 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]

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]

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]

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?

[meiswjn]

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.

[rsandell]

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?

[meiswjn]

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

[meiswjn]

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.

[meiswjn]

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

[rsandell]

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]

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]

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]

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]

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]

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]

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.