From 53642efcbee232d2cd110f04e3ad91958f118d8f Mon Sep 17 00:00:00 2001 From: Michal Duda Date: Mon, 7 Jun 2021 12:19:21 +0200 Subject: [PATCH] SONAR-14871 GitHub project PR decoration settings validation --- .../GithubGlobalSettingsValidator.java | 60 ++++++++++++ .../ws/AlmIntegrationsWSModule.java | 2 + .../server/almsettings/ws/ValidateAction.java | 33 +------ .../GithubGlobalSettingsValidatorTest.java | 92 +++++++++++++++++++ .../almsettings/ws/ValidateActionTest.java | 75 ++++----------- 5 files changed, 179 insertions(+), 83 deletions(-) create mode 100644 server/sonar-webserver-webapi/src/main/java/org/sonar/server/almintegration/validator/GithubGlobalSettingsValidator.java create mode 100644 server/sonar-webserver-webapi/src/test/java/org/sonar/server/almintegration/validator/GithubGlobalSettingsValidatorTest.java diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/almintegration/validator/GithubGlobalSettingsValidator.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/almintegration/validator/GithubGlobalSettingsValidator.java new file mode 100644 index 00000000000..6dd4c540d27 --- /dev/null +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/almintegration/validator/GithubGlobalSettingsValidator.java @@ -0,0 +1,60 @@ +/* + * SonarQube + * Copyright (C) 2009-2021 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.server.almintegration.validator; + +import java.util.Optional; +import org.sonar.alm.client.github.GithubApplicationClient; +import org.sonar.alm.client.github.GithubApplicationClientImpl; +import org.sonar.alm.client.github.config.GithubAppConfiguration; +import org.sonar.api.server.ServerSide; +import org.sonar.db.alm.setting.AlmSettingDto; + +import static org.apache.commons.lang.StringUtils.isBlank; + +@ServerSide +public class GithubGlobalSettingsValidator { + + private final GithubApplicationClient githubApplicationClient; + + public GithubGlobalSettingsValidator(GithubApplicationClientImpl githubApplicationClient) { + this.githubApplicationClient = githubApplicationClient; + } + + public GithubAppConfiguration validate(AlmSettingDto settings) { + long appId; + try { + appId = Long.parseLong(Optional.ofNullable(settings.getAppId()).orElseThrow(() -> new IllegalArgumentException("Missing appId"))); + } catch (NumberFormatException e) { + throw new IllegalArgumentException("Invalid appId; " + e.getMessage()); + } + if (isBlank(settings.getClientId())) { + throw new IllegalArgumentException("Missing Client Id"); + } + if (isBlank(settings.getClientSecret())) { + throw new IllegalArgumentException("Missing Client Secret"); + } + GithubAppConfiguration configuration = new GithubAppConfiguration(appId, settings.getPrivateKey(), settings.getUrl()); + + githubApplicationClient.checkApiEndpoint(configuration); + githubApplicationClient.checkAppPermissions(configuration); + + return configuration; + } +} diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/almintegration/ws/AlmIntegrationsWSModule.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/almintegration/ws/AlmIntegrationsWSModule.java index 528b78e98bd..8bc96cd0439 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/almintegration/ws/AlmIntegrationsWSModule.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/almintegration/ws/AlmIntegrationsWSModule.java @@ -20,6 +20,7 @@ package org.sonar.server.almintegration.ws; import org.sonar.core.platform.Module; +import org.sonar.server.almintegration.validator.GithubGlobalSettingsValidator; import org.sonar.server.almintegration.ws.azure.ImportAzureProjectAction; import org.sonar.server.almintegration.ws.azure.ListAzureProjectsAction; import org.sonar.server.almintegration.ws.azure.SearchAzureReposAction; @@ -47,6 +48,7 @@ public class AlmIntegrationsWSModule extends Module { SearchBitbucketServerReposAction.class, SearchBitbucketCloudReposAction.class, GetGithubClientIdAction.class, + GithubGlobalSettingsValidator.class, ImportGithubProjectAction.class, ListGithubOrganizationsAction.class, ListGithubRepositoriesAction.class, diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/almsettings/ws/ValidateAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/almsettings/ws/ValidateAction.java index 813554e28ca..1fb38f73cee 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/almsettings/ws/ValidateAction.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/almsettings/ws/ValidateAction.java @@ -22,9 +22,6 @@ package org.sonar.server.almsettings.ws; import org.sonar.alm.client.azure.AzureDevOpsHttpClient; import org.sonar.alm.client.bitbucket.bitbucketcloud.BitbucketCloudRestClient; import org.sonar.alm.client.bitbucketserver.BitbucketServerRestClient; -import org.sonar.alm.client.github.GithubApplicationClient; -import org.sonar.alm.client.github.GithubApplicationClientImpl; -import org.sonar.alm.client.github.config.GithubAppConfiguration; import org.sonar.alm.client.gitlab.GitlabHttpClient; import org.sonar.api.server.ws.Request; import org.sonar.api.server.ws.Response; @@ -32,10 +29,9 @@ import org.sonar.api.server.ws.WebService; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.alm.setting.AlmSettingDto; +import org.sonar.server.almintegration.validator.GithubGlobalSettingsValidator; import org.sonar.server.user.UserSession; -import static org.apache.commons.lang.StringUtils.isBlank; - public class ValidateAction implements AlmSettingsWsAction { private static final String PARAM_KEY = "key"; @@ -45,19 +41,19 @@ public class ValidateAction implements AlmSettingsWsAction { private final AlmSettingsSupport almSettingsSupport; private final AzureDevOpsHttpClient azureDevOpsHttpClient; private final GitlabHttpClient gitlabHttpClient; - private final GithubApplicationClient githubApplicationClient; + private final GithubGlobalSettingsValidator githubGlobalSettingsValidator; private final BitbucketServerRestClient bitbucketServerRestClient; private final BitbucketCloudRestClient bitbucketCloudRestClient; public ValidateAction(DbClient dbClient, UserSession userSession, AlmSettingsSupport almSettingsSupport, AzureDevOpsHttpClient azureDevOpsHttpClient, - GithubApplicationClientImpl githubApplicationClient, GitlabHttpClient gitlabHttpClient, + GithubGlobalSettingsValidator githubGlobalSettingsValidator, GitlabHttpClient gitlabHttpClient, BitbucketServerRestClient bitbucketServerRestClient, BitbucketCloudRestClient bitbucketCloudRestClient) { this.dbClient = dbClient; this.userSession = userSession; this.almSettingsSupport = almSettingsSupport; this.azureDevOpsHttpClient = azureDevOpsHttpClient; - this.githubApplicationClient = githubApplicationClient; + this.githubGlobalSettingsValidator = githubGlobalSettingsValidator; this.gitlabHttpClient = gitlabHttpClient; this.bitbucketServerRestClient = bitbucketServerRestClient; this.bitbucketCloudRestClient = bitbucketCloudRestClient; @@ -94,7 +90,7 @@ public class ValidateAction implements AlmSettingsWsAction { validateGitlab(almSettingDto); break; case GITHUB: - validateGitHub(almSettingDto); + githubGlobalSettingsValidator.validate(almSettingDto); break; case BITBUCKET: validateBitbucketServer(almSettingDto); @@ -124,25 +120,6 @@ public class ValidateAction implements AlmSettingsWsAction { gitlabHttpClient.checkWritePermission(almSettingDto.getUrl(), almSettingDto.getPersonalAccessToken()); } - private void validateGitHub(AlmSettingDto settings) { - long appId; - try { - appId = Long.parseLong(settings.getAppId()); - } catch (NumberFormatException e) { - throw new IllegalArgumentException("Invalid appId; " + e.getMessage()); - } - if (isBlank(settings.getClientId())) { - throw new IllegalArgumentException("Missing Client Id"); - } - if (isBlank(settings.getClientSecret())) { - throw new IllegalArgumentException("Missing Client Secret"); - } - GithubAppConfiguration configuration = new GithubAppConfiguration(appId, settings.getPrivateKey(), settings.getUrl()); - - githubApplicationClient.checkApiEndpoint(configuration); - githubApplicationClient.checkAppPermissions(configuration); - } - private void validateBitbucketServer(AlmSettingDto almSettingDto) { bitbucketServerRestClient.validateUrl(almSettingDto.getUrl()); bitbucketServerRestClient.validateToken(almSettingDto.getUrl(), almSettingDto.getPersonalAccessToken()); diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/almintegration/validator/GithubGlobalSettingsValidatorTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/almintegration/validator/GithubGlobalSettingsValidatorTest.java new file mode 100644 index 00000000000..efc2679fe28 --- /dev/null +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/almintegration/validator/GithubGlobalSettingsValidatorTest.java @@ -0,0 +1,92 @@ +/* + * SonarQube + * Copyright (C) 2009-2021 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.server.almintegration.validator; + +import org.junit.Test; +import org.mockito.ArgumentCaptor; +import org.sonar.alm.client.github.GithubApplicationClientImpl; +import org.sonar.alm.client.github.config.GithubAppConfiguration; +import org.sonar.db.alm.setting.AlmSettingDto; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.sonar.db.almsettings.AlmSettingsTesting.newGithubAlmSettingDto; + +public class GithubGlobalSettingsValidatorTest { + private final GithubApplicationClientImpl appClient = mock(GithubApplicationClientImpl.class); + private final GithubGlobalSettingsValidator underTest = new GithubGlobalSettingsValidator(appClient); + + @Test + public void github_global_settings_validation() { + AlmSettingDto almSettingDto = newGithubAlmSettingDto() + .setClientId("clientId") + .setClientSecret("clientSecret"); + + GithubAppConfiguration configuration = underTest.validate(almSettingDto); + + ArgumentCaptor configurationArgumentCaptor = ArgumentCaptor.forClass(GithubAppConfiguration.class); + verify(appClient).checkApiEndpoint(configurationArgumentCaptor.capture()); + verify(appClient).checkAppPermissions(configurationArgumentCaptor.capture()); + assertThat(configuration.getId()).isEqualTo(configurationArgumentCaptor.getAllValues().get(0).getId()); + assertThat(configuration.getId()).isEqualTo(configurationArgumentCaptor.getAllValues().get(1).getId()); + } + + @Test + public void github_validation_checks_invalid_appId() { + AlmSettingDto almSettingDto = newGithubAlmSettingDto() + .setAppId("abc") + .setClientId("clientId") + .setClientSecret("clientSecret"); + + assertThatThrownBy(() -> underTest.validate(almSettingDto)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Invalid appId; For input string: \"abc\""); + } + + @Test + public void github_validation_checks_missing_appId() { + AlmSettingDto almSettingDto = newGithubAlmSettingDto().setAppId(null); + + assertThatThrownBy(() -> underTest.validate(almSettingDto)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Missing appId"); + } + + @Test + public void github_validation_checks_missing_clientId() { + AlmSettingDto almSettingDto = newGithubAlmSettingDto().setClientId(null); + + assertThatThrownBy(() -> underTest.validate(almSettingDto)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Missing Client Id"); + } + + @Test + public void github_validation_checks_missing_clientSecret() { + AlmSettingDto almSettingDto = newGithubAlmSettingDto().setClientSecret(null); + + assertThatThrownBy(() -> underTest.validate(almSettingDto)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Missing Client Secret"); + + } +} diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/almsettings/ws/ValidateActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/almsettings/ws/ValidateActionTest.java index a5b6f47d117..c576b29d772 100644 --- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/almsettings/ws/ValidateActionTest.java +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/almsettings/ws/ValidateActionTest.java @@ -21,18 +21,17 @@ package org.sonar.server.almsettings.ws; import org.junit.Rule; import org.junit.Test; -import org.junit.rules.ExpectedException; import org.mockito.ArgumentCaptor; import org.sonar.alm.client.azure.AzureDevOpsHttpClient; import org.sonar.alm.client.bitbucket.bitbucketcloud.BitbucketCloudRestClient; import org.sonar.alm.client.bitbucketserver.BitbucketServerRestClient; -import org.sonar.alm.client.github.GithubApplicationClientImpl; -import org.sonar.alm.client.github.config.GithubAppConfiguration; import org.sonar.alm.client.gitlab.GitlabHttpClient; +import org.sonar.api.resources.ResourceTypes; import org.sonar.api.server.ws.WebService; import org.sonar.db.DbTester; import org.sonar.db.alm.setting.AlmSettingDto; import org.sonar.db.user.UserDto; +import org.sonar.server.almintegration.validator.GithubGlobalSettingsValidator; import org.sonar.server.almsettings.MultipleAlmFeatureProvider; import org.sonar.server.component.ComponentFinder; import org.sonar.server.exceptions.ForbiddenException; @@ -50,48 +49,43 @@ import static org.mockito.Mockito.verify; public class ValidateActionTest { - @Rule - public ExpectedException expectedException = ExpectedException.none(); @Rule public UserSessionRule userSession = UserSessionRule.standalone(); @Rule public DbTester db = DbTester.create(); private final MultipleAlmFeatureProvider multipleAlmFeatureProvider = mock(MultipleAlmFeatureProvider.class); - private final ComponentFinder componentFinder = new ComponentFinder(db.getDbClient(), null); + private final ComponentFinder componentFinder = new ComponentFinder(db.getDbClient(), mock(ResourceTypes.class)); private final AlmSettingsSupport almSettingsSupport = new AlmSettingsSupport(db.getDbClient(), userSession, componentFinder, multipleAlmFeatureProvider); private final AzureDevOpsHttpClient azureDevOpsHttpClient = mock(AzureDevOpsHttpClient.class); private final GitlabHttpClient gitlabHttpClient = mock(GitlabHttpClient.class); - private final GithubApplicationClientImpl githubApplicationClient = mock(GithubApplicationClientImpl.class); + private final GithubGlobalSettingsValidator githubGlobalSettingsValidator = mock(GithubGlobalSettingsValidator.class); private final BitbucketServerRestClient bitbucketServerRestClient = mock(BitbucketServerRestClient.class); private final BitbucketCloudRestClient bitbucketCloudRestClient = mock(BitbucketCloudRestClient.class); private final WsActionTester ws = new WsActionTester( - new ValidateAction(db.getDbClient(), userSession, almSettingsSupport, azureDevOpsHttpClient, githubApplicationClient, gitlabHttpClient, + new ValidateAction(db.getDbClient(), userSession, almSettingsSupport, azureDevOpsHttpClient, githubGlobalSettingsValidator, gitlabHttpClient, bitbucketServerRestClient, bitbucketCloudRestClient)); @Test public void fail_when_key_does_not_match_existing_alm_setting() { UserDto user = db.users().insertUser(); userSession.logIn(user).setSystemAdministrator(); + TestRequest request = ws.newRequest() + .setParam("key", "unknown"); - expectedException.expect(NotFoundException.class); - expectedException.expectMessage("ALM setting with key 'unknown' cannot be found"); - - ws.newRequest() - .setParam("key", "unknown") - .execute(); + assertThatThrownBy(request::execute) + .isInstanceOf(NotFoundException.class) + .hasMessage("ALM setting with key 'unknown' cannot be found"); } @Test public void fail_when_missing_administer_system_permission() { UserDto user = db.users().insertUser(); userSession.logIn(user); + TestRequest request = ws.newRequest() + .setParam("key", "any key"); - expectedException.expect(ForbiddenException.class); - - ws.newRequest() - .setParam("key", "any key") - .execute(); + assertThatThrownBy(request::execute).isInstanceOf(ForbiddenException.class); } @Test @@ -117,42 +111,13 @@ public class ValidateActionTest { .setParam("key", almSetting.getKey()) .execute(); - ArgumentCaptor configurationArgumentCaptor = ArgumentCaptor.forClass(GithubAppConfiguration.class); - verify(githubApplicationClient).checkApiEndpoint(configurationArgumentCaptor.capture()); - verify(githubApplicationClient).checkAppPermissions(configurationArgumentCaptor.capture()); - - assertThat(configurationArgumentCaptor.getAllValues()).hasSize(2) - .extracting(GithubAppConfiguration::getApiEndpoint) - .contains(almSetting.getUrl(), almSetting.getUrl()); - } - - @Test - public void github_validation_checks_invalid_appId() { - AlmSettingDto almSetting = insertAlmSetting(db.almSettings().insertGitHubAlmSetting(settings -> settings.setAppId("abc") - .setClientId("clientId").setClientSecret("clientSecret"))); - - assertThatThrownBy(() -> ws.newRequest() - .setParam("key", almSetting.getKey()) - .execute()).isInstanceOf(IllegalArgumentException.class).hasMessage("Invalid appId; For input string: \"abc\""); - } - - @Test - public void github_validation_checks_missing_clientId() { - AlmSettingDto almSetting = insertAlmSetting(db.almSettings().insertGitHubAlmSetting(s -> s.setClientId(null))); - - assertThatThrownBy(() -> ws.newRequest() - .setParam("key", almSetting.getKey()) - .execute()).isInstanceOf(IllegalArgumentException.class).hasMessage("Missing Client Id"); - } - - @Test - public void github_validation_checks_missing_clientSecret() { - AlmSettingDto almSetting = insertAlmSetting(db.almSettings().insertGitHubAlmSetting(s -> s.setClientSecret(null))); - - assertThatThrownBy(() -> ws.newRequest() - .setParam("key", almSetting.getKey()) - .execute()).isInstanceOf(IllegalArgumentException.class).hasMessage("Missing Client Secret"); - + ArgumentCaptor almSettingDtoArgumentCaptor = ArgumentCaptor.forClass(AlmSettingDto.class); + verify(githubGlobalSettingsValidator).validate(almSettingDtoArgumentCaptor.capture()); + assertThat(almSettingDtoArgumentCaptor.getAllValues()).hasSize(1); + assertThat(almSettingDtoArgumentCaptor.getValue().getClientId()).isEqualTo(almSetting.getClientId()); + assertThat(almSettingDtoArgumentCaptor.getValue().getClientSecret()).isEqualTo(almSetting.getClientSecret()); + assertThat(almSettingDtoArgumentCaptor.getValue().getAlm()).isEqualTo(almSetting.getAlm()); + assertThat(almSettingDtoArgumentCaptor.getValue().getAppId()).isEqualTo(almSetting.getAppId()); } @Test -- 2.39.5