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

Ability to apply UserDetailsChecker to OAuth2 Resource Server flows #6237

Closed
edeandrea opened this issue Dec 5, 2018 · 11 comments
Closed

Ability to apply UserDetailsChecker to OAuth2 Resource Server flows #6237

edeandrea opened this issue Dec 5, 2018 · 11 comments
Assignees
Labels
for: team-attention This ticket should be discussed as a team before proceeding in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement

Comments

@edeandrea
Copy link
Contributor

Spun out from #6219.

We need the ability to apply a UserDetailsChecker to the OAuth2 Resource Server flows.

My initial thoughts on implementation would be to have converters (i.e. Converter<Jwt, AbstractAuthenticationToken> & Converter<Jwt, Mono<AbstractAuthenticationToken>>) that can take an underlying Converter and apply a UserDetailsChecker to it before returning the AbstractAuthenticationToken/Mono<AbstractAuthenticationToken>.

That way in the configuration DSL a consumer could choose to have this ability or not via something like

http.oauth2ResourceServer()
  .jwt()
    .jwtAuthenticationConverter(new ReactiveJwtPostAutneticationChecksAuthenticationConverter(userDetailsService));

I'm open to discussing the correct approach on how best to integrate it and am happy to supply a PR for the correct approach.

If you'd rather me submit a PR with what I have and discuss the approach on the PR thats fine too.

@jzheaux
Copy link
Contributor

jzheaux commented Dec 6, 2018

@edeandrea Thanks for your proposal!

The tricky part is that there isn't a UserDetails in play with Resource Server. Really, the typical semantics of a Resource Server are not user-based, but authority-based.

Likely, any converter we roll into master should play nicely with these semantics. For example, the default, JwtAuthenticationConverter returns an authentication token of type JwtAuthenticationToken whose principal is a Jwt. While nothing in the framework strictly requires either of these to be the case, changing a Jwt out for something like a UserDetails alters the underlying meaning of the Authentication.

As you can imagine, it's confusing for users to hear "remember that if you use this converter, then the token is no longer the principal, meaning that such-and-such features will work differently for you now."

(And to be clear, none of the above means that the implementation you are proposing is wrong-headed. It just may mean that it isn't a fit to be rolled into master. I am going to mark this ticket for team discussion, though, to see what @rwinch et al think.)

Happily, though, as you have discovered, you can do this with your own Converter implementation. To confirm, are you blocked on progress without your proposed changes?

@jzheaux jzheaux added the for: team-attention This ticket should be discussed as a team before proceeding label Dec 6, 2018
@edeandrea
Copy link
Contributor Author

Thanks @jzheaux No at this moment I'm not blocked - I just hate having to write my own custom stuff when (in theory) things really belong "up" a level :)

As a framework developer here at my company we always prefer to not have to write custom code and instead "wire" spring code together to make things work the way we want to work.

At first when I was implementing this I was under the same impression as you - that everything you should need to perform authorization/authentication should be on the token.

Then after looking at how some of our applications behave - we have some applications which use more than one identity source (meaning in OAuth-speak a single resource server with multiple authorization servers). When a request comes in, the resource server needs to determine from the issuer which authorization server can authorize the request.

In our situation (which may not be completely ideal and not 100% true OAuth2) some of our applications maintain their own role-based authentication mappings in their own data sources outside of the authorization server.

This means that the concept of a UserDetailsService still applies, as the subject that comes in on the token might not be the actual id of the user principal the application knows/cares about (i.e. PCF's SSO tile passes some random GUID as the subject on the Bearer token, so the resource server has to call the userInfo endpoint to retrieve the actual user's identity as the application knows it). From there we can attach that known principal (i.e. a user id/email address/etc) to a UserDetailsService to retrieve the user's information.

Its somewhat of a hybrid solution between custom authentication & pure/true OAuth, but I think you'll find many large organizations have a mix of legacy & new and they have to have something in-between.

This would allow the application to then apply the UserDetailsChecker as well as what is in #6243 into this flow.

At the end of the day the JwtAuthenticationToken is what is stored as the Authentication object within the SecurityContextHolder / ReactiveSecurityContextHolder - its just that it is validated that the user hasn't expired/been locked out/etc as well as with #6243 that the user's authorities as known by the application are merged into the authorities of the JwtAuthenticationToken.

I have this all built out in our framework here within our organization - so if submitting a PR for you & the team to look at and talk about would make things easier I'm happy to do that.

@jgrandja
Copy link
Contributor

jgrandja commented Dec 6, 2018

@edeandrea Are you aware that you can override JwtAuthenticationConverter.extractAuthorities() to enhance the JwtAuthenticationToken.getAuthorities()? Here is a link to the reference.

For example, by overriding JwtAuthenticationConverter.extractAuthorities() you can derive your authorities from any source really. Whether it's in the Resource Server's internal store or some external store. I think this hook is what you're looking for?

I'm also not sure if integrating UserDetailsService makes sense here. The equivalent for OAuth is OAuth2UserService which would make more sense IMO. But still not sure if that makes sense here either.

@edeandrea
Copy link
Contributor Author

Yes I saw that - but how would that work on the reactive side when we're dealing with a ReactiveUserDetailsService? ReactiveUserDetailsService returns a Mono<UserDetails>. We would instead now have to build a ReactiveJwtAuthenticationConverter. Currently JwtReactiveAuthenticationManager uses new ReactiveJwtAuthenticationConverterAdapter(new JwtAuthenticationConverter()) as its default jwtAuthenticationConverter. Wouldn't this need to be re-worked a bit?

Like I mentioned what we're doing isn't 100% OAuth2 stuff - we have a hybrid where we are using Jwt tokens as an authentication mechanism as a bridge between old siteminder-based authentication and cloud-based authentication. We're trying to use Spring Security's new oauth2 resource server abstractions as an implementation while trying to assemble some of the building blocks together.

@jzheaux
Copy link
Contributor

jzheaux commented Dec 7, 2018

@edeandrea it might be nice to see what you are doing, though I don't think we are ready for a PR. Would you mind checking something into a branch in your fork and linking to that here in the ticket?

@edeandrea
Copy link
Contributor Author

I can certainly do that. I will get something up today.

@edeandrea
Copy link
Contributor Author

I pushed commit edeandrea/spring-security@b6b9037 to the gh-6237 branch in my local repo.

This was done in a way to be completely non-intrusive at all to the way current things work. It just offers a different Converter<Jwt, AbstractAuthenticationToken> / Converter<Jwt, Mono<AbstractAuthenticationToken>> that someone can choose to use instead of the defaults.

Let me know your thoughts.

@jzheaux
Copy link
Contributor

jzheaux commented Dec 11, 2018

@edeandrea Thanks again for all the information you provided for this ticket.

Based on your branch, it appears that you just need a hook for refining the list of authorities. To that end, I've created a ticket to create a ReactiveJwtAuthenticationConverter which can be used instead of the reactive adapter.

As for the proposal, to have an official implementation that supports a UserDetailsService, I don't yet see the need nor do I have a clear picture yet on what something like that would look like. The particular use case of using a UserDetailsService simply to gather authorities is too narrow for general usage since the typical use case for a UserDetailsService is to query for a user and ostensibly have that user be part of the resulting Authentication. Invoking a UserDetailsService and then dropping the resulting user in favor of only the authorities makes me wonder why even use a UserDetailsService?

If we go down the other route for the moment and imagine trading out the JWT for a representation of the user in the Authentication, then OAuth2User might make more sense in that case since it contains the subject, an attribute set and an authorities set. But I think more evidence is necessary to show what the common pattern is (since there isn't any spec guidance for that kind of composition) before we proceed with something like that. Ideally whatever kind of pivot point we create officially will play nicely with other features like token relay, fine-grained authorization rules, and others. Dropping the JWT from the resulting Authentication is problematic in enough ways that I think it merits additional pause before committing to any implementation that offers an opinion on how to do that.

To your comment about bridges, I understand. In a previous life, I had similar experiences with Cognito and needing to bridge bearer token authentication into our existing Authentication representation. If a common approach to this kind of thing becomes clearer, then let's take another look.

@jzheaux jzheaux closed this as completed Dec 11, 2018
@edeandrea
Copy link
Contributor Author

Thanks @jzheaux for the feedback.

I'd also say another thing that would be nice to have is the ability to decorate the JwtAuthenticationToken returned by JwtAuthenticationConverter.convert. JwtAuthenticationToken.getName always assumes that the name is the subject on the token, which isn't always the case.

The Cognito example you brought up above is a great use case. The access token, while having a subject (which is the primary key in identifying the user in Cognito), doesn't have what the application needs as its primary identifier (i.e. an active directory user name, email, etc). What we end up having to do is once the token is decoded & validated, we have to call Cognito's userInfo endpoint to retrieve that information. In there is what we consider the user principal and what we need to set on the Authentication.

What we've done is to decorate the JwtAuthenticationToken with our own and be able to override the name.

We've had to do it in a clunky way due to the fact that JwtAuthenticationConverter.convert is declared final.

It would be nice maybe to add a protected method that does the construction of the JwtAuthenticationToken that could be overridden.

@xak2000
Copy link
Contributor

xak2000 commented Jan 15, 2020

I created somehow related issue #7834.

I think, if it will be implemented, then we obtain an ability to use UserDetailsService or any other service to create a principal based on JWT claims.

@jzheaux, you say:

the typical semantics of a Resource Server are not user-based, but authority-based.

Can you explain a bit? I don't understand the point. Yes, authorities play a big role in Resource Server semantics, but User (subject) is also important. They both complement each other.

For example: a token contain scopes that you, as an owner, grant to an application to load your photos. But resource server doesn't contain only your photos. So, it usually needs to check not only scopes from the token, but also a userId, and also a user authorities.

Yes, user authorities, which are totally different thing than scopes.

  • Authorities is what Resource Server allow this user to do.
  • Scopes is what User allows to do with his resources on his behalf.

If token have "read:photos" scope, but user (subject of the token) doesn't have authorities to read his photos from this resource server (maybe he is banned or his premium membership is ended), then resource server must not respond to request, authenticated with this token, with user photos. Because even if user allowed it (granting scopes), the resource server don't allow it for this user anymore.

Does this make any sense?

@jzheaux
Copy link
Contributor

jzheaux commented Jan 15, 2020

Yes, it does make sense to differentiate between the two types of authorities; that is, authorities that the resource server grants to the user and authorities that the user grants to the client. These are authority-based concerns that already fit nicely into the current model (e.g. with a granted authorities converter).

the typical semantics of a Resource Server are not user-based, but authority-based ... could you explain a bit?

For sure!

  • HTTP Basic is user-based since "who" is intrinsic to the protocol. IOW, we can't use HTTP Basic without talking about the user.
  • JWT, on the other hand, is authority-based since it's a statement about what the bearer can do, not who the bearer is. It may incidentally (and helpfully) contain information about user(s), but such is not the basis of the protocol. IOW, we can use JWT without talking about the user.

Like I said earlier in this ticket, it's not necessarily wrong-headed to use UserDetailsService, but this is typically quite a bit more relevant for a service that is managing and authenticating end users. Note methods like isAccountNonExpired and isAccountNonLocked. Certainly, these could be useful in specialized cases, but I wouldn't think them a typical concern of a Resource Server, which is why this ticket was closed.

That said, I took a look at #7834 and like the idea. I think that either exposing the principal constructor parameter or possibly making JwtBearerTokenAuthenticationConverter configurable could be options to achieve what you want -- we can have that conversation over on that ticket, though.

@jzheaux jzheaux added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: team-attention This ticket should be discussed as a team before proceeding in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants