spring-attic / spring-security-oauth

Support for adding OAuth1(a) and OAuth2 features (consumer and provider) for Spring web applications.
http://github.com/spring-projects/spring-security-oauth
Apache License 2.0
4.7k stars 4.04k forks source link

HTTP response codes are lost #191

Open fhanik opened 10 years ago

fhanik commented 10 years ago

We are using the Oauth2 RestTemplate to make a rest call with a Oauth token.

On the server side, we generate an AuthenticationException that gets caught by the default web exception translator

This creates UnauthorizedException

On the client side, the response is extracted, but the error code is lost, and hard coded. The Oauth2ErrorHandler

So what was a 401 response from the server, is received as a 400 error code in @ExceptionHandler because the UnauthorizedException returns a string "unauthorized" that is not correctly translated by the JSON deserializer

I could create a PR that would remove all "abitrary" strings in Oauth2Exception sub classes and deserializer, or we could serialize the error code instead of hard coding it.

dsyer commented 10 years ago

We can probably do something along those lines for some additional special cases, but I also suspect you might be doing something wrong on the server - if it uses an OAuth2AuthenticationEntryPoint you should be getting a proper WWW-Authenticate: Bearer ... header with your 401, and that would be interpreted directly by the OAuth2RestTemplate (in the error handler).

william-tran commented 10 years ago

I'm sort of thinking aloud on a solution.

OAuth2Exceptions seem to all be generated by an HttpMessageConverter, whose read method takes in an HttpInputMessage, which doesn't expose the status code. So it's difficult to get at the status code at construction time. You could tack on the status code after the HttpMessageConverter generates the OAuth2Exception (or subclass), but to keep these Exceptions immutable you'd need to construct a new OAuth2Exception using the generated OAuth2Exception along with the status code. And this decoration would have to happen in a few places: org.springframework.security.oauth2.client.token.OAuth2AccessTokenSupport.AccessTokenErrorHandler.handleError(ClientHttpResponse) org.springframework.security.oauth2.client.http.OAuth2ErrorHandler.handleError(ClientHttpResponse)

Also the decision on whether the error is actually an OAuth2 error in these places is a bit too encompassing according to the spec. In the case of AccessTokenErrorHandler, it should follow section 5.2 (https://tools.ietf.org/html/rfc6749#section-5.2) which says that 400 (or 401 for invalid_client) can be sent, but we'll throw an OAuth Exception for any error response that can be read as an OAuth2Exception.

In the case of OAuth2ErrorHandler, an OAuth2Exception is thrown whenever a 4xx error is received, and there is a {"error": value that is one of the strings in OAuth2ExceptionJackson2Deserializer. This could unintentionally throw an OAuth2Exception subclass if a response conforms to the above. eg.

HTTP/1.1 409 Conflict
{"error":"invalid_client","message":"you tried to register a client but the client_id is taken"}

This would throw an InvalidClientException which pertains to authentication, but you're already authenticated (an actual authentication error wouldn't have a 409).

OAuth2ErrorHandler should be governed by https://tools.ietf.org/html/rfc6749#section-7.2 but it doesn't specify exactly what the errors could be. We could assume however, that in case of 401 or 403 we should attempt to throw a specific OAuth2Exception, otherwise error handling should be delegated to the wrapped ResponseErrorHandler.

paperpunk commented 9 years ago

what's the current workaround to get the actual http status code instead of the hardcoded 400?

dsyer commented 9 years ago

What's your exact scenario? You have a token? Who is sending the 400?

william-tran commented 9 years ago

I'll take a crack at a PR along the lines of my previous comment, but I'm also curious what @paperpunk's scenario is.

paperpunk commented 9 years ago
william-tran commented 9 years ago

I don't see how it would proceed through handleError with a 500 status code, this line here would ensure that only 4xx errors would possibly cause an Oauth2Exception to be thrown: https://github.com/spring-projects/spring-security-oauth/blob/master/spring-security-oauth2/src/main/java/org/springframework/security/oauth2/client/http/OAuth2ErrorHandler.java#L82

What version are you using?

paperpunk commented 9 years ago

it looks like one of the project dependencies is bringing in spring-security-oauth2 1.0.5...

is the only way to fix this - to upgrade to the latest release, or can something be configured in 1.0.5 to bypass handleError() in case of a 500 status?

william-tran commented 9 years ago

You can call setErrorHandler() and pass in your own sublcass of OAuth2ErrorHandler, and override the method handleError:

@Override
public void handleError(ClientHttpResponse response) throws IOException {
    if (response.getStatusCode().is5xxServerError()) {
        this.errorHandler.handleError(response);
    } else {
        super.handleError(response);
    }
}

The OAuth2ErrorHandler has a regular DefaultResponseErrorHandler inside it (this.errorHandler), and can delegate to it in case the error is not OAuth2 related. At least that's the intent, as versions have progressed the decision around what is and isn't an OAuth2 error has become narrower (eg: don't include 5xx errors). I'm actually proposing that only 400, 401 and 403 could possibly result in an OAuth2Exception