JwtGrantedAuthoritiesConverter should allow configuring the authority prefix

[jzheaux]

Related to #6945 and #7100

It would be nice to configure JwtGrantedAuthoritiesConverter with what scope prefix to use for the resulting GrantedAuthoritys:

JwtGrantedAuthoritiesConverter converter = new JwtGrantedAuthoritiesConverter();
converter.setAuthorityPrefix("ROLE_");

This would involve adding a new method to JwtGrantedAuthoritiesConverter, namely setAuthorityPrefix. The code would then use this prefix instead of the hardcoded "SCOPE_" prefix.

The default value should still be "SCOPE_".

[ch4mpy]

Changed the spec a bit: have prefix configurable per authority claim name

I did this because authorities claim name actually has to be a collection of claim names (default is ["scope", "scp"]) and, also, it is very convenient to add a claim to scan for more authorities. But, in such a case, it is likely that required prefix would change with claim name.

IMO, legit use-case would be parsing authorities claim with no prefix at all and eventually adding SCOPE_ prefixed authorities retrieved from scope and scp claims.

[jzheaux]

I disagree that the claim name needs to be a collection of claim names. The fact that it originally is a collection of claim names is an internal implementation detail to the class.

Let's keep this class very simple - if the application needs to do something complex like adding a different prefix for different claims, then it can implement the interface quite easily itself.

[ch4mpy]

I suggest you close my PRs and provide the implementation you already have in mind, then.

[jzheaux]

Thank you, @ch4mpy, for the PRs. I can tell that you think deeply and want your contributions to be helpful to the community.

I'm a little confused by your last comment, though. Do you feel like we are at an impasse?

Actually, I've just been focusing on your comments here to make sure that the description in the ticket reflects the work to be done before doing a PR review. Once we're aligned here on the contract, I figure that the PR step will go quickly.

Do you have concerns about simply doing setAuthorityPrefix(String)?

Since a single prefix for a single claim is a more common scenario than multiple claims and multiple prefixes, it should be the simpler of the two to articulate.

Also, having something simpler like setAuthorityPrefix(String) makes this class a building block for more complex use cases like your own. You could then use multiple instances of JwtGrantedAuthoritiesConverter and perform an aggregation. Perhaps there is merit in adding some kind of aggregation converter to Spring Framework as well.

setAuthorityPrefix(String) is also nice because it's evocative of setRolePrefix(String), which appears in several places throughout the codebase.

[ch4mpy]

Since a single prefix for a single claim is a more common scenario than multiple claims and multiple prefixes, it should be the simpler of the two to articulate.

Well, currently its not a single claim, but a list of claims (scope and scp).

a single prefix for a single claim is a more common scenario

It seams likely to me that adding a claim to existing ones (namely authorities in addition to scope and scp) would be a quite common case.

It also seams likely that prefix would be different with added claims (empty string seems a good default for authorities)

Based on two preceding statements, something like addAuthoritiesClaimName("authorities", "") is way more useful than setting each independently.

Once we're aligned here on the contract, I figure that the PR step will go quickly.

I'm not sitting in your office, have no other channel to chat with you than Github, have different native language & time-zone and very episodic internet connection. So, I doubt that aligning the contract or finalizing a PR would be any quick.
Additionally, I feel like you have a precise idea of the solution you would accept to merge. Maybe everybody would save time if you provide it directly.

[jzheaux]

Additionally, I feel like you have a precise idea of the solution you would accept to merge. Maybe everybody would save time if you provide it directly.

The only thing I have in mind is what is in the description on the ticket. How I imagine this goes is: An individual (me, in this case) proposes a change in a ticket, and an individual (you, in this case) offers to fulfill that ticket. As you have pointed out, anyone can offer to fulfill the ticket, and of course, anyone can propose another ticket.

The reason you and I are having a discussion is that you have proposed an alternative contract to what's described in the ticket. You've demonstrated interest in getting this contract right, and, because of that, I'm very interested in working with you to determine how to improve the ticket's proposal or determine that its proposal is invalid, adjusting or closing the ticket as necessary.

To be clear, my goal with this conversation is to determine whether or not to adjust the description in the ticket (or close the ticket as invalid). And, yes, if the description does not ultimately get adjusted, I'd expect anyone who volunteers to fulfill the ticket to follow the description. Even if I weren't a committer, I'd expect this in the tickets I propose.

Or, if a community member wants to offer their own proposal, then they do so in the ticket as a comment (as you did), which triggers a discussion that hopefully results in a new agreement, the description is adjusted, and now, once again, an individual can submit a PR that follows the description in the ticket.

Parenthetically, I hope you recognize that I spend the time that I do on these discussions because of how important your feedback and ideas are to me. If this were about me getting my way, I'd just make the change to the code and move on.

a list of claims (scope and scp).

The reason there is a list of claims is that JwtGrantedAuthoritiesConverter doesn't know which single claim an application is expecting from incoming JWTs. You'll note that the code short-circuits once it finds a claim in the list. If it knew which single claim an application wanted to use, then it wouldn't loop.

adding a claim to existing ones

If an application knows which single claim it is expecting (authorities, for example), then why would I want to add a claim to a list? It would be more efficient to state which single claim to expect and avoid the loop altogether.

The typical application isn't, for example, a multi-tenant resource server that is accepting JWTs from many different issuers, each with their own representation. It's more typical to be accepting JWTs from a single issuer that has a single representation. We certainly want to support the multi-tenant scenario as well, and it seems to me that maintaining the single-claim focus of this class allows for that scenario via composition.

The scenario where a single JWT contains more than one claim isn't supported by this class, and adding a claim to the list wouldn't change that. But, being able to indicate which claim to use would support this scenario because I can now compose more than one converter together, each configured with their own claim name.

something like addAuthoritiesClaimName("authorities", "") is way more useful than setting each independently

If this class continues to focus on determining a single claim, then another class can be added that addresses more complex scenarios, automatically composing JwtGrantedAuthoritiesConverters together. I think it will take more time to see how the community uses JwtGrantedAuthoritiesConverter to know what's best, though. Maintaining the focus on a single claim will allow the community greater compositional flexibility, resulting in a better collective understanding of how more complex scenarios ought to be represented.

And because it's so simple to create a custom converter, it's better to limit the functionality in this class than to try and have it answer every use case.

[ch4mpy]

I know you are spending too much time in this thread. This is exactly what I mean when I write "everybody would save time".

Maybe, you're misinterpreting my frustration: it has little to do with the contract or implementation I propose here being rejected. Actually, I'm not much interested in converters reading authorities from token claims.

I opened #6945 because I think

• scope claim should be used not to transport authorities but to determine what subset of subject authorities should be included in Authentication object
• there is even no requirements for authorities to be contained in token claims.

The ticket was closed with not much arguments explaining why my OAuth2 specs understanding is wrong and we are now talking about implementation details of a solution providing improvements on none of above...

It was about the same thing when I, for instance, raised a potential design smell on AbstractOAuth2Token (allowing different values of issue or expiry instants in members and claims) or stressed the point that only annotations allow to unit-test secured @Service.
I shot a question about the necessity to include more than token claims in authentication in #6830 but have not much hope it will be considered any differently.

Result is I'm not using anymore big parts of the OAuth2 framework: JwtAuthenticationToken, OAuth2IntrospectionAuthenticationToken, any of AbstractOAuth2Token descendent or even the unit-test flow APIs I contributed.

I instead use:

• OAuth2ClaimSetAuthentication<T extends UnmodifiableClaimSet & Principal>, an Authentication implementation of my own
• JwtClaimSet or IntrospectionClaimSet instead of AbstractOAuth2Token descendents
• annotation support you skipped, because I have secured @Service

I already referenced it here and there, but you can find all that on Github and maven-central.

[jzheaux]

If I'm understanding correctly, you are feeling like your voice isn't being heard, and for that, I apologize. Let's see what we can do here.

The ticket was closed

If you feel like there is more to discuss, you can always open #6945 back up. The reason I closed it was because it appeared at the time that we were at a consensus that resulted in #7100 and #7101.

I'll not comment on that discussion here, but please feel free to continue there on that ticket to help me see what I am missing from your original points.

Actually, I'm not much interested in converters reading authorities from token claims.

Does this ticket's proposal prevent the API from achieving the points of yours that are not yet addressed?

If the proposal prevents the progress you want, can you help me understand why your new proposal gets the API closer to that goal? I think it's important we make the right decision here.

If it doesn't prevent said progress, then do I understand correctly that you'd prefer to not contribute a PR for this ticket which follows the proposal?

[ch4mpy]

do I understand correctly that you'd prefer to not contribute a PR for this ticket which follows the proposal?

Yes.

This is what I meant with

I suggest you close my PRs and provide the implementation you already have in mind, then

Sorry if it was un unclear.

[jzheaux]

I'm going to try and infer a few things from your comments, @ch4mpy. Please feel free to chime in if I've misinterpreted anything. If I don't hear back from you in a few days, I'll assume that we are good to move forward with this ticket as-is.

You stated in this ticket a few concerns. It appears that most of these are addressed already via #6945 (comment), though I'll add a quick point or two here.

The first concern you raised is that JwtGrantedAuthoritiesConverter should take a set of claim-name/claim-prefix pairs so that it can support looking up multiple claims in a JWT, giving different prefixes accordingly. This configuration is easily achieved by keeping JwtGrantedAuthoritiesConverter focused on a single claim and encouraging composition, e.g.:

JwtGrantedAuthoritiesConverter authoritiesConverter =
    new JwtGrantedAuthoritiesConverter();
authoritiesConverter.setAuthorityPrefix("");
authoritiesConverter.setAuthoritiesClaimName("authorities");
JwtGrantedAuthoritiesConverter scopeConverter =
    new JwtGrantedAuthoritiesConverter();
scopeConverter.setAuthoritiesClaimName("scope");
Converter<Jwt, List<GrantedAuthorities>> jointConverter = 
    new MyCompositeJwtGrantedAuthoritiesConverter(authoritiesConverter, scopeConverter);

Certainly, it may be reasonable to introduce a composite class into Spring Security, though such is out of scope for this ticket.

For the most part, it's anticipated that a typical application won't pull claims from multiple locations in the same JWT, but this construction can be simplified in a separate ticket should it prove a common practice.

Second, you stated your opinion that "scope claim should be used not to transport authorities but to determine what subset of subject authorities should be included in Authentication object". Please see my linked comment. I'll simply add here that as more community members manifest an affinity for this practice, it'll become clearer what role the framework has in supporting it.

As far as this ticket is concerned, though, it doesn't appear that adding the setAuthorityPrefix method will adversely affect any future plans to add support for this practice.

Third, you stated, "there is even no requirements for authorities to be contained in token claims." See my linked comment about how an application can add additional enforcement requirements via a JwtAuthenticationConverter. Again, if this demonstrates itself as a common practice, it'll be clearer what code should be rolled into Spring Security.

Adding setAuthorityPrefix shouldn't prevent you from customizing your authentication requirements.

Related to this is a point you made about wanting JwtGrantedAuthoritiesConverter to look for the authorities claim by default. See my linked comment for why we should hold off on this. Making this configurable will be taken care of by #7100.

Fourth, you brought up some concerns you've raised in the past from other tickets, though these, while important observations, appear unrelated to the proposal. As always, if you feel there is more to discuss on any of the tickets you alluded to, feel free to reopen the discussion.

To sum up, while I hope that you continue to share feedback and make contributions, it appears that the existing proposal doesn't adversely affect how you'd like to see the codebase evolve. As such, I say we are a "go" for this ticket.

Let's keep an eye on how the community reacts to these simpler changes to find out how the codebase can best support the practices you advocate.

[spring-projects-issues]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[andifalk]

Hi @jzheaux
has anybody already claimed this first timers issue?
Would make sense to implement this one together with #7100. If this is ok for you I would be glad to take over implementation for both.

[jzheaux]

@andifalk, sure, that would be great. Please feel free to take this and #7100.