From 3a99ed1448f43fd342c8bf8aae2b525ae2e39a8d Mon Sep 17 00:00:00 2001 From: Jacek Date: Fri, 14 Aug 2020 12:39:17 +0200 Subject: [PATCH] SONAR-13665 Fix api/webhooks/deliveries to return appropriate 'componentKey' in response --- .../webhook/ws/WebhookDeliveriesAction.java | 53 ++++++++----- .../ws/WebhookDeliveriesActionTest.java | 79 +++++++++++-------- 2 files changed, 77 insertions(+), 55 deletions(-) diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/webhook/ws/WebhookDeliveriesAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/webhook/ws/WebhookDeliveriesAction.java index 7567211391c..af45ff0cd5e 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/webhook/ws/WebhookDeliveriesAction.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/webhook/ws/WebhookDeliveriesAction.java @@ -19,8 +19,13 @@ */ package org.sonar.server.webhook.ws; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; import java.util.List; -import java.util.Optional; +import java.util.Map; +import java.util.function.Function; +import java.util.stream.Collectors; import javax.annotation.Nullable; import org.sonar.api.server.ws.Request; import org.sonar.api.server.ws.Response; @@ -32,12 +37,12 @@ import org.sonar.db.DbSession; import org.sonar.db.project.ProjectDto; import org.sonar.db.webhook.WebhookDeliveryLiteDto; import org.sonar.server.component.ComponentFinder; +import org.sonar.server.exceptions.ForbiddenException; import org.sonar.server.user.UserSession; import org.sonarqube.ws.Common; import org.sonarqube.ws.Webhooks; import static com.google.common.base.Preconditions.checkArgument; -import static java.util.Objects.requireNonNull; import static org.apache.commons.lang.StringUtils.isNotBlank; import static org.sonar.api.server.ws.WebService.Param.PAGE; import static org.sonar.api.server.ws.WebService.Param.PAGE_SIZE; @@ -110,60 +115,67 @@ public class WebhookDeliveriesAction implements WebhooksWsAction { } private Data loadFromDatabase(@Nullable String webhookUuid, @Nullable String ceTaskId, @Nullable String projectKey, int page, int pageSize) { - ProjectDto project; + Map projectUuidMap; List deliveries; int totalElements; try (DbSession dbSession = dbClient.openSession(false)) { if (isNotBlank(webhookUuid)) { totalElements = dbClient.webhookDeliveryDao().countDeliveriesByWebhookUuid(dbSession, webhookUuid); deliveries = dbClient.webhookDeliveryDao().selectByWebhookUuid(dbSession, webhookUuid, offset(page, pageSize), pageSize); - project = getProjectDto(dbSession, deliveries); + projectUuidMap = getProjectsDto(dbSession, deliveries); } else if (projectKey != null) { - project = componentFinder.getProjectByKey(dbSession, projectKey); + ProjectDto project = componentFinder.getProjectByKey(dbSession, projectKey); + projectUuidMap = new HashMap<>(); + projectUuidMap.put(project.getUuid(), project); totalElements = dbClient.webhookDeliveryDao().countDeliveriesByComponentUuid(dbSession, project.getUuid()); deliveries = dbClient.webhookDeliveryDao().selectOrderedByComponentUuid(dbSession, project.getUuid(), offset(page, pageSize), pageSize); } else { totalElements = dbClient.webhookDeliveryDao().countDeliveriesByCeTaskUuid(dbSession, ceTaskId); deliveries = dbClient.webhookDeliveryDao().selectOrderedByCeTaskUuid(dbSession, ceTaskId, offset(page, pageSize), pageSize); - project = getProjectDto(dbSession, deliveries); + projectUuidMap = getProjectsDto(dbSession, deliveries); } } - return new Data(project, deliveries).withPagingInfo(page, pageSize, totalElements); + return new Data(projectUuidMap, deliveries).withPagingInfo(page, pageSize, totalElements); } - private ProjectDto getProjectDto(DbSession dbSession, List deliveries) { - Optional deliveredComponentUuid = deliveries + private Map getProjectsDto(DbSession dbSession, List deliveries) { + Map deliveredComponentUuid = deliveries .stream() - .map(WebhookDeliveryLiteDto::getComponentUuid) - .findFirst(); + .collect(Collectors.toMap(WebhookDeliveryLiteDto::getUuid, WebhookDeliveryLiteDto::getComponentUuid)); - if (deliveredComponentUuid.isPresent()) { - return componentFinder.getProjectByUuid(dbSession, deliveredComponentUuid.get()); + if (!deliveredComponentUuid.isEmpty()) { + return dbClient.projectDao().selectByUuids(dbSession, new HashSet<>(deliveredComponentUuid.values())) + .stream() + .collect(Collectors.toMap(ProjectDto::getUuid, Function.identity())); } else { - return null; + return Collections.emptyMap(); } } private static class Data { - private final ProjectDto project; + private final Map projectUuidMap; private final List deliveryDtos; private int pageIndex; private int pageSize; private int totalElements; - Data(@Nullable ProjectDto project, List deliveries) { + Data(Map projectUuidMap, List deliveries) { this.deliveryDtos = deliveries; if (deliveries.isEmpty()) { - this.project = null; + this.projectUuidMap = projectUuidMap; } else { - this.project = requireNonNull(project); + checkArgument(!projectUuidMap.isEmpty()); + this.projectUuidMap = projectUuidMap; } } void ensureAdminPermission(UserSession userSession) { - if (project != null) { - userSession.checkProjectPermission(UserRole.ADMIN, project); + if (!projectUuidMap.isEmpty()) { + List projectsUserHasAccessTo = userSession.keepAuthorizedProjects(UserRole.ADMIN, projectUuidMap.values()); + if (projectsUserHasAccessTo.size() != projectUuidMap.size()) { + throw new ForbiddenException("Insufficient privileges"); + } } } @@ -171,6 +183,7 @@ public class WebhookDeliveriesAction implements WebhooksWsAction { Webhooks.DeliveriesWsResponse.Builder responseBuilder = Webhooks.DeliveriesWsResponse.newBuilder(); Webhooks.Delivery.Builder deliveryBuilder = Webhooks.Delivery.newBuilder(); for (WebhookDeliveryLiteDto dto : deliveryDtos) { + ProjectDto project = projectUuidMap.get(dto.getComponentUuid()); copyDtoToProtobuf(project, dto, deliveryBuilder); responseBuilder.addDeliveries(deliveryBuilder); } diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/webhook/ws/WebhookDeliveriesActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/webhook/ws/WebhookDeliveriesActionTest.java index 00346d79ebc..a67677fdb0b 100644 --- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/webhook/ws/WebhookDeliveriesActionTest.java +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/webhook/ws/WebhookDeliveriesActionTest.java @@ -22,7 +22,6 @@ package org.sonar.server.webhook.ws; import org.junit.Before; import org.junit.Rule; import org.junit.Test; -import org.junit.rules.ExpectedException; import org.sonar.api.server.ws.WebService; import org.sonar.api.utils.System2; import org.sonar.api.web.UserRole; @@ -36,19 +35,18 @@ import org.sonar.server.component.TestComponentFinder; import org.sonar.server.exceptions.ForbiddenException; import org.sonar.server.exceptions.UnauthorizedException; import org.sonar.server.tester.UserSessionRule; +import org.sonar.server.ws.TestRequest; import org.sonar.server.ws.WsActionTester; import org.sonarqube.ws.Webhooks; import static org.assertj.core.api.Assertions.assertThat; -import static org.sonar.db.component.ComponentTesting.newPrivateProjectDto; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.tuple; import static org.sonar.db.webhook.WebhookDeliveryTesting.newDto; import static org.sonar.test.JsonAssert.assertJson; public class WebhookDeliveriesActionTest { - @Rule - public ExpectedException expectedException = ExpectedException.none(); - @Rule public UserSessionRule userSession = UserSessionRule.standalone(); @@ -60,6 +58,7 @@ public class WebhookDeliveriesActionTest { private WsActionTester ws; private ComponentDto project; + private ComponentDto otherProject; @Before public void setUp() { @@ -67,6 +66,7 @@ public class WebhookDeliveriesActionTest { WebhookDeliveriesAction underTest = new WebhookDeliveriesAction(dbClient, userSession, componentFinder); ws = new WsActionTester(underTest); project = db.components().insertPrivateProject(c -> c.setDbKey("my-project")); + otherProject = db.components().insertPrivateProject(c -> c.setDbKey("other-project")); } @Test @@ -79,9 +79,9 @@ public class WebhookDeliveriesActionTest { @Test public void throw_UnauthorizedException_if_anonymous() { - expectedException.expect(UnauthorizedException.class); - - ws.newRequest().execute(); + TestRequest request = ws.newRequest(); + assertThatThrownBy(request::execute) + .isInstanceOf(UnauthorizedException.class); } @Test @@ -164,17 +164,30 @@ public class WebhookDeliveriesActionTest { WebhookDeliveryDto dto1 = newDto().setComponentUuid(project.uuid()).setCeTaskUuid("t1").setWebhookUuid("wh-1-uuid"); WebhookDeliveryDto dto2 = newDto().setComponentUuid(project.uuid()).setCeTaskUuid("t1").setWebhookUuid("wh-1-uuid"); WebhookDeliveryDto dto3 = newDto().setComponentUuid(project.uuid()).setCeTaskUuid("t2").setWebhookUuid("wh-2-uuid"); + + WebhookDeliveryDto dto4 = newDto().setComponentUuid(otherProject.uuid()).setCeTaskUuid("t4").setWebhookUuid("wh-1-uuid"); + WebhookDeliveryDto dto5 = newDto().setComponentUuid(otherProject.uuid()).setCeTaskUuid("t5").setWebhookUuid("wh-1-uuid"); + dbClient.webhookDeliveryDao().insert(db.getSession(), dto1); dbClient.webhookDeliveryDao().insert(db.getSession(), dto2); dbClient.webhookDeliveryDao().insert(db.getSession(), dto3); + dbClient.webhookDeliveryDao().insert(db.getSession(), dto4); + dbClient.webhookDeliveryDao().insert(db.getSession(), dto5); db.commit(); - userSession.logIn().addProjectPermission(UserRole.ADMIN, project); + userSession.logIn().addProjectPermission(UserRole.ADMIN, project, otherProject); Webhooks.DeliveriesWsResponse response = ws.newRequest() .setParam("webhook", "wh-1-uuid") .executeProtobuf(Webhooks.DeliveriesWsResponse.class); - assertThat(response.getDeliveriesCount()).isEqualTo(2); - assertThat(response.getDeliveriesList()).extracting(Webhooks.Delivery::getId).containsOnly(dto1.getUuid(), dto2.getUuid()); + assertThat(response.getDeliveriesCount()).isEqualTo(4); + assertThat(response.getDeliveriesList()).extracting(Webhooks.Delivery::getId) + .containsOnly(dto1.getUuid(), dto2.getUuid(), dto4.getUuid(), dto5.getUuid()); + assertThat(response.getDeliveriesList()).extracting(Webhooks.Delivery::getId, Webhooks.Delivery::getComponentKey) + .containsOnly( + tuple(dto1.getUuid(), project.getDbKey()), + tuple(dto2.getUuid(), project.getDbKey()), + tuple(dto4.getUuid(), otherProject.getDbKey()), + tuple(dto5.getUuid(), otherProject.getDbKey())); } @Test @@ -242,12 +255,11 @@ public class WebhookDeliveriesActionTest { db.commit(); userSession.logIn().addProjectPermission(UserRole.USER, project); - expectedException.expect(ForbiddenException.class); - expectedException.expectMessage("Insufficient privileges"); - - ws.newRequest() - .setParam("componentKey", project.getDbKey()) - .execute(); + TestRequest request = ws.newRequest() + .setParam("componentKey", project.getDbKey()); + assertThatThrownBy(request::execute) + .isInstanceOf(ForbiddenException.class) + .hasMessage("Insufficient privileges"); } @Test @@ -258,37 +270,34 @@ public class WebhookDeliveriesActionTest { db.commit(); userSession.logIn().addProjectPermission(UserRole.USER, project); - expectedException.expect(ForbiddenException.class); - expectedException.expectMessage("Insufficient privileges"); - - ws.newRequest() - .setParam("ceTaskId", dto.getCeTaskUuid()) - .execute(); + TestRequest request = ws.newRequest() + .setParam("ceTaskId", dto.getCeTaskUuid()); + assertThatThrownBy(request::execute) + .isInstanceOf(ForbiddenException.class) + .hasMessage("Insufficient privileges"); } @Test public void throw_IAE_if_both_component_and_task_parameters_are_set() { userSession.logIn(); - expectedException.expect(IllegalArgumentException.class); - expectedException.expectMessage("Either 'ceTaskId' or 'componentKey' or 'webhook' must be provided"); - - ws.newRequest() + TestRequest request = ws.newRequest() .setParam("componentKey", project.getDbKey()) - .setParam("ceTaskId", "t1") - .execute(); + .setParam("ceTaskId", "t1"); + assertThatThrownBy(request::execute) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Either 'ceTaskId' or 'componentKey' or 'webhook' must be provided"); } @Test public void throw_IAE_if_both_component_and_webhook_are_set() { userSession.logIn(); - expectedException.expect(IllegalArgumentException.class); - expectedException.expectMessage("Either 'ceTaskId' or 'componentKey' or 'webhook' must be provided"); - - ws.newRequest() + TestRequest request = ws.newRequest() .setParam("componentKey", project.getDbKey()) - .setParam("webhook", "wh-uuid") - .execute(); + .setParam("webhook", "wh-uuid"); + assertThatThrownBy(request::execute) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Either 'ceTaskId' or 'componentKey' or 'webhook' must be provided"); } } -- 2.39.5