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

Check for opensaml version on OpenSaml support classes #10693

Closed
wants to merge 3 commits into from

Conversation

igorpele
Copy link
Contributor

In OpenSaml support classes the Saml2VersionUtils class is used to check whether a supported version of OpenSaml is found on the classpath.

Closes gh-10567

In OpenSaml support classes the Saml2VersionUtils class is used to check whether a supported version of OpenSaml is found on the classpath.

Closes spring-projectsgh-10567
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 10, 2022
@marcusdacoregio marcusdacoregio self-assigned this Jan 11, 2022
@marcusdacoregio marcusdacoregio added in: saml2 An issue in SAML2 modules status: duplicate A duplicate of another issue type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 11, 2022
@marcusdacoregio marcusdacoregio added this to the 5.7.x milestone Jan 11, 2022
@marcusdacoregio
Copy link
Contributor

Hi @igorpele, just a heads up that we are discussing this solution and I should get back to you soon.

@marcusdacoregio
Copy link
Contributor

Waiting for #10817 to get merged, since the way to verify the OpenSAML version may change

@marcusdacoregio marcusdacoregio modified the milestones: 5.7.x, 5.8.x Jul 29, 2022
Comment on lines 81 to 88
private static String getVersion() {
String version = Version.getVersion();
if (version != null) {
return version;
}
return Version.class.getModule().getDescriptor().version().map(Object::toString)
.orElseThrow(() -> new IllegalStateException("cannot determine OpenSAML version"));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We have changed the way we check the version, can you change it to:

Suggested change
private static String getVersion() {
String version = Version.getVersion();
if (version != null) {
return version;
}
return Version.class.getModule().getDescriptor().version().map(Object::toString)
.orElseThrow(() -> new IllegalStateException("cannot determine OpenSAML version"));
}
private static String getVersion() {
String version = Version.getVersion();
if (StringUtils.hasText(version)) {
return version;
}
boolean openSaml4ClassPresent = ClassUtils
.isPresent("org.opensaml.core.xml.persist.impl.PassthroughSourceStrategy", null);
if (openSaml4ClassPresent) {
return OPEN_SAML_4_VERSION;
}
throw new IllegalStateException("cannot determine OpenSAML version");
}

There is more detail here

* classpath.
*
* @author Igor Pelesić
* @since 5.7
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change it to @since 5.8?

pelesic added 2 commits September 14, 2022 16:43
    In OpenSaml support classes the Saml2VersionUtils class is used to
check whether a supported version of OpenSaml is found on the classpath.

    Closes spring-projectsgh-10567
@igorpele
Copy link
Contributor Author

igorpele commented Sep 14, 2022

Hi @marcusdacoregio is this ok now?
But I am not sure, as now if Version.getVersion() returns no value, Saml2VersionUtils.getVersion now returns the OPEN_SAML4_VERSION (4) which always be below 4.1.0 which is supported by spring-security? The "old" approach returned the accurate version of OpenSaml from classpath something like "4.0.1".

Or can I assume that if the class "org.opensaml.core.xml.persist.impl.PassthroughSourceStrategy" is present on the classpath that the OpenSaml version is higher than 4.1.0?

@marcusdacoregio
Copy link
Contributor

Hey @igorpele, thanks for your contribution.

Unfortunately, this has not made it into our 5.8 release and we do not have another release planned for the 5.x line.
In Spring Security 6, we are only using OpenSAML4, therefore there is no need for a version check.

We might still do something like this if we add OpenSAML5 #11658.

@marcusdacoregio marcusdacoregio removed this from the 5.8.x milestone Nov 3, 2022
@marcusdacoregio marcusdacoregio added status: declined A suggestion or change that we don't feel we should currently apply and removed status: duplicate A duplicate of another issue labels Nov 3, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: saml2 An issue in SAML2 modules status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check for opensaml version on OpenSaml support classes
3 participants