From bd65ba41e06877cb55ccefe7b024cacce865e151 Mon Sep 17 00:00:00 2001 From: Arpit Mohan Date: Fri, 7 Feb 2020 15:41:45 +0530 Subject: [PATCH] Correcting the error response returned by the AclFilter to match the ErrorDTO returned from controller functions This ensures consistent responses from different parts of our application. The client can then rely on the server to provide a constistent response structure. --- .../server/exceptions/AppsmithError.java | 2 +- .../exceptions/GlobalExceptionHandler.java | 45 +++++++++++++------ .../appsmith/server/filters/AclFilter.java | 38 +++++++++++++--- 3 files changed, 65 insertions(+), 20 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java index 2eba7a4227..59e07a05aa 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java @@ -30,7 +30,7 @@ public enum AppsmithError { JSON_PROCESSING_ERROR(400, 4022, "Json processing error with error {0}"), INVALID_CREDENTIALS(200, 4023, "Invalid credentials provided. Did you input the credentials correctly?"), DUPLICATE_KEY(409, 4024, "Duplicate key error"), - UNAUTHORIZED_ACCESS(401, 4025, "Unauthorized access"), + UNAUTHORIZED_ACCESS(403, 4025, "Unauthorized access"), INVALID_DATASOURCE_NAME(400, 4026, "Datasource name is invalid"), NO_RESOURCE_FOUND(404, 4027, "Unable to find {0} with id {1}"), GENERIC_BAD_REQUEST(400, 4028, "Bad Request: {0}"), diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java index 5c484bcb66..9260b66513 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java @@ -7,6 +7,7 @@ import com.segment.analytics.Analytics; import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.HttpStatus; +import org.springframework.security.access.AccessDeniedException; import org.springframework.web.bind.annotation.ControllerAdvice; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.ResponseBody; @@ -14,6 +15,7 @@ import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.ServerWebInputException; import reactor.core.publisher.Mono; + /** * This class catches all the Exceptions and formats them into a proper ResponseDTO object before * sending it to the client. @@ -51,43 +53,57 @@ public class GlobalExceptionHandler { @ExceptionHandler @ResponseBody public Mono> catchDuplicateKeyException(org.springframework.dao.DuplicateKeyException e, ServerWebExchange exchange) { - exchange.getResponse().setStatusCode(HttpStatus.BAD_REQUEST); + AppsmithError appsmithError = AppsmithError.DUPLICATE_KEY; + exchange.getResponse().setStatusCode(HttpStatus.resolve(appsmithError.getHttpErrorCode())); log.error("", e); rollbar.log(e); - return Mono.just(new ResponseDTO<>(HttpStatus.BAD_REQUEST.value(), new ErrorDTO(AppsmithError.DUPLICATE_KEY.getAppErrorCode(), - AppsmithError.DUPLICATE_KEY.getMessage(e.getMessage())))); + return Mono.just(new ResponseDTO<>(appsmithError.getHttpErrorCode(), new ErrorDTO(appsmithError.getAppErrorCode(), + appsmithError.getMessage(e.getMessage())))); } @ExceptionHandler @ResponseBody public Mono> catchTimeoutException(java.util.concurrent.TimeoutException e, ServerWebExchange exchange) { - exchange.getResponse().setStatusCode(HttpStatus.GATEWAY_TIMEOUT); + AppsmithError appsmithError = AppsmithError.PLUGIN_EXECUTION_TIMEOUT; + exchange.getResponse().setStatusCode(HttpStatus.resolve(appsmithError.getHttpErrorCode())); log.error("", e); rollbar.log(e); - return Mono.just(new ResponseDTO<>(HttpStatus.GATEWAY_TIMEOUT.value(), new ErrorDTO(AppsmithError.PLUGIN_EXECUTION_TIMEOUT.getHttpErrorCode(), - AppsmithError.PLUGIN_EXECUTION_TIMEOUT.getMessage()))); + return Mono.just(new ResponseDTO<>(appsmithError.getHttpErrorCode(), new ErrorDTO(appsmithError.getAppErrorCode(), + appsmithError.getMessage()))); } @ExceptionHandler @ResponseBody public Mono> catchServerWebInputException(ServerWebInputException e, ServerWebExchange exchange) { - exchange.getResponse().setStatusCode(HttpStatus.BAD_REQUEST); + AppsmithError appsmithError = AppsmithError.GENERIC_BAD_REQUEST; + exchange.getResponse().setStatusCode(HttpStatus.resolve(appsmithError.getHttpErrorCode())); log.error("", e); rollbar.log(e); - return Mono.just(new ResponseDTO<>(HttpStatus.BAD_REQUEST.value(), new ErrorDTO(AppsmithError.GENERIC_BAD_REQUEST.getHttpErrorCode(), - AppsmithError.GENERIC_BAD_REQUEST.getMessage(e.getReason())))); + return Mono.just(new ResponseDTO<>(appsmithError.getHttpErrorCode(), new ErrorDTO(appsmithError.getAppErrorCode(), + appsmithError.getMessage(e.getReason())))); } @ExceptionHandler @ResponseBody public Mono> catchPluginException(AppsmithPluginException e, ServerWebExchange exchange) { - exchange.getResponse().setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR); + AppsmithError appsmithError = AppsmithError.INTERNAL_SERVER_ERROR; + exchange.getResponse().setStatusCode(HttpStatus.resolve(appsmithError.getHttpErrorCode())); log.error("", e); rollbar.log(e); - return Mono.just(new ResponseDTO<>(HttpStatus.INTERNAL_SERVER_ERROR.value(), new ErrorDTO(AppsmithError.INTERNAL_SERVER_ERROR.getHttpErrorCode(), + return Mono.just(new ResponseDTO<>(appsmithError.getHttpErrorCode(), new ErrorDTO(appsmithError.getAppErrorCode(), e.getLocalizedMessage()))); } + @ExceptionHandler + @ResponseBody + public Mono> catchAccessDeniedException(AccessDeniedException e, ServerWebExchange exchange) { + AppsmithError appsmithError = AppsmithError.UNAUTHORIZED_ACCESS; + exchange.getResponse().setStatusCode(HttpStatus.resolve(appsmithError.getHttpErrorCode())); + log.error("", e); + rollbar.log(e); + return Mono.just(new ResponseDTO<>(appsmithError.getHttpErrorCode(), new ErrorDTO(appsmithError.getAppErrorCode(), + appsmithError.getMessage()))); + } /** * This function catches the generic Exception class and is meant to be a catch all to ensure that we don't leak * any information to the client. Ideally, the function #catchAppsmithException should be used @@ -99,10 +115,11 @@ public class GlobalExceptionHandler { @ExceptionHandler @ResponseBody public Mono> catchException(Exception e, ServerWebExchange exchange) { - exchange.getResponse().setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR); + AppsmithError appsmithError = AppsmithError.INTERNAL_SERVER_ERROR; + exchange.getResponse().setStatusCode(HttpStatus.resolve(appsmithError.getHttpErrorCode())); log.error("", e); rollbar.log(e); - return Mono.just(new ResponseDTO<>(HttpStatus.INTERNAL_SERVER_ERROR.value(), new ErrorDTO(AppsmithError.INTERNAL_SERVER_ERROR.getHttpErrorCode(), - AppsmithError.INTERNAL_SERVER_ERROR.getMessage()))); + return Mono.just(new ResponseDTO<>(appsmithError.getHttpErrorCode(), new ErrorDTO(appsmithError.getAppErrorCode(), + appsmithError.getMessage()))); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/filters/AclFilter.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/filters/AclFilter.java index 6338358023..fd09f64a45 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/filters/AclFilter.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/filters/AclFilter.java @@ -2,11 +2,17 @@ package com.appsmith.server.filters; import com.appsmith.server.acl.AclService; import com.appsmith.server.acl.OpaResponse; +import com.appsmith.server.dtos.ResponseDTO; import com.appsmith.server.exceptions.AppsmithError; -import com.appsmith.server.exceptions.AppsmithException; +import com.appsmith.server.exceptions.ErrorDTO; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.core.io.buffer.DataBuffer; import org.springframework.http.HttpMethod; +import org.springframework.http.HttpStatus; +import org.springframework.http.MediaType; import org.springframework.http.server.reactive.ServerHttpRequest; import org.springframework.stereotype.Component; import org.springframework.web.server.ServerWebExchange; @@ -19,10 +25,12 @@ import reactor.core.publisher.Mono; public class AclFilter implements WebFilter { final AclService aclService; + final ObjectMapper objectMapper; @Autowired - public AclFilter(AclService aclService) { + public AclFilter(AclService aclService, ObjectMapper objectMapper) { this.aclService = aclService; + this.objectMapper = objectMapper; } /** @@ -62,8 +70,28 @@ public class AclFilter implements WebFilter { log.debug("Got ACL response: {}", acl); return acl; }) - .filter(acl -> acl.getResult()) - .switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.UNAUTHORIZED_ACCESS))) - .flatMap(acl -> chain.filter(exchange)); + .flatMap(acl -> { + if (acl != null && acl.getResult()) { + // Acl returned true. Continue with the filter chain + return chain.filter(exchange); + } + + // The Acl response is false. Return unauthorized exception to the client + // We construct the error response JSON here because throwing an exception here doesn't get caught + // in the {@see GlobalExceptionHandler}. + AppsmithError error = AppsmithError.UNAUTHORIZED_ACCESS; + exchange.getResponse().setStatusCode(HttpStatus.resolve(error.getHttpErrorCode())); + exchange.getResponse().getHeaders().setContentType(MediaType.APPLICATION_JSON); + try { + ResponseDTO responseBody = new ResponseDTO<>(error.getHttpErrorCode(), new ErrorDTO(error.getAppErrorCode(), + error.getMessage())); + String responseStr = objectMapper.writeValueAsString(responseBody); + DataBuffer buffer = exchange.getResponse().bufferFactory().allocateBuffer().write(responseStr.getBytes()); + return exchange.getResponse().writeWith(Mono.just(buffer)); + } catch (JsonProcessingException e) { + log.error("Exception caught while serializing JSON in AclFilter. Cause: ", e); + return exchange.getResponse().writeWith(Mono.empty()); + } + }); } }