Improve API for delegation-based strategy

[daniel-shuy]

The current API for delegation-based strategy with OidcUserService/OidcReactiveOAuth2UserService is error-prone because it requires understanding the inner workings of OidcUserService/OidcReactiveOAuth2UserService (see gh-12275 and gh-12281 for the original discussion).

This improves the API by:

• Making OidcUserService#getUser(OidcUserRequest, OidcUserInfo, Set) protected
• OidcReactiveOAuth2UserService: Abstract out logic to set DefaultOidcUser nameAttributeKey from OidcUserRequest into a new protected getUser(OidcUserRequest, OidcUserInfo, Set) method

This allows the delegation-based strategy to be massively simplified (especially for reactive).

Using the example in the doc, the configuration can be simplified from:

@Bean
public ReactiveOAuth2UserService<OidcUserRequest, OidcUser> oidcUserService() {
	final OidcReactiveOAuth2UserService delegate = new OidcReactiveOAuth2UserService();

	return (userRequest) -> {
		// Delegate to the default implementation for loading a user
		return delegate.loadUser(userRequest)
				.flatMap((oidcUser) -> {
					OAuth2AccessToken accessToken = userRequest.getAccessToken();
					Set<GrantedAuthority> mappedAuthorities = new HashSet<>();

					// TODO
					// 1) Fetch the authority information from the protected resource using accessToken
					// 2) Map the authority information to one or more GrantedAuthority's and add it to mappedAuthorities

					// 3) Create a copy of oidcUser but use the mappedAuthorities instead
					ProviderDetails providerDetails = userRequest.getClientRegistration().getProviderDetails();
					String userNameAttributeName = providerDetails.getUserInfoEndpoint().getUserNameAttributeName();
					if (StringUtils.hasText(userNameAttributeName)) {
						oidcUser = new DefaultOidcUser(mappedAuthorities, oidcUser.getIdToken(), oidcUser.getUserInfo(), userNameAttributeName);
					} else {
						oidcUser = new DefaultOidcUser(mappedAuthorities, oidcUser.getIdToken(), oidcUser.getUserInfo());
					}

					return Mono.just(oidcUser);
				});
	};
}

to:

@Bean
public ReactiveOAuth2UserService<OidcUserRequest, OidcUser> oidcUserService() {
	return new OidcReactiveOAuth2UserService() {
		@Override
		protected OidcUser getUser(OidcUserRequest userRequest, OidcUserInfo userInfo, Set<GrantedAuthority> authorities) {
			OAuth2AccessToken accessToken = userRequest.getAccessToken();
			Set<GrantedAuthority> mappedAuthorities = new HashSet<>();

			// TODO
			// 1) Fetch the authority information from the protected resource using accessToken
			// 2) Map the authority information to one or more GrantedAuthority's and add it to mappedAuthorities

			// Delegate to the default implementation for getting a user
			OidcUser oidcUser = super.loadUser(userRequest, userInfo, mappedAuthorities);

			return oidcUser;
		}
	};
}

[daniel-shuy]

Any update on this? Its been awhile

[sjohnr]

Thanks for the ping @daniel-shuy! I have not had time to look at this recently, but I will add it to my TODO list for after the 6.1 release next week.

[sjohnr]

@daniel-shuy, thanks for the PR!

However, I don't find myself in favor of introducing a protected method and inheritance in order to facilitate easier delegation-based examples. I'm also having a hard time understanding what scenario is being solved for here.

I understand that the docs demonstrate using delegation and use the example of mapping authorities as the use case for the customization. However, the previous example of Using a GrantedAuthoritiesMapper would suffice in most cases. Do you truly have an advanced scenario that requires delegation? Or were you led that way by the docs only due to the example? Can you describe your use case for the customization here?

[daniel-shuy]

@sjohnr

I'm also having a hard time understanding what scenario is being solved for here.
I understand that the docs demonstrate using delegation and use the example of mapping authorities as the use case for the customization. However, the previous example of Using a GrantedAuthoritiesMapper would suffice in most cases. Do you truly have an advanced scenario that requires delegation? Or were you led that way by the docs only due to the example? Can you describe your use case for the customization here?

Yes, to configure spring-security-oauth2-client for Keycloak.

Keycloak stores the roles in the access token payload as e.g.

{
  // ...
  "realm_access": {
    "roles": [
      // Roles
    ]
  },
  "resource_access": {
    "client": {
      "roles": [
        // Authorities
      ]
    },
    // ...
  },
  // ...
}

Therefore to map Keycloak roles to Spring Security authorities, a custom OAuth2UserService/ReactiveOAuth2UserService needs to be created (a GrantedAuthoritiesMapper does not have access to the OidcUserRequest to get the access token payload):

@Bean
public ReactiveOAuth2UserService<OidcUserRequest, OidcUser> oidcUserService() {
	final OidcReactiveOAuth2UserService delegate = new OidcReactiveOAuth2UserService();

	return (userRequest) -> {
		// Delegate to the default implementation for loading a user
		return delegate.loadUser(userRequest)
				.flatMap((oidcUser) -> {
					OAuth2AccessToken accessToken = userRequest.getAccessToken();
					Set<GrantedAuthority> mappedAuthorities = new HashSet<>();

					// map Keycloak roles in token to Spring Security authorities

					ProviderDetails providerDetails = userRequest.getClientRegistration().getProviderDetails();
					String userNameAttributeName = providerDetails.getUserInfoEndpoint().getUserNameAttributeName();
					if (StringUtils.hasText(userNameAttributeName)) {
						oidcUser = new DefaultOidcUser(mappedAuthorities, oidcUser.getIdToken(), oidcUser.getUserInfo(), userNameAttributeName);
					} else {
						oidcUser = new DefaultOidcUser(mappedAuthorities, oidcUser.getIdToken(), oidcUser.getUserInfo());
					}

					return Mono.just(oidcUser);
				});
	};
}

The current API is very hard to understand (requires understanding the inner workings of OidcUserService/OidcReactiveOAuth2UserService) and error prone (the bug in the documentation still isn't fixed, see gh-12281).

[daniel-shuy]

However, I don't find myself in favor of introducing a protected method and inheritance in order to facilitate easier delegation-based examples.

What about adding a setter (e.g. setOidcUserRequestGrantedAuthoritiesConverter) to OidcUserService and OidcReactiveOAuth2UserService to take in a Converter<OidcUserRequest, Collection<GrantedAuthority>>, that will map the OidcUserRequest to additional GrantedAuthoritys, like JwtAuthenticationConverter#setJwtGrantedAuthoritiesConverter(Converter<Jwt, Collection>)?

[sjohnr]

Thanks for the use case @daniel-shuy, that's helpful.

First, I would suggest using Keycloak's mappers to map claims to the id_token, as this is by far the easiest way to get access to roles. Having said that, I'm not an expert in Keycloak administration so perhaps that doesn't make sense or is not possible for you.

In any case, because the Converter interface only accepts a single parameter, and the mapping to authorities requires both OidcUserRequest and OidcUserInfo, perhaps we could add something like:

private BiFunction<OidcUserRequest, OidcUserInfo, Collection<GrantedAuthority>> authoritiesMapper = ...;

We could do something very similar in DefaultOAuth2UserService. What do you think?

[daniel-shuy]

@sjohnr

First, I would suggest using Keycloak's mappers to map claims to the id_token, as this is by far the easiest way to get access to roles. Having said that, I'm not an expert in Keycloak administration so perhaps that doesn't make sense or is not possible for you.

Yes it is possible, but I would prefer not having to change the Keycloak configuration as much as possible, just to get it working with Spring Security, as I am trying to write an open source library that can be reused by others, to replace keycloak-spring-boot-starter, which is now deprecated (https://www.keycloak.org/2022/02/adapter-deprecation.html).

---

In any case, because the Converter interface only accepts a single parameter, and the mapping to authorities requires both OidcUserRequest and OidcUserInfo

Ah, I was thinking to simply map them to SimpleGrantedAuthoritys, but good point, it should map them to OidcUserAuthoritys/OAuth2UserAuthoritys. What about something like:

public interface OidcUserRequestAuthoritiesConverter implements Converter<OidcUserRequest, Collection<String>> {
}

public interface OAuth2UserRequestAuthoritiesConverter implements Converter<OAuth2UserRequest, Collection<String>> {
}

Then a setter (e.g. setOidcUserRequestGrantedAuthoritiesConverter/setOAuth2UserRequestAuthoritiesConverter) can be added to OidcUserService and DefaultOAuth2UserService respectively to map the OidcUserRequest/OAuth2UserRequest to a Collection<String>, and loadUser() can be enhanced to map the Collection<String> to OidcUserAuthoritys/OAuth2UserAuthoritys.

---

We could do something very similar in DefaultOAuth2UserService. What do you think?

Both OidcUserService and DefaultOAuth2UserService need to be modified. Even though OidcUserService delegates to DefaultOAuth2UserService, the authorities from the OAuth2User returned by DefaultOAuth2UserService are not used by the OidcUserService.

[sjohnr]

I am trying to write an open source library that can be reused by others, to replace keycloak-spring-boot-starter, which is now deprecated (https://www.keycloak.org/2022/02/adapter-deprecation.html).

I see. Thanks!

Ah, I was thinking to simply map them to SimpleGrantedAuthoritys, but good point, it should map them to OidcUserAuthoritys/OAuth2UserAuthoritys. What about something like:

public interface OidcUserRequestAuthoritiesConverter implements Converter<OidcUserRequest, Collection<String>> {
}

public interface OAuth2UserRequestAuthoritiesConverter implements Converter<OAuth2UserRequest, Collection<String>> {
}

I'm not sure this will work. As I mentioned, the OidcUserAuthority requires the OidcUserInfo which is looked up earlier in the loadUser method. I also think that if we want to introduce a new hook into OidcUserService, it would ideally add quite a bit more flexibility.

We could introduce a @FunctionalInterface, but it needs to take 2 parameters (OidcUserRequest and OidcUserInfo) and return a Collection<GrantedAuthority> (or specifically a Collection<? extends GrantedAuthority> like GrantedAuthoritiesMapper does). In this way, the entire set of authorities can be replaced instead of just added to.

Also, AuthorityUtils is available to map from String... to a List<GrantedAuthority> so we don't need to do that mapping for users.

Lastly, I think any new hook we add here should extend from the hook we add for DefaultOAuth2UserService (with generics), since these two classes form a hierarchy.

[daniel-shuy]

Oops, sorry, I meant to write:

public interface OidcUserRequestAuthoritiesConverter {
    Collection<String> toAuthorities(OidcUserRequest userRequest, OidcUserInfo userInfo);
}

public interface OAuth2UserRequestAuthoritiesConverter {
    Collection<String> toAuthorities(OAuth2UserRequest userRequest, Map<String, Object> userAttributes);
}

---

We could introduce a @FunctionalInterface, but it needs to take 2 parameters (OidcUserRequest and OidcUserInfo) and return a Collection<GrantedAuthority> (or specifically a Collection<? extends GrantedAuthority> like GrantedAuthoritiesMapper does). In this way, the entire set of authorities can be replaced instead of just added to.

Also, AuthorityUtils is available to map from String... to a List<GrantedAuthority> so we don't need to do that mapping for users.

I have a question - I see OidcUserService#loadUser(OidcUserRequest) creating a single OidcUserAuthority while mapping the token scopes as SimpleGrantedAuthoritys and DefaultOAuth2UserService#loadUser(OAuth2UserRequest) creating a single OAuth2UserAuthority, while mapping the token scopes as SimpleGrantedAuthoritys. Should the mapper map all roles to OidcUserAuthoritys/OAuth2UserAuthoritys, or like OidcUserService/DefaultOAuth2UserService, create one OidcUserAuthority/OAuth2UserAuthority and map the rest as SimpleGrantedAuthoritys?

---

Lastly, I think any new hook we add here should extend from the hook we add for DefaultOAuth2UserService (with generics), since these two classes form a hierarchy.

I don't think that is possible, as OAuth2UserAuthority requires a Map<String, Object> (user attributes), but OidcUserInfo does not extend Map<String, Object>.

[sjohnr]

Oops, sorry, I meant to write:

public interface OidcUserRequestAuthoritiesConverter {
    Collection<String> toAuthorities(OidcUserRequest userRequest, OidcUserInfo userInfo);
}

public interface OAuth2UserRequestAuthoritiesConverter {
    Collection<String> toAuthorities(OAuth2UserRequest userRequest, Map<String, Object> userAttributes);
}

That's looking better for sure! However, I don't think they should return Collection<String>. I think the user should decide what representation of GrantedAuthority to return. What do you think of this?

I have a question - I see OidcUserService#loadUser(OidcUserRequest) creating a single OidcUserAuthority while mapping the token scopes as SimpleGrantedAuthoritys and DefaultOAuth2UserService#loadUser(OAuth2UserRequest) creating a single OAuth2UserAuthority, while mapping the token scopes as SimpleGrantedAuthoritys. Should the mapper map all roles to OidcUserAuthoritys/OAuth2UserAuthoritys, or like OidcUserService/DefaultOAuth2UserService, create one OidcUserAuthority/OAuth2UserAuthority and map the rest as SimpleGrantedAuthoritys?

I think there would need to be an advantage to changing it, which I'm not sure about right now. If there is, we'd probably want to address that separately.

Currently, we return a Collection of GrantedAuthority and later invoke GrantedAuthoritiesMapper where the user has to instanceof OidcUserAuthority or instanceof OAuth2UserAuthority to pull that authority out of the collection in order to use it, which may be tedious.

That's why I'm excited about introducing this new option in OidcUserService and OAuth2UserService as it feels like it could be easier to use!

I don't think that is possible, as OAuth2UserAuthority requires a Map<String, Object> (user attributes), but OidcUserInfo does not extend Map<String, Object>.

That's a fair point. I was just thinking, if possible the hooks should be closely related. But you're right, probably not a hierarchy.

[daniel-shuy]

That's looking better for sure! However, I don't think they should return Collection<String>. I think the user should decide what representation of GrantedAuthority to return. What do you think of this?

I'm kind of on the fence about this. If the mapper should follow OidcUserService#loadUser(OidcUserRequest)/DefaultOAuth2UserService(OAuth2UserRequest), to create a single OidcUserAuthority/OAuth2UserAuthority and map the other roles as SimpleGrantedAuthoritys, then the issue with returning Collection<GrantedAuthority>, is that it requires the user to know the inner workings of OidcUserService/OAuth2UserService. But on the other hand it offers more flexibility compared to returning a Collection<String>.

Another option is to simply map everything as SimpleGrantedAuthoritys. In that case the OidcUserInfo or OAuth2User attributes (Map<String, Object>) won't even be required.

What is the purpose/advantage of using OidcUserAuthority/OAuth2UserAuthority over SimpleGrantedAuthority?

[sjohnr]

What is the purpose/advantage of using OidcUserAuthority/OAuth2UserAuthority over SimpleGrantedAuthority?

I'm not 100% sure the background of the existing feature, but I believe it's purpose is to allow the instanceof checks I mentioned. It allows compatibility with the existing GrantedAuthoritiesMapper interface.

I'm kind of on the fence about this. If the mapper should follow OidcUserService#loadUser(OidcUserRequest)/DefaultOAuth2UserService(OAuth2UserRequest), to create a single OidcUserAuthority/OAuth2UserAuthority and map the other roles as SimpleGrantedAuthoritys, then the issue with returning Collection<GrantedAuthority>, is that it requires the user to know the inner workings of OidcUserService/OAuth2UserService.

We will need to enhance the docs along with this change. But if we add this capability, I think the inner workings of the class become less important for users, because usually you would choose one of:

a) customize using this new option, so you don't usually require additional mapping and don't need the special authority
b) customize using the existing GrantedAuthoritiesMapper option, which uses the existing documented feature
c) customize both, in which case you are aware of the details of both custom implementations and don't need to know about anything else

Does that make sense?

Another option is to simply map everything as SimpleGrantedAuthoritys. In that case the OidcUserInfo or OAuth2User attributes (Map<String, Object>) won't even be required.

I want to be careful not to expand the scope of this enhancement. While we could add mappings to a collection containing that existing authority, as stated above I think there's better flexibility and a simpler contract by allowing the user to fully override the mapped collection. We just need to document that is what the user is doing by customizing.

[daniel-shuy]

Sounds good! I'm in favor of giving users the flexibility while enhancing the docs

[sjohnr]

@daniel-shuy just checking in. Are you able to make updates to this PR based on my comment above?

[sjohnr]

@daniel-shuy I'm going to close this PR in favor of the approach I outlined above. I've created a branch with changes for the servlet side to demonstrate. I will proceed with the reactive side shortly so we can try for 6.3.0-M3. Please let me know if you have any thoughts.