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.

Sign up for GitHub

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

[JENKINS-63350] update pac4j to 3.9.0 #90

Merged
merged 17 commits into from
Feb 25, 2021
Merged

[JENKINS-63350] update pac4j to 3.9.0 #90

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
5786d35
[JENKINS-63350] update pac4j to 3.8
kuisathaverat Aug 8, 2020
68aaa10
add placeholders to compile
kuisathaverat Aug 8, 2020
76b6c1a
chore: remove dummy classes
kuisathaverat Nov 13, 2020
ffadcb1
feat: update pac4j to 3.9.0 and Jenkins Core to 2.266
kuisathaverat Nov 13, 2020
579a46c
chore: grooming
kuisathaverat Nov 13, 2020
8ad3af6
Merge branch 'master' into JENKINS-63350
kuisathaverat Nov 13, 2020
5188258
fix: maven complain in windows
kuisathaverat Nov 13, 2020
1beb7e3
fix: compile for the correct version
kuisathaverat Nov 13, 2020
81b5bb7
chore: update incremental maven plugin
kuisathaverat Nov 13, 2020
867faa0
chore: suggested changes on the Jenkinsfile
kuisathaverat Nov 13, 2020
e15c310
fix: exclude transitive dependencies
kuisathaverat Nov 13, 2020
867464d
docs: fix javadoc warning
kuisathaverat Nov 13, 2020
9810e6e
fix: throw exception
kuisathaverat Nov 13, 2020
aa2a2f1
Apply suggestions from code review
kuisathaverat Nov 13, 2020
cb0a8f6
fix: ignore parameters in the URL for validation
kuisathaverat Nov 15, 2020
1b22692
Merge branch 'JENKINS-63350' of github.com:kuisathaverat/saml-plugin …
kuisathaverat Nov 15, 2020
7c7a310
Update pom.xml
kuisathaverat Feb 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions Jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
//def buildConfiguration = buildPlugin.recommendedConfigurations()

def lts = "2.176.1"
def weekly = "2.199"
def lts = "2.249.3"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

??

def weekly = "2.266"
kuisathaverat marked this conversation as resolved.
Show resolved Hide resolved
def buildConfiguration = [
/*
[ platform: "linux", jdk: "8", jenkins: lts, javaLevel: "8" ],
[ platform: "windows", jdk: "8", jenkins: lts, javaLevel: "8" ],
[ platform: "linux", jdk: "11", jenkins: lts, javaLevel: "8" ],
[ platform: "windows", jdk: "11", jenkins: lts, javaLevel: "8" ],
*/
// Also build on recent weekly
// [ platform: "linux", jdk: "11", jenkins: weekly, javaLevel: "8" ],
// [ platform: "windows", jdk: "11", jenkins: weekly, javaLevel: "8" ]
[ platform: "linux", jdk: "11", jenkins: weekly, javaLevel: "8" ],
[ platform: "windows", jdk: "11", jenkins: weekly, javaLevel: "8" ]
kuisathaverat marked this conversation as resolved.
Show resolved Hide resolved
]

buildPlugin(configurations: buildConfiguration)
63 changes: 37 additions & 26 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ under the License.
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>3.55</version>
<version>4.3</version>
</parent>

<artifactId>saml</artifactId>
Expand All @@ -42,9 +42,9 @@ under the License.
</scm>

<properties>
<revision>1.1.8</revision>
<revision>2.0.0</revision>
<changelist>-SNAPSHOT</changelist>
<jenkins.version>2.176.1</jenkins.version>
<jenkins.version>2.266</jenkins.version>
<java.level>8</java.level>
<jcasc.version>1.35</jcasc.version>
</properties>
Expand Down Expand Up @@ -78,6 +78,17 @@ under the License.
<id>repo.jenkins-ci.org</id>
<url>https://repo.jenkins-ci.org/public/</url>
</repository>
<!-- TODO remove when https://issues.jenkins.io/browse/INFRA-2809 is resolved -->
<repository>
<id>shib-release</id>
<url>https://build.shibboleth.net/nexus/content/groups/public</url>
<snapshots>
<enabled>false</enabled>
</snapshots>
<releases>
<enabled>true</enabled>
</releases>
</repository>
kuisathaverat marked this conversation as resolved.
Show resolved Hide resolved
kuisathaverat marked this conversation as resolved.
Show resolved Hide resolved
</repositories>
<pluginRepositories>
<pluginRepository>
Expand All @@ -90,8 +101,13 @@ under the License.
<dependency>
<groupId>org.pac4j</groupId>
<artifactId>pac4j-saml</artifactId>
<version>1.9.9</version>
<!-- versions 4.x.x require JDK 11 -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then set java.level to 11 I guess? Not sure of status of jenkinsci/plugin-pom#133.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is fine, 3.9.0 was released a few days ago

<version>3.9.0</version>
<exclusions>
<exclusion>
<groupId>org.springframework</groupId>
<artifactId>spring-beans</artifactId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may need to exclude more. Pay attention to what gets bundled in the *.hpi (also printed to build log).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, there is a bunch of transitive dependencies that can be removed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any way to acknowledge(remove the warnings) the dependencies? there are some warnings that come from the hpi plugin, but those dependencies are needed and I do not want to add all those dependencies manually to the plugin pom.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any way to acknowledge(remove the warnings)

Yes but do not do that.

there are some warnings that come from the hpi plugin, but those dependencies are needed

You can exclude dependencies you know you will not use, or you know you can use in the version supplied by core. Or you can switch to pluginFirstClassLoader and forget about accurate testing with JenkinsRule (must do it manually). Think twice, actually several times, before doing anything out of the ordinary. https://www.jenkins.io/doc/developer/plugin-development/dependencies-and-class-loading/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have excluded all that are not needed or are already in the Jenkins Core. so I guess that either I add these libraries to the pom or I live with these warnings


[WARNING] Bundling transitive dependency jcommander-1.48.jar (via pac4j-saml)
--
[WARNING] Bundling transitive dependency woodstox-core-5.0.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency istack-commons-runtime-3.0.8.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency FastInfoset-1.2.16.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency metrics-core-3.1.2.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency jakarta.xml.bind-api-2.3.2.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency javax.json-api-1.0.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency joda-time-2.9.9.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency xmlsectool-2.0.0.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency java-support-7.5.0.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency spymemcached-2.12.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency httpclient-4.5.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency httpcore-4.4.8.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency xmlsec-2.1.4.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency velocity-1.7.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency stax2-api-3.1.4.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency cryptacular-1.2.4.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency jaxb-runtime-2.3.2.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency txw2-2.3.2.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency javax.json-1.0.4.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency stax-ex-1.8.1.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency ldaptive-1.0.13.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-core-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-messaging-api-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-messaging-impl-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-profile-api-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-profile-impl-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-saml-api-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-saml-impl-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-security-api-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-security-impl-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-soap-api-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-soap-impl-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-storage-api-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-storage-impl-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-xmlsec-api-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-xmlsec-impl-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency pac4j-core-3.9.0.jar (via pac4j-saml)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will live with the warnings to remove them I have to add more than 300 no-sense lines to the pom to manage these dependencies manually.

</exclusion>
<exclusion>
<groupId>org.springframework</groupId>
<artifactId>spring-core</artifactId>
Expand All @@ -104,17 +120,17 @@ under the License.
<groupId>org.slf4j</groupId>
<artifactId>jcl-over-slf4j</artifactId>
</exclusion>
<exclusion>
<groupId>commons-codec</groupId>
<artifactId>commons-codec</artifactId>
</exclusion>
<exclusion>
<groupId>org.bouncycastle</groupId>
<artifactId>bcprov-jdk15on</artifactId>
</exclusion>
<exclusion>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpclient</artifactId>
<groupId>org.dom4j</groupId>
<artifactId>dom4j</artifactId>
</exclusion>
<exclusion>
<groupId>commons-codec</groupId>
<artifactId>commons-codec</artifactId>
</exclusion>
</exclusions>
</dependency>
Expand All @@ -129,21 +145,6 @@ under the License.
<artifactId>bouncycastle-api</artifactId>
<version>2.18</version>
</dependency>
<dependency>
<groupId>net.shibboleth.utilities</groupId>
<artifactId>java-support</artifactId>
<version>7.2.0</version>
<exclusions>
<exclusion>
<groupId>commons-codec</groupId>
<artifactId>commons-codec</artifactId>
</exclusion>
<exclusion>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
Expand Down Expand Up @@ -171,10 +172,20 @@ under the License.
</dependencies>
<dependencyManagement>
<dependencies>
<dependency>
<groupId>antlr</groupId>
<artifactId>antlr</artifactId>
</dependency>
<dependency>
<groupId>io.jenkins.tools.bom</groupId>
<artifactId>bom-2.249.x</artifactId>
<version>17</version>
<type>pom</type>
<scope>import</scope>
</dependency>
<dependency>
<groupId>org.cryptacular</groupId>
<artifactId>cryptacular</artifactId>
<version>1.2.4</version>
</dependency>
<!-- TODO: remove when CVE-2019-12400 is resolved -->
<dependency>
Expand Down
14 changes: 10 additions & 4 deletions src/main/java/org/jenkinsci/plugins/saml/OpenSAMLWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@
import org.pac4j.core.context.J2EContext;
import org.pac4j.core.context.WebContext;
import org.pac4j.saml.client.SAML2Client;
import org.pac4j.saml.config.SAML2Configuration;

import java.io.IOException;
import java.util.logging.Logger;

import static java.util.logging.Level.*;
Expand Down Expand Up @@ -91,9 +93,9 @@ protected WebContext createWebContext() {
* @return a SAML2Client object to interact with the IdP service.
*/
protected SAML2Client createSAML2Client() {
final SAML2ClientConfigurationCustom config = new SAML2ClientConfigurationCustom();
final SAML2Configuration config = new SAML2Configuration();
config.setIdentityProviderMetadataResource(new SamlFileResource(SamlSecurityRealm.getIDPMetadataFilePath()));
config.setDestinationBindingType(samlPluginConfig.getBinding());
config.setAuthnRequestBindingType(samlPluginConfig.getBinding());
config.setWantsAssertionsSigned(true);

SamlEncryptionData encryptionData = samlPluginConfig.getEncryptionData();
Expand Down Expand Up @@ -150,10 +152,14 @@ protected SAML2Client createSAML2Client() {
config.setServiceProviderMetadataResource(new SamlFileResource(SamlSecurityRealm.getSPMetadataFilePath()));
final SAML2Client saml2Client = new SAML2Client(config);
saml2Client.setCallbackUrl(samlPluginConfig.getConsumerServiceUrl());
saml2Client.init(createWebContext());
saml2Client.init();

if (LOG.isLoggable(FINE)) {
LOG.fine(saml2Client.getServiceProviderMetadataResolver().getMetadata());
try {
LOG.fine(saml2Client.getServiceProviderMetadataResolver().getMetadata());
} catch (IOException e) {
LOG.fine("Is not possible to show the metadata : " + e.getMessage());
}
}
return saml2Client;
}
Expand Down
21 changes: 0 additions & 21 deletions src/main/java/org/jenkinsci/plugins/saml/SAML2ClientConfigurationCustom.java
View file
Open in desktop

This file was deleted.

53 changes: 51 additions & 2 deletions src/main/java/org/jenkinsci/plugins/saml/SamlFileResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,19 @@
package org.jenkinsci.plugins.saml;

import org.apache.commons.io.FileUtils;
import org.apache.commons.lang.NotImplementedException;
import org.pac4j.core.exception.TechnicalException;
import org.pac4j.core.io.WritableResource;
import org.springframework.core.io.Resource;
import org.springframework.core.io.WritableResource;

import javax.annotation.Nonnull;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.UnsupportedEncodingException;
import java.net.URI;
import java.net.URL;
import java.util.logging.Logger;

/**
Expand Down Expand Up @@ -61,11 +65,36 @@ public boolean exists() {
return getFile().exists();
}

@Override
public boolean isReadable() {
return getFile().canRead();
}

@Override
public boolean isOpen() {
return false;
}

@Override
public URL getURL() {
throw new NotImplementedException();
}

@Override
public URI getURI() {
throw new NotImplementedException();
}

@Override
public String getFilename() {
return fileName;
}

@Override
public String getDescription() {
return fileName;
}

@Override
public InputStream getInputStream() throws IOException {
return FileUtils.openInputStream(getFile());
Expand All @@ -76,8 +105,28 @@ public File getFile() {
return new File(fileName);
}

@Override
public long contentLength() {
return getFile().length();
}

@Override
public long lastModified() {
return getFile().lastModified();
}

@Override
public Resource createRelative(String s) {
throw new NotImplementedException();
}

@Override
public boolean isWritable() {
return getFile().canWrite();
}

@Override
public OutputStream getOutputStream() throws IOException {
return FileUtils.openOutputStream(getFile());
}
}
}
2 changes: 1 addition & 1 deletion src/main/java/org/jenkinsci/plugins/saml/SamlRedirectActionWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.StaplerResponse;
import org.pac4j.core.client.RedirectAction;
import org.pac4j.core.redirect.RedirectAction;
import org.pac4j.core.context.WebContext;
import org.pac4j.core.exception.HttpAction;
import org.pac4j.saml.client.SAML2Client;
Expand Down
10 changes: 9 additions & 1 deletion src/main/java/org/jenkinsci/plugins/saml/SamlSPMetadataWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import org.kohsuke.stapler.StaplerResponse;
import org.pac4j.saml.client.SAML2Client;

import java.io.IOException;

/**
* build the Service Provider(SP) metadata from the configuration.
*/
Expand All @@ -41,6 +43,12 @@ public SamlSPMetadataWrapper(SamlPluginConfig samlPluginConfig, StaplerRequest r
@Override
protected HttpResponse process() throws IllegalStateException {
final SAML2Client client = createSAML2Client();
return HttpResponses.plainText(client.getServiceProviderMetadataResolver().getMetadata());
String metadata = "";
try {
metadata = client.getServiceProviderMetadataResolver().getMetadata();
} catch (IOException e) {
new IllegalStateException(e);
}
return HttpResponses.text(metadata);
}
}
4 changes: 2 additions & 2 deletions src/main/java/org/jenkinsci/plugins/saml/SamlSecurityRealm.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@
import org.jenkinsci.plugins.saml.user.SamlCustomProperty;
import org.kohsuke.stapler.*;
import org.kohsuke.stapler.interceptor.RequirePOST;
import org.pac4j.core.client.RedirectAction;
import org.pac4j.core.client.RedirectAction.RedirectType;
import org.pac4j.core.redirect.RedirectAction;
import org.pac4j.core.redirect.RedirectAction.RedirectType;
import org.springframework.dao.DataAccessException;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you not plan to switch to Spring Security versions of Jenkins interfaces as well, now that you are on a baseline with JEP-227?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, what is the replacement for DataAccessException?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is none.

Best to look at other PRs associated with JEP-227 for examples.

import org.pac4j.saml.profile.SAML2Profile;

Expand Down
Loading