Gh 11661 Configurable authentication converter for resource-servers with token introspection

[ch4mpy]

This could close https://github.com/spring-projects/spring-security/issues/11661

It adds configurable authentication converter for resource-servers with token introspection (something very similar to what JwtAuthenticationConverter does for resource-servers with JWT decoder).

The new (Reactive)OpaqueTokenAuthenticationConverter is given responsibility for converting successful token introspection result into an Authentication instance (which is currently done by a private methods of OpaqueTokenAuthenticationProvider and OpaqueTokenReactiveAuthenticationManager).

(Reactive)BearerTokenAuthenticationConverter, the default (Reactive)OpaqueTokenAuthenticationConverter, behave the same as current private convert(OAuth2AuthenticatedPrincipal principal, String token) methods: map authorities from scope attribute and build a BearerTokenAuthentication.

New unit test classes are created for default authentication converters and tests related to authorities mapping are moved there.

Existing tests for OpaqueTokenAuthenticationProvider and OpaqueTokenReactiveAuthenticationManager are modified to simply assert that introspector and authenticationConverter are called and that resulting Authentication is propagated.

P.S. I set 6.0 as target but can of course modify this to 5.8 or whatever depending on how you schedule the feature (additional XSDs could need an update depending on the branch this is merged to).

[ch4mpy]

@sjohnr and @marcusdacoregio I believe this PR to be ready for review.

[sjohnr]

Thanks @ch4mpy! As we're working through some priority tasks for 5.8 and 6.0, it may be a slight delay before I'm able to take a look at this.

[ch4mpy]

@sjohnr I understand. Just put it together right now because I had a little time and the idea was fresh in my mind.

I couldn't find the issue / PR I'm duplicating. Could you please point me to it?

[sjohnr]

Thanks for taking the time! We use the duplicate label for book-keeping so only one issue (either the issue or the PR) goes into the release notes, and so whoever did the work gets credit. Since you opened the issue AND the PR, I just marked this PR as the duplicate.

[ch4mpy]

@sjohnr I added a separate commit for 5.8 XSD and doc, just in case you have some time to review any time soon. The commit is easy to remove otherwise.

There is no breaking change with this PR, default behavior should remain the same.

[sjohnr]

Thanks @ch4mpy. Apologies, I haven't been able to review it thoroughly yet, as I'm still heads down. However, glancing at it, I wanted to mention one thing to think about. You may want to consider the Converter interface from spring-core. It's used fairly extensively throughout the framework already, and we'd probably want to prefer that over introducing new interfaces. Switching that over will get you a bit ahead on review feedback.

I'll let you know when I have a bit more time to review.

[ch4mpy]

Actually, I had considered using the Converter interface but had two reserves :

• current convertion methods have two parameters (token string and OAuth2AuthenticatedPrincipal) => I'd have to create a new class, let's say IntrospectionResult, to wrap the two parameters into a single one.
• due to generics erasure or something, in an application or two where I wanted to override several converters, I had to curry it for beans to be correctly wired (do semething like public interface ABConverter extends Converter<A, B> {}).

I'll update the PR with:

• Converter<IntrospectionResult, ? extends Authentication> instead of OpaqueTokenAuthenticationConverter
• Converter<IntrospectionResult, Mono<? extends Authentication>> instead of ReactiveOpaqueTokenAuthenticationConverter

Please confirm that IntrospectionResult is fine for naming the converter source type.

[sjohnr]

Please confirm that IntrospectionResult is fine for naming the converter source type.

I'll spend some time thinking about that.

[ch4mpy]

@sjohnr I:

• rebased (twice, was lucky enough to pick an unstable main the first time)
• reverted to OpaqueTokenAuthenticationConverter interface and made it a @FunctionalInterface
• found a way to share code between servlet and reactive confs for default OpaqueTokenAuthenticationConverter
• added a test to assert XML conf for optional OpaqueTokenAuthenticationConverter is actually used
• removed breaking change by adding constructors with OpaqueTokenAuthenticationConverter argument (and keeping constructors without such an argument)
• removed var and record occurences

[ch4mpy]

Rebased on main after https://github.com/spring-projects/spring-security/pull/11773 was merged and squashed again.

[ch4mpy]

@sjohnr, do you expect more changes from me?

Also, what about OpaqueTokenAuthenticationFactory instead of OpaqueTokenAuthenticationConverter?

[ch4mpy]

we will pause at that point and wait on this PR until after November (after the 5.8/6.0 release).

So I should remove modifications to 5.8 xsd and rnc too, maybe? And also modify @Since comments. Do you know what the target version will be?

[ch4mpy]

@sjohnr I think I fixed everything as you requested, but static BearerTokenAuthentication convert(String introspectedToken, OAuth2AuthenticatedPrincipal authenticatedPrincipal) for which I set visibility to package instead of private to keep code de-duplication

[sjohnr]

So I should remove modifications to 5.8 xsd and rnc too, maybe? And also modify @since comments. Do you know what the target version will be?

We can wait on that, but thanks for asking! I'll try and keep you updated if anything changes.

[sjohnr]

Thanks for the updates @ch4mpy. After discussing with the team, I think we're good to merge PRs from the community into 5.8. As I mentioned, we've switched to merge-forward as a strategy, so we ultimately want to target 5.8 for this PR. If you happen to have a bit more time, you could squash commits and move the commit to the 5.8.x branch to see if it moves without conflicts (I believe you can edit the base of this PR without opening a new one). If you don't have time, don't worry about it and I can try and find time in the next week or two to get it moved over.

[ch4mpy]

@sjohnr squashed and rebased on 5.8.x.

Of course, 6.0 xsd and rnc modifications where lost in the process.

[sjohnr]

Merged to 5.8.x as 1efb63387f14dc7858f445884f34b70af30b85b4. I added polish in 355ef2111739e0eafb1e0d2fbc8d19ba13f00f0a and 1aee40dcca30e5d6eddaf22006ae26286e41ba89.