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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Null permissions fix #33

Merged
merged 7 commits into from Oct 4, 2017

Conversation

Projects
None yet
2 participants
@BastienAr
Copy link
Contributor

BastienAr commented Sep 22, 2017

Add a null filter when creating a new Role. When creating a new Role through groovy init script one can add a null Permission by mistake (using a permission from an non-installed plugin). Mainly because Permission.fromId("package.permission.name"), often used to retrieved a permission, could easily return null by mistake. Yet this will no throw an exception until trying to save the jenkins instance.

If you don't save the jenkins instance to xml, null permission are ignored, and you will not get any feedback. If you save the jenkins instance, you will get a NPE and java.lang.RuntimeException :

java.lang.RuntimeException: Failed to serialize jenkins.model.Jenkins#authorizationStrategy for class hudson.model.Hudson
        at hudson.util.RobustReflectionConverter$2.writeField(RobustReflectionConverter.java:256)
        at hudson.util.RobustReflectionConverter$2.visit(RobustReflectionConverter.java:224)
        at com.thoughtworks.xstream.converters.reflection.PureJavaReflectionProvider.visitSerializableFields(PureJavaReflectionProvider.java:138)
        at hudson.util.RobustReflectionConverter.doMarshal(RobustReflectionConverter.java:209)
        at hudson.util.RobustReflectionConverter.marshal(RobustReflectionConverter.java:150)
        at com.thoughtworks.xstream.core.AbstractReferenceMarshaller.convert(AbstractReferenceMarshaller.java:69)
        at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:58)
        at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:43)
        at com.thoughtworks.xstream.core.TreeMarshaller.start(TreeMarshaller.java:82)
        at com.thoughtworks.xstream.core.AbstractTreeMarshallingStrategy.marshal(AbstractTreeMarshallingStrategy.java:37)
        at com.thoughtworks.xstream.XStream.marshal(XStream.java:1026)
        at com.thoughtworks.xstream.XStream.marshal(XStream.java:1015)
        at com.thoughtworks.xstream.XStream.toXML(XStream.java:988)
        at hudson.XmlFile.write(XmlFile.java:181)
        at jenkins.model.Jenkins.save(Jenkins.java:3184)
        at jenkins.model.Jenkins.setNumExecutors(Jenkins.java:2755)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite$PojoCachedMethodSite.invoke(PojoMetaMethodSite.java:192)
        at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite.call(PojoMetaMethodSite.java:56)
        at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:48)
        at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:113)
        at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:125)
        at 4_master-configuration.run(4_master-configuration.groovy:14)
        at groovy.lang.GroovyShell.evaluate(GroovyShell.java:585)
        at jenkins.util.groovy.GroovyHookScript.execute(GroovyHookScript.java:136)
        at jenkins.util.groovy.GroovyHookScript.execute(GroovyHookScript.java:127)
        at jenkins.util.groovy.GroovyHookScript.run(GroovyHookScript.java:110)
        at hudson.init.impl.GroovyInitScript.init(GroovyInitScript.java:41)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at hudson.init.TaskMethodFinder.invoke(TaskMethodFinder.java:104)
        at hudson.init.TaskMethodFinder$TaskImpl.run(TaskMethodFinder.java:175)
        at org.jvnet.hudson.reactor.Reactor.runTask(Reactor.java:282)
        at jenkins.model.Jenkins$7.runTask(Jenkins.java:1073)
        at org.jvnet.hudson.reactor.Reactor$2.run(Reactor.java:210)
        at org.jvnet.hudson.reactor.Reactor$Node.run(Reactor.java:117)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
        at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.NullPointerException
        at com.michelin.cio.hudson.plugins.rolestrategy.RoleBasedAuthorizationStrategy$ConverterImpl.marshal(RoleBasedAuthorizationStrategy.java:477)
        at hudson.util.XStream2$AssociatedConverterImpl.marshal(XStream2.java:370)
        at com.thoughtworks.xstream.core.AbstractReferenceMarshaller.convert(AbstractReferenceMarshaller.java:69)
        at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:58)
        at com.thoughtworks.xstream.core.AbstractReferenceMarshaller$1.convertAnother(AbstractReferenceMarshaller.java:84)
        at hudson.util.RobustReflectionConverter.marshallField(RobustReflectionConverter.java:265)
        at hudson.util.RobustReflectionConverter$2.writeField(RobustReflectionConverter.java:252)
        ... 43 more

I thought it would be nicer if the user is notified with a warn (*hello you 馃憢 , you have some weird permission here 馃 *), and a same behavior either you save or not the jenkins instance (no exception w/o serialization => no exception with serialization)

@@ -83,6 +86,9 @@
Role(String name, Pattern pattern, Set < Permission > permissions) {
this.name = name;
this.pattern = pattern;
if (permissions.remove(null)) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 22, 2017

Member

Note that you remove permissions from the input collections. Which may be non-modifyable Set...
A correct fix would be to iterate through permissions and add only non-Null ones via this.permissions.add().

It's fine for me since there is no way to annotate collection arguments in the target Java baseline

This comment has been minimized.

Copy link
@BastienAr

BastienAr Sep 26, 2017

Author Contributor

Yop, you got a point. Didn't thought about unmodifiable Set. Thanks for spotting that 馃憣

this.permissions.addAll(permissions);
for(Permission perm : permissions) {
if(perm == null ){
LOGGER.warning("Found some null permission(s)");

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 26, 2017

Member

it's not helpful without a stacktrace

This comment has been minimized.

Copy link
@BastienAr

BastienAr Sep 26, 2017

Author Contributor

Do you mean a full stack trace print of the current thread ? Or more precise logs ? Btw logs on permission can't be very helpful, since you will not get which permission(s) has been resolved to null. We could print the full permissions set, and let the user do the difference, but it could be very verbose.

@oleg-nenashev
Copy link
Member

oleg-nenashev left a comment

Just LOGGER.log(Level.WARNING, "Found some null permission(s) in role " + role, new IllegalStateException()) or so

@oleg-nenashev
Copy link
Member

oleg-nenashev left a comment

I should have suggested `IllegalArgumentException. Will patch later. The rest of the code looks good to me

pom.xml Outdated
<dependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest-junit</artifactId>
<version>2.0.0.0</version>

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Oct 2, 2017

Member

I will check if there are binary compat issue risks before merging

This comment has been minimized.

Copy link
@BastienAr

BastienAr Oct 2, 2017

Author Contributor

If so, I could tweak the tests not using hamcrest matchers

@oleg-nenashev oleg-nenashev reopened this Oct 4, 2017

@oleg-nenashev oleg-nenashev merged commit 02f7fec into jenkinsci:master Oct 4, 2017

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@BastienAr BastienAr deleted the BastienAr:null-permissions-fix branch Oct 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.