Saml2LoginConfigure Null pointer exception due to Java module introduction

[AbdulR3hman]

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:

spring-security/config/src/main/java/org/springframework/security/config/annotation/web/configurers/saml2/Saml2LoginConfigurer.java

Lines 346 to 355 in bff3779

	private Saml2AuthenticationRequestFactory getResolver(B http) {
	Saml2AuthenticationRequestFactory resolver = getSharedOrBean(http, Saml2AuthenticationRequestFactory.class);
	if (resolver == null) {
	if (Version.getVersion().startsWith("4")) {
	return new OpenSaml4AuthenticationRequestFactory();
	}
	return new OpenSamlAuthenticationRequestFactory();
	}
	return resolver;
	}

This was introduced by this issue: #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 .

[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())?

[AbdulR3hman]

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

[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.

[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.

[AbdulR3hman]

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