Spring-security: ConversionService should be configurable for BasicLookupStrategy and JdbcAclService

[pwheel]

Summary

In #4424 I added AclClassIdUtils to handle some common logic for dealing with acl classes with types other than Long. I thought this class would be internal, then decided it was better to have it explicitly defined.

The class has package level visibility, but should be public.

Actual Behavior

Unable to define a bean of type AclClassIdUtils using Java Config as the class is not public.

Expected Behavior

Should be able to define a bean of type AclClassIdUtils using Java Config.

Configuration

@Bean
public AclClassIdUtils aclClassIdUtils() {

    AclClassIdUtils classIdUtils = new AclClassIdUtils();
    classIdUtils.setConversionService(new DefaultConversionService());

    return classIdUtils
}

UPDATE: This ticket's proposal changed during the time it was being implemented. Instead of exposing AclClassIdUtils as public, it was resolved to make BasicLookupStrategy and JdbcAclService expose setConversionService which they'd ultimately provide to AclClassIdUtils.

For example in BasicLookupStrategy doing:

public void setConversionService(ConversionService conversionService) {
    this.aclClassIdUtils = new AclClassIdUtils(conversionService);
}

In this way, #4424 is addressed while still keeping AclClassIdUtils package-private.

[mehdichitforoosh]

Hi.
@pwheel when this issue will be closed?
I need to use ConversionService in AclClassIdUtils in java config but i can't because it is internal.
I see also issue #4424 ,It seems the problem still exists...
Thanks.

[TomRK1089]

Any movement on this? I'd also mention that #4424 is not truly solved in my opinion. On Postgres, using a UUID causes an error because the queries to check for existing IDs attempt to cast between a proper UUID type in Postgres and the varchar column definition in the included schemas.

If schemas are going to be included with the library, they should work out of the box, is my expectation. If not, then make it clear I need to write a MutableAclService from scratch. As is I've wasted two days trying to get JdbcMutableAclService to work with non-Long identifiers.

[nenaraab]

Hi, i still have the issue to inject a ConversionService into the AclClassIdUtils class. The AclClassIdUtils class itself can not be instantiated as it is package protected. It can not be overwritten, as there is no interface defined...

My problem:

• in my case, my acl_object_identity.object_id_identity can be of type java.lang.Long (no issue), java.lang.String and UUID.
• Therefore I've introduced the column class_id_type in table acl_class
• I've added setAclClassIdSupported(true) to both my BasicLookupStrategy and JdbcMutableAclService
• But, even in case of ids of type String (!) i run finally into NumberFormatException, because the conversionService is null in AclClassIdUtils (same as documented here) and finally it is unable to convert the id of type String to String...
• Anyhow... in any case, String or UUID I'm unable to provide/inject a ConversionService to the BasicLookupStrategy or finally into the AclClassIdUtils class.

My questions:

• This commit looks promising. But it seems not (yet?) be merged into master.
• Any other workaround, especially for object-id of type String?

References

• related issue: #4424
• related commit: https://github.com/pwheel/spring-security/commit/8efec0243f0f7580c9100d0ac034ac881907cc0a

[rwinch]

@nenaraab Thanks for the bump. The PR is now merged into master. Can you give it a try? You will be able to find the SNAPSHOT in about 30 minutes in the https://repo.spring.io/libs-snapshot Maven repository

[nenaraab]

@rwinch
Thanks a lot for your fast reaction! This fix solves the issue!

Minor suggestion:
I would suggest to initialize the conversionService as part of the AclClassIdUtils with GenericConversionService as this would then fix the issue of requiring an converter for String to String conversions.

[rwinch]

@nenaraab Thanks for the suggestion. Would you be interested in submitting a PR that includes a test?

[pwheel]

Thanks @rwinch for merging 👍
Interesting feedback @nenaraab. Thanks for confirming it works 😄 My use case was with UUIDs, so I missed this when I wrote it. Agree it would be slicker to default as you suggest. I'll wait to see your response to the above…

[nenaraab]

@pwheel @rwinch Sorry for the delay... please find my pull request (incl. Unit tests) referenced.

[SayakMukhopadhyay]

Any idea which version of Spring security this would be available in? I have seen that it is available in 5.2.0M1 and the GA is scheduled to be in July. It would be great if this is available in 5.1.5. This would mean that I can update spring boot to 2.1.4 and get this fix in a week or so.

[jzheaux]

@SayakMukhopadhyay, we try and follow semantic versioning principles, which means no new public APIs in a patch release.

Since this PR exposes new public methods (e.g. BasicLookingStrategy#setConversionService), it means that we should wait for the 5.2 release.

[SayakMukhopadhyay]

That's a bummer. In the meantime I have worked around the issue using reflection. For anyone looking for help here is the relevant code snippet:

try {
    Field aclClassIdUtilsField = basicLookupStrategy.getClass().getDeclaredField("aclClassIdUtils");
    aclClassIdUtilsField.setAccessible(true);
    Object aclClassIdUtilsObject = aclClassIdUtilsField.get(basicLookupStrategy);
    Field conversionServiceField = aclClassIdUtilsObject.getClass().getDeclaredField("conversionService");
    conversionServiceField.setAccessible(true);
    conversionServiceField.set(aclClassIdUtilsObject, conversionService());
} catch (NoSuchFieldException | IllegalAccessException e) {
    e.printStackTrace();
}

[jzheaux]

Thanks for reaching out with a workaround, @SayakMukhopadhyay, and sorry if you are experiencing some heartburn over it. The community as a whole gets a lot of benefit from knowing that patch releases are always backward-compatible, which is why we adhere to it.

[LBoraz]

this is still an issue in 5.1.6.RELEASE

[satnav]

Even in the 5.2.0.RELEASE this is still an issue

[jzheaux]

@LBoraz and @satnav, I think there's some confusion caused by a difference between the title of the ticket and the final decision that was made in the PR.

Please see the updated ticket description for details.