Skip to content

Add constructor to JwtAuthenticationToken that takes principal #7834

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

Closed
xak2000 opened this issue Jan 15, 2020 · 10 comments
Closed

Add constructor to JwtAuthenticationToken that takes principal #7834

xak2000 opened this issue Jan 15, 2020 · 10 comments
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: invalid An issue that we don't feel is valid

Comments

@xak2000
Copy link
Contributor

xak2000 commented Jan 15, 2020

Currently, JwtAuthenticationToken always pass one Jwt instance to super constructor (AbstractOAuth2TokenAuthenticationToken) as token, principal and credentials. So, that implies that all three values must always be a jwt token itself.

I don't see any reason why principal in JWT token must always be the token itself. It already represents both token and credentials. But for principal I want more flexibility: an ability to also load user (represented by JWT "sub" claim) from DB or external service.

In my application I want to use a JWT token, but I also want to create a custom class that will represent a principal, like UserDetials implementation or just a new custom class.

AbstractOAuth2TokenAuthenticationToken has a constructor, that takes token, principal and credentials separately.

It would be good, if JwtAuthenticationToken also have a constructor, that at least takes principal separately.

If this will be added, then it will also be good if JwtAuthenticationConverter will have an optional property

Converter<Jwt, Object> jwtPrincipalConverter;

which, if set, will be used to convert Jwt to principal (by loading it from DB, external service, or just by creating a more application-friendly User object from Jwt token) before calling new JwtAuthenticationToken constructor.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 15, 2020
@jzheaux jzheaux self-assigned this Jan 15, 2020
@jzheaux jzheaux added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 16, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Feb 3, 2020

@xak2000

I also want to create a custom class that will represent a principal, like UserDetials implementation or just a new custom class.

OAuth2AuthenticatedPrincipal was introduced in 5.2 - does the contract there suit your needs (instead of UserDetails, for example)?

5.2 also introduced BearerTokenAuthentication, a more generic implementation of Authentication. Instead of Jwt, it takes an OAuth2AuthenticatedPrincipal as its principal.

Given that, you might consider doing:

Converter<Jwt, BearerTokenAuthentication> converter = jwt -> {
    Collection<GrantedAuthority> authorities = // ... 
    OAuth2AccessToken token = new OAuth2AccessToken(BEARER, 
            jwt.getTokenValue(), jwt.getIssuedAt(), jwt.getExpiresAt());
    OAuth2AuthenticatedPrincipal principal = // ... custom principal
    return new BearerTokenAuthentication(principal, token, authorities);
};

And then calling the jwtAuthenticationConverter DSL method.

Of course, if you need a completely custom class, the contract allows you to do that too:

Converter<Jwt, FooAuthentication> converter = jwt -> {
    FooUser user = // ...
    return new FooAuthentication(jwt, user);
}

@jzheaux
Copy link
Contributor

jzheaux commented Feb 3, 2020

I'm going to close this issue at this point, but feel free to still post here or reopen if you feel there's more to discuss.

@jzheaux jzheaux closed this as completed Feb 3, 2020
@xak2000
Copy link
Contributor Author

xak2000 commented Feb 4, 2020

@jzheaux

I understand that I can use BearerTokenAuthentication, but I wanted to use Jwt class instance as credentials instead of more generic OAuth2AccessToken, e.g. to be able to use getHeaders() and getClaims() or any other useful methods (if they appear in the future) that make sense only for JWT token.

But BearerTokenAuthentication requires OAuth2AccessToken instance as credentials (so, I can't just pass a Jwt instance here). It also requires OAuth2AuthenticatedPrincipal as principal, that I can implement in my custom User class, of course (but honestly don't want to - I don't understand the benefits. It is better to cast it anyway to be able to access custom methods of implementation, because it is more compile-safe to use something like principal.isActive() instead of principal.getAttribute('active')).

JwtAuthenticationToken just much more suits the case, because it's actually a JWT authentication, not an abstract bearer token authentication.

Of course, if you need a completely custom class, the contract allows you to do that too

Actually, I already done this. :-)

I just copy-pasted JwtAuthenticationToken and add new constructor here:

public JwtAuthenticationToken(Jwt jwt, Object principal, Collection<? extends GrantedAuthority> authorities) {
	super(jwt, principal, jwt, authorities);
	this.setAuthenticated(true);
	this.name = jwt.getSubject();
}

I also copy-pasted JwtAuthenticationConverter and added here Converter<Jwt, AuthenticatedUser> jwtToPrincipalConverter field, that, if set, used to convert Jwt to my custom principal (AuthenticatedUser is my custom principal class).

Actually, in my custom JwtAuthenticationConverter I use AuthenticatedUser.getAuthorities() to fill authorities of JwtAuthenticationToken in addition to authorities, received from JwtGrantedAuthoritiesConverter (so, JwtAuthenticationToken will contain a merge of both authorities lists - first list from scopes of a JWT token, where each authority has SCOPE_ prefix and second list from principal authorities (loaded from DB)).

It looks like this:

public class JwtAuthenticationConverter implements Converter<Jwt, AbstractAuthenticationToken> {

	private final Converter<Jwt, AuthenticatedUser> jwtToPrincipalConverter;

	private Converter<Jwt, Collection<GrantedAuthority>> jwtGrantedAuthoritiesConverter
			= new JwtGrantedAuthoritiesConverter();


	public JwtAuthenticationConverter(Converter<Jwt, AuthenticatedUser> jwtToPrincipalConverter) {
		Assert.notNull(jwtToPrincipalConverter, "jwtToPrincipalConverter cannot be null");
		this.jwtToPrincipalConverter = jwtToPrincipalConverter;
	}

	public JwtAuthenticationConverter(
			Converter<Jwt, AuthenticatedUser> jwtToPrincipalConverter,
			Converter<Jwt, Collection<GrantedAuthority>> jwtGrantedAuthoritiesConverter) {

		this(jwtToPrincipalConverter);
		setJwtGrantedAuthoritiesConverter(jwtGrantedAuthoritiesConverter);
	}


	@Override
	public final AbstractAuthenticationToken convert(Jwt jwt) {
		AuthenticatedUser principal = jwtToPrincipalConverter.convert(jwt);
		Collection<GrantedAuthority> jwtAuthorities = jwtGrantedAuthoritiesConverter.convert(jwt);
		Collection<GrantedAuthority> authorities = Stream.of(jwtAuthorities, principal.getAuthorities())
				.filter(Objects::nonNull)
				.flatMap(Collection::stream)
				.distinct()
				.collect(ImmutableList.toImmutableList());

		return new JwtAuthenticationToken(jwt, principal, authorities);
	}

	public void setJwtGrantedAuthoritiesConverter(Converter<Jwt, Collection<GrantedAuthority>> jwtGrantedAuthoritiesConverter) {
		Assert.notNull(jwtGrantedAuthoritiesConverter, "jwtGrantedAuthoritiesConverter cannot be null");
		this.jwtGrantedAuthoritiesConverter = jwtGrantedAuthoritiesConverter;
	}

}

This fact means I need a custom JwtAuthenticationConverter anyway. I can't use a composite JwtGrantedAuthoritiesConverter to return already merged list of authorities because in this case both JwtGrantedAuthoritiesConverter and Converter<Jwt, AuthenticatedUser> jwtToPrincipalConverter will need to convert (actually, load from DB) principal from Jwt instance. So this conversion will be fired two times. It's unacceptable if course.

Also you can see I made Converter<Jwt, AuthenticatedUser> jwtToPrincipalConverter to be required field. But this is just a convenience for me. If could be optional of course. But to extract and merge authorities from here, JwtAuthenticationConverter.convert(Jwt jwt) method need to implement this logic anyway.

So, to be honest, I don't see how we can improve current implementation of JwtAuthenticationConverter to be so generic, that can be useful in most cases.

I'm good with it.

But JwtAuthenticationToken is other story. The only reason to copy-paste it for me is to add new constructor. It's super class already have all required implementation. So, I think that adding this constructor to the framework will be beneficial. Or do you think it's very unnatural case to have custom principal inside JWT Authentication?

@jzheaux
Copy link
Contributor

jzheaux commented Mar 27, 2020

@xak2000 Good questions, and thank you for the detailed explanation. You bring up a good point that BearerTokenAuthentication would require extra work to make the Jwt available, like creating a custom OAuth2AuthenticatedPrincipal that includes Jwt as a member.

Or do you think it's very unnatural case to have custom principal inside JWT Authentication?

No, not terribly. I think in that case I'd want a constructor like:

public JwtAuthenticationToken(Jwt jwt, Object principal, Object credentials, Collection<...> authorities) {

So as to not push the semantics of the Jwt instance one direction or another. For example, a constructor of (Jwt jwt, Object principal, Collection<...> authorities) does feel odd to me since it would imply that Jwt is a credential, not a principal, which deviates from the existing constructors.

If I may turn around the question to you, does it seem unnatural to include a Jwt member in your custom principal?

@xak2000
Copy link
Contributor Author

xak2000 commented Mar 28, 2020

@jzheaux Thanks for the response!

No, not terribly. I think in that case I'd want a constructor like:

public JwtAuthenticationToken(Jwt jwt, Object principal, Object credentials, Collection<...> authorities) {

Maybe you are right. It would be more general solution constructor than my version (that implies that Jwt is always a credential). But honestly, I can't imagine a situation when you want JwtAuthenticationToken with credentials other than Jwt itself.

The principal is other case. It's a usual situation (in my opinion) when you use JWT token as an authentication mechanism, but still want a user to be your principal. JWT is just an authentication mechanism in this case. But you still need to load User object from some storage, and make server-driven decisions on authorization.

JWT can participate in authorization too (by using scopes to restrict user's permissions) - it is helpful if JWT is intended to be used by 3rd party client, like in OAuth2. The important difference between server-loaded user authorities and scopes from JWT-token is that scopes don't allow user to do things. They doesn't grant any permissions to user. They restrict permissions already granted to user. E.g. if user doesn't have permission to send messages in the chat, it doesn't matter if JWT token contains scope=send_to_chat. To send message to chat user must have both send_to_chat authority (exclusively server-side decision) and send_to_chat scope. This way user can issue a JWT token that can be used (by 3rd party client) to send chat messages from him, but only if user himself has permissions to send chat messages. If admin will take decision to ban user in that chat for 3 days, then scope from JWT token will not help to circumvent the ban.

But if JWT is used only for 1st party clients, even this is not necessary. The only required claim in this case is sub (user id).

If I may turn around the question to you, does it seem unnatural to include a Jwt member in your custom principal?

Honestly, I don't remember full context of the problem now, sorry. It was 2 months ago.

At first glance it looks good.

If Jwt is used only as authentication token, then it looks slightly strange to me. In this case Jwt is not part of authenticated principal, it's a credentials.

If Jwt is used as authentication token and authorization (to restrict user authorities by scopes), then Jwt can be considered as a part of authenticated principal, I think, because in this case an authenticated principal is not just a user, represented by this token, but a 3rd party client with restricted permissions, that sends request on behalf of that user.

But, even then, BearerTokenAuthentication can't be used because it requires OAuth2AccessToken as credentials. And OAuth2AccessToken is sibling of Jwt. So, we still need JwtAuthenticationToken implementation of Authentication. And it already includes Jwt. So, I don't see necessity to make Jwt part of my custom principal.

@michaelzangl
Copy link

michaelzangl commented Jul 31, 2020

Workaround to have a user / other object as principal:

class MyUserJwtAuthenticationToken extends JwtAuthenticationToken {

	private final MyUserType user;

	public MyUserJwtAuthenticationToken(Jwt jwt, MyUserType user) {
		super(jwt);
		this.user = Objects.requireNonNull(user, "user");
	}

	@Override
	public MyUserType getPrincipal() {
		return user;
	}
}

You need to tell spring that this is a valid authentication:

@Component
public class GlobalAuthenticationConfigurer extends GlobalAuthenticationConfigurerAdapter {
	@Override
	public void configure(AuthenticationManagerBuilder auth) throws Exception {
		auth.authenticationProvider(new AuthenticationProvider() {
			@Override
			public Authentication authenticate(Authentication authentication) throws AuthenticationException {
				return authentication;
			}

			@Override
			public boolean supports(Class<?> authentication) {
				return MyUserJwtAuthenticationToken.class.isAssignableFrom(authentication);
			}
		});
	}
}

And you need to add the converter to your securtiy config, depending on the configure method you use:

authenticationTokenConverter = jwt -> new MyUserJwtAuthenticationToken(jwt, /* code to find user using jwt.getSubject() */);
// ...
authenticationProvider.setJwtAuthenticationConverter(authenticationTokenConverter);
// ...
.jwt(jwtConfig -> jwtConfig.jwtAuthenticationConverter(
								authenticationTokenConverter));

@jzheaux
Copy link
Contributor

jzheaux commented Jul 31, 2020

@michaelzangl Thanks for posting your solution.

Can you elaborate on why registering an authentication provider is necessary? It seems like this should be sufficient:

private Converter<Jwt, MyUserJwtAuthenticationToken> authenticationConverter() {
    return jwt -> {
        MyUser user = // .. get user
        return new MyUserJwtAuthenticationToken(jwt, user);
    };
}

// ...

http
    // ...
    .oauth2ResourceServer(oauth2 -> oauth2
        .jwt(jwt -> jwt
            .jwtAuthenticationConverter(authenticationConverter())
        )
    );

@michaelzangl
Copy link

@jzheaux Have you tested it?

I don't know why, but when I added it to the application, the MyUserJwtAuthenticationToken got passed to all available authentication providers again. Swing first calls the authentication providers with the normal Bearer token, then the JWT conversion is done, then it passes through the MyUserJwtAuthenticationToken again - but if you do not do the conversion, it uses the normal JwtAuthenticationToken and does not pass it through the authentication providers.

At some point I just stopped debugging and hacked this fix.

@kirankasa
Copy link

@jzheaux Hi Josh, can you consider adding the constructor which takes the principal(custom object).
I have a scenario where the same service acts as both OAuth-client as well as resource-server(for some other clients). The resource server can accept JWT tokens as well as allow users to authenticate with opened providers and access the API.
In this case, while injecting @AuthenticationPrincipal, Value can be either be 'OidcUser' or 'JWT' depends on how the user got authenticated. For converting OidcUser to a custom object there is a hook whereas for JWT we need to copy existing classes and modify the constructor. Is there any alternate way? I have created a sample app to demonstrate the use case.

@T3rm1
Copy link

T3rm1 commented Jun 5, 2022

Any updates on this issues in the meantime? It really surprises me that this has not moved forward. Isn't the principle intended to be the user domain object? There should be an easy way to set it.

@michaelzangl You forgot to call setAuthenticated(true) in your subclass of JwtAuthenticationToken. When you do that, your instance won't be passed to all providers again.

However this is still not an acceptable workaround because JwtAuthenticationConverter has some functionality like extractAuthorities(jwt) that you'd need to duplicate in your own converter. It would be much better if we can just use the existing converter and have some callback where you'd construct the principal.

michaldo added a commit to michaldo/spring-security that referenced this issue Sep 13, 2022
There is a code in https://github.com/spring-projects/spring-security/blob/5.7.3/config/src/main/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurer.java#L405
OAuth2ResourceServerConfigurer
```
Converter<Jwt, ? extends AbstractAuthenticationToken> getJwtAuthenticationConverter() {
			if (this.jwtAuthenticationConverter == null) {
				if (this.context.getBeanNamesForType(JwtAuthenticationConverter.class).length > 0) {
					this.jwtAuthenticationConverter = this.context.getBean(JwtAuthenticationConverter.class);
				}
				else {
					this.jwtAuthenticationConverter = new JwtAuthenticationConverter();
				}
			}
			return this.jwtAuthenticationConverter;
		}
```

What is benefit of **getBean(JwtAuthenticationConverter)** when JwtAuthenticationConverter has final key method?

My business case is similar to: spring-projects#7834

I am quite satisfied how JWT is converted in JWTAuthenticationToken, but rest of my application needs custom Authentication or at least custom Principal.
I'm going to implement own JwtAuthenticationConverter which takes JWTAuthenticationToken and returns something customized

Current JwtAuthenticationConverter allows me only customize Authentication *name* and *authorities*. I want customize at least Principal at most whole Authentication
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: invalid An issue that we don't feel is valid
Projects
None yet
Development

No branches or pull requests

6 participants