expected exception not caught correctly in authorization's error handler

ftorkler commented 9 years ago

(OAuth2-)Exceptions caused by errors while initiating an OAuth2 authorize flow are handled by AuthorizationEndpoint#handleOAuth2Exception. In the chain of handling this exceptions there is an uncaught but expected ClientRegistrationException thrown in DefaultAuthorizationRequestManager#createAuthorizationRequest. This happens when the ClientDetailsService tries to retrieving the client details. The thrown exception then bubbles to the top and is not caught anywhere.

affected version: 1.0.4.RELEASE (but current version (2.0.3) seems to have same issue)

How to reproduce

Assuming that a valid initial call look like this:

/oauth/authorize?response_type=code&redirect_uri=http://myhost/authorized&client_id=myid

We make two manipulations to force the error: 1) omit the response_type (this will trigger an oauth2 exception so the exception handler kicks in) 2) use an unknown client id (this will trigger an unexpected non-oauth2 exception while handling the error of bullet '1')

/oauth/authorize?redirect_uri=http://myhost/authorized&client_id=my_unknown_id

dsyer commented 9 years ago

And what would be the expected outcome? Isn't it reasonable to expect that clients have a valid id?

ftorkler commented 9 years ago

I expect no uncaught exception (causing stack traces) and a proper redirect to an error page instead, like it is the case when only the response_type or only the client_id was manipulated. The problem only occurs when both parameters are manipulated at the same time together (though there might exist other combinations i am not aware of).

And yes, in a perfect world we would only get valid client ids and always a response type in the request - but our production environment tells us there are people out there, who manipulate the requests (due to compromising attempts and not due to misuse).

dsyer commented 9 years ago

I see. I was confused because I thought for some reason you wanted to handle the exception in the REST client.

dsyer commented 9 years ago

Actually this is already handled as far as I can tell, and I added some integration tests to make sure. You should get a redirect to /oauth/error with the error code and description. Is that not the case?

ftorkler commented 9 years ago

I tried to come up with an integration test for the exact problem i was describing. NOTE: This code is not tested.

Integration Test

@Test
public void testWrongClientIdAndOmittedResponseTypeProvided() throws Exception {
    // test wrong client id together with an omitted response_type
    ResponseEntity<String> response = attemptToGetConfirmationPage("no-such-client", "http://anywhere", null);
    // With no client id you get an InvalidClientException on the server which is forwarded to /oauth/error
    assertEquals(HttpStatus.UNAUTHORIZED, response.getStatusCode());
    String body = response.getBody();
    assertTrue("Wrong body: " + body, body.contains("<html"));
    assertTrue("Wrong body: " + body, body.contains("Bad client credentials"));
}
private ResponseEntity<String> attemptToGetConfirmationPage(String clientId, String redirectUri) {
    return attemptToGetConfirmationPage(clientId, redirectUri, "code");
}
private ResponseEntity<String> attemptToGetConfirmationPage(String clientId, String redirectUri, String responseType) {
    HttpHeaders headers = getAuthenticatedHeaders();
    return http.getForString(getAuthorizeUrl(clientId, redirectUri, responseType, "read"), headers);
}
private String getAuthorizeUrl(String clientId, String redirectUri, String responseType, String scope) {
    UriBuilder uri = http.buildUri(authorizePath()).queryParam("state", "mystateid").queryParam("scope", scope);
    if (responseType != null) {
        uri.queryParam("response_type", responseType);
    }
    if (clientId != null) {
        uri.queryParam("client_id", clientId);
    }
    if (redirectUri != null) {
        uri.queryParam("redirect_uri", redirectUri);
    }
    return uri.build().toString();
}

(Wrong) Http Response

HTTP/1.1 500 Unsupported response types: [none] Content-Type: text/html;charset=ISO-8859-1 Content-Length: 10959

<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=ISO-8859-1"/>
<title>Error 500 Unsupported response types: [none]</title>
</head>
<body><h2>HTTP ERROR 500</h2>
<p>Problem accessing /oauth/authorize. Reason:
<pre>    Unsupported response types: [none]</pre></p><h3>Caused by:</h3><pre>error="unsupported_response_type", error_description="Unsupported response types: [none]"
    at org.springframework.security.oauth2.provider.endpoint.AuthorizationEndpoint.authorize(AuthorizationEndpoint.java:128)
    at org.springframework.security.oauth2.provider.endpoint.AuthorizationEndpoint$$FastClassByCGLIB$$a7d45a6.invoke(&lt;generated&gt;)
    at org.springframework.cglib.proxy.MethodProxy.invoke(MethodProxy.java:204)
    at org.springframework.aop.framework.CglibAopProxy$CglibMethodInvocation.invokeJoinpoint(CglibAopProxy.java:698)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:150)
    at org.springframework.aop.aspectj.MethodInvocationProceedingJoinPoint.proceed(MethodInvocationProceedingJoinPoint.java:80)
    at net.xxx.oauth2.XXXAdvice.handleAuthorize(XXXAdvice.java:45)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:606)
    at org.springframework.aop.aspectj.AbstractAspectJAdvice.invokeAdviceMethodWithGivenArgs(AbstractAspectJAdvice.java:621)
    at org.springframework.aop.aspectj.AbstractAspectJAdvice.invokeAdviceMethod(AbstractAspectJAdvice.java:610)
    at org.springframework.aop.aspectj.AspectJAroundAdvice.invoke(AspectJAroundAdvice.java:65)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:161)
    at org.springframework.aop.interceptor.ExposeInvocationInterceptor.invoke(ExposeInvocationInterceptor.java:91)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
    at org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:631)
    at org.springframework.security.oauth2.provider.endpoint.AuthorizationEndpoint$$EnhancerByCGLIB$$6e7758af.authorize(<generated>)
    ...
    // followed by long long stack trace :)

dsyer commented 9 years ago

Your test actually passes for me (because of this conditional in the exception handler: line 468). You are using the same code? Or maybe you need to refine the test to more closely match your scenario?

dsyer commented 9 years ago

Still waiting...

dsyer commented 9 years ago

Ping.

spring-attic / spring-security-oauth