Spring Security Saml2LoginConfigure Null pointer exception due to Java module introduction

Describe the bug

The issue is a Java null pointer exception resulted from checking for OpenSAML version within the SAML2 login configurer when used with Java Modules rather than Classpath.

Exception:

Caused by: java.lang.NullPointerException: null
    at spring.security.config@5.5.1/org.springframework.security.config.annotation.web.configurers.saml2.Saml2LoginConfigurer$AuthenticationRequestEndpointConfig.getResolver(Saml2LoginConfigurer.java:349) ~[spring-security-config-5.5.1.jar:na]
    at spring.security.config@5.5.1/org.springframework.security.config.annotation.web.configurers.saml2.Saml2LoginConfigurer$AuthenticationRequestEndpointConfig.build(Saml2LoginConfigurer.java:340) ~[spring-security-config-5.5.1.jar:na]
    at 

This is due to the code that doesn't check if the nullable Version class is null:

https://github.com/spring-projects/spring-security/blob/bff377904a60ce2ed97b382be5c057d2ed6dd683/config/src/main/java/org/springframework/security/config/annotation/web/configurers/saml2/Saml2LoginConfigurer.java#L346-L355

This was introduced by this issue: https://github.com/spring-projects/spring-security/issues/9095

To Reproduce

Simply import saml2 provider: (with spring-boot version: 2.5.2)

        <dependency>
            <groupId>org.springframework.security</groupId>
            <artifactId>spring-security-saml2-service-provider</artifactId>
        </dependency

And build with maven without module-info.java file, which means it will be using the classpath instead. This will work, however migrating to module will make the Version of Opensaml completely null resulting in the above exception.

Expected behavior

Check for null Version class as it is allowed to be nullable org.opensaml.core.Version .

Comment From: jzheaux

Thanks for the report, @AbdulR3hman.

It won't fix the problem to check for null since Spring Security must know the version of OpenSAML to select the correct implementation.

Perhaps the configurer could look for a class that is present in OpenSAML 4 that does not exist in OpenSAML 3. Are you able to submit a PR that replaces Version.getVersion().startsWith("4") with something like ClassUtils.isPresent("some.opensaml4.Class", Version.class.getClassLoader())?

Comment From: AbdulR3hman

@jzheaux I'd be glad to do a PR. I'll make the changes based on your recommendations and take it from there.

Comment From: AbdulR3hman

Hi @jzheaux I've made the changes based on your recommendations; there was only two classes introduced in OpenSaml4 that are not in 3. However I didn't open the PR because there is an alternative solution to this; might not be as beautiful as the original solution, but the original solution might be confusing as to why we are checking for a random class.

This is the alternative solution that we've tested on Opensaml 3 and 4 with both Module Path and Class Path:

    private void registerDefaultAuthenticationProvider(B http) {

        if ((Version.getVersion() != null && Version.getVersion().startsWith("4"))
                || (Version.class.getModule().getDescriptor() != null
                && Version.class.getModule().getDescriptor().version().isPresent()
                && Version.class.getModule().getDescriptor().version().get().toString().startsWith("4"))) {
            http.authenticationProvider(postProcess(new OpenSaml4AuthenticationProvider()));
        } else {
            http.authenticationProvider(postProcess(new OpenSamlAuthenticationProvider()));
        }
    }

Let me know which one you prefer and I'll make the PR accordingly.

Comment From: jzheaux

What you've described sounds workable, though I'd recommend adjusting it this way:

private String version() {
    String version = Version.getVersion();
    if (version != null) {
        return version;
    }
    return Version.class.getModule().getDescriptor().version()
            .map(Objects::toString)
            .orElseThrow(() -> new IllegalStateException("cannot determine OpenSAML version"));
}

If the version cannot be determined, then the application should fail to start.

Comment From: AbdulR3hman

That's great, I'll get a PR ready.

Comment From: AbdulR3hman

as request. However I had one case failing locally, and it was regarding javadoc creation. I assumed the test was due to some mis-configured local settings. (although I was simply running it from WSL2 with JDK11, no IDE or anything like that to localize the issue) but no luck. This wasn't an issue with the last run executed by you. So I've chose to ignore it for now.