From 379c21e88c639d0622daa61fc0209c3d577f96a3 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Fri, 24 Nov 2017 13:23:27 +0100 Subject: [PATCH] SONAR-10088 Prevent updating built-in quality gate when updating condition --- .../QualityGateConditionsUpdater.java | 11 +-- .../qualitygate/ws/UpdateConditionAction.java | 34 ++++---- .../QualityGateConditionsUpdaterTest.java | 14 ++-- .../ws/UpdateConditionActionTest.java | 79 ++++++++++++++----- 4 files changed, 84 insertions(+), 54 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualitygate/QualityGateConditionsUpdater.java b/server/sonar-server/src/main/java/org/sonar/server/qualitygate/QualityGateConditionsUpdater.java index 1515445854b..430891455a3 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/qualitygate/QualityGateConditionsUpdater.java +++ b/server/sonar-server/src/main/java/org/sonar/server/qualitygate/QualityGateConditionsUpdater.java @@ -75,9 +75,8 @@ public class QualityGateConditionsUpdater { return newCondition; } - public QualityGateConditionDto updateCondition(DbSession dbSession, long condId, String metricKey, String operator, + public QualityGateConditionDto updateCondition(DbSession dbSession, QualityGateConditionDto condition, String metricKey, String operator, @Nullable String warningThreshold, @Nullable String errorThreshold, @Nullable Integer period) { - QualityGateConditionDto condition = getNonNullCondition(dbSession, condId); MetricDto metric = getNonNullMetric(dbSession, metricKey); validateCondition(metric, operator, warningThreshold, errorThreshold, period); checkConditionDoesNotAlreadyExistOnSameMetricAndPeriod(getConditions(dbSession, condition.getQualityGateId(), condition.getId()), metric, period); @@ -109,14 +108,6 @@ public class QualityGateConditionsUpdater { return metric; } - private QualityGateConditionDto getNonNullCondition(DbSession dbSession, long id) { - QualityGateConditionDto condition = dbClient.gateConditionDao().selectById(id, dbSession); - if (condition == null) { - throw new NotFoundException("There is no condition with id=" + id); - } - return condition; - } - private Collection getConditions(DbSession dbSession, long qGateId, @Nullable Long conditionId) { Collection conditions = dbClient.gateConditionDao().selectForQualityGate(dbSession, qGateId); if (conditionId == null) { diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualitygate/ws/UpdateConditionAction.java b/server/sonar-server/src/main/java/org/sonar/server/qualitygate/ws/UpdateConditionAction.java index 5bfd1c58e9e..02684fbefe6 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/qualitygate/ws/UpdateConditionAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/qualitygate/ws/UpdateConditionAction.java @@ -25,13 +25,12 @@ import org.sonar.api.server.ws.WebService; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.qualitygate.QualityGateConditionDto; -import org.sonar.server.organization.DefaultOrganizationProvider; +import org.sonar.db.qualitygate.QualityGateDto; import org.sonar.server.qualitygate.QualityGateConditionsUpdater; -import org.sonar.server.user.UserSession; import org.sonarqube.ws.Qualitygates.UpdateConditionResponse; +import static com.google.common.base.Preconditions.checkState; import static org.sonar.core.util.Protobuf.setNullable; -import static org.sonar.db.permission.OrganizationPermission.ADMINISTER_QUALITY_GATES; import static org.sonar.server.qualitygate.ws.QualityGatesWs.addConditionParams; import static org.sonar.server.ws.WsUtils.writeProtobuf; import static org.sonar.server.qualitygate.ws.QualityGatesWsParameters.ACTION_UPDATE_CONDITION; @@ -44,17 +43,14 @@ import static org.sonar.server.qualitygate.ws.QualityGatesWsParameters.PARAM_WAR public class UpdateConditionAction implements QualityGatesWsAction { - private final UserSession userSession; private final DbClient dbClient; private final QualityGateConditionsUpdater qualityGateConditionsUpdater; - private final DefaultOrganizationProvider defaultOrganizationProvider; + private final QualityGatesWsSupport wsSupport; - public UpdateConditionAction(UserSession userSession, DbClient dbClient, QualityGateConditionsUpdater qualityGateConditionsUpdater, - DefaultOrganizationProvider defaultOrganizationProvider) { - this.userSession = userSession; + public UpdateConditionAction(DbClient dbClient, QualityGateConditionsUpdater qualityGateConditionsUpdater, QualityGatesWsSupport wsSupport) { this.dbClient = dbClient; this.qualityGateConditionsUpdater = qualityGateConditionsUpdater; - this.defaultOrganizationProvider = defaultOrganizationProvider; + this.wsSupport = wsSupport; } @Override @@ -77,8 +73,6 @@ public class UpdateConditionAction implements QualityGatesWsAction { @Override public void handle(Request request, Response response) { - userSession.checkPermission(ADMINISTER_QUALITY_GATES, defaultOrganizationProvider.get().getUuid()); - int id = request.mandatoryParamAsInt(PARAM_ID); String metric = request.mandatoryParam(PARAM_METRIC); String operator = request.mandatoryParam(PARAM_OPERATOR); @@ -87,14 +81,18 @@ public class UpdateConditionAction implements QualityGatesWsAction { Integer period = request.paramAsInt(PARAM_PERIOD); try (DbSession dbSession = dbClient.openSession(false)) { - QualityGateConditionDto condition = qualityGateConditionsUpdater.updateCondition(dbSession, id, metric, operator, warning, error, period); + QualityGateConditionDto condition = wsSupport.getCondition(dbSession, id); + QualityGateDto qualityGateDto = dbClient.qualityGateDao().selectById(dbSession, condition.getQualityGateId()); + checkState(qualityGateDto != null, "Condition '%s' is linked to an unknown quality gate '%s'", id, condition.getQualityGateId()); + wsSupport.checkCanEdit(qualityGateDto); + QualityGateConditionDto updatedCondition = qualityGateConditionsUpdater.updateCondition(dbSession, condition, metric, operator, warning, error, period); UpdateConditionResponse.Builder updateConditionResponse = UpdateConditionResponse.newBuilder() - .setId(condition.getId()) - .setMetric(condition.getMetricKey()) - .setOp(condition.getOperator()); - setNullable(condition.getWarningThreshold(), updateConditionResponse::setWarning); - setNullable(condition.getErrorThreshold(), updateConditionResponse::setError); - setNullable(condition.getPeriod(), updateConditionResponse::setPeriod); + .setId(updatedCondition.getId()) + .setMetric(updatedCondition.getMetricKey()) + .setOp(updatedCondition.getOperator()); + setNullable(updatedCondition.getWarningThreshold(), updateConditionResponse::setWarning); + setNullable(updatedCondition.getErrorThreshold(), updateConditionResponse::setError); + setNullable(updatedCondition.getPeriod(), updateConditionResponse::setPeriod); writeProtobuf(updateConditionResponse.build(), request, response); dbSession.commit(); } diff --git a/server/sonar-server/src/test/java/org/sonar/server/qualitygate/QualityGateConditionsUpdaterTest.java b/server/sonar-server/src/test/java/org/sonar/server/qualitygate/QualityGateConditionsUpdaterTest.java index ae441916ebb..2e4fb975138 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/qualitygate/QualityGateConditionsUpdaterTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/qualitygate/QualityGateConditionsUpdaterTest.java @@ -216,7 +216,7 @@ public class QualityGateConditionsUpdaterTest { public void update_condition() { QualityGateConditionDto condition = insertCondition(coverageMetricDto.getId(), "LT", null, "80", null); - QualityGateConditionDto result = underTest.updateCondition(dbSession, condition.getId(), "coverage", "GT", "60", null, 1); + QualityGateConditionDto result = underTest.updateCondition(dbSession, condition, "coverage", "GT", "60", null, 1); verifyCondition(result, coverageMetricDto.getId(), "GT", "60", null, 1); } @@ -225,7 +225,7 @@ public class QualityGateConditionsUpdaterTest { public void update_condition_over_leak_period() { QualityGateConditionDto condition = insertCondition(coverageMetricDto.getId(), "GT", "80", null, 1); - QualityGateConditionDto result = underTest.updateCondition(dbSession, condition.getId(), "coverage", "LT", null, "80", null); + QualityGateConditionDto result = underTest.updateCondition(dbSession, condition, "coverage", "LT", null, "80", null); verifyCondition(result, coverageMetricDto.getId(), "LT", null, "80", null); } @@ -234,7 +234,7 @@ public class QualityGateConditionsUpdaterTest { public void update_condition_on_rating_metric() { QualityGateConditionDto condition = insertCondition(ratingMetricDto.getId(), "LT", null, "3", null); - QualityGateConditionDto result = underTest.updateCondition(dbSession, condition.getId(), ratingMetricDto.getKey(), "GT", "4", null, null); + QualityGateConditionDto result = underTest.updateCondition(dbSession, condition, ratingMetricDto.getKey(), "GT", "4", null, null); verifyCondition(result, ratingMetricDto.getId(), "GT", "4", null, null); } @@ -245,7 +245,7 @@ public class QualityGateConditionsUpdaterTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("The metric 'Reliability Rating' cannot be used on the leak period"); - underTest.updateCondition(dbSession, condition.getId(), ratingMetricDto.getKey(), "GT", "4", null, 1); + underTest.updateCondition(dbSession, condition, ratingMetricDto.getKey(), "GT", "4", null, 1); } @Test @@ -258,7 +258,7 @@ public class QualityGateConditionsUpdaterTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("The metric 'Not core rating' cannot be used"); - underTest.updateCondition(dbSession, condition.getId(), metricDto.getKey(), "GT", "4", null, 1); + underTest.updateCondition(dbSession, condition, metricDto.getKey(), "GT", "4", null, 1); } @Test @@ -273,7 +273,7 @@ public class QualityGateConditionsUpdaterTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("Metric '" + metricKey + "' cannot be used to define a condition."); - underTest.updateCondition(dbSession, condition.getId(), metricDto.getKey(), "GT", "60", null, 1); + underTest.updateCondition(dbSession, condition, metricDto.getKey(), "GT", "60", null, 1); } @Test @@ -284,7 +284,7 @@ public class QualityGateConditionsUpdaterTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("Condition on metric 'Coverage' over leak period already exists."); // Update condition not on leak period to be on leak period => will fail as this condition already exist - underTest.updateCondition(dbSession, conditionNotOnLeakPeriod.getId(), coverageMetricDto.getKey(), "GT", "80", null, 1); + underTest.updateCondition(dbSession, conditionNotOnLeakPeriod, coverageMetricDto.getKey(), "GT", "80", null, 1); } @DataProvider diff --git a/server/sonar-server/src/test/java/org/sonar/server/qualitygate/ws/UpdateConditionActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/qualitygate/ws/UpdateConditionActionTest.java index cd1737f6713..be65a16c8ee 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/qualitygate/ws/UpdateConditionActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/qualitygate/ws/UpdateConditionActionTest.java @@ -19,8 +19,6 @@ */ package org.sonar.server.qualitygate.ws; -import java.util.ArrayList; -import java.util.List; import javax.annotation.Nullable; import org.junit.Rule; import org.junit.Test; @@ -35,6 +33,7 @@ import org.sonar.db.organization.OrganizationDto; import org.sonar.db.qualitygate.QualityGateConditionDto; import org.sonar.db.qualitygate.QualityGateDto; import org.sonar.server.exceptions.ForbiddenException; +import org.sonar.server.exceptions.NotFoundException; import org.sonar.server.organization.TestDefaultOrganizationProvider; import org.sonar.server.qualitygate.QualityGateConditionsUpdater; import org.sonar.server.tester.UserSessionRule; @@ -42,7 +41,9 @@ import org.sonar.server.ws.TestRequest; import org.sonar.server.ws.WsActionTester; import org.sonarqube.ws.Qualitygates.CreateConditionResponse; +import static java.lang.String.format; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.tuple; import static org.sonar.api.measures.Metric.ValueType.INT; import static org.sonar.db.permission.OrganizationPermission.ADMINISTER_QUALITY_GATES; import static org.sonar.server.qualitygate.ws.QualityGatesWsParameters.PARAM_ERROR; @@ -66,7 +67,8 @@ public class UpdateConditionActionTest { private TestDefaultOrganizationProvider defaultOrganizationProvider = TestDefaultOrganizationProvider.from(db); private DbClient dbClient = db.getDbClient(); private DbSession dbSession = db.getSession(); - private UpdateConditionAction underTest = new UpdateConditionAction(userSession, dbClient, new QualityGateConditionsUpdater(dbClient), defaultOrganizationProvider); + private UpdateConditionAction underTest = new UpdateConditionAction(dbClient, new QualityGateConditionsUpdater(dbClient), + new QualityGatesWsSupport(dbClient, userSession, defaultOrganizationProvider)); private WsActionTester ws = new WsActionTester(underTest); @@ -74,7 +76,7 @@ public class UpdateConditionActionTest { public void update_warning_condition() throws Exception { logInAsQualityGateAdmin(); QualityGateDto qualityGate = db.qualityGates().insertQualityGate(); - MetricDto metric = db.measures().insertMetric(m -> m.setValueType(INT.name()).setHidden(false)); + MetricDto metric = insertMetric(); QualityGateConditionDto condition = db.qualityGates().addCondition(qualityGate, metric, c -> c.setOperator("GT").setWarningThreshold(null).setErrorThreshold("80").setPeriod(null)); @@ -87,7 +89,7 @@ public class UpdateConditionActionTest { public void update_error_condition() throws Exception { logInAsQualityGateAdmin(); QualityGateDto qualityGate = db.qualityGates().insertQualityGate(); - MetricDto metric = db.measures().insertMetric(m -> m.setValueType(INT.name()).setHidden(false)); + MetricDto metric = insertMetric(); QualityGateConditionDto condition = db.qualityGates().addCondition(qualityGate, metric, c -> c.setOperator("GT").setWarningThreshold(null).setErrorThreshold("80").setPeriod(null)); @@ -100,20 +102,20 @@ public class UpdateConditionActionTest { public void update_condition_over_leak_period() throws Exception { logInAsQualityGateAdmin(); QualityGateDto qualityGate = db.qualityGates().insertQualityGate(); - MetricDto metric = db.measures().insertMetric(m -> m.setValueType(INT.name()).setHidden(false)); + MetricDto metric = insertMetric(); QualityGateConditionDto condition = db.qualityGates().addCondition(qualityGate, metric, c -> c.setOperator("GT").setWarningThreshold(null).setErrorThreshold("80").setPeriod(null)); executeRequest(condition.getId(), metric.getKey(), "LT", null, "90", 1); - assertCondition(qualityGate, metric,"LT", null, "90", 1); + assertCondition(qualityGate, metric, "LT", null, "90", 1); } @Test public void test_response() throws Exception { logInAsQualityGateAdmin(); QualityGateDto qualityGate = db.qualityGates().insertQualityGate(); - MetricDto metric = db.measures().insertMetric(m -> m.setValueType(INT.name()).setHidden(false)); + MetricDto metric = insertMetric(); QualityGateConditionDto condition = db.qualityGates().addCondition(qualityGate, metric, c -> c.setOperator("GT").setWarningThreshold(null).setErrorThreshold("80").setPeriod(null)); @@ -127,11 +129,51 @@ public class UpdateConditionActionTest { assertThat(response.getPeriod()).isEqualTo(1); } + @Test + public void fail_to_update_built_in_quality_gate() { + logInAsQualityGateAdmin(); + QualityGateDto qualityGate = db.qualityGates().insertQualityGate(qg -> qg.setBuiltIn(true)); + MetricDto metric = insertMetric(); + QualityGateConditionDto condition = db.qualityGates().addCondition(qualityGate, metric); + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage(format("Operation forbidden for built-in Quality Gate '%s'", qualityGate.getName())); + + executeRequest(condition.getId(), metric.getKey(), "LT", "90", "45", 1); + } + + @Test + public void fail_on_unknown_condition() { + logInAsQualityGateAdmin(); + QualityGateDto qualityGate = db.qualityGates().insertQualityGate(); + MetricDto metric = insertMetric(); + db.qualityGates().addCondition(qualityGate, metric); + + expectedException.expect(NotFoundException.class); + expectedException.expectMessage("No quality gate condition with id '123'"); + + executeRequest(123L, metric.getKey(), "LT", "90", null, null); + } + + @Test + public void fail_when_condition_match_unknown_quality_gate() { + logInAsQualityGateAdmin(); + MetricDto metric = insertMetric(); + QualityGateConditionDto condition = new QualityGateConditionDto().setQualityGateId(123L); + db.getDbClient().gateConditionDao().insert(condition, dbSession); + db.commit(); + + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage(format("Condition '%s' is linked to an unknown quality gate '%s'", condition.getId(), 123L)); + + executeRequest(condition.getId(), metric.getKey(), "LT", "90", null, null); + } + @Test public void throw_ForbiddenException_if_not_gate_administrator() throws Exception { userSession.logIn(); QualityGateDto qualityGate = db.qualityGates().insertQualityGate(); - MetricDto metric = db.measures().insertMetric(m -> m.setValueType(INT.name()).setHidden(false)); + MetricDto metric = insertMetric(); QualityGateConditionDto condition = db.qualityGates().addCondition(qualityGate, metric); expectedException.expect(ForbiddenException.class); @@ -147,7 +189,7 @@ public class UpdateConditionActionTest { OrganizationDto org = db.organizations().insert(); userSession.logIn().addPermission(ADMINISTER_QUALITY_GATES, org); QualityGateDto qualityGate = db.qualityGates().insertQualityGate(); - MetricDto metric = db.measures().insertMetric(m -> m.setValueType(INT.name()).setHidden(false)); + MetricDto metric = insertMetric(); QualityGateConditionDto condition = db.qualityGates().addCondition(qualityGate, metric); expectedException.expect(ForbiddenException.class); @@ -167,15 +209,10 @@ public class UpdateConditionActionTest { } private void assertCondition(QualityGateDto qualityGate, MetricDto metric, String operator, @Nullable String warning, @Nullable String error, @Nullable Integer period) { - List conditionDtoList = new ArrayList<>(dbClient.gateConditionDao().selectForQualityGate(dbSession, qualityGate.getId())); - assertThat(conditionDtoList).hasSize(1); - QualityGateConditionDto condition = conditionDtoList.get(0); - assertThat(condition.getQualityGateId()).isEqualTo(qualityGate.getId()); - assertThat(condition.getMetricId()).isEqualTo(metric.getId().longValue()); - assertThat(condition.getOperator()).isEqualTo(operator); - assertThat(condition.getWarningThreshold()).isEqualTo(warning); - assertThat(condition.getErrorThreshold()).isEqualTo(error); - assertThat(condition.getPeriod()).isEqualTo(period); + assertThat(dbClient.gateConditionDao().selectForQualityGate(dbSession, qualityGate.getId())) + .extracting(QualityGateConditionDto::getQualityGateId, QualityGateConditionDto::getMetricId, QualityGateConditionDto::getOperator, + QualityGateConditionDto::getWarningThreshold, QualityGateConditionDto::getErrorThreshold, QualityGateConditionDto::getPeriod) + .containsExactlyInAnyOrder(tuple(qualityGate.getId(), metric.getId().longValue(), operator, warning, error, period)); } private CreateConditionResponse executeRequest(long conditionId, String metricKey, String operator, @Nullable String warning, @Nullable String error, @@ -200,4 +237,8 @@ public class UpdateConditionActionTest { userSession.logIn().addPermission(ADMINISTER_QUALITY_GATES, db.getDefaultOrganization()); } + private MetricDto insertMetric() { + return db.measures().insertMetric(m -> m.setValueType(INT.name()).setHidden(false)); + } + } -- 2.39.5