Spring Security Make handling of error responses from OAuth 2.0 providers more flexible

Expected Behavior

org.springframework.security.oauth2.client.web.server.ServerOAuth2AuthorizationCodeAuthenticationTokenConverter should be more flexible while handling error responses from OAuth 2.0 providers.

Current Behavior

At this moment org.springframework.security.oauth2.client.web.server.ServerOAuth2AuthorizationCodeAuthenticationTokenConverter can throw an IllegalArgumentException while handling error response from OAuth 2.0 provider in case error parameters are named in a nonstandard way. In my case instead of error I get error_code and instead of error_description I get error_message.

Context

Method convertResponse of class org.springframework.security.oauth2.client.web.server.ServerOAuth2AuthorizationCodeAuthenticationTokenConverter should delegate to some implementation of new strategy interface for converting of authorization callback request to OAuth2AuthorizationResponse. It may seem as an overkill but ServerOAuth2AuthorizationCodeAuthenticationTokenConverter can even contain a Map of such implementations referenced by client registration id with default implementation providing current behavior.

P.S. I can prepare a PR for this issue.

Comment From: jgrandja

@rchigvintsev Can you please provide details on your specific use case? You mentioned:

ServerOAuth2AuthorizationCodeAuthenticationTokenConverter can throw an IllegalArgumentException while handling error response from OAuth 2.0 provider

Please provide specific details on the error condition so I can better understand.

Comment From: rchigvintsev

Sure. IllegalArgumentException is thrown by static method error of org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationResponse class when it finds out that error code is null or empty. The reason why code is empty is that org.springframework.security.oauth2.client.web.server.OAuth2AuthorizationResponseUtils expects only "code" parameter whereas some OAuth 2.0 providers can use different parameter names for error code (and error description).

Comment From: jgrandja

Thanks for the details on your use case @rchigvintsev . I now understand.

Regarding you proposal:

Method convertResponse of class org.springframework.security.oauth2.client.web.server.ServerOAuth2AuthorizationCodeAuthenticationTokenConverter should delegate to some implementation of new strategy interface for converting of authorization callback request to OAuth2AuthorizationResponse.

I would rather not introduce a new strategy interface for this use case. Our primary goal is to implement to spec and provide customization hooks for non-standard behaviour, which is specific to your use case, e.g. error_code and error_message. This can be achieved by supplying a custom ServerAuthenticationConverter to either OAuth2ClientSpec.authenticationConverter() or OAuth2LoginSpec.authenticationConverter().

This should work for your use case?

Comment From: rchigvintsev

Of course, it is fine to use a custom ServerAuthenticationConverter if a solution with a strategy interface looks like an overengineering. The issue with this approach is that I should do quite a lot of copy-pasting in order to create a new converter. In this particular case, I do not want to change most of the logic of ServerOAuth2AuthorizationCodeAuthenticationTokenConverter class, only part related to response converting. So it would be nice to at least make this class friendlier for extending (convertResponse method can become an instance method with protected visibility). This approach is not ideal because I still have to copy code from OAuth2AuthorizationResponseUtils but perhaps it's a happy medium. What do you think?

Comment From: jgrandja

@rchigvintsev

The issue with this approach is that I should do quite a lot of copy-pasting in order to create a new converter

Using a delegation-based pattern would allow you to use the existing logic as-is.

ServerOAuth2AuthorizationCodeAuthenticationTokenConverter delegate =
        new ServerOAuth2AuthorizationCodeAuthenticationTokenConverter(clientRegistrationRepository);
delegate.setAuthorizationRequestRepository(authorizationRequestRepository);

ServerAuthenticationConverter authenticationConverter = (exchange) -> {
    if (!exchange.getRequest().getQueryParams().containsKey("error_code")) {
        return delegate.convert(exchange);
    }
    URI uriWithError = UriComponentsBuilder.fromUri(exchange.getRequest().getURI())
            .queryParam(OAuth2ParameterNames.ERROR, exchange.getRequest().getQueryParams().get("error_code"))
            .build()
            .toUri();
    ServerWebExchange adaptedExchange = exchange.mutate()
            .request(builder -> builder.uri(uriWithError))
            .build();
    return delegate.convert(adaptedExchange);
};

This should work for non-compliant error parameters.

Comment From: rchigvintsev

Although I would prefer a bit cleaner way this solution should work too. Thank you for your time! I'm going to close this issue.