From 6f7aa1156eca80ea8431beadac18f06abce5d42f Mon Sep 17 00:00:00 2001 From: nscuro Date: Sat, 6 Jul 2024 21:50:07 +0200 Subject: [PATCH] Add `ProblemDetails` for invalid policy conditions The frontend can properly interpret `ProblemDetails` and present a more helpful error message to users than the current generic "request failed" one. Signed-off-by: nscuro --- .../resources/v1/BomResource.java | 8 +-- .../resources/v1/PolicyConditionResource.java | 24 ++++---- .../exception/JsonMappingExceptionMapper.java | 13 ++--- .../InvalidCelExpressionProblemDetails.java | 55 +++++++++++++++++++ .../resources/v1/problems/ProblemDetails.java | 15 ++++- .../resources/v1/vo/CelExpressionError.java | 22 -------- .../v1/PolicyConditionResourceTest.java | 40 +++++++++++++- 7 files changed, 124 insertions(+), 53 deletions(-) create mode 100644 src/main/java/org/dependencytrack/resources/v1/problems/InvalidCelExpressionProblemDetails.java delete mode 100644 src/main/java/org/dependencytrack/resources/v1/vo/CelExpressionError.java diff --git a/src/main/java/org/dependencytrack/resources/v1/BomResource.java b/src/main/java/org/dependencytrack/resources/v1/BomResource.java index 0638de75e..5c66d7bfb 100644 --- a/src/main/java/org/dependencytrack/resources/v1/BomResource.java +++ b/src/main/java/org/dependencytrack/resources/v1/BomResource.java @@ -542,13 +542,7 @@ static void validate(final byte[] bomBytes) { if (!e.getValidationErrors().isEmpty()) { problemDetails.setErrors(e.getValidationErrors()); } - - final Response response = Response.status(Response.Status.BAD_REQUEST) - .header("Content-Type", ProblemDetails.MEDIA_TYPE_JSON) - .entity(problemDetails) - .build(); - - throw new WebApplicationException(response); + throw new WebApplicationException(problemDetails.toResponse()); } catch (RuntimeException e) { LOGGER.error("Failed to validate BOM", e); final Response response = Response.status(Response.Status.INTERNAL_SERVER_ERROR).build(); diff --git a/src/main/java/org/dependencytrack/resources/v1/PolicyConditionResource.java b/src/main/java/org/dependencytrack/resources/v1/PolicyConditionResource.java index 20f9b644e..a8a87556d 100644 --- a/src/main/java/org/dependencytrack/resources/v1/PolicyConditionResource.java +++ b/src/main/java/org/dependencytrack/resources/v1/PolicyConditionResource.java @@ -49,14 +49,12 @@ import org.dependencytrack.policy.cel.CelPolicyScriptHost; import org.dependencytrack.policy.cel.CelPolicyScriptHost.CacheMode; import org.dependencytrack.policy.cel.CelPolicyType; -import org.dependencytrack.resources.v1.vo.CelExpressionError; -import org.projectnessie.cel.common.CELError; +import org.dependencytrack.resources.v1.problems.InvalidCelExpressionProblemDetails; +import org.dependencytrack.resources.v1.problems.ProblemDetails; import org.projectnessie.cel.tools.ScriptCreateException; import javax.jdo.FetchPlan; import javax.jdo.PersistenceManager; -import java.util.ArrayList; -import java.util.Map; /** * JAX-RS resources for processing policies. @@ -174,18 +172,22 @@ private void maybeValidateExpression(final PolicyCondition policyCondition) { } if (policyCondition.getViolationType() == null) { - throw new BadRequestException(Response.status(Response.Status.BAD_REQUEST).entity("Expression conditions must define a violation type").build()); + final var problemDetails = new ProblemDetails(); + problemDetails.setStatus(400); + problemDetails.setTitle("Invalid policy condition"); + problemDetails.setDetail("Expression conditions must define a violation type"); + throw new BadRequestException(problemDetails.toResponse()); } try { CelPolicyScriptHost.getInstance(CelPolicyType.COMPONENT).compile(policyCondition.getValue(), CacheMode.NO_CACHE); } catch (ScriptCreateException e) { - final var celErrors = new ArrayList(); - for (final CELError error : e.getIssues().getErrors()) { - celErrors.add(new CelExpressionError(error.getLocation().line(), error.getLocation().column(), error.getMessage())); - } - - throw new BadRequestException(Response.status(Response.Status.BAD_REQUEST).entity(Map.of("celErrors", celErrors)).build()); + final var problemDetails = new InvalidCelExpressionProblemDetails(e.getIssues()); + problemDetails.setStatus(400); + problemDetails.setTitle("Invalid policy expression"); + problemDetails.setDetail("The provided policy expression contains %d error(s)" + .formatted(problemDetails.getErrors().size())); + throw new BadRequestException(problemDetails.toResponse()); } } diff --git a/src/main/java/org/dependencytrack/resources/v1/exception/JsonMappingExceptionMapper.java b/src/main/java/org/dependencytrack/resources/v1/exception/JsonMappingExceptionMapper.java index 89db6a804..d83e5dfd8 100644 --- a/src/main/java/org/dependencytrack/resources/v1/exception/JsonMappingExceptionMapper.java +++ b/src/main/java/org/dependencytrack/resources/v1/exception/JsonMappingExceptionMapper.java @@ -21,13 +21,13 @@ import com.fasterxml.jackson.core.exc.StreamConstraintsException; import com.fasterxml.jackson.databind.JsonMappingException; import jakarta.annotation.Priority; +import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.ext.ExceptionMapper; +import jakarta.ws.rs.ext.Provider; import org.dependencytrack.resources.v1.problems.ProblemDetails; import org.dependencytrack.resources.v1.vo.BomSubmitRequest; import org.dependencytrack.resources.v1.vo.VexSubmitRequest; -import jakarta.ws.rs.core.Response; -import jakarta.ws.rs.ext.ExceptionMapper; -import jakarta.ws.rs.ext.Provider; import java.util.Objects; /** @@ -43,12 +43,7 @@ public Response toResponse(final JsonMappingException exception) { problemDetails.setStatus(400); problemDetails.setTitle("The provided JSON payload could not be mapped"); problemDetails.setDetail(createDetail(exception)); - - return Response - .status(Response.Status.BAD_REQUEST) - .type(ProblemDetails.MEDIA_TYPE_JSON) - .entity(problemDetails) - .build(); + return problemDetails.toResponse(); } private static String createDetail(final JsonMappingException exception) { diff --git a/src/main/java/org/dependencytrack/resources/v1/problems/InvalidCelExpressionProblemDetails.java b/src/main/java/org/dependencytrack/resources/v1/problems/InvalidCelExpressionProblemDetails.java new file mode 100644 index 000000000..98c3eece6 --- /dev/null +++ b/src/main/java/org/dependencytrack/resources/v1/problems/InvalidCelExpressionProblemDetails.java @@ -0,0 +1,55 @@ +/* + * This file is part of Dependency-Track. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + * Copyright (c) OWASP Foundation. All Rights Reserved. + */ +package org.dependencytrack.resources.v1.problems; + +import io.swagger.v3.oas.annotations.Parameter; +import org.projectnessie.cel.Issues; + +import java.util.List; + +/** + * @since 5.5.0 + */ +public class InvalidCelExpressionProblemDetails extends ProblemDetails { + + public record CelExpressionError( + @Parameter(description = "The line in which the error was identified") Integer line, + @Parameter(description = "The column in which the error was identified") Integer column, + @Parameter(description = "The message describing the error") String message + ) { + } + + @Parameter(description = "Errors identified during expression compilation") + private final List errors; + + public InvalidCelExpressionProblemDetails(final Issues issues) { + this.errors = issues.getErrors().stream() + .map(error -> new CelExpressionError( + error.getLocation().line(), + error.getLocation().column(), + error.getMessage() + )) + .toList(); + } + + public List getErrors() { + return errors; + } + +} diff --git a/src/main/java/org/dependencytrack/resources/v1/problems/ProblemDetails.java b/src/main/java/org/dependencytrack/resources/v1/problems/ProblemDetails.java index d4f3881b8..5cdfdd5b2 100644 --- a/src/main/java/org/dependencytrack/resources/v1/problems/ProblemDetails.java +++ b/src/main/java/org/dependencytrack/resources/v1/problems/ProblemDetails.java @@ -20,6 +20,8 @@ import com.fasterxml.jackson.annotation.JsonInclude; import io.swagger.v3.oas.annotations.media.Schema; +import jakarta.ws.rs.core.HttpHeaders; +import jakarta.ws.rs.core.Response; import java.net.URI; @@ -29,7 +31,10 @@ */ @Schema( description = "An RFC 9457 problem object", - subTypes = InvalidBomProblemDetails.class + subTypes = { + InvalidBomProblemDetails.class, + InvalidCelExpressionProblemDetails.class + } ) @JsonInclude(JsonInclude.Include.NON_NULL) public class ProblemDetails { @@ -69,6 +74,14 @@ public class ProblemDetails { ) private URI instance; + public Response toResponse() { + return Response + .status(status) + .header(HttpHeaders.CONTENT_TYPE, MEDIA_TYPE_JSON) + .entity(this) + .build(); + } + public URI getType() { return type; } diff --git a/src/main/java/org/dependencytrack/resources/v1/vo/CelExpressionError.java b/src/main/java/org/dependencytrack/resources/v1/vo/CelExpressionError.java deleted file mode 100644 index 36eeb5f7e..000000000 --- a/src/main/java/org/dependencytrack/resources/v1/vo/CelExpressionError.java +++ /dev/null @@ -1,22 +0,0 @@ -/* - * This file is part of Dependency-Track. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - * SPDX-License-Identifier: Apache-2.0 - * Copyright (c) OWASP Foundation. All Rights Reserved. - */ -package org.dependencytrack.resources.v1.vo; - -public record CelExpressionError(Integer line, Integer column, String message) { -} diff --git a/src/test/java/org/dependencytrack/resources/v1/PolicyConditionResourceTest.java b/src/test/java/org/dependencytrack/resources/v1/PolicyConditionResourceTest.java index 822040223..90c70115d 100644 --- a/src/test/java/org/dependencytrack/resources/v1/PolicyConditionResourceTest.java +++ b/src/test/java/org/dependencytrack/resources/v1/PolicyConditionResourceTest.java @@ -78,6 +78,32 @@ public void testCreateExpressionCondition() { """); } + @Test + public void testCreateExpressionConditionWithoutViolationType() { + final Policy policy = qm.createPolicy("policy", Policy.Operator.ANY, Policy.ViolationState.FAIL); + + final Response response = jersey.target("%s/%s/condition".formatted(V1_POLICY, policy.getUuid())) + .request() + .header(X_API_KEY, apiKey) + .put(Entity.entity(""" + { + "subject": "EXPRESSION", + "value": "true" + } + """, MediaType.APPLICATION_JSON)); + + assertThat(response.getStatus()).isEqualTo(400); + assertThat(response.getHeaderString("Content-Type")).isEqualTo("application/problem+json"); + assertThatJson(getPlainTextBody(response)) + .isEqualTo(""" + { + "status": 400, + "title": "Invalid policy condition", + "detail": "Expression conditions must define a violation type" + } + """); + } + @Test public void testCreateExpressionConditionWithError() { final Policy policy = qm.createPolicy("policy", Policy.Operator.ANY, Policy.ViolationState.FAIL); @@ -94,10 +120,14 @@ public void testCreateExpressionConditionWithError() { """, MediaType.APPLICATION_JSON)); assertThat(response.getStatus()).isEqualTo(400); + assertThat(response.getHeaderString("Content-Type")).isEqualTo("application/problem+json"); assertThatJson(getPlainTextBody(response)) .isEqualTo(""" { - "celErrors": [ + "status": 400, + "title": "Invalid policy expression", + "detail": "The provided policy expression could not be compiled", + "errors": [ { "line": 1, "column": 9, @@ -158,12 +188,16 @@ public void testUpdateExpressionConditionWithError() { """.formatted(condition.getUuid()), MediaType.APPLICATION_JSON)); assertThat(response.getStatus()).isEqualTo(400); + assertThat(response.getHeaderString("Content-Type")).isEqualTo("application/problem+json"); assertThatJson(getPlainTextBody(response)) .isEqualTo(""" { - "celErrors": [ + "status": 400, + "title": "Invalid policy expression", + "detail": "The provided policy expression could not be compiled", + "errors": [ { - "line": 1, + "line":1, "column": 9, "message": "undefined field 'doesNotExist'" }