Custom ErrorHandler with OAuth2ErrorHandler

[kldavis4]

I've written a custom ResponseErrorHandler and configured it on an OAuth2RestTemplate. I am making calls to an Spring OAuth2 resource. In addition to Spring OAuth2 errors, the resource endpoints return application specific errors non-oauth errors. For example, I have method level security configured, and when a user tries to load an entity they don't have permission to, a spring AccessDeniedException is thrown and a 403 error is returned along with some metadata in json format. To handle the error, OAuth2ErrorHandler first calls hasError() on my custom error handler. Then handleError is called. The handleError method on OAuth2ErrorHandler assumes that the error is an OAuth2Exception and deserializes it using OAuth2ExceptionJackson2Deserializer. From what I can tell, the deserializer doesn't ever return null and so the error handler always throws an exception before the custom error handler is invoked. Is this the expected behavior?

If so, it seems like I will have to handle OAuth2Exception as a special case and inspect the additionalParams field to get the custom error. Alternatively, instead of adding the custom error handler, I will have to extend OAuth2ErrorHandler to change its behavior.

[Dave Syer]

I don't think that custom error handler hook has been very well thought out, which probably means it isn't used very much. We can try and improve it if your use case is reasonable.

You want exceptions thrown by the framework on the server side to be handled automatically, but the ones that you throw yourself have to be scooped up by the custom handler? Is that right? I guess the question you have to ask is, how could you in principle distinguish your responses from a framework response.

To make it work as things stand you have to ensure that the OAuth2ExceptionJackson2Deserializer is not triggered, so you might have some options. The framework exceptions all get deserialized because the content type application/json can be deserialized to an OAuth2Exception. You could use a custom content type I suppose, or you could inject a different MessageConverter, so that the default behaviour isn't triggered for your custom responses.

Or if you have any suggestions for a more sensible design of the error handler it would be good to hear.

[kldavis4]

Thanks for your response.

As you stated, I would like the OAuth2 related exceptions to be handled by the OAuth2ErrorHandler, and anything else to go to the custom handler.

I think what would work for me is if a custom error handler is configured, the deserializer operates in a strict mode, where it requires a matching error_type in the json content and if one isn't found it returns null instead of a generic OAuth2Exception. Then the oauth2 error handler can delegate to the custom error handler. If a custom error handler isn't configured, then it should use the current behavior and throw a generic OAuth2Exception with the additional params.

I am not sure how tricky that would be to implement. I assume you can't extract a response twice from the ClientHttpResponse? I suppose it would need to create a copy of the response in memory and pass that copy to the custom error handler. Might be too kludgy though.

I guess if there isn't a good solution, I would vote to remove the custom error handler functionality as it doesn't seem to be doing anything useful.

[Dave Syer]

OK. I think the reason that it's the way it is must be so that the framework can grow and create new error messages without needing to fix the client error handling all the time.

You didn't answer the question of how you could tell that an incoming message was one of yours versus one from the framework.

Can you try injecting a custom MessageConverter - you need one that wraps a MappingJacksonMessageConverter (or the Jackson 2 version if that's what you are using), and delegates to it but returns null if it detects a custom error.

[kldavis4]

I want to distinguish the incoming message based on the message content (not content type). If error_type is one of the ones that is known about by the deserializer (invalid_client, invalid_token, etc), then the OAuth2ErrorHandler should not delegate to the custom error handler. If it does not know about the error or the error_type property is missing, it should delegate to the custom error handler. Ideally I guess what I want is a way to customize the deserialization behavior, so in some cases it will deserialize to the OAuth2Exception and in other cases I can decide what type of exception is being returned.

I'll try as you suggest

[caseylucas]

@kldavis4, I'm curious if you had any luck. We have the same problem when we try to communicate with OAuth2 protected resources that use http status codes (and custom (json) content) when responding to clients. I would think that this is a very typical use case.

[kldavis4]

@caseylucas, here is the error handler that I eventually implemented to deal with this:

import com.acme.api.ApiException;
import org.apache.commons.lang.StringUtils;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.springframework.http.client.ClientHttpResponse;
import org.springframework.security.oauth2.client.http.OAuth2ErrorHandler;
import org.springframework.security.oauth2.client.resource.OAuth2ProtectedResourceDetails;
import org.springframework.security.oauth2.common.exceptions.OAuth2Exception;

import java.io.IOException;
import java.util.Map;

public class ErrorResponseHandler extends OAuth2ErrorHandler {
    private static final Log log = LogFactory.getLog(ErrorResponseHandler.class);

    public ErrorResponseHandler(OAuth2ProtectedResourceDetails resource) {
        super(resource);
    }

    @Override
    public boolean hasError(ClientHttpResponse response) throws IOException {
        switch (response.getStatusCode()) {
            case OK:
                return false;
            case NOT_MODIFIED:
                return false;
            case SEE_OTHER:
                return false;
            default:
                return true;
        }
    }

    @Override
    public void handleError(ClientHttpResponse response) throws IOException {
        try {
            super.handleError(response);
        } catch ( OAuth2Exception e ) {
            Map<String,String> additionalInformation = e.getAdditionalInformation();

            if ( additionalInformation == null ) throw e;

            String msg = additionalInformation.get("error_message");
            String type = additionalInformation.get("error_type");
            String code = additionalInformation.get("error_code");

            RuntimeException error;

            try {
                if ( StringUtils.isBlank(type) ) {
                    if ( !StringUtils.isBlank(msg) ) {
                        throw new IOException(msg);
                    } else {
                        throw e;
                    }
                } else {
                    error = extractException(type, msg, code, response.getStatusCode().value());
                }
            } catch ( RuntimeException ex ) {
                throw ex;
            } catch ( Throwable t ) {
                throw new RuntimeException("Failed to extract exception from client response", t);
            }

            if ( error != null )
                throw error;
            else
                throw e;
        }
    }

    protected RuntimeException extractException(String type, String message, String code, int httpStatus) {
        Class errorType;
        try {
            errorType = Class.forName(type);
        } catch (ClassNotFoundException e) {
            log.error("Failed to extract exception of type: " + type + " message: " + message , e);
            throw new RuntimeException(message != null ? message : "Failed to extract exception of type: " + type + " message: " + message,e);
        }

        Throwable t;
        if ( errorType.equals(ApiException.class)) {
            t = new ApiException(message, code, httpStatus);
        } else {
            try {
                t = (Throwable)errorType.getDeclaredConstructor(String.class).newInstance(message);
            } catch (Throwable e) {
                log.error("Failed to instantiate Throwable of type: " + type + " message: " + message, e );
                throw new RuntimeException(message != null ? message : "Failed to instantiate Throwable of type: " + type + " message: " + message,e);
            }
        }

        if ( !RuntimeException.class.isAssignableFrom(t.getClass()) ) {
            throw new RuntimeException("Unhandled error type: " + type + " message: " + message, t );
        }

        return (RuntimeException)t;
    }
}

It basically catches the OAuth2Exception and interrogates it for additionalInformation. If there is none, it throws the exception. If there is additional information, it uses it to create and throw a new exception. This requires the internal api to always return json objects with the correct properties (error_message, error_type, error_code), which I do with a global exception handler. There is also a pretty tight coupling between the client and the service, in that the exception class specified in the error_type should be some type that the client knows about.

Not ideal, but it is working for us.

[caseylucas]

Thanks for the update and code. I was hoping for something generic but we may resort to following your model.

[caseylucas]

In case someone else is interested in an alternative "fix" for this problem... I derived from OAuth2ErrorHandler, overriding handleError. My implementation calls the superclass but if I detect that the superclass didn't parse out a specific OAuth2Exception derivative, I assume it's really an application level error. The following code is scala, but it could be translated to java pretty easily.

class CustomOAuth2ErrorHandler(resourceDetails: OAuth2ProtectedResourceDetails,
                               delegateErrorHandler : ResponseErrorHandler = new DefaultResponseErrorHandler)
  extends OAuth2ErrorHandler(delegateErrorHandler, resourceDetails) {

  override def handleError(response: ClientHttpResponse) {
    if (response.getStatusCode.series() == HttpStatus.Series.CLIENT_ERROR) {
      // need to create buffered ClientHttpResponse since response input stream may need to be
      // consumed multiple times
      val bufferedResponse = new ClientHttpResponse {
        def getStatusCode = response.getStatusCode
        private lazy val lazyBody : Array[Byte] = FileCopyUtils.copyToByteArray(response.getBody)
        def getBody = new ByteArrayInputStream(lazyBody)
        def getHeaders = response.getHeaders
        def getStatusText = response.getStatusText
        def close() { response.close() }
        def getRawStatusCode: Int = response.getRawStatusCode
      }

      try {
        super.handleError(bufferedResponse)
      } catch {
        case ex: OAuth2Exception => {
          if ((ex.getClass != classOf[OAuth2Exception]) || (bufferedResponse.getRawStatusCode == 401)) {
            // found a derived class so superclass was able to parse out a specific error ... or ...
            // status was 401 (401 always means need a legit token)
            throw ex
          } else {
            delegateErrorHandler.handleError(bufferedResponse)
          }
        }
      }
    } else {
      delegateErrorHandler.handleError(response)
    }
  }
}