Skip to:

  1. Skip to Jira Navigation
  2. Skip to Side Navigation
  3. Skip to Main Content

Add a new CredentialFactoryBean type which allows null objects from createInstance

Description

For reference see .

It appears the createInstance bean factory should not return null objects, but it is not enforced by spring so it, in fact, can return null objects (See Possible inconsistency between FactoryBean and AbstractFactoryBean nullability · Issue #31464 · spring-projects/spring-framework (github.com)) which are dealt with correctly in spring because the getObject() method of the FactoryBean interface allows null objects.

Confusing. Either way, our AbstractComponentAwareFactoryBean does not allow null objects to be created, but it can be useful for CredentialFactoryBean types to return null. So we should build a suitable replacement hierarchy of credential factory bean types to handle the null case. This can be added to commons for now, but might later end up in one of the IdP’s shared libraries.

Environment

None

Activity

Show:

Philip Smart October 25, 2023 at 3:37 PM

After discussion, we reverted this change. Really the fix needed to correct the contract in the Spring base class. That was fixed in [JSSH-41] Add nullable factory bean support - Shibboleth Jira (atlassian.net).

For now, BasicJWKCredentialFactoryBean will just reference the non-null parent methods in shib-spring 9.0.0 (even though it can return null). When we move commons to shib-spring 9.1.0 it will update to use the nullable parent methods.

We then either pull the failIfResourceIsNull property from the next version of commons and require a new version of the RP and hence IdP (which contains the new shib-spring). Or we deprecate the method and defer until a future release, e.g.:

I’ll leave this open for now.

Philip Smart October 25, 2023 at 12:07 PM

Actually, given throwIfBeanIsNull is on AbstractNullableCredentialFactoryBean it makes more sense to call the property throwIfCredentialIsNull. It is possible we will want to move that exception-handling logic up to the parent AbstractNullableComponentAwareFactoryBean, if so it would need to be switched back to throwIfBeanIsNull or similar. I will make this change for now, and we can adjust if required.

Philip Smart October 25, 2023 at 9:40 AM
Edited

Philip Smart October 24, 2023 at 4:00 PM

I’ve created two new factory classes: AbstractNullableCredentialFactoryBean and AbstractNullableComponentAwareFactoryBean — I added nullable to the names to signal something was unordinary about them, but I guess they do not need it. 

AbstractNullableComponentAwareFactoryBean allows the createInstance and doCreateInstance methods to return null. This, as discussed, violates the parent's non-null contract for createInstance but not that of the getObject method on the FactoryBean interface. The non-null warning from the parent's method has been suppressed by annotation. 

doCreateInstance in AbstractNullableCredentialFactoryBean delegates the work to a new doCreateCredential method implemented in a child class.  doCreateInstance will throw an exception if the bean returned from doCreateCredential is null and the new throwIfBeanIsNull boolean is set to true (which is the default). Otherwise, if throwIfBeanIsNull is false, it will return a null bean.

I’ve left, but deprecated, the setFailIfResourceIsNull in the BasicJWKCredentialFactoryBean — this just passes the value upward to the setThrowIfBeanIsNull(boolean). That way it is compatible with the existing config. 

I’ll look to commit that tomorrow to make it more concrete.

Details

Assignee

Reporter

Components

Created October 20, 2023 at 1:55 PM
Updated October 25, 2023 at 3:37 PM