Merge branch 'improve-exception-handling' into '2.x'

Improve exception handling

See merge request !52

src/main/java/de/thm/arsnova/controller/AbstractControllerExceptionHandler.java

+package de.thm.arsnova.controller;
+
+import org.springframework.beans.factory.annotation.Value;
+
+import java.util.HashMap;
+import java.util.Map;
+
+public class AbstractControllerExceptionHandler {
+	/* Since exception messages might contain sensitive data, they are not exposed by default. */
+	@Value("${api.expose-error-messages:false}") private boolean exposeMessages;
+
+	protected Map<String, Object> handleException(Throwable e) {
+		final Map<String, Object> result = new HashMap<>();
+		result.put("errorType", e.getClass().getSimpleName());
+		if (exposeMessages) {
+			result.put("errorMessage", e.getMessage());
+		}
+
+		return result;
+	}
+}

src/main/java/de/thm/arsnova/controller/SecurityExceptionControllerAdvice.java → src/main/java/de/thm/arsnova/controller/ControllerExceptionHandler.java

@@ -26,6 +26,7 @@ import de.thm.arsnova.exceptions.PayloadTooLargeException;
 import de.thm.arsnova.exceptions.PreconditionFailedException;
 import de.thm.arsnova.exceptions.UnauthorizedException;
 import org.springframework.http.HttpStatus;
+import org.springframework.http.converter.HttpMessageNotReadableException;
 import org.springframework.security.access.AccessDeniedException;
 import org.springframework.security.authentication.AnonymousAuthenticationToken;
 import org.springframework.security.authentication.AuthenticationCredentialsNotFoundException;
@@ -33,101 +34,104 @@ import org.springframework.security.core.Authentication;
 import org.springframework.security.core.context.SecurityContextHolder;
 import org.springframework.web.bind.annotation.ControllerAdvice;
 import org.springframework.web.bind.annotation.ExceptionHandler;
+import org.springframework.web.bind.annotation.ResponseBody;
 import org.springframework.web.bind.annotation.ResponseStatus;
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
-import java.util.HashMap;
 import java.util.Map;
 
 /**
- * Translates security/authentication related exceptions into HTTP status codes.
+ * Translates exceptions into HTTP status codes.
  */
 @ControllerAdvice
-public class SecurityExceptionControllerAdvice {
-
-	@ExceptionHandler
-	@ResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR)
-	public Map<String, String> defaultExceptionHandler(
-			final Exception e,
-			final HttpServletRequest req
-			) {
-		final Map<String, String> result = new HashMap<>();
-		result.put("code", "500");
-		result.put("status", "Internal server error");
-		result.put("message", e.getMessage());
-		return result;
-	}
-
-	@ResponseStatus(HttpStatus.NOT_FOUND)
+public class ControllerExceptionHandler extends AbstractControllerExceptionHandler {
 	@ExceptionHandler(NotFoundException.class)
-	public void handleNotFoundException(final Exception e, final HttpServletRequest request) {
-		/* No implementation - handled solely by annotations */
+	@ResponseBody
+	@ResponseStatus(HttpStatus.NOT_FOUND)
+	public Map<String, Object> handleNotFoundException(final Exception e, final HttpServletRequest request) {
+		return handleException(e);
 	}
 
-	@ResponseStatus(HttpStatus.UNAUTHORIZED)
 	@ExceptionHandler(UnauthorizedException.class)
-	public void handleUnauthorizedException(final Exception e, final HttpServletRequest request) {
-		/* No implementation - handled solely by annotations */
+	@ResponseBody
+	@ResponseStatus(HttpStatus.UNAUTHORIZED)
+	public Map<String, Object> handleUnauthorizedException(final Exception e, final HttpServletRequest request) {
+		return handleException(e);
 	}
 
-	@ResponseStatus(HttpStatus.UNAUTHORIZED)
 	@ExceptionHandler(AuthenticationCredentialsNotFoundException.class)
-	public void handleAuthenticationCredentialsNotFoundException(final Exception e, final HttpServletRequest request) {
-		/* No implementation - handled solely by annotations */
+	@ResponseStatus(HttpStatus.UNAUTHORIZED)
+	@ResponseBody
+	public Map<String, Object> handleAuthenticationCredentialsNotFoundException(final Exception e, final HttpServletRequest request) {
+		return handleException(e);
 	}
 
 	@ExceptionHandler(AccessDeniedException.class)
-	public void handleAccessDeniedException(
+	@ResponseBody
+	public Map<String, Object> handleAccessDeniedException(
 			final Exception e,
 			final HttpServletRequest request,
 			final HttpServletResponse response
 			) {
 		final Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
-		if (
-				authentication == null
+		if (authentication == null
 				|| authentication.getPrincipal() == null
-				|| authentication instanceof AnonymousAuthenticationToken
-				) {
+				|| authentication instanceof AnonymousAuthenticationToken) {
 			response.setStatus(HttpStatus.UNAUTHORIZED.value());
-			return;
+		} else {
+			response.setStatus(HttpStatus.FORBIDDEN.value());
 		}
-		response.setStatus(HttpStatus.FORBIDDEN.value());
+
+		return handleException(e);
 	}
 
-	@ResponseStatus(HttpStatus.FORBIDDEN)
 	@ExceptionHandler(ForbiddenException.class)
-	public void handleForbiddenException(final Exception e, final HttpServletRequest request) {
-		/* No implementation - handled solely by annotations */
+	@ResponseBody
+	@ResponseStatus(HttpStatus.FORBIDDEN)
+	public Map<String, Object> handleForbiddenException(final Exception e, final HttpServletRequest request) {
+		return handleException(e);
 	}
 
-	@ResponseStatus(HttpStatus.NO_CONTENT)
 	@ExceptionHandler(NoContentException.class)
-	public void handleNoContentException(final Exception e, final HttpServletRequest request) {
-		/* No implementation - handled solely by annotations */
+	@ResponseBody
+	@ResponseStatus(HttpStatus.NO_CONTENT)
+	public Map<String, Object> handleNoContentException(final Exception e, final HttpServletRequest request) {
+		return handleException(e);
 	}
 
-	@ResponseStatus(HttpStatus.BAD_REQUEST)
 	@ExceptionHandler(BadRequestException.class)
-	public void handleBadRequestException(final Exception e, final HttpServletRequest request) {
-		/* No implementation - handled solely by annotations */
+	@ResponseBody
+	@ResponseStatus(HttpStatus.BAD_REQUEST)
+	public Map<String, Object> handleBadRequestException(final Exception e, final HttpServletRequest request) {
+		return handleException(e);
 	}
 
-	@ResponseStatus(HttpStatus.PRECONDITION_FAILED)
 	@ExceptionHandler(PreconditionFailedException.class)
-	public void handlePreconditionFailedException(final Exception e, final HttpServletRequest request) {
-		/* No implementation - handled solely by annotations */
+	@ResponseBody
+	@ResponseStatus(HttpStatus.PRECONDITION_FAILED)
+	public Map<String, Object> handlePreconditionFailedException(final Exception e, final HttpServletRequest request) {
+		return handleException(e);
 	}
 
-	@ResponseStatus(HttpStatus.NOT_IMPLEMENTED)
 	@ExceptionHandler(NotImplementedException.class)
-	public void handleNotImplementedException(final Exception e, final HttpServletRequest request) {
-		/* No implementation - handled solely by annotations */
+	@ResponseBody
+	@ResponseStatus(HttpStatus.NOT_IMPLEMENTED)
+	public Map<String, Object> handleNotImplementedException(final Exception e, final HttpServletRequest request) {
+		return handleException(e);
 	}
 
-	@ResponseStatus(HttpStatus.PAYLOAD_TOO_LARGE)
 	@ExceptionHandler(PayloadTooLargeException.class)
-	public void handlePayloadTooLargeException(final Exception e, final HttpServletRequest request) {
-		/* No implementation - handled solely by annotations */
+	@ResponseBody
+	@ResponseStatus(HttpStatus.PAYLOAD_TOO_LARGE)
+	public Map<String, Object> handlePayloadTooLargeException(final Exception e, final HttpServletRequest request) {
+		return handleException(e);
+	}
+
+	@ExceptionHandler(HttpMessageNotReadableException.class)
+	@ResponseBody
+	@ResponseStatus(HttpStatus.BAD_REQUEST)
+	public Map<String, Object> handleHttpMessageNotReadableException(final Exception e, final HttpServletRequest request) {
+		return handleException(e);
 	}
 }

src/main/java/de/thm/arsnova/controller/DefaultControllerExceptionHandler.java

+package de.thm.arsnova.controller;
+
+import org.springframework.core.annotation.AnnotationUtils;
+import org.springframework.http.HttpStatus;
+import org.springframework.web.bind.annotation.ControllerAdvice;
+import org.springframework.web.bind.annotation.ExceptionHandler;
+import org.springframework.web.bind.annotation.ResponseBody;
+import org.springframework.web.bind.annotation.ResponseStatus;
+
+import javax.servlet.http.HttpServletRequest;
+import java.util.Map;
+
+@ControllerAdvice
+public class DefaultControllerExceptionHandler extends AbstractControllerExceptionHandler {
+	@ExceptionHandler
+	@ResponseBody
+	@ResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR)
+	public Map<String, Object> defaultExceptionHandler(
+			final Exception e,
+			final HttpServletRequest req
+	) throws Exception {
+		/* If the exception is annotated with @ResponseStatus rethrow it and let
+		 * the framework handle it.
+		 * See https://spring.io/blog/2013/11/01/exception-handling-in-spring-mvc. */
+		if (AnnotationUtils.findAnnotation(e.getClass(), ResponseStatus.class) != null) {
+			throw e;
+		}
+
+		return handleException(e);
+	}
+}

src/site/markdown/development.md

@@ -37,6 +37,9 @@ Run the following command to download the dependencies and startup the backend w
 
 After a few seconds the ARSnova API will be accessible at <http://localhost:8080/>.
 
+You can adjust the amount of debug logging by changing the log levels in [log4j-dev.properties](src/main/resources/log4j-dev.properties).
+Additionally, you can enable exception messages in API responses by setting the boolean property `api.expose-error-messages` in `arsnova.properties`.
+
 
 ## Continuous Integration
 

src/test/java/de/thm/arsnova/controller/CourseControllerTest.java

@@ -89,15 +89,15 @@ public class CourseControllerTest {
 	public void testShouldIndicateNotImplementedIfInactiveClient() throws Exception {
 		setAuthenticated(true, "ptsr00");
 
-		mockMvc.perform(get("/mycourses").accept(MediaType.TEXT_PLAIN))
-		.andExpect(status().isNotImplemented());
+		mockMvc.perform(get("/mycourses").accept(MediaType.APPLICATION_JSON))
+				.andExpect(status().isNotImplemented());
 	}
 
 	@Test
 	public void testShouldNotReturnCurrentUsersCoursesIfUnauthorized() throws Exception {
 		setAuthenticated(false, "nobody");
 
-		mockMvc.perform(get("/mycourses").accept(MediaType.TEXT_PLAIN))
-		.andExpect(status().isUnauthorized());
+		mockMvc.perform(get("/mycourses").accept(MediaType.APPLICATION_JSON))
+				.andExpect(status().isUnauthorized());
 	}
 }