From 27580e726fa907c57590a0935673d1d7c620c57a Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Fri, 11 Apr 2014 16:46:09 +0100 Subject: [PATCH] Add generic error handling to BasicErrorController Since Spring supports gobal error handling through @ControllerAdvice, it is quite easy to set up more meta-data about an exception for the BasicErrorController. You need to be careful not to swallow Security exceptions, and probably others (optionally) so this feature needs a bit more work. See gh-538 --- .../ManagementSecurityAutoConfiguration.java | 19 +++++ .../actuate/web/BasicErrorController.java | 58 +++++++++++++- .../BasicErrorControllerIntegrationTests.java | 76 ++++++++++++++++++- .../spring-boot-sample-actuator/pom.xml | 8 ++ .../sample/actuator/SampleController.java | 35 ++++++++- 5 files changed, 190 insertions(+), 6 deletions(-) diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/ManagementSecurityAutoConfiguration.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/ManagementSecurityAutoConfiguration.java index 316b97e17c..7959aaa536 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/ManagementSecurityAutoConfiguration.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/ManagementSecurityAutoConfiguration.java @@ -22,6 +22,8 @@ import java.util.List; import java.util.Set; import javax.annotation.PostConstruct; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.actuate.endpoint.Endpoint; @@ -46,6 +48,7 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.Ordered; import org.springframework.core.annotation.Order; +import org.springframework.security.access.AccessDeniedException; import org.springframework.security.config.annotation.web.WebSecurityConfigurer; import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.config.annotation.web.builders.WebSecurity; @@ -53,9 +56,12 @@ import org.springframework.security.config.annotation.web.builders.WebSecurity.I import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; import org.springframework.security.config.annotation.web.configuration.WebSecurityConfiguration; import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter; +import org.springframework.security.core.AuthenticationException; import org.springframework.security.web.AuthenticationEntryPoint; import org.springframework.security.web.authentication.www.BasicAuthenticationEntryPoint; import org.springframework.util.StringUtils; +import org.springframework.web.bind.annotation.ControllerAdvice; +import org.springframework.web.bind.annotation.ExceptionHandler; /** * {@link EnableAutoConfiguration Auto-configuration} for security of framework endpoints. @@ -85,6 +91,19 @@ public class ManagementSecurityAutoConfiguration { return new IgnoredPathsWebSecurityConfigurerAdapter(); } + @ConditionalOnWebApplication + @ControllerAdvice + @Order(Ordered.HIGHEST_PRECEDENCE + 10) + protected static class SecurityExceptionRethrowingAdvice { + + @ExceptionHandler({ AccessDeniedException.class, AuthenticationException.class }) + public void handle(HttpServletRequest request, HttpServletResponse response, + Exception e) throws Exception { + throw e; + } + + } + @Configuration protected static class ManagementSecurityPropertiesConfiguration implements SecurityPrequisite { diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/web/BasicErrorController.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/web/BasicErrorController.java index 99d273e5eb..698543b1f0 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/web/BasicErrorController.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/web/BasicErrorController.java @@ -24,19 +24,26 @@ import java.util.Map; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.context.embedded.AbstractEmbeddedServletContainerFactory; +import org.springframework.core.annotation.Order; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.stereotype.Controller; +import org.springframework.validation.BindException; +import org.springframework.validation.BindingResult; +import org.springframework.web.bind.annotation.ControllerAdvice; +import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.ResponseBody; import org.springframework.web.context.request.RequestAttributes; import org.springframework.web.context.request.ServletRequestAttributes; import org.springframework.web.servlet.ModelAndView; +import org.springframework.web.servlet.mvc.support.DefaultHandlerExceptionResolver; /** * Basic global error {@link Controller}, rendering servlet container error codes and @@ -48,12 +55,16 @@ import org.springframework.web.servlet.ModelAndView; * @author Dave Syer */ @Controller +@ControllerAdvice +@Order(0) public class BasicErrorController implements ErrorController { private static final String ERROR_KEY = "error"; private final Log logger = LogFactory.getLog(BasicErrorController.class); + private DefaultHandlerExceptionResolver resolver = new DefaultHandlerExceptionResolver(); + @Value("${error.path:/error}") private String errorPath; @@ -62,6 +73,24 @@ public class BasicErrorController implements ErrorController { return this.errorPath; } + @ExceptionHandler(Exception.class) + public void handle(HttpServletRequest request, HttpServletResponse response, + Exception e) throws Exception { + this.resolver.resolveException(request, response, null, e); + if (response.getStatus() == HttpServletResponse.SC_OK) { + response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + } + // There's only one exception so it's easier for the error controller to identify + // it this way... + request.setAttribute(ErrorController.class.getName(), e); + if (e instanceof BindException) { + // ... but other error pages might be looking for it here as well + request.setAttribute( + BindingResult.MODEL_KEY_PREFIX + ((BindException) e).getObjectName(), + e); + } + } + @RequestMapping(value = "${error.path:/error}", produces = "text/html") public ModelAndView errorHtml(HttpServletRequest request) { Map map = extract(new ServletRequestAttributes(request), false, @@ -96,7 +125,7 @@ public class BasicErrorController implements ErrorController { map.put("timestamp", new Date()); try { Throwable error = (Throwable) attributes.getAttribute( - "javax.servlet.error.exception", RequestAttributes.SCOPE_REQUEST); + ErrorController.class.getName(), RequestAttributes.SCOPE_REQUEST); Object obj = attributes.getAttribute("javax.servlet.error.status_code", RequestAttributes.SCOPE_REQUEST); int status = 999; @@ -108,12 +137,16 @@ public class BasicErrorController implements ErrorController { map.put(ERROR_KEY, "None"); } map.put("status", status); + if (error == null) { + error = (Throwable) attributes.getAttribute( + "javax.servlet.error.exception", RequestAttributes.SCOPE_REQUEST); + } if (error != null) { while (error instanceof ServletException && error.getCause() != null) { error = ((ServletException) error).getCause(); } map.put("exception", error.getClass().getName()); - map.put("message", error.getMessage()); + addMessage(map, error); if (trace) { StringWriter stackTrace = new StringWriter(); error.printStackTrace(new PrintWriter(stackTrace)); @@ -129,6 +162,9 @@ public class BasicErrorController implements ErrorController { RequestAttributes.SCOPE_REQUEST); map.put("message", message == null ? "No message available" : message); } + String path = (String) attributes.getAttribute( + "javax.servlet.error.request_uri", RequestAttributes.SCOPE_REQUEST); + map.put("path", path == null ? "No path available" : path); return map; } catch (Exception ex) { @@ -141,4 +177,22 @@ public class BasicErrorController implements ErrorController { } } + protected void addMessage(Map map, Throwable error) { + if (error instanceof BindingResult) { + BindingResult result = (BindingResult) error; + if (result.getErrorCount() > 0) { + map.put("errors", result.getAllErrors()); + map.put("message", + "Validation failed for object='" + result.getObjectName() + + "'. Error count: " + result.getErrorCount()); + } + else { + map.put("message", "No errors"); + } + } + else { + map.put("message", error.getMessage()); + } + } + } diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/web/BasicErrorControllerIntegrationTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/web/BasicErrorControllerIntegrationTests.java index 29c8c40a8a..dccd3fed58 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/web/BasicErrorControllerIntegrationTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/web/BasicErrorControllerIntegrationTests.java @@ -18,6 +18,8 @@ package org.springframework.boot.actuate.web; import java.util.Map; +import javax.servlet.DispatcherType; +import javax.servlet.ServletContext; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -35,11 +37,16 @@ import org.springframework.boot.test.SpringApplicationConfiguration; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.http.MediaType; +import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; import org.springframework.test.context.web.WebAppConfiguration; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.MvcResult; +import org.springframework.test.web.servlet.RequestBuilder; import org.springframework.test.web.servlet.setup.MockMvcBuilders; +import org.springframework.validation.BindException; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RestController; import org.springframework.web.context.WebApplicationContext; import org.springframework.web.servlet.View; import org.springframework.web.servlet.view.AbstractView; @@ -67,7 +74,7 @@ public class BasicErrorControllerIntegrationTests { } @Test - public void testErrorForMachineClient() throws Exception { + public void testDirectAccessForMachineClient() throws Exception { MvcResult response = this.mockMvc.perform(get("/error")) .andExpect(status().is5xxServerError()).andReturn(); String content = response.getResponse().getContentAsString(); @@ -75,7 +82,32 @@ public class BasicErrorControllerIntegrationTests { } @Test - public void testErrorForBrowserClient() throws Exception { + public void testErrorForMachineClient() throws Exception { + MvcResult result = this.mockMvc.perform(get("/")) + .andExpect(status().is5xxServerError()).andReturn(); + MvcResult response = this.mockMvc.perform(new ErrorDispatcher(result, "/error")) + .andReturn(); + String content = response.getResponse().getContentAsString(); + assertTrue("Wrong content: " + content, content.contains("Expected!")); + } + + @Test + public void testBindingExceptionForMachineClient() throws Exception { + // In a real container the response is carried over into the error dispatcher, but + // in the mock a new one is created so we have to assert the status at this + // intermediate point + MvcResult result = this.mockMvc.perform(get("/bind")) + .andExpect(status().is4xxClientError()).andReturn(); + MvcResult response = this.mockMvc.perform(new ErrorDispatcher(result, "/error")) + .andReturn(); + // And the rendered status code is always wrong (but would be 400 in a real + // system) + String content = response.getResponse().getContentAsString(); + assertTrue("Wrong content: " + content, content.contains("Error count: 1")); + } + + @Test + public void testDirectAccessForBrowserClient() throws Exception { MvcResult response = this.mockMvc .perform(get("/error").accept(MediaType.TEXT_HTML)) .andExpect(status().isOk()).andReturn(); @@ -106,6 +138,46 @@ public class BasicErrorControllerIntegrationTests { }; } + @RestController + protected static class Errors { + + public String getFoo() { + return "foo"; + } + + @RequestMapping("/") + public String home() { + throw new IllegalStateException("Expected!"); + } + + @RequestMapping("/bind") + public String bind() throws Exception { + BindException error = new BindException(this, "test"); + error.rejectValue("foo", "bar.error"); + throw error; + } + + } + + } + + private class ErrorDispatcher implements RequestBuilder { + + private MvcResult result; + private String path; + + public ErrorDispatcher(MvcResult result, String path) { + this.result = result; + this.path = path; + } + + @Override + public MockHttpServletRequest buildRequest(ServletContext servletContext) { + MockHttpServletRequest request = this.result.getRequest(); + request.setDispatcherType(DispatcherType.ERROR); + request.setRequestURI(this.path); + return request; + } } } diff --git a/spring-boot-samples/spring-boot-sample-actuator/pom.xml b/spring-boot-samples/spring-boot-sample-actuator/pom.xml index 807a780993..d09d13eb79 100644 --- a/spring-boot-samples/spring-boot-sample-actuator/pom.xml +++ b/spring-boot-samples/spring-boot-sample-actuator/pom.xml @@ -35,6 +35,10 @@ org.springframework.boot spring-boot-starter-remote-shell + + org.hibernate + hibernate-validator + org.springframework.boot spring-boot-starter-test @@ -44,6 +48,10 @@ log4j log4j commons-logging commons-logging --> + + org.hibernate + hibernate-validator + diff --git a/spring-boot-samples/spring-boot-sample-actuator/src/main/java/sample/actuator/SampleController.java b/spring-boot-samples/spring-boot-sample-actuator/src/main/java/sample/actuator/SampleController.java index e68f8fcceb..8fd31cee43 100644 --- a/spring-boot-samples/spring-boot-sample-actuator/src/main/java/sample/actuator/SampleController.java +++ b/spring-boot-samples/spring-boot-sample-actuator/src/main/java/sample/actuator/SampleController.java @@ -17,12 +17,17 @@ package sample.actuator; import java.util.Collections; +import java.util.Date; +import java.util.LinkedHashMap; import java.util.Map; +import org.hibernate.validator.constraints.NotBlank; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Description; import org.springframework.stereotype.Controller; +import org.springframework.validation.annotation.Validated; import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.bind.annotation.ResponseBody; @Controller @@ -32,16 +37,42 @@ public class SampleController { @Autowired private HelloWorldService helloWorldService; - @RequestMapping("/") + @RequestMapping(value="/", method=RequestMethod.GET) @ResponseBody - public Map helloWorld() { + public Map hello() { return Collections.singletonMap("message", this.helloWorldService.getHelloMessage()); } + @RequestMapping(value="/", method=RequestMethod.POST) + @ResponseBody + public Map olleh(@Validated Message message) { + Map model = new LinkedHashMap(); + model .put("message", message.getValue()); + model.put("title", "Hello Home"); + model.put("date", new Date()); + return model; + } + @RequestMapping("/foo") @ResponseBody public String foo() { throw new IllegalArgumentException("Server error"); } + + protected static class Message { + + @NotBlank(message="Message value cannot be empty") + private String value; + + public String getValue() { + return value; + } + + public void setValue(String value) { + this.value = value; + } + } + + }