SONAR-10088 Prevent updating built-in quality gate when updating condition

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 1515445854be1691188943c2344aa70365249052..430891455a307ae39e4c9b427c8de36347a4e52b 100644 (file)
--- 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<QualityGateConditionDto> getConditions(DbSession dbSession, long qGateId, @Nullable Long conditionId) {
     Collection<QualityGateConditionDto> 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 5bfd1c58e9e4025674e169e1387136072334b7f9..02684fbefe6f9a6ede5e9ff0e5ab73c918f00650 100644 (file)
--- 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 ae441916ebb18b9f1d210e498fcf47b2e49ae79f..2e4fb975138552ee4a700d4062c18a9c47e857f6 100644 (file)
--- 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 cd1737f67132ddce47397eb33e02917dab5dcc0a..be65a16c8ee75b861ca162fa7ea016746c6a8f1a 100644 (file)
--- 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<QualityGateConditionDto> 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));
+  }
+
 }