github-oauth-plugin icon indicating copy to clipboard operation
github-oauth-plugin copied to clipboard

WIP: Add email domain filter

Open jdmulloy opened this issue 7 years ago • 19 comments

This is useful for operators who have users whose Github accounts have multiple email addresses and their default address is a personal address. The current implementation leads to Jenkins storing the default github address, which in some cases is a personal account. This change allows an operator to specify the domain of their org, and the plugin will look at all the emails for a user and if it finds one that matches the specified domain it will prefer that email over the primary. If the setting is empty it will behave as before.

I also added a checkbox to always override the stored value with what is found in Github.

This is very much a work in progress. I'm not an experienced Java developer and I did this in one working day. It probably also needs some tests for the new functionality. I'd appreciate feedback.

jdmulloy avatar Feb 01 '18 23:02 jdmulloy

This makes sense. I'll take a look.

samrocketman avatar Feb 02 '18 03:02 samrocketman

@jenkinsci/code-reviewers Please check out this change.

samrocketman avatar Feb 02 '18 03:02 samrocketman

@jdmulloy mind if I make a feature request? A CSV list of domains in order of preference. Some companies have contractors who would be using contractor emails. If not, that's fine, I could try adding it later.

samrocketman avatar Feb 02 '18 22:02 samrocketman

@samrocketman I've added in some logging and added the csv feature.

jdmulloy avatar Feb 07 '18 21:02 jdmulloy

Thanks for humoring my request @jdmulloy. I'll test running and upgrading to your version to ensure stability. If all goes well I'll merge it. I think this is a pretty cool feature.

samrocketman avatar Feb 08 '18 07:02 samrocketman

Once it's approved I can squash the commits to make the history cleaner.

jdmulloy avatar Feb 08 '18 19:02 jdmulloy

I have not forgotten about this change. I was going to look at it over the weekend and did not get a chance. I'll see about looking at it during the evenings this week.

samrocketman avatar Feb 12 '18 09:02 samrocketman

@samrocketman I'll work on overloading the method and fixing the tests. How do I run the tests that are failing? When I build the plugin I don't get any test failures, so there is probably something I'm missing. I can't find any failing tests via this PR, but I may not know where to look.

Thanks

jdmulloy avatar Feb 14 '18 15:02 jdmulloy

Actually I think I figured out what you were saying. You were talking about the tests that were failing until I changed them. Correct?

jdmulloy avatar Feb 14 '18 15:02 jdmulloy

@samrocketman I added the deprecated method, but it fails to compile with the following error. I can't figure out why it can't find the method. I've tried googling.

[INFO] 
[INFO] --- maven-resources-plugin:2.7:resources (default-resources) @ github-oauth ---
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] Copying 4 resources
[INFO] 
[INFO] --- maven-compiler-plugin:3.3:compile (default-compile) @ github-oauth ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 11 source files to /home/jdmulloy/git/github-oauth-plugin/target/classes
[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /home/jdmulloy/git/github-oauth-plugin/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java:[164,9] cannot find symbol
  symbol:   method GithubSecurityRealm(java.lang.String,java.lang.String,java.lang.String,java.lang.String,java.lang.String,java.lang.String,java.lang.Boolean)
  location: class org.jenkinsci.plugins.GithubSecurityRealm
[INFO] 1 error
[INFO] -------------------------------------------------------------
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 8.973 s
[INFO] Finished at: 2018-02-14T12:36:43-05:00
[INFO] Final Memory: 41M/155M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.3:compile (default-compile) on project github-oauth: Compilation failure
[ERROR] /home/jdmulloy/git/github-oauth-plugin/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java:[164,9] cannot find symbol
[ERROR]   symbol:   method GithubSecurityRealm(java.lang.String,java.lang.String,java.lang.String,java.lang.String,java.lang.String,java.lang.String,java.lang.Boolean)
[ERROR]   location: class org.jenkinsci.plugins.GithubSecurityRealm
[ERROR] 
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException

jdmulloy avatar Feb 14 '18 17:02 jdmulloy

Let me know when your'e ready for me to review/test this change for merging.

samrocketman avatar Feb 16 '18 06:02 samrocketman

Please squash and rebase on master.

samrocketman avatar Feb 18 '18 05:02 samrocketman

@samrocketman I've squashed and rebased on master. I tested upgrading the plugin and it works now. I had to fix handling of the forceGithubEmail setting being missing in config.xml. The unmarshaling code was setting it to Null instead of False.

The todo comment for handling the boolean forceGithubEmail is still there. The useSecurity setting in core uses "true" in config.xml to store true, but I couldn't find the relevant marshaling and unmarshaling code for that. So I couldn't figure out how that's done in core.

jdmulloy avatar Feb 26 '18 17:02 jdmulloy

Thanks I'll look it over and test it.

samrocketman avatar Mar 03 '18 19:03 samrocketman

Upgraded by uploading via the Advanced and checking the restart Jenkins checkbox. Jenkins threw a stacktrace upon upgrade.

Click here for stack trace
java.lang.NullPointerException
	at com.thoughtworks.xstream.io.xml.PrettyPrintWriter.writeText(PrettyPrintWriter.java:235)
	at com.thoughtworks.xstream.io.xml.PrettyPrintWriter.writeText(PrettyPrintWriter.java:231)
	at com.thoughtworks.xstream.io.xml.PrettyPrintWriter.setValue(PrettyPrintWriter.java:214)
	at com.thoughtworks.xstream.io.WriterWrapper.setValue(WriterWrapper.java:45)
	at org.jenkinsci.plugins.GithubSecurityRealm$ConverterImpl.marshal(GithubSecurityRealm.java:285)
	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)
Caused: java.lang.RuntimeException: Failed to serialize jenkins.model.Jenkins#securityRealm 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:3198)
	at jenkins.model.Jenkins.saveQuietly(Jenkins.java:3204)
	at jenkins.model.Jenkins.setSecurityRealm(Jenkins.java:2576)
	at jenkins.model.Jenkins$17.run(Jenkins.java:3168)
	at org.jvnet.hudson.reactor.TaskGraphBuilder$TaskImpl.run(TaskGraphBuilder.java:169)
	at org.jvnet.hudson.reactor.Reactor.runTask(Reactor.java:282)
	at jenkins.model.Jenkins$5.runTask(Jenkins.java:1068)
	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:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Caused: org.jvnet.hudson.reactor.ReactorException
	at org.jvnet.hudson.reactor.Reactor.execute(Reactor.java:269)
	at jenkins.InitReactorRunner.run(InitReactorRunner.java:47)
	at jenkins.model.Jenkins.executeReactor(Jenkins.java:1102)
	at jenkins.model.Jenkins.<init>(Jenkins.java:904)
	at hudson.model.Hudson.<init>(Hudson.java:86)
	at hudson.model.Hudson.<init>(Hudson.java:82)
	at hudson.WebAppMain$3.run(WebAppMain.java:233)
Caused: hudson.util.HudsonFailedToLoad
	at hudson.WebAppMain$3.run(WebAppMain.java:250)

To reproduce this issue I used the my jenkins-bootstrap-jervis project which you can use settings.groovy to configure the GitHub OAuth plugin. Uncomment the bottom of settings.groovy to configure github-oauth.

dependencies.gradle of jenkins-bootstrap-jervis will show you what versions of plugins I tested.

samrocketman avatar Mar 04 '18 04:03 samrocketman

However, when I restarted Jenkins the stacktrace went away.

samrocketman avatar Mar 04 '18 04:03 samrocketman

I'm going to investigate this more before merging.

samrocketman avatar Mar 04 '18 04:03 samrocketman

Also note, if you wanted to test this yourself without the complication of learning the scripts I publish packages of the version I tested. https://github.com/samrocketman/jenkins-bootstrap-jervis/releases/tag/jervis-bootstrap-2.89.4.3

samrocketman avatar Mar 04 '18 04:03 samrocketman

@jdmulloy I just released a bunch of security fixes and a critical bug fix. Unfortunately, I didn't get a chance to test your change as part of the release. Would you mind merging in the latest master and resolving the merge conflicts?

If not, then I can take a look and update it after I move.

samrocketman avatar Dec 07 '18 07:12 samrocketman