Allow comma-delimited scopes in OAuth2 access token response

[bfanyuk]

Expected Behavior

Should be possible to configure scope delimiter if server sends scopes as comma-delimited string (e.g. GitHub does this).

Current Behavior

Delimiter is hard coded here

spring-security/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/DefaultMapOAuth2AccessTokenResponseConverter.java

Lines 80 to 86 in de104e2

	private static Set<String> getScopes(Map<String, Object> tokenResponseParameters) {
	if (tokenResponseParameters.containsKey(OAuth2ParameterNames.SCOPE)) {
	String scope = getParameterValue(tokenResponseParameters, OAuth2ParameterNames.SCOPE);
	return new HashSet<>(Arrays.asList(StringUtils.delimitedListToStringArray(scope, " ")));
	}
	return Collections.emptySet();
	}

The following makes redefining this behaviour inefficient:

• getScopes is private static method
• DefaultMapOAuth2AccessTokenResponseConverter is final class
• DefaultAuthorizationCodeTokenResponseClient is final class

Context

GitHub OAuth returns comma delimited scopes instead of space delimited as assumed by spring security.
Redefining default behaviour is quire cumbersome and inefficient as it creates redundant instance of OAuth2AccessTokenResponseClient per application and redundant instance of OAuth2AccessTokenResponse for each login event:

public class AdvancedAccessTokenResponseClient implements OAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> {

    private OAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> delegate = 
            new DefaultAuthorizationCodeTokenResponseClient();

    @Override
    public OAuth2AccessTokenResponse getTokenResponse(OAuth2AuthorizationCodeGrantRequest authorizationGrantRequest) {
        OAuth2AccessTokenResponse tokenResponse = delegate.getTokenResponse(authorizationGrantRequest);
        Set<String> scopes = tokenResponse.getAccessToken().getScopes();
        Set<String> scopesNew = scopes.stream().flatMap(s -> Arrays.stream(s.split(","))).collect(Collectors.toSet());
        if (scopesNew.equals(scopes))
            return tokenResponse;
        return OAuth2AccessTokenResponse.withResponse(tokenResponse).scopes(scopesNew).build();
    }
}

[sjohnr]

Thanks @bfanyuk. Do you have a scenario or use case in mind that runs into an issue? Are you using the scopes programmatically or basing authorization rules for your client application on them?

[bfanyuk]

Hello, @sjohnr! Thanks for checking this out.
I would like to hide certain endpoints based on scopes like this ...

@PreAuthorize("hasAuthority('SCOPE_email')")
fun getEmail(): String { }

... or this ...

@Bean
fun securityFilterChain(http: HttpSecurity): SecurityFilterChain {
    http {
        authorizeHttpRequests {
            authorize("/email", hasAuthority("SCOPE_email"))
        }
    }
    return http.build()
}

E.g. what is described here fits the most.

[sjohnr]

@bfanyuk RFC 6749 defines scope as space-delimited so GitHub would appear to be off-spec here. Having said that, I'm wondering if you have tried a GrantedAuthoritiesMapper as a workaround yet?

	@Bean
	public GrantedAuthoritiesMapper userAuthoritiesMapper() {
		return (authorities) -> authorities.stream().flatMap(this::mapAuthority).toList();
	}

	private Stream<GrantedAuthority> mapAuthority(GrantedAuthority authority) {
		if (authority.getAuthority().startsWith("SCOPE_")) {
			var scopes = authority.getAuthority().substring("SCOPE_".length());
			return Stream.of(StringUtils.delimitedListToStringArray(scopes, ","))
				.map((scope) -> new SimpleGrantedAuthority("SCOPE_" + scope));
		}
		return Stream.of(authority);
	}

This works in an OAuth2 Client application to map authorities during login. You can of course optimize this if intermediary streams are not desired.

Regarding adding the ability to delimit scopes with comma, it is a very deeply nested component in order to achieve this customization and so I don't know that it would make things easier for users to be able to customize this. Also, since Spring Security is operating to spec and the workaround is quite easy, I don't feel it's worth pursuing a change in the framework.

I'll leave this open for a bit longer so you can try out the above workaround, but I plan to close this issue. If you feel strongly about it and want to contribute a PR, I can work with you on it instead of closing the issue (but I'd again re-iterate that I don't feel it would provide much value).