Please remove OAuth2AuthoritiesPopulator

[rwinch]

Summary

I see a few problems with OAuth2AuthoritiesPopulator

I believe we should remove OAuth2AuthoritiesPopulator. We do not need this yet. It should be encapsulated within AuthenticationProvider. In the end this API is probably not necessary. The current API takes an Authentication and returns an Authentication which means this could be accomplished using the AuthenticationProvider interface and delegating to another AuthenticationProvider and then modifying the Authentication. This is also more flexible since it would work with any AuthenticationProvider API vs just JwtAuthenticationProvider.

The method name populateAuthorities does not lend to be very flexible. The method accepts an Authentication and returns an Authentication which means it could totally transform the Authentication type. With the input/output of Authentication, the interface and method name should be named to AuthenticationMapper or something like that.

As stated above OAuth2AuthoritiesPopulator is a bit limiting. Why is this an OAuth specific API?

The current implementation of OAuth2AuthoritiesPopulator just transforms the roles and provides new roles. This means we could leverage GrantedAuthoritiesMapper. Alternatively, (preferred for first iteration) I would encapsulate the logic inside the JwtAuthenticationProvider and we can extract it out later.

[jgrandja]

@rwinch I provided similar feedback on this but unfortunately my feedback comments (in the commit) is gone since a forced rebase happened after the fact.

Either way, we do need a way to extract authority information from the token and map it to 1 or more GrantedAuthority. What I proposed as a replacement for OAuth2AuthoritiesPopulator is this:

public interface OAuth2TokenAuthoritiesExtractor<T extends AbstractOAuth2Token> {

    Collection<? extends GrantedAuthority> extractAuthorities(T token);

}

Although, I'm not sure if I totally like the extract in the class name and method.

[rwinch]

Why is this OAuth specific (the class name and type boundary)?

More importantly, why do we even need the API? For now we should encapsulate the logic and extract it if/when necessary making sure to do so in a way that is not OAuth specific so it introducing the API has more meaning and flexibility.

[jgrandja]

Why is this OAuth specific (the class name and type boundary)?

Yes, the type boundary. This can be Jwt (extends AbstractOAuth2Token) and when we implement Token Introspection (opaque support) this might be IntrospectedAccessToken (extends AbstractOAuth2Token). This could also work for IdToken (also extends AbstractOAuth2Token), although I don't see a need for an OAuth2TokenAuthoritiesExtractor implementation for IdToken. But if we do later this will work here too.

why do we even need the API

Give this use case, where a token has authority information in the attribute/claim called groups and authorities, for example:

{
  "groups":["admin", "supervisor", "manager"], 
  "authorities": ["read", "write", "delete"]
}

The user wants to be able to map these authorities to GrantedAuthority's which will be composed within the Authentication.

How do you propose the user would do this if they don't have this OAuth2TokenAuthoritiesExtractor hook?

[rwinch]

A better wording is probably...why would this API need to be OAuth Specific? Is it possible/likely that we would want to extract authorities from other types of tokens (i.e. SAML, CAS)? If so, a generic type and bounds on the consuming class make a lot more sense.

Give this use case, where a token has authority information in the attribute/claim called groups and authorities, for example:

This is suppose to be an absolute minimum PR. So I should emphasize my statement:

if/when necessary making sure to do so in a way that is not OAuth specific so it introducing the API has more meaning and flexibility.

[jgrandja]

is it possible/likely that we would want to extract authorities from other types of tokens (i.e. SAML, CAS)?

Of course it is. However, CAS has already been implemented and so has SAML. Are you proposing we standardize on a base class for Token and than re-factor SAML and CAS? This is news to me as we haven't had any of these types of discussions.

I'm just not sure if this makes sense to me by opening things up to be more generic and re-useable. I don't see any issue with CAS, SAML and OAuth having their own base class for Token.

This is suppose to be an absolute minimum PR

So what are the authorities going to look like on the JwtAuthentication? I must be missing something here?

[rwinch]

Even if APIs have been written it doesn't mean that they wouldn't benefit from enhancements that can be done passively. This sort of change would be very easy to be passive. SAML is under a rewrite too. However, it could easily be bound by Authentication or a specific token type too.

The most important thing is that this PR is suppose to be absolute minimal. I'm not saying we don't add this functionality ever. What I'm saying is that we can provide the MVP with the code encapsulated. So my suggestion is to put this off until later.

[jgrandja]

As far as this comment goes:

So what are the authorities going to look like on the JwtAuthentication? I must be missing something here?

What will be the GrantedAuthority instance(s) in the JwtAuthentication?

[rwinch]

The result JwtAuthentication (and authorities) will be the same as they are today. We will just avoid exposing a new interface/implementation and instead encapsulate the logic.

[jgrandja]

I don't agree that this should be excluded from the initial PR. Providing an easy way for a user to hook into custom authorities extracting/mapping is needed. And having them implement an AuthenticationProvider to achieve this is not an easy option for the user.

I can agree to leave this out of the 1st PR. However, I'm assuming we will provide this capability in 5.1. Is this your expectation as well?

[rwinch]

However, I'm assuming we will provide this capability in 5.1. Is this your expectation as well?

Yes. I think leaving it out will help a lot. I think there is going to be a lot of back and forth on this API (there already has) so I'd like a PR focused on that.

[jgrandja]

Ok makes sense. I'll add that to the list of PR's

[jzheaux]

Just adding a few comments that I hadn't seen posted yet.

@jgrandja already referred to the general circumstance, where we need to derive authorities in a custom way. Specifically, this is the result of research around Keycloak's use cases. Note a related ticket from the sandbox: https://github.com/jzheaux/spring-security-oauth2-resource-server/issues/37 The Authorities Populator is simply a renaming in order to accommodate the extended contract advocated therein.

In that issue, @rwinch, I believe I understood you when you said:

I'd prefer to make this a bit more generic. Accepting an Authentication and returning an Authentication would be ideal. This allows for more powerful transformations.

Derived from this comment, we have JwtAuthoritiesPopulator which simply extracts scopes and converts them into SimpleGrantedAuthoritys.

Originally, the name was AuthoritiesExtractor and the contract was AbstractOAuth2Token -> Collection<? extends GrantedAuthority>, and then the class was updated to AuthoritiesPopulator and the contract to Authentication -> Authentication.

In the 2 implementations of this class that I did (JwtAuthoritiesPopulator and KeycloakAuthoritiesPopulator), I converted from an Authentication to an AbstractOAuth2Token and then proceeded. I'm not yet clear whether or not this would be typical, but given that, I am considering changing the contract to <T extends AbstractOAuth2Token> -> Authentication, thus the name change to OAuth2AuthoritiesPopulator, though I agree that this name is in flux.

Regarding your point about when to introduce this. I suppose it comes down to a growing clarity of what this particular PR is about. In the five PRs we discussed, I don't see one where this feature would be added, and yet it is, indeed, necessary in order to support Keycloak's use case. For that reason, I've included it in this PR, though I don't mind deferring it.

There is a minor problem with embedding it in the AuthenticationProvider, and that is scope attribute name configuration. For us to correctly transform scopes into authorities, we need to know the name of the scope attribute. Originally, the AuthenticationProvider held the property for scope attribute name, and thus a setter. Now, with Authorities Populator, the scope attribute name is configured there instead.

So, if we first place it in AuthenticationProvider, then we now have a public setter (scope attribute name) in AuthenticationProvider that we cannot remove, even though we know (accepting my premise from above) that is not where it will land.

There are two options for how we could avoid this, as I see it:

1. Only support scope in this PR, making it unconfigurable. This has the downside of not being able to support Authorization Servers like Okta. (Of course, follow-up PRs will remedy this)
2. Be "smart" about it, having a list of well-known attribute names and cycling through them. I'd propose "scope" and "scp" to begin.

So, long story short, I am asking two questions and proposing one thing:

Q: @rwinch, can you clarify what you meant in https://github.com/jzheaux/spring-security-oauth2-resource-server/issues/37? Do you still advocate a Authentication -> Authentication contract, just with a differently-named class? Q: We've identified this as a need in order to integrate with Keycloak. Is this an additional PR we should include in our list of PRs (referring to the original five)? P: If we are going to remove Authorities Populator in this PR, then I propose that we have JwtAuthenticationProvider support both "scope" and "scp", discovering which by inspecting the JWT.

EDIT: disregard my second question as I missed the tail-end comments about adding a PR.