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

Use Groovy sandbox and support direct JSON input #24

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

knoxfighter
Copy link

Use the Security Plugins SecureGroovyScript instead of creating a GroovyShell manually. Also the Security Plugin is managing the Scrit-Approval directly.
When using SecureGroovyScript it is also very easy to use the groovy sandbox, which now has also an option which can be used.
That was not enough for me: I also added an direct JSON input, that will read its json directly and is not based on any groovyScript and furthermore needs no approval at all.

@knoxfighter
Copy link
Author

This is working in our environment for a while now. So, please revisit this and i try to reach the maintainer or become the maintainer myself :)

@phene
Copy link

phene commented Apr 18, 2019

I would really like this json file support. Script approvals are being a major hassle.

@vimil
Copy link
Member

vimil commented Apr 30, 2019

Seems like the diff shows a lot of changes. Are they due to formatting?

@stijnvandrunen
Copy link

stijnvandrunen commented Aug 15, 2019

I'd love this to be integrated, we are running into issues with the evaluated Groovy script String being too large (64K max) due to the number of options presented in our ExtendedChoiceParameter. This would address that issue as well as the annoying continuous script approvals.

@tlwhitec
Copy link

tlwhitec commented Sep 3, 2019

Hey, I just found this and must say thank you @knoxfighter!

Your commit 8b1690c quite kills the file diff, tho. There are wrong line endings

$ file extended-choice-parameter-plugin/.../ExtendedChoiceParameterDefinition.java 
extended-choice-parameter-plugin/.../ExtendedChoiceParameterDefinition.java: ASCII text, with CRLF line terminators

$ file extended-choice-parameter-plugin-knox/.../ExtendedChoiceParameterDefinition.java 
extended-choice-parameter-plugin-knox/.../ExtendedChoiceParameterDefinition.java: ASCII text

There seems to be quite a lot formatting/whitespace changes as well, I'm not sure if that's a best approach for a PR. At least there's an option for this to be turned off in the github diff tool.

Also could you please rebase the branch on top of master? It seems there are some small conflicts now.
I'll gladly help with this, if you don't have the time, but just tell me. I simply can't wait for a release with this ;)

# Conflicts:
#	pom.xml
#	src/main/java/com/cwctravel/hudson/plugins/extended_choice_parameter/ExtendedChoiceParameterDefinition.java
#	src/main/resources/com/cwctravel/hudson/plugins/extended_choice_parameter/ExtendedChoiceParameterDefinition/jsonContent.jelly
#	src/main/resources/com/cwctravel/hudson/plugins/extended_choice_parameter/JSONEditorPageDecorator/header.jelly
@knoxfighter
Copy link
Author

knoxfighter commented Sep 4, 2019

Hi, i completly forgot about this PR :)
I merged the master branch back in again and fixed all errors. It should work now, but i dont have a project to test it, in real conditions.

About that line ending: As far as i can see it, the line-endings are the same, in the master and in my Branch:

/original-ecpp/src/main/java/com/cwctravel/hudson/plugins/extended_choice_parameter (master)
$ file ExtendedChoiceParameterDefinition.java
ExtendedChoiceParameterDefinition.java: ASCII text, with CRLF line terminators
----------------
/extended-choice-parameter-plugin/src/main/java/com/cwctravel/hudson/plugins/extended_choice_parameter (groovy-sandbox)
$ file ExtendedChoiceParameterDefinition.java
ExtendedChoiceParameterDefinition.java: ASCII text, with CRLF line terminators

Edit: Something seems real off, with the line endings...

@knoxfighter
Copy link
Author

Ok, for some reason, all line endings where LF. I replaced them with CRLF, like the original file and pushed it. Now the diff also looks much better :)

@tlwhitec
Copy link

tlwhitec commented Sep 5, 2019

Hello @vimil!
It seems @knoxfighter there fixed the formatting and the PR looks pretty sensible.
Sandboxing is a blessing these days, as some Jenkins admins (including mine) usually don't allow anything beside that and if they do, it's on a single case basis, which makes the script development virtually impossible.

Could you pls tell us what needs to be done for this to appear in a release? Thank you!

@tlwhitec
Copy link

Hi @knoxfighter, it's been a week since you updated the branch without any action from the maintainer and he seems, all in all, very inactive. Is there any chance you could help us out? I don't have a clue how this works in the Jenkins world.

@knoxfighter
Copy link
Author

@tlwhitec I also have no idea, how the jenkins world works. Also i have no interest in maintaining this repo, cause i dont use jenkins anymore.

@tlwhitec
Copy link

That's fair. Thank you for your help, anyways. Let's hope the maintainer finds some time to look at this, for which I'll be insanely grateful.

@vimil
Copy link
Member

vimil commented Oct 6, 2019

Seems like there are breaking changes with this pull request.

I think this pull request should be broken down into multiple logical prs. Too many unrelated changes in it.

I have not tested this pull request but
I think classpath functionality is being removed.
script approval will no longer work.

@tlwhitec
Copy link

tlwhitec commented Oct 6, 2019

Isn't that covered by the Security plugin instead? Thanks a lot for your input @knoxfighter!

@knoxfighter
Copy link
Author

knoxfighter commented Oct 6, 2019

Its like @tlwhitec says, the Security Plugin is managing the classpath. When the classpath option, also had more things to do, than the Security Plugin can manage, than it is breaking. If not, this PR shouldn't be breaking :)
And the Script Approval is also managed by the Security Plugin

@aleskiontherun
Copy link

In my case the current classpath implementation doesn't work almost completely. I created this ticket about a week ago, but didn't have any response so far https://issues.jenkins-ci.org/browse/JENKINS-60419. So if this PR fixes that, it would be just awesome!

@vimil I understand that you want your git history to be nice and clean, but since you are both busy people and @tlwhitec doesn't even use Jenkins anymore, maybe you could find some time to test and merge it anyway? I'm sure a lot of people would benefit from it.

@fivepapertigers
Copy link

This would be huge. I have a JSON schema that will need to change with some regularity, and getting administrative approval for that change is burdensome.

@aleskiontherun
Copy link

Any chance of getting this merged?

@bpedersen2
Copy link

Ping...

@DuMaM
Copy link

DuMaM commented Nov 24, 2022

@knoxfighter Could you update this PR 😉

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