From 908cb9a5c6f73444e0a60e808f20a15820bb380f Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Tue, 28 Nov 2017 08:54:10 +0100 Subject: [PATCH] SONAR-10096 Forbid deleting built-in quality gate --- .../server/qualitygate/QualityGates.java | 27 ----- .../server/qualitygate/ws/DestroyAction.java | 28 ++++- .../qualitygate/ws/DestroyActionTest.java | 106 ++++++++++++++---- .../qualitygate/ws/QualityGatesWsTest.java | 29 +---- .../tests/qualityGate/QualityGateTest.java | 10 +- 5 files changed, 111 insertions(+), 89 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualitygate/QualityGates.java b/server/sonar-server/src/main/java/org/sonar/server/qualitygate/QualityGates.java index 983350a7e8c..8564a66bf00 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/qualitygate/QualityGates.java +++ b/server/sonar-server/src/main/java/org/sonar/server/qualitygate/QualityGates.java @@ -23,7 +23,6 @@ import com.google.common.base.Strings; import java.util.ArrayList; import java.util.List; import javax.annotation.Nullable; -import org.apache.commons.lang.StringUtils; import org.sonar.api.web.UserRole; import org.sonar.db.DbClient; import org.sonar.db.DbSession; @@ -87,20 +86,6 @@ public class QualityGates { } } - @Deprecated // GJT - public void delete(long idToDelete) { - checkIsQualityGateAdministrator(); - QualityGateDto qGate = getNonNullQgate(idToDelete); - try (DbSession session = dbClient.openSession(false)) { - if (isDefault(qGate)) { - propertiesDao.deleteGlobalProperty(SONAR_QUALITYGATE_PROPERTY, session); - } - propertiesDao.deleteProjectProperties(SONAR_QUALITYGATE_PROPERTY, Long.toString(idToDelete), session); - dao.delete(qGate, session); - session.commit(); - } - } - /** * Use {@link QualityGateUpdater#setDefault(DbSession, QualityGateDto)} * @deprecated @@ -134,18 +119,6 @@ public class QualityGates { dbSession.commit(); } - private boolean isDefault(QualityGateDto qGate) { - return qGate.getId().equals(getDefaultId()); - } - - private Long getDefaultId() { - PropertyDto defaultQgate = propertiesDao.selectGlobalProperty(SONAR_QUALITYGATE_PROPERTY); - if (defaultQgate == null || StringUtils.isBlank(defaultQgate.getValue())) { - return null; - } - return Long.valueOf(defaultQgate.getValue()); - } - private QualityGateDto getNonNullQgate(long id) { try (DbSession dbSession = dbClient.openSession(false)) { return getNonNullQgate(dbSession, id); diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualitygate/ws/DestroyAction.java b/server/sonar-server/src/main/java/org/sonar/server/qualitygate/ws/DestroyAction.java index f2aca2ffea6..c90a1ba85ba 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/qualitygate/ws/DestroyAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/qualitygate/ws/DestroyAction.java @@ -22,14 +22,21 @@ package org.sonar.server.qualitygate.ws; import org.sonar.api.server.ws.Request; import org.sonar.api.server.ws.Response; import org.sonar.api.server.ws.WebService; -import org.sonar.server.qualitygate.QualityGates; +import org.sonar.db.DbClient; +import org.sonar.db.DbSession; +import org.sonar.db.qualitygate.QualityGateDto; +import org.sonar.server.qualitygate.QualityGateFinder; public class DestroyAction implements QualityGatesWsAction { - private final QualityGates qualityGates; + private final DbClient dbClient; + private final QualityGatesWsSupport wsSupport; + private final QualityGateFinder finder; - public DestroyAction(QualityGates qualityGates) { - this.qualityGates = qualityGates; + public DestroyAction(DbClient dbClient, QualityGatesWsSupport wsSupport, QualityGateFinder finder) { + this.dbClient = dbClient; + this.wsSupport = wsSupport; + this.finder = finder; } @Override @@ -49,8 +56,17 @@ public class DestroyAction implements QualityGatesWsAction { @Override public void handle(Request request, Response response) { - qualityGates.delete(QualityGatesWs.parseId(request, QualityGatesWsParameters.PARAM_ID)); - response.noContent(); + long qualityGateId = request.mandatoryParamAsLong(QualityGatesWsParameters.PARAM_ID); + try (DbSession dbSession = dbClient.openSession(false)) { + + QualityGateDto qualityGate = finder.getById(dbSession, qualityGateId); + wsSupport.checkCanEdit(qualityGate); + + dbClient.qualityGateDao().delete(qualityGate, dbSession); + + dbSession.commit(); + response.noContent(); + } } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/qualitygate/ws/DestroyActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/qualitygate/ws/DestroyActionTest.java index 8500f667081..d4e01ff13f1 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/qualitygate/ws/DestroyActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/qualitygate/ws/DestroyActionTest.java @@ -19,7 +19,6 @@ */ package org.sonar.server.qualitygate.ws; -import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -28,17 +27,21 @@ import org.sonar.api.utils.System2; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.DbTester; -import org.sonar.db.property.PropertyDto; 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.QualityGates; +import org.sonar.server.qualitygate.QualityGateFinder; import org.sonar.server.tester.UserSessionRule; import org.sonar.server.ws.WsActionTester; +import static java.lang.String.format; import static java.lang.String.valueOf; +import static org.apache.commons.lang.StringUtils.EMPTY; import static org.assertj.core.api.Assertions.assertThat; import static org.sonar.db.permission.OrganizationPermission.ADMINISTER_QUALITY_GATES; -import static org.sonarqube.ws.client.qualitygate.QualityGatesWsParameters.PARAM_ID; +import static org.sonar.db.permission.OrganizationPermission.ADMINISTER_QUALITY_PROFILES; +import static org.sonar.server.qualitygate.ws.QualityGatesWsParameters.PARAM_ID; public class DestroyActionTest { @@ -50,18 +53,13 @@ public class DestroyActionTest { public DbTester db = DbTester.create(System2.INSTANCE); private DbClient dbClient = db.getDbClient(); - private DbSession dbSession = db.getSession(); private TestDefaultOrganizationProvider organizationProvider = TestDefaultOrganizationProvider.from(db); - private QualityGates qualityGates = new QualityGates(dbClient, userSession, organizationProvider); - private WsActionTester ws; - private DestroyAction underTest; - - @Before - public void setUp() { - underTest = new DestroyAction(qualityGates); - ws = new WsActionTester(underTest); + private QualityGateFinder qualityGateFinder = new QualityGateFinder(dbClient); + private QualityGatesWsSupport wsSupport = new QualityGatesWsSupport(db.getDbClient(), userSession, organizationProvider); - } + private DbSession dbSession = db.getSession(); + private DestroyAction underTest = new DestroyAction(dbClient, wsSupport, qualityGateFinder); + private WsActionTester ws = new WsActionTester(underTest); @Test public void definition() { @@ -76,7 +74,7 @@ public class DestroyActionTest { } @Test - public void should_delete_quality_gate() { + public void delete_quality_gate() { userSession.addPermission(ADMINISTER_QUALITY_GATES, db.getDefaultOrganization()); QualityGateDto qualityGate = db.qualityGates().insertQualityGate(); Long qualityGateId = qualityGate.getId(); @@ -90,12 +88,36 @@ public class DestroyActionTest { } @Test - public void should_delete_quality_gate_even_if_default() { + public void fail_when_invalid_id() { + userSession.addPermission(ADMINISTER_QUALITY_GATES, db.getDefaultOrganization()); + + String invalidId = "invalid-id"; + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage(format("The 'id' parameter cannot be parsed as a long value: %s", invalidId)); + + ws.newRequest() + .setParam(PARAM_ID, valueOf(invalidId)) + .execute(); + } + + @Test + public void should_fail_when_missing_id() { + userSession.addPermission(ADMINISTER_QUALITY_GATES, db.getDefaultOrganization()); + + expectedException.expect(IllegalArgumentException.class); + + ws.newRequest() + .setParam(PARAM_ID, valueOf(EMPTY)) + .execute(); + } + + @Test + public void delete_quality_gate_even_if_default() { userSession.addPermission(ADMINISTER_QUALITY_GATES, db.getDefaultOrganization()); QualityGateDto qualityGate = db.qualityGates().insertQualityGate(qg -> qg.setName("To Delete")); + db.qualityGates().setDefaultQualityGate(qualityGate); Long qualityGateId = qualityGate.getId(); - db.properties().insertProperty(new PropertyDto().setKey("sonar.qualitygate").setValue(Long.toString(qualityGateId))); - assertThat(db.getDbClient().qualityGateDao().selectById(dbSession, qualityGateId)).isNotNull(); ws.newRequest() .setParam(PARAM_ID, valueOf(qualityGateId)) @@ -105,23 +127,59 @@ public class DestroyActionTest { } @Test - public void should_delete_quality_gate_if_non_default_when_a_default_exist() { + public void delete_quality_gate_if_non_default_when_a_default_exist() { userSession.addPermission(ADMINISTER_QUALITY_GATES, db.getDefaultOrganization()); QualityGateDto qualityGate = db.qualityGates().insertQualityGate(qg -> qg.setName("To Delete")); Long toDeleteQualityGateId = qualityGate.getId(); assertThat(db.getDbClient().qualityGateDao().selectById(dbSession, toDeleteQualityGateId)).isNotNull(); - QualityGateDto defaultqualityGate = db.qualityGates().insertQualityGate(qg -> qg.setName("Default")); - Long defaultQualityGateId = defaultqualityGate.getId(); - db.properties().insertProperty(new PropertyDto().setKey("sonar.qualitygate").setValue(Long.toString(defaultQualityGateId))); - assertThat(db.getDbClient().qualityGateDao().selectById(dbSession, defaultQualityGateId)).isNotNull(); + QualityGateDto defaultQualityGate = db.qualityGates().insertQualityGate(qg -> qg.setName("Default")); + db.qualityGates().setDefaultQualityGate(defaultQualityGate); ws.newRequest() .setParam(PARAM_ID, valueOf(toDeleteQualityGateId)) .execute(); assertThat(db.getDbClient().qualityGateDao().selectById(dbSession, toDeleteQualityGateId)).isNull(); - assertThat(db.getDbClient().qualityGateDao().selectById(dbSession, defaultQualityGateId)).isNotNull(); + } + + @Test + public void does_not_delete_built_in_quality_gate() { + userSession.addPermission(ADMINISTER_QUALITY_GATES, db.getDefaultOrganization()); + QualityGateDto qualityGate = db.qualityGates().insertQualityGate(qg -> qg.setBuiltIn(true)); + Long qualityGateId = qualityGate.getId(); + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage(format("Operation forbidden for built-in Quality Gate '%s'", qualityGate.getName())); + + ws.newRequest() + .setParam(PARAM_ID, valueOf(qualityGateId)) + .execute(); + + assertThat(db.getDbClient().qualityGateDao().selectById(dbSession, qualityGateId)).isNotNull(); + } + + @Test + public void fail_on_unknown_quality_gate() { + userSession.addPermission(ADMINISTER_QUALITY_GATES, db.getDefaultOrganization()); + + expectedException.expect(NotFoundException.class); + + ws.newRequest() + .setParam(PARAM_ID, "123") + .execute(); + } + + @Test + public void fail_when_not_quality_gates_administer() { + userSession.logIn("john").addPermission(ADMINISTER_QUALITY_PROFILES, db.getDefaultOrganization()); + QualityGateDto qualityGate = db.qualityGates().insertQualityGate(qg -> qg.setName("old name")); + + expectedException.expect(ForbiddenException.class); + + ws.newRequest() + .setParam(PARAM_ID, qualityGate.getId().toString()) + .execute(); } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/qualitygate/ws/QualityGatesWsTest.java b/server/sonar-server/src/test/java/org/sonar/server/qualitygate/ws/QualityGatesWsTest.java index 16e57207294..6c9bd4b541b 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/qualitygate/ws/QualityGatesWsTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/qualitygate/ws/QualityGatesWsTest.java @@ -35,7 +35,6 @@ import org.sonar.db.qualitygate.ProjectQgateAssociation; import org.sonar.db.qualitygate.ProjectQgateAssociationQuery; import org.sonar.db.qualitygate.QualityGateDto; import org.sonar.server.component.ComponentFinder; -import org.sonar.server.exceptions.BadRequestException; import org.sonar.server.qualitygate.QgateProjectFinder; import org.sonar.server.qualitygate.QgateProjectFinder.Association; import org.sonar.server.qualitygate.QualityGates; @@ -70,7 +69,6 @@ public class QualityGatesWsTest { new SearchAction(projectFinder), new CreateAction(null, null, null, null), new CopyAction(qGates), - new DestroyAction(qGates), new SetAsDefaultAction(qGates), selectAction, new DeselectAction(qGates, mock(DbClient.class), mock(ComponentFinder.class)))); @@ -82,7 +80,7 @@ public class QualityGatesWsTest { assertThat(controller).isNotNull(); assertThat(controller.path()).isEqualTo("api/qualitygates"); assertThat(controller.description()).isNotEmpty(); - assertThat(controller.actions()).hasSize(8); + assertThat(controller.actions()).hasSize(7); Action copy = controller.action("copy"); assertThat(copy).isNotNull(); @@ -93,14 +91,6 @@ public class QualityGatesWsTest { assertThat(copy.param("name")).isNotNull(); assertThat(copy.isInternal()).isFalse(); - Action destroy = controller.action("destroy"); - assertThat(destroy).isNotNull(); - assertThat(destroy.handler()).isNotNull(); - assertThat(destroy.since()).isEqualTo("4.3"); - assertThat(destroy.isPost()).isTrue(); - assertThat(destroy.param("id")).isNotNull(); - assertThat(destroy.isInternal()).isFalse(); - Action setDefault = controller.action("set_as_default"); assertThat(setDefault).isNotNull(); assertThat(setDefault.handler()).isNotNull(); @@ -132,23 +122,6 @@ public class QualityGatesWsTest { .assertJson("{\"id\":42,\"name\":\"Copied QG\"}"); } - @Test - public void destroy_nominal() throws Exception { - Long id = 42L; - tester.newPostRequest("api/qualitygates", "destroy").setParam("id", id.toString()).execute() - .assertNoContent(); - } - - @Test(expected = IllegalArgumentException.class) - public void destroy_without_id() throws Exception { - tester.newPostRequest("api/qualitygates", "destroy").execute(); - } - - @Test(expected = BadRequestException.class) - public void destroy_with_invalid_id() throws Exception { - tester.newPostRequest("api/qualitygates", "destroy").setParam("id", "polop").execute(); - } - @Test public void search_with_query() throws Exception { long gateId = 12345L; diff --git a/tests/src/test/java/org/sonarqube/tests/qualityGate/QualityGateTest.java b/tests/src/test/java/org/sonarqube/tests/qualityGate/QualityGateTest.java index 76f3f677092..718ef39a601 100644 --- a/tests/src/test/java/org/sonarqube/tests/qualityGate/QualityGateTest.java +++ b/tests/src/test/java/org/sonarqube/tests/qualityGate/QualityGateTest.java @@ -50,6 +50,7 @@ import org.sonarqube.ws.MediaTypes; import org.sonarqube.ws.Organizations.Organization; import org.sonarqube.ws.Projects.CreateWsResponse.Project; import org.sonarqube.ws.Qualitygates; +import org.sonarqube.ws.Qualitygates.ProjectStatusResponse; import org.sonarqube.ws.Users; import org.sonarqube.ws.client.GetRequest; import org.sonarqube.ws.client.PostRequest; @@ -65,6 +66,7 @@ import org.sonarqube.ws.client.qualitygates.UpdateConditionRequest; import static org.apache.commons.lang.RandomStringUtils.randomAlphabetic; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.groups.Tuple.tuple; +import static org.sonarqube.ws.Qualitygates.ProjectStatusResponse.Status.ERROR; import static util.ItUtils.concat; import static util.ItUtils.extractCeTaskId; import static util.ItUtils.getMeasure; @@ -210,11 +212,11 @@ public class QualityGateTest { String taskId = getTaskIdInLocalReport(projectDir("qualitygate/xoo-sample")); String analysisId = getAnalysisId(taskId); - ProjectStatusWsResponse projectStatusWsResponse = tester.wsClient().qualityGates().projectStatus(new ProjectStatusRequest().setAnalysisId(analysisId)); - ProjectStatusWsResponse.ProjectStatus projectStatus = projectStatusWsResponse.getProjectStatus(); - assertThat(projectStatus.getStatus()).isEqualTo(ProjectStatusWsResponse.Status.ERROR); + ProjectStatusResponse projectStatusWsResponse = tester.wsClient().qualityGates().projectStatus(new ProjectStatusRequest().setAnalysisId(analysisId)); + ProjectStatusResponse.ProjectStatus projectStatus = projectStatusWsResponse.getProjectStatus(); + assertThat(projectStatus.getStatus()).isEqualTo(ERROR); assertThat(projectStatus.getConditionsCount()).isEqualTo(1); - ProjectStatusWsResponse.Condition condition = projectStatus.getConditionsList().get(0); + ProjectStatusResponse.Condition condition = projectStatus.getConditionsList().get(0); assertThat(condition.getMetricKey()).isEqualTo("ncloc"); assertThat(condition.getErrorThreshold()).isEqualTo("7"); } -- 2.39.5