Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add authorities extraction #21

Open
jzheaux opened this issue Jun 25, 2018 · 0 comments
Open

Add authorities extraction #21

jzheaux opened this issue Jun 25, 2018 · 0 comments
Milestone

Comments

@jzheaux
Copy link
Owner

jzheaux commented Jun 25, 2018

At times, it will be necessary to customize how scopes are derived from incoming JWTs.

In most cases, taking each scope from the "scope" or "scp" attribute and prepending "SCOPE_" on each will be sufficient; however, when that is not the case, then we should have a hook for users to customize this.

Three contracts have been proposed to this point:

Collection<? extends GrantedAuthority> extract(AbstractOAuth2Token token)
Authentication extract(AbstractOAuth2Token token)
Authentication extract(Authentication token)

The merits of each have been discussed in various threads [1] [2], and the final contract is still open for debate.

[1] - #7
[2] - jzheaux/spring-security-oauth2-resource-server#37

Thank you, by the way, for everyone's contribution to this discussion. Below, I'll summarize some of the thoughts from the above tickets (as well as add a couple more) and explain why I propose AuthoritiesPopulator: Authentication -> Collection<GrantedAuthorities>

  • Why not just extend AuthenticationProvider and not have this API at all? First, we of course could do this. And in part, we already can since, in the worst case, a user can completely replace, say, JwtAuthenticationProvider with their own implementation. For me, this comes down to the contract. One, for a user to simply extend and replace the standing authentication provider for a given flow requires him to replicate all the other work that is not related to authorities extraction as well. While this isn't a deal-breaker, it's suboptimal. Two, for us to refactor and simply refactor the standing authentication provider have a sub authentication provider is semantically weaker, potentially causing confusion about its purpose and potentially giving folks the green light to do crazy things.

    Introducing this hook is a reasonable decomposition for providers that need to perform this same transformation of an external token into a set of authorities.

  • Why not just send an AbstractOAuth2Token? There are circumstances where the context of the external token will be insufficient to determine what the list of authorities should be. For example, consider the use case of a multi-tenant resource server. Differentiating configuration by the hostname is common in the multi-tenant space, and users will likely need to then differentiate at the filter level where the request object is available. It would be nice to place this resolved tenant state into an instance of Authentication (say BearerTokenAuthenticationToken or an extension of it), and then pass this token into a custom extractor. Then, the user could use the delegate selection pattern to identify the appropriate extractor for that tenant.

    Targeting the domain-specific object in the contract also makes it slightly less reusable. Doing Authentication -> Collection<GrantedAuthorities> makes it easier for a user to fashion an instance that works across various authentication flows (even, say, non token-based ones).

  • Could we do something more general? Maybe something that could one day suffice for SAML, CAS, etc.? The notion of authorities extraction exists in Spring Security in the way of LdapAuthoritiesPopulator, whose contract is (DirContextOperations, String) -> Collection<GrantedAuthorities>. LdapAuthenticationProvider has a similar flow, e.g. authenticate the token, transform to domain object, extract authorities using that domain object. LdapAuthoritiesPopulator's contract troubles me a bit because of the narrow context that the user then has to create her list of granted authorities, namely DirContextOperations and a String username.

    If we had a more general contract (take an Authentication), then the user would potentially have more context to draw from, and it would be easier for us to maintain passivity over time. We cannot make the same argument as easily with AbstractOAuth2Token since, at least with Jwt, its semantics don't allow us to tack on additional context as state.

    (Also, we could potentially have LdapAuthoritiesPopulator extends AuthoritiesPopulator and provide it with a default method to extract the necessary state from Authentication and call the legacy method, e.g.:

    public default Collection<? extends GrantedAuthority>
        getGrantedAuthorities(Authentication authentication) {
            return this.getGrantedAuthorities
                (authentication.getDirContextOperations(), authentication.getPrincipal());
    }

    )

  • Why not return an Authentication? We don't need to return an Authentication. And since this seriously changes the semantics of the contract (it's not just about authorities anymore), I don't prefer it unless it is clearly needed. Specifically, the GrantedAuthority contract itself is basic enough that it is easy to create a domain-specific one. For this reason, I believe that in most cases, folks will be able to attach any necessary state onto custom GrantedAuthority objects, and won't be hindered by not being able to replace the Authentication object.

    In the case of JWT support, this would be immediately applicable when attaching whatever token information was used to establish a particular authority. For example (pseudocode):

    public MyOAuth2ScopeGrantedAuthority implements GrantedAuthority {
        public MyOAuth2ScopeGrantedAuthority(String authority, T myReasons) {
            // ...
        }
     
        // ...
    }
  • Why populator instead of mapper, extractor, or something else? Regarding the name, the reason I like AuthoritiesPopulator is because of LdapAuthoritiesPopulator. These appear to fulfill the same general need, and it thus offers a conceptual bridge. Also, since this proposal focuses on taking an Authentication, it's contract is no longer OAuth2-specific.

@jzheaux jzheaux added this to the PR 2 milestone Jun 25, 2018
@jzheaux jzheaux changed the title Add authorities populator Add authorities extraction Jun 25, 2018
jzheaux added a commit that referenced this issue Jun 25, 2018
Since this is not precisely necessary to minimally support JWK sets
in Resource Server, we are pushing this code to another PR.

Fixes: gh-7
Issue: gh-21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant