From f2e3bc8778ce6226b754ac65ee97fe958715b818 Mon Sep 17 00:00:00 2001 From: Katarina Valalikova Date: Wed, 1 Mar 2017 11:04:22 +0100 Subject: [PATCH] improving error hanlding - REST API --- .../midpoint/model/impl/ModelRestService.java | 52 +++++++++---------- .../MidpointRestAuthenticationHandler.java | 10 ++-- .../model/impl/util/RestServiceUtil.java | 47 ++++++++++------- .../testing/rest/TestAbstractRestService.java | 14 +++++ .../testing/rest/TestJsonProvider.java | 2 - .../testing/rest/TestXmlProvider.java | 2 - .../testing/rest/TestYamlProvider.java | 3 +- 7 files changed, 73 insertions(+), 57 deletions(-) diff --git a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/ModelRestService.java b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/ModelRestService.java index 68fe1010e02..7796c8ab62d 100644 --- a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/ModelRestService.java +++ b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/ModelRestService.java @@ -187,7 +187,7 @@ public Response getValuePolicyForUser(@PathParam("id") String oid, @Context Mess builder.entity(policy); response = builder.build(); } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(parentResult, ex); } parentResult.computeStatus(); @@ -239,7 +239,7 @@ public Response getObject(@PathParam("type") String type, builder.entity(object); response = builder.build(); } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(parentResult, ex); } parentResult.computeStatus(); @@ -262,8 +262,9 @@ public Response addObject(@PathParam("type") String type, Class clazz = ObjectTypes.getClassFromRestType(type); if (!object.getCompileTimeClass().equals(clazz)){ finishRequest(task); - return RestServiceUtil.createErrorResponseBuilder(Status.BAD_REQUEST, "Request to add object of type " - + object.getCompileTimeClass().getSimpleName() + " to the collection of " + type).build(); + parentResult.recordFatalError("Request to add object of type " + + object.getCompileTimeClass().getSimpleName() + " to the collection of " + type); + return RestServiceUtil.createErrorResponseBuilder(Status.BAD_REQUEST, parentResult).build(); } @@ -289,7 +290,7 @@ public Response addObject(@PathParam("type") String type, //validateIfRequested(object, options, builder, task, parentResult); response = builder.build(); } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(parentResult, ex); } parentResult.computeStatus(); @@ -323,9 +324,10 @@ public Response addObject(@PathParam("type") String type, Class clazz = ObjectTypes.getClassFromRestType(type); if (!object.getCompileTimeClass().equals(clazz)){ finishRequest(task); - return RestServiceUtil.createErrorResponseBuilder(Status.BAD_REQUEST, "Request to add object of type " + parentResult.recordFatalError("Request to add object of type " + object.getCompileTimeClass().getSimpleName() - + " to the collection of " + type).build(); + + " to the collection of " + type); + return RestServiceUtil.createErrorResponseBuilder(Status.BAD_REQUEST, parentResult).build(); } ModelExecuteOptions modelExecuteOptions = ModelExecuteOptions.fromRestOptions(options); @@ -346,10 +348,8 @@ public Response addObject(@PathParam("type") String type, Response.accepted().location(resourceURI) : Response.created(resourceURI); // (not used currently) //validateIfRequested(object, options, builder, task, parentResult); - } catch (ObjectAlreadyExistsException e) { - builder = Response.serverError().entity(e.getMessage()); } catch (Exception ex) { - builder = RestServiceUtil.createErrorResponseBuilder(ex); + builder = RestServiceUtil.createErrorResponseBuilder(parentResult, ex); } parentResult.computeStatus(); Response response = RestServiceUtil.createResultHeaders(builder, parentResult).build(); @@ -389,7 +389,7 @@ public Response deleteObject(@PathParam("type") String type, @PathParam("id") St model.deleteObject(clazz, id, modelExecuteOptions, task, parentResult); response = Response.noContent().build(); } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(parentResult, ex); } parentResult.computeStatus(); @@ -424,7 +424,7 @@ public Response modifyObjectPatch(@PathParam("type") Stri model.modifyObject(clazz, oid, modifications, modelExecuteOptions, task, parentResult); response = Response.noContent().build(); } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(parentResult, ex); } parentResult.computeStatus(); @@ -456,7 +456,7 @@ public Response notifyChange(ResourceObjectShadowChangeDescriptionType changeDes // } // response = Response.seeOther((uriInfo.getBaseUriBuilder().path(this.getClass(), "getObject").build(ObjectTypes.TASK.getRestType(), task.getOid()))).build(); } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(parentResult, ex); } parentResult.computeStatus(); @@ -478,10 +478,8 @@ public Response findShadowOwner(@PathParam("oid") String shadowOid, @Context Mes try { PrismObject user = model.findShadowOwner(shadowOid, task, parentResult); response = Response.ok().entity(user).build(); - } catch (ConfigurationException e) { - response = RestServiceUtil.createErrorResponseBuilder(Status.INTERNAL_SERVER_ERROR, e.getMessage()).build(); } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(parentResult, ex); } parentResult.computeStatus(); @@ -517,7 +515,7 @@ public Response searchObjects(@PathParam("type") String type, QueryType queryTyp response = Response.ok().entity(listType).build(); } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(parentResult, ex); } parentResult.computeStatus(); @@ -551,7 +549,7 @@ public Response importFromResource(@PathParam("resourceOid") String resourceOid, response = Response.seeOther((uriInfo.getBaseUriBuilder().path(this.getClass(), "getObject") .build(ObjectTypes.TASK.getRestType(), task.getOid()))).build(); } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(parentResult, ex); } parentResult.computeStatus(); @@ -574,7 +572,7 @@ public Response testResource(@PathParam("resourceOid") String resourceOid, @Cont testResult = model.testResource(resourceOid, task); response = Response.ok(testResult).build(); } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(parentResult, ex); } if (testResult != null) { @@ -603,7 +601,7 @@ public Response suspendTasks(@PathParam("oid") String taskOid, @Context MessageC response = Response.status(Status.INTERNAL_SERVER_ERROR).entity(parentResult.getMessage()).build(); } } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(parentResult, ex); } finishRequest(task); @@ -629,7 +627,7 @@ public Response suspendAndDeleteTasks(@PathParam("oid") String taskOid, @Context response = Response.status(Status.INTERNAL_SERVER_ERROR).entity(parentResult.getMessage()).build(); } } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(parentResult, ex); } finishRequest(task); @@ -656,7 +654,7 @@ public Response resumeTasks(@PathParam("oid") String taskOid, @Context MessageCo response = Response.status(Status.INTERNAL_SERVER_ERROR).entity(parentResult.getMessage()).build(); } } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(parentResult, ex); } finishRequest(task); @@ -685,7 +683,7 @@ public Response scheduleTasksNow(@PathParam("oid") String taskOid, @Context Mess response = Response.status(Status.INTERNAL_SERVER_ERROR).entity(parentResult.getMessage()).build(); } } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(parentResult, ex); } finishRequest(task); @@ -729,7 +727,7 @@ public Response executeScript(ScriptingExpressionType scr response = builder.build(); } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(result, ex); LoggingUtils.logUnexpectedException(LOGGER, "Couldn't execute script.", ex); } @@ -777,7 +775,7 @@ public Response compare(PrismObject clientObject, response = builder.build(); } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(result, ex); } result.computeStatus(); @@ -801,7 +799,7 @@ public Response getLogFileSize(@Context MessageContext mc) { builder.entity(String.valueOf(size)); response = builder.build(); } catch (Exception ex) { - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(result, ex); } result.computeStatus(); @@ -830,7 +828,7 @@ public Response getLog(@QueryParam("fromPosition") Long fromPosition, @QueryPara response = builder.build(); } catch (Exception ex) { LoggingUtils.logUnexpectedException(LOGGER, "Cannot get log file content: fromPosition={}, maxSize={}", ex, fromPosition, maxSize); - response = RestServiceUtil.handleException(ex); + response = RestServiceUtil.handleException(result, ex); } result.computeStatus(); diff --git a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/security/MidpointRestAuthenticationHandler.java b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/security/MidpointRestAuthenticationHandler.java index 72148e9a20d..b2a62560b12 100644 --- a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/security/MidpointRestAuthenticationHandler.java +++ b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/security/MidpointRestAuthenticationHandler.java @@ -79,14 +79,14 @@ public void handleRequest(Message m, ContainerRequestContext requestCtx) { AuthorizationPolicy policy = (AuthorizationPolicy)m.get(AuthorizationPolicy.class); if (policy == null){ - requestCtx.abortWith(Response.status(401).header("WWW-Authenticate", "Basic").build()); + requestCtx.abortWith(Response.status(Status.UNAUTHORIZED).header("WWW-Authenticate", "Basic").build()); return; } String enteredUsername = policy.getUserName(); if (enteredUsername == null){ - requestCtx.abortWith(Response.status(401).header("WWW-Authenticate", "Basic").build()); + requestCtx.abortWith(Response.status(Status.UNAUTHORIZED).header("WWW-Authenticate", "Basic").build()); return; } @@ -104,12 +104,12 @@ public void handleRequest(Message m, ContainerRequestContext requestCtx) { token = authenticationEvaluator.authenticateUserPassword(connEnv, enteredUsername, enteredPassword); } catch (UsernameNotFoundException | BadCredentialsException e) { LOGGER.trace("Exception while authenticating username '{}' to REST service: {}", enteredUsername, e.getMessage(), e); - requestCtx.abortWith(Response.status(401).header("WWW-Authenticate", "Basic authentication failed. Cannot authenticate user.").build()); + requestCtx.abortWith(Response.status(Status.UNAUTHORIZED).header("WWW-Authenticate", "Basic authentication failed. Cannot authenticate user.").build()); return; } catch (DisabledException | LockedException | CredentialsExpiredException | AccessDeniedException | AuthenticationCredentialsNotFoundException | AuthenticationServiceException e) { LOGGER.trace("Exception while authenticating username '{}' to REST service: {}", enteredUsername, e.getMessage(), e); - requestCtx.abortWith(Response.status(403).build()); + requestCtx.abortWith(Response.status(Status.FORBIDDEN).build()); return; } @@ -128,7 +128,7 @@ public void handleRequest(Message m, ContainerRequestContext requestCtx) { securityEnforcer.authorize(AuthorizationConstants.AUTZ_REST_ALL_URL, null, null, null, null, null, authorizeResult); } catch (SecurityViolationException e){ securityHelper.auditLoginFailure(enteredUsername, user, connEnv, "Not authorized"); - requestCtx.abortWith(Response.status(403).build()); + requestCtx.abortWith(Response.status(Status.FORBIDDEN).build()); return; } catch (SchemaException e) { securityHelper.auditLoginFailure(enteredUsername, user, connEnv, "Schema error: "+e.getMessage()); diff --git a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/util/RestServiceUtil.java b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/util/RestServiceUtil.java index ed80ad5e604..a79cf3a9b15 100644 --- a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/util/RestServiceUtil.java +++ b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/util/RestServiceUtil.java @@ -24,6 +24,8 @@ import com.evolveum.midpoint.security.api.ConnectionEnvironment; import com.evolveum.midpoint.task.api.Task; import com.evolveum.midpoint.util.exception.*; +import com.evolveum.midpoint.xml.ns._public.common.common_3.OperationResultType; + import org.apache.cxf.jaxrs.ext.MessageContext; import javax.ws.rs.core.MediaType; @@ -41,43 +43,50 @@ public class RestServiceUtil { public static final String OPERATION_RESULT_STATUS = "OperationResultStatus"; public static final String OPERATION_RESULT_MESSAGE = "OperationResultMessage"; - public static Response handleException(Exception ex) { - return createErrorResponseBuilder(ex).build(); + public static Response handleException(OperationResult result, Exception ex) { + return createErrorResponseBuilder(result, ex).build(); } - public static Response.ResponseBuilder createErrorResponseBuilder(Exception ex) { + public static Response.ResponseBuilder createErrorResponseBuilder(OperationResult result, Exception ex) { if (ex instanceof ObjectNotFoundException) { - return createErrorResponseBuilder(Response.Status.NOT_FOUND, ex); + return createErrorResponseBuilder(Response.Status.NOT_FOUND, result); } - if (ex instanceof CommunicationException) { - return createErrorResponseBuilder(Response.Status.GATEWAY_TIMEOUT, ex); + if (ex instanceof CommunicationException || ex instanceof TunnelException) { + return createErrorResponseBuilder(Response.Status.GATEWAY_TIMEOUT, result); } - if (ex instanceof SecurityViolationException) { - return createErrorResponseBuilder(Response.Status.FORBIDDEN, ex); + if (ex instanceof SecurityViolationException || ex instanceof AuthorizationException) { + return createErrorResponseBuilder(Response.Status.FORBIDDEN, result); } - + if (ex instanceof ConfigurationException) { - return createErrorResponseBuilder(Response.Status.BAD_GATEWAY, ex); + return createErrorResponseBuilder(Response.Status.BAD_GATEWAY, result); + } + + if (ex instanceof SchemaException + || ex instanceof NoFocusNameSchemaException + || ex instanceof ExpressionEvaluationException) { + return createErrorResponseBuilder(Response.Status.BAD_REQUEST, result); } - if (ex instanceof SchemaException - || ex instanceof PolicyViolationException + if (ex instanceof PolicyViolationException || ex instanceof ConsistencyViolationException - || ex instanceof ObjectAlreadyExistsException) { - return createErrorResponseBuilder(Response.Status.CONFLICT, ex); + || ex instanceof ObjectAlreadyExistsException + || ex instanceof ConcurrencyException) { + return createErrorResponseBuilder(Response.Status.CONFLICT, result); } - return createErrorResponseBuilder(Response.Status.INTERNAL_SERVER_ERROR, ex); + return createErrorResponseBuilder(Response.Status.INTERNAL_SERVER_ERROR, result); } - private static Response.ResponseBuilder createErrorResponseBuilder(Response.Status status, Exception ex) { - return createErrorResponseBuilder(status, ex.getMessage()); + public static Response.ResponseBuilder createErrorResponseBuilder(Response.Status status, OperationResult result) { + result.computeStatusIfUnknown(); + return createErrorResponseBuilder(status, result.createOperationResultType()); } - public static Response.ResponseBuilder createErrorResponseBuilder(Response.Status status, String message) { - return Response.status(status).entity(message).type(MediaType.TEXT_PLAIN); + public static Response.ResponseBuilder createErrorResponseBuilder(Response.Status status, OperationResultType message) { + return Response.status(status).entity(message); } public static ModelExecuteOptions getOptions(UriInfo uriInfo){ diff --git a/testing/rest/src/test/java/com/evolveum/midpoint/testing/rest/TestAbstractRestService.java b/testing/rest/src/test/java/com/evolveum/midpoint/testing/rest/TestAbstractRestService.java index 62f09dec562..d83ab292df8 100644 --- a/testing/rest/src/test/java/com/evolveum/midpoint/testing/rest/TestAbstractRestService.java +++ b/testing/rest/src/test/java/com/evolveum/midpoint/testing/rest/TestAbstractRestService.java @@ -73,6 +73,8 @@ import com.evolveum.midpoint.xml.ns._public.common.common_3.ActivationStatusType; import com.evolveum.midpoint.xml.ns._public.common.common_3.ObjectTemplateType; import com.evolveum.midpoint.xml.ns._public.common.common_3.ObjectType; +import com.evolveum.midpoint.xml.ns._public.common.common_3.OperationResultStatusType; +import com.evolveum.midpoint.xml.ns._public.common.common_3.OperationResultType; import com.evolveum.midpoint.xml.ns._public.common.common_3.RoleType; import com.evolveum.midpoint.xml.ns._public.common.common_3.ShadowType; import com.evolveum.midpoint.xml.ns._public.common.common_3.SystemObjectsType; @@ -270,6 +272,10 @@ public void test002GetNonExistingUser() { TestUtil.displayThen(TEST_NAME); assertStatus(response, 404); + OperationResultType result = response.readEntity(OperationResultType.class); + assertNotNull("Error response must contain operation result", result); + LOGGER.info("Returned result: {}", result); + assertEquals("Unexpected operation result status", OperationResultStatusType.FATAL_ERROR, result.getStatus()); IntegrationTestTools.display("Audit", dummyAuditService); dummyAuditService.assertRecords(2); @@ -511,6 +517,10 @@ public void test103AddUserBadTargetCollection() throws Exception { displayResponse(response); assertStatus(response, 400); + OperationResultType result = response.readEntity(OperationResultType.class); + assertNotNull("Error response must contain operation result", result); + LOGGER.info("Returned result: {}", result); + assertEquals("Unexpected operation result status", OperationResultStatusType.FATAL_ERROR, result.getStatus()); IntegrationTestTools.display("Audit", dummyAuditService); dummyAuditService.assertRecords(2); @@ -640,6 +650,10 @@ public void test123DarthAdderAssignModifierHimself() throws Exception { TestUtil.displayThen(TEST_NAME); displayResponse(response); assertStatus(response, 403); + OperationResultType result = response.readEntity(OperationResultType.class); + assertNotNull("Error response must contain operation result", result); + LOGGER.info("Returned result: {}", result); + assertEquals("Unexpected operation result status", OperationResultStatusType.FATAL_ERROR, result.getStatus()); IntegrationTestTools.display("Audit", dummyAuditService); dummyAuditService.assertRecords(4); diff --git a/testing/rest/src/test/java/com/evolveum/midpoint/testing/rest/TestJsonProvider.java b/testing/rest/src/test/java/com/evolveum/midpoint/testing/rest/TestJsonProvider.java index 93ecdd7d232..f4fad3d0756 100644 --- a/testing/rest/src/test/java/com/evolveum/midpoint/testing/rest/TestJsonProvider.java +++ b/testing/rest/src/test/java/com/evolveum/midpoint/testing/rest/TestJsonProvider.java @@ -35,8 +35,6 @@ import com.evolveum.midpoint.model.impl.rest.MidpointXmlProvider; import com.evolveum.midpoint.prism.PrismObject; -@Consumes("*/*") -@Produces("*/*") public class TestJsonProvider extends MidpointJsonProvider { diff --git a/testing/rest/src/test/java/com/evolveum/midpoint/testing/rest/TestXmlProvider.java b/testing/rest/src/test/java/com/evolveum/midpoint/testing/rest/TestXmlProvider.java index 37ce88a3110..367d43081f3 100644 --- a/testing/rest/src/test/java/com/evolveum/midpoint/testing/rest/TestXmlProvider.java +++ b/testing/rest/src/test/java/com/evolveum/midpoint/testing/rest/TestXmlProvider.java @@ -34,8 +34,6 @@ import com.evolveum.midpoint.model.impl.rest.MidpointXmlProvider; import com.evolveum.midpoint.prism.PrismObject; -@Consumes("*/*") -@Produces("*/*") public class TestXmlProvider extends MidpointXmlProvider { diff --git a/testing/rest/src/test/java/com/evolveum/midpoint/testing/rest/TestYamlProvider.java b/testing/rest/src/test/java/com/evolveum/midpoint/testing/rest/TestYamlProvider.java index ed605ac21d2..354acdbb07a 100644 --- a/testing/rest/src/test/java/com/evolveum/midpoint/testing/rest/TestYamlProvider.java +++ b/testing/rest/src/test/java/com/evolveum/midpoint/testing/rest/TestYamlProvider.java @@ -37,8 +37,7 @@ import com.evolveum.midpoint.util.logging.Trace; import com.evolveum.midpoint.util.logging.TraceManager; -@Consumes("*/*") -@Produces("*/*") + public class TestYamlProvider extends MidpointYamlProvider { private static final Trace LOGGER = TraceManager.getTrace(TestYamlProvider.class);