spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.79k stars 5.89k forks source link

Allow comma-delimited scopes in OAuth2 access token response #15878

Open bfanyuk opened 2 weeks ago

bfanyuk commented 2 weeks ago

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 https://github.com/spring-projects/spring-security/blob/de104e22b7855172876d7be6dc6ef882755da60a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/DefaultMapOAuth2AccessTokenResponseConverter.java#L80-L86

The following makes redefining this behaviour inefficient:

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

@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).