Wrong username attribute when mapping authorities with delegate OidcUserService

[daniel-shuy]

Describe the bug
When using the Delegation-based Strategy with OidcUserService as documented at https://docs.spring.io/spring-security/reference/6.0/servlet/oauth2/login/advanced.html#oauth2login-advanced-map-authorities-oauth2userservice, and the spring.security.oauth2.client.provider.<provider>.user-name-attribute is set, the username retrieved by SecurityContextHolder.getContext().getAuthentication().getName() is wrong.

This is because DefaultOidcUser has the following constructors:

• DefaultOidcUser(Collection<? extends GrantedAuthority> authorities, OidcIdToken idToken, OidcUserInfo userInfo)
• DefaultOidcUser(Collection<? extends GrantedAuthority> authorities, OidcIdToken idToken, OidcUserInfo userInfo, String nameAttributeKey)
  where the first calls the second, with the nameAtrributeKey defaulting to sub.

The authority mapping replaces the authorities, but copies over the existing idToken and userInfo. It is however unable to copy over the nameAttributeKey because the OidcUser interface does not have a getter for nameAttributeKey.

Perhaps a builder could be added to DefaultOidcUser that copies the values over from an existing OidcUser (including the nameAttributeKey). This avoids having to change the OidcUser interface.

To Reproduce

1. Configure an OIDC client with spring-boot-starter-oauth2-client.
2. Configure a custom OAuth2UserService to map authorities. For the simplest example to reproduce this issue:

@Configuration
@EnableWebSecurity
public class OAuth2LoginSecurityConfig {

	@Bean
	public SecurityFilterChain filterChain(HttpSecurity http) throws Exception {
		http
			.oauth2Login(oauth2 -> oauth2
			    .userInfoEndpoint(userInfo -> userInfo
			        .oidcUserService(this.oidcUserService())
			        ...
			    )
			);
		return http.build();
	}

	private OAuth2UserService<OidcUserRequest, OidcUser> oidcUserService() {
		final OidcUserService delegate = new OidcUserService();

		return (userRequest) -> {
			OidcUser oidcUser = delegate.loadUser(userRequest);

			return new DefaultOidcUser(oidcUser.getAuthorities(), oidcUser.getIdToken(), oidcUser.getUserInfo());
		};
	}
}

4. Configure spring.security.oauth2.client.provider.<provider>.user-name-attribute, e.g.

spring:
  security:
    oauth2:
      client:
        provider:
          keycloak:
            user-name-attribute: preferred_username

5. Call SecurityContextHolder.getContext().getAuthentication().getName().

Expected behavior
SecurityContextHolder.getContext().getAuthentication().getName() should return the username from the configured username attribute (e.g. preferred_username from the example given above).

Sample

WIP - Will provide one soon

[sjohnr]

Thanks for getting in touch, but it feels like this is a question that would be better suited to Stack Overflow. We prefer to use GitHub issues only for bugs and enhancements. Feel free to update this issue with a link to the re-posted question (so that other people can find it) or add a minimal sample that reproduces this issue if you feel this is a genuine bug.

Having said that, take a look at the following code from OidcUserService which should be helpful to you:

spring-security/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/userinfo/OidcUserService.java

Lines 149 to 154 in a9506d1

	ProviderDetails providerDetails = userRequest.getClientRegistration().getProviderDetails();
	String userNameAttributeName = providerDetails.getUserInfoEndpoint().getUserNameAttributeName();
	if (StringUtils.hasText(userNameAttributeName)) {
	return new DefaultOidcUser(authorities, userRequest.getIdToken(), userInfo, userNameAttributeName);
	}
	return new DefaultOidcUser(authorities, userRequest.getIdToken(), userInfo);

I'm going to close this issue as answered, but if you feel like I have misunderstood anything, please let me know and we can re-open if needed.

[daniel-shuy]

@sjohnr thanks, I didn't think of getting the userNameAttributeName from the OidcUserRequest. Perhaps the example in https://docs.spring.io/spring-security/reference/servlet/oauth2/login/advanced.html#oauth2login-advanced-map-authorities-oauth2userservice could be updated to include it, e.g.

@Configuration
@EnableWebSecurity
public class OAuth2LoginSecurityConfig {

	@Bean
	public SecurityFilterChain filterChain(HttpSecurity http) throws Exception {
		http
			.oauth2Login(oauth2 -> oauth2
			    .userInfoEndpoint(userInfo -> userInfo
			        .oidcUserService(this.oidcUserService())
			        ...
			    )
			);
		return http.build();
	}

	private OAuth2UserService<OidcUserRequest, OidcUser> oidcUserService() {
		final OidcUserService delegate = new OidcUserService();

		return (userRequest) -> {
			// Delegate to the default implementation for loading a user
			OidcUser oidcUser = delegate.loadUser(userRequest);

			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 oidcUser;
		};
	}
}

Perhaps the API can also be improved by making the OidcUserService#getUser(OidcUserRequest, OidcUserInfo, Set) method protected so that it will be easier to map authorities (or even other values) without having to know the inner workings of OidcUserService. e.g. the example in https://docs.spring.io/spring-security/reference/servlet/oauth2/login/advanced.html#oauth2login-advanced-map-authorities-oauth2userservice could then be simplified as:

@Configuration
@EnableWebSecurity
public class OAuth2LoginSecurityConfig {

	@Bean
	public SecurityFilterChain filterChain(HttpSecurity http) throws Exception {
		http
			.oauth2Login(oauth2 -> oauth2
			    .userInfoEndpoint(userInfo -> userInfo
			        .oidcUserService(this.oidcUserService())
			        ...
			    )
			);
		return http.build();
	}

	private OAuth2UserService<OidcUserRequest, OidcUser> oidcUserService() {
		return new OidcUserService() {
			@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
				return super.getUser(userRequest, userInfo, mappedAuthorities);
			}
		};
	}
}

removing the need to get the userNameAttributeName from userRequest and manually instantiating DefaultOidcUser.

[daniel-shuy]

@sjohnr I've created gh-12281 to fix the examples in the documentation, and gh-12282 to improve the delegation-based strategy API, could you help to review them?