From 32fbddcc7a8607e56d2a1e0ab4831cd640e1e444 Mon Sep 17 00:00:00 2001 From: Aurelien Poscia Date: Mon, 5 Sep 2022 14:25:16 +0200 Subject: [PATCH] SONAR-17266 Implement authentification for GitHub webhook requests --- .../sonar/db/alm/setting/AlmSettingDao.java | 7 + .../db/alm/setting/AlmSettingMapper.java | 1 + .../sonar/db/alm/setting/AlmSettingMapper.xml | 1 - .../db/alm/setting/AlmSettingDaoTest.java | 62 +++++-- .../db/almsettings/AlmSettingsTesting.java | 13 -- .../GithubWebhookAuthentication.java | 127 ++++++++++++++- .../RequestAuthenticatorImpl.java | 1 - .../server/user/GithubWebhookUserSession.java | 2 +- .../GithubWebhookAuthenticationTest.java | 154 ++++++++++++++++-- .../user/GithubWebhookUserSessionTest.java | 5 +- .../org.mockito.plugins.MockMaker | 1 + 11 files changed, 330 insertions(+), 44 deletions(-) create mode 100644 server/sonar-webserver-auth/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/AlmSettingDao.java b/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/AlmSettingDao.java index 7a2ccb744fe..a382b9d3ed6 100644 --- a/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/AlmSettingDao.java +++ b/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/AlmSettingDao.java @@ -64,6 +64,13 @@ public class AlmSettingDao implements Dao { return Optional.ofNullable(getMapper(dbSession).selectByKey(key)); } + public Optional selectByAlmAndAppId(DbSession dbSession, ALM alm, String appId) { + return selectByAlm(dbSession, alm) + .stream() + .filter(almSettingDto -> appId.equals(almSettingDto.getAppId())) + .findAny(); + } + public List selectByAlm(DbSession dbSession, ALM alm) { return getMapper(dbSession).selectByAlm(alm.getId()); } diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/AlmSettingMapper.java b/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/AlmSettingMapper.java index bb3382c61c3..8c9203cb95a 100644 --- a/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/AlmSettingMapper.java +++ b/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/AlmSettingMapper.java @@ -40,4 +40,5 @@ public interface AlmSettingMapper { void update(@Param("dto") AlmSettingDto almSettingDto); int deleteByKey(@Param("key") String key); + } diff --git a/server/sonar-db-dao/src/main/resources/org/sonar/db/alm/setting/AlmSettingMapper.xml b/server/sonar-db-dao/src/main/resources/org/sonar/db/alm/setting/AlmSettingMapper.xml index 2e45e8bc34c..76eb4fffd51 100644 --- a/server/sonar-db-dao/src/main/resources/org/sonar/db/alm/setting/AlmSettingMapper.xml +++ b/server/sonar-db-dao/src/main/resources/org/sonar/db/alm/setting/AlmSettingMapper.xml @@ -102,5 +102,4 @@ DELETE FROM alm_settings WHERE kee = #{key, jdbcType=VARCHAR} - diff --git a/server/sonar-db-dao/src/test/java/org/sonar/db/alm/setting/AlmSettingDaoTest.java b/server/sonar-db-dao/src/test/java/org/sonar/db/alm/setting/AlmSettingDaoTest.java index 74e3c69a238..44eee7b433f 100644 --- a/server/sonar-db-dao/src/test/java/org/sonar/db/alm/setting/AlmSettingDaoTest.java +++ b/server/sonar-db-dao/src/test/java/org/sonar/db/alm/setting/AlmSettingDaoTest.java @@ -19,7 +19,11 @@ */ package org.sonar.db.alm.setting; +import java.util.Iterator; import java.util.List; +import java.util.Optional; +import java.util.stream.IntStream; +import java.util.stream.Stream; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -33,14 +37,15 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import static org.sonar.db.alm.setting.ALM.GITHUB; -import static org.sonar.db.almsettings.AlmSettingsTesting.newAlmSettingDtoWithEmptySecrets; +import static org.sonar.db.alm.setting.ALM.GITLAB; import static org.sonar.db.almsettings.AlmSettingsTesting.newGithubAlmSettingDto; -import static org.sonar.db.almsettings.AlmSettingsTesting.newGithubAlmSettingDtoWithNonRequiredField; public class AlmSettingDaoTest { private static final long NOW = 1000000L; private static final String A_UUID = "SOME_UUID"; + private static final AlmSettingDto ALM_SETTING_WITH_WEBHOOK_SECRET = newGithubAlmSettingDto().setWebhookSecret("webhook_secret"); + private final TestSystem2 system2 = new TestSystem2().setNow(NOW); @Rule public DbTester db = DbTester.create(system2); @@ -52,22 +57,23 @@ public class AlmSettingDaoTest { @Before public void setUp() { - when(uuidFactory.create()).thenReturn(A_UUID); + Iterator values = Stream.iterate(0, i -> i + 1).iterator(); + when(uuidFactory.create()).thenAnswer(answer -> A_UUID + "_" + values.next()); } @Test public void selectByUuid() { - AlmSettingDto expected = newGithubAlmSettingDtoWithNonRequiredField(); + AlmSettingDto expected = ALM_SETTING_WITH_WEBHOOK_SECRET; underTest.insert(dbSession, expected); - AlmSettingDto result = underTest.selectByUuid(dbSession, A_UUID).orElse(null); + AlmSettingDto result = underTest.selectByUuid(dbSession, expected.getUuid()).orElse(null); assertThat(result).usingRecursiveComparison().isEqualTo(expected); } @Test public void selectByUuid_shouldNotFindResult_whenUuidIsNotPresent() { - AlmSettingDto expected = newGithubAlmSettingDtoWithNonRequiredField(); + AlmSettingDto expected = ALM_SETTING_WITH_WEBHOOK_SECRET; underTest.insert(dbSession, expected); assertThat(underTest.selectByUuid(dbSession, "foo")).isNotPresent(); @@ -75,7 +81,7 @@ public class AlmSettingDaoTest { @Test public void selectByKey() { - AlmSettingDto expected = newGithubAlmSettingDtoWithNonRequiredField(); + AlmSettingDto expected = ALM_SETTING_WITH_WEBHOOK_SECRET; underTest.insert(dbSession, expected); AlmSettingDto result = underTest.selectByKey(dbSession, expected.getKey()).orElse(null); @@ -85,7 +91,7 @@ public class AlmSettingDaoTest { @Test public void selectByKey_shouldNotFindResult_whenKeyIsNotPresent() { - AlmSettingDto expected = newGithubAlmSettingDtoWithNonRequiredField(); + AlmSettingDto expected = ALM_SETTING_WITH_WEBHOOK_SECRET; underTest.insert(dbSession, expected); assertThat(underTest.selectByKey(dbSession, "foo")).isNotPresent(); @@ -93,7 +99,7 @@ public class AlmSettingDaoTest { @Test public void selectByKey_withEmptySecrets() { - AlmSettingDto expected = newAlmSettingDtoWithEmptySecrets(); + AlmSettingDto expected = newGithubAlmSettingDto().setWebhookSecret(null); underTest.insert(dbSession, expected); AlmSettingDto result = underTest.selectByKey(dbSession, expected.getKey()).orElse(null); @@ -142,7 +148,7 @@ public class AlmSettingDaoTest { //WHEN underTest.update(dbSession, expected, false); //THEN - AlmSettingDto result = underTest.selectByUuid(dbSession, A_UUID).orElse(null); + AlmSettingDto result = underTest.selectByUuid(dbSession, expected.getUuid()).orElse(null); assertThat(result).usingRecursiveComparison().isEqualTo(expected); } @@ -156,4 +162,40 @@ public class AlmSettingDaoTest { assertThat(underTest.selectByKey(dbSession, almSettingDto.getKey())).isNotPresent(); } + @Test + public void selectByAlmAndAppId_whenSingleMatch_returnsCorrectObject() { + String appId = "APP_ID"; + AlmSettingDto expectedAlmSettingDto = db.almSettings().insertGitHubAlmSetting(almSettingDto -> almSettingDto.setAppId(appId)); + db.almSettings().insertGitHubAlmSetting(almSettingDto -> almSettingDto.setAppId(null)); + + Optional result = underTest.selectByAlmAndAppId(dbSession, GITHUB, appId); + + assertThat(result).isPresent(); + assertThat(result.get()).usingRecursiveComparison().isEqualTo(expectedAlmSettingDto); + } + + @Test + public void selectByAlmAndAppId_whenAppIdSharedWithAnotherAlm_returnsCorrectOne() { + String appId = "APP_ID"; + db.almSettings().insertGitHubAlmSetting(almSettingDto -> almSettingDto.setAppId(appId)); + AlmSettingDto gitLabAlmSettingDto = db.almSettings().insertGitlabAlmSetting(almSettingDto -> almSettingDto.setAppId(appId)); + + Optional result = underTest.selectByAlmAndAppId(dbSession, GITLAB, appId); + + assertThat(result).isPresent(); + assertThat(result.get()).usingRecursiveComparison().isEqualTo(gitLabAlmSettingDto); + } + + @Test + public void selectByAlmAndAppId_withMultipleConfigurationWithSameAppId_returnsAnyAndDoesNotFail() { + String appId = "APP_ID"; + IntStream.of(1, 10).forEach(i -> db.almSettings().insertGitHubAlmSetting(almSettingDto -> almSettingDto.setAppId(appId))); + IntStream.of(1, 5).forEach(i -> db.almSettings().insertGitHubAlmSetting(almSettingDto -> almSettingDto.setAppId(null))); + + Optional result = underTest.selectByAlmAndAppId(dbSession, GITHUB, appId); + + assertThat(result).isPresent(); + assertThat(result.get().getAppId()).isEqualTo(appId); + } + } diff --git a/server/sonar-db-dao/src/testFixtures/java/org/sonar/db/almsettings/AlmSettingsTesting.java b/server/sonar-db-dao/src/testFixtures/java/org/sonar/db/almsettings/AlmSettingsTesting.java index 8745acb1631..b880b05a624 100644 --- a/server/sonar-db-dao/src/testFixtures/java/org/sonar/db/almsettings/AlmSettingsTesting.java +++ b/server/sonar-db-dao/src/testFixtures/java/org/sonar/db/almsettings/AlmSettingsTesting.java @@ -33,10 +33,6 @@ public class AlmSettingsTesting { } - public static AlmSettingDto newGithubAlmSettingDtoWithNonRequiredField() { - return newGithubAlmSettingDto().setWebhookSecret(randomAlphanumeric(160)); - } - public static AlmSettingDto newGithubAlmSettingDto() { return new AlmSettingDto() .setKey(randomAlphanumeric(200)) @@ -48,15 +44,6 @@ public class AlmSettingsTesting { .setAlm(ALM.GITHUB); } - public static AlmSettingDto newAlmSettingDtoWithEmptySecrets() { - return new AlmSettingDto() - .setKey(randomAlphanumeric(200)) - .setUrl(randomAlphanumeric(2000)) - .setAppId(randomNumeric(8)) - .setClientId(randomNumeric(8)) - .setAlm(ALM.GITHUB); - } - public static AlmSettingDto newAzureAlmSettingDto() { return new AlmSettingDto() .setKey(randomAlphanumeric(200)) diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/GithubWebhookAuthentication.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/GithubWebhookAuthentication.java index f732d83b970..2fb47dd24d0 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/GithubWebhookAuthentication.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/GithubWebhookAuthentication.java @@ -19,26 +19,143 @@ */ package org.sonar.server.authentication; +import com.google.common.annotations.VisibleForTesting; +import java.io.IOException; +import java.security.MessageDigest; import java.util.Optional; import javax.servlet.http.HttpServletRequest; -import org.apache.commons.lang.StringUtils; +import org.apache.commons.codec.digest.HmacAlgorithms; +import org.apache.commons.codec.digest.HmacUtils; +import org.sonar.api.config.internal.Encryption; +import org.sonar.api.config.internal.Settings; +import org.sonar.api.utils.log.Logger; +import org.sonar.api.utils.log.Loggers; +import org.sonar.db.DbClient; +import org.sonar.db.DbSession; +import org.sonar.db.alm.setting.ALM; import org.sonar.server.authentication.event.AuthenticationEvent; +import org.sonar.server.authentication.event.AuthenticationException; +import static java.lang.String.format; +import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.stream.Collectors.joining; +import static org.apache.commons.lang.StringUtils.isEmpty; import static org.sonar.server.user.GithubWebhookUserSession.GITHUB_WEBHOOK_USER_NAME; public class GithubWebhookAuthentication { + private static final Logger LOG = Loggers.get(GithubWebhookAuthentication.class); + + @VisibleForTesting + static final String GITHUB_SIGNATURE_HEADER = "x-hub-signature-256"; + + @VisibleForTesting + static final String GITHUB_APP_ID_HEADER = "x-github-hook-installation-target-id"; + + @VisibleForTesting + static final String MSG_AUTHENTICATION_FAILED = "Failed to authenticate payload from Github webhook. Either the webhook was called by unexpected clients" + + " or the webhook secret set in SonarQube does not match the one from Github."; + + @VisibleForTesting + static final String MSG_UNAUTHENTICATED_GITHUB_CALLS_DENIED = "Unauthenticated calls from GitHub are forbidden. A webhook secret must be defined in the GitHub App with Id %s."; + + @VisibleForTesting + static final String MSG_NO_WEBHOOK_SECRET_FOUND = "Webhook secret for your GitHub app with Id %s must be set in SonarQube."; + + @VisibleForTesting + static final String MSG_NO_BODY_FOUND = "No body found in GitHub Webhook event."; + + private static final String SHA_256_PREFIX = "sha256="; + private final AuthenticationEvent authenticationEvent; - public GithubWebhookAuthentication(AuthenticationEvent authenticationEvent) { + private final DbClient dbClient; + + private final Encryption encryption; + + public GithubWebhookAuthentication(AuthenticationEvent authenticationEvent, DbClient dbClient, Settings settings) { this.authenticationEvent = authenticationEvent; + this.dbClient = dbClient; + this.encryption = settings.getEncryption(); } public Optional authenticate(HttpServletRequest request) { - String authorizationHeader = request.getHeader("x-hub-signature-256"); - if (StringUtils.isEmpty(authorizationHeader)) { + String githubAppId = request.getHeader(GITHUB_APP_ID_HEADER); + if (isEmpty(githubAppId)) { + return Optional.empty(); + } + + String githubSignature = getGithubSignature(request, githubAppId); + String body = getBody(request); + String webhookSecret = getWebhookSecret(githubAppId); + + String computedSignature = computeSignature(body, webhookSecret); + if (!checkEqualityTimingAttacksSafe(githubSignature, computedSignature)) { + logAuthenticationProblemAndThrow(MSG_AUTHENTICATION_FAILED); + } + return createAuthResult(request); + } + + private static String getGithubSignature(HttpServletRequest request, String githubAppId) { + String githubSignature = request.getHeader(GITHUB_SIGNATURE_HEADER); + if (isEmpty(githubSignature) ) { + logAuthenticationProblemAndThrow(format(MSG_UNAUTHENTICATED_GITHUB_CALLS_DENIED, githubAppId)); + } + return githubSignature; + } + + private static String getBody(HttpServletRequest request) { + Optional body = getBodyInternal(request); + if (body.isEmpty() || isEmpty(body.get())) { + logAuthenticationProblemAndThrow(MSG_NO_BODY_FOUND); + } + return body.get(); + } + + + private static Optional getBodyInternal(HttpServletRequest request) { + try { + String body = request.getReader().lines().collect(joining(System.lineSeparator())); + return Optional.of(body); + } catch (IOException e) { + LOG.debug("Unexpected error while trying to get the body of github webhook request", e); return Optional.empty(); } - //TODO SONAR-17269 implement authentication algorithm + } + + private String getWebhookSecret(String githubAppId) { + String webhookSecret = fetchWebhookSecret(githubAppId); + if (isEmpty(webhookSecret)) { + logAuthenticationProblemAndThrow(format(MSG_NO_WEBHOOK_SECRET_FOUND, githubAppId)); + } + return webhookSecret; + } + + private String fetchWebhookSecret(String appId) { + try (DbSession dbSession = dbClient.openSession(false)) { + return dbClient.almSettingDao().selectByAlmAndAppId(dbSession, ALM.GITHUB, appId) + .map(almSettingDto -> almSettingDto.getDecryptedWebhookSecret(encryption)) + .orElse(null); + } + } + + private static void logAuthenticationProblemAndThrow(String message) { + LOG.warn(message); + throw AuthenticationException.newBuilder() + .setSource(AuthenticationEvent.Source.githubWebhook()) + .setMessage(message) + .build(); + } + + private static String computeSignature(String body, String webhookSecret) { + HmacUtils hmacUtils = new HmacUtils(HmacAlgorithms.HMAC_SHA_256, webhookSecret); + return SHA_256_PREFIX + hmacUtils.hmacHex(body); + } + + private static boolean checkEqualityTimingAttacksSafe(String githubSignature, String computedSignature) { + return MessageDigest.isEqual(githubSignature.getBytes(UTF_8), computedSignature.getBytes(UTF_8)); + } + + private Optional createAuthResult(HttpServletRequest request) { UserAuthResult userAuthResult = UserAuthResult.withGithubWebhook(); authenticationEvent.loginSuccess(request, GITHUB_WEBHOOK_USER_NAME, AuthenticationEvent.Source.githubWebhook()); return Optional.of(userAuthResult); diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/RequestAuthenticatorImpl.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/RequestAuthenticatorImpl.java index 23286abadea..1afd415ebb4 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/RequestAuthenticatorImpl.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/RequestAuthenticatorImpl.java @@ -90,7 +90,6 @@ public class RequestAuthenticatorImpl implements RequestAuthenticator { } private UserAuthResult loadUser(HttpServletRequest request, HttpServletResponse response) { - Function> createUserAuthResult = type -> userDto -> new UserAuthResult(userDto, type); // SSO authentication should come first in order to update JWT if user from header is not the same is user from JWT return httpHeadersAuthentication.authenticate(request, response).map(createUserAuthResult.apply(SSO)) diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/GithubWebhookUserSession.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/GithubWebhookUserSession.java index e819d335f15..ac658860b2f 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/GithubWebhookUserSession.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/GithubWebhookUserSession.java @@ -32,7 +32,7 @@ public class GithubWebhookUserSession extends AbstractUserSession { @Override public String getLogin() { - throw new IllegalStateException("GithubWebhookUserSession does not contain a login."); + return GITHUB_WEBHOOK_USER_NAME; } @Override diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/GithubWebhookAuthenticationTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/GithubWebhookAuthenticationTest.java index 667213caea5..96b48fee579 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/GithubWebhookAuthenticationTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/GithubWebhookAuthenticationTest.java @@ -19,32 +19,164 @@ */ package org.sonar.server.authentication; +import java.io.BufferedReader; +import java.io.IOException; +import java.io.StringReader; +import java.util.Optional; import javax.servlet.http.HttpServletRequest; +import org.junit.Before; +import org.junit.Rule; import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.InjectMocks; -import org.mockito.Mock; -import org.mockito.junit.MockitoJUnitRunner; +import org.mockito.Mockito; +import org.sonar.api.config.internal.Encryption; +import org.sonar.api.config.internal.Settings; +import org.sonar.api.utils.log.LogAndArguments; +import org.sonar.api.utils.log.LogTester; +import org.sonar.api.utils.log.LoggerLevel; +import org.sonar.db.DbTester; +import org.sonar.db.alm.setting.AlmSettingDto; import org.sonar.server.authentication.event.AuthenticationEvent; +import org.sonar.server.authentication.event.AuthenticationException; +import org.springframework.lang.Nullable; +import static java.lang.String.format; +import static java.util.Objects.requireNonNullElse; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.fail; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static org.sonar.server.authentication.GithubWebhookAuthentication.GITHUB_APP_ID_HEADER; +import static org.sonar.server.authentication.GithubWebhookAuthentication.GITHUB_SIGNATURE_HEADER; +import static org.sonar.server.authentication.GithubWebhookAuthentication.MSG_AUTHENTICATION_FAILED; +import static org.sonar.server.authentication.GithubWebhookAuthentication.MSG_NO_BODY_FOUND; +import static org.sonar.server.authentication.GithubWebhookAuthentication.MSG_NO_WEBHOOK_SECRET_FOUND; +import static org.sonar.server.authentication.GithubWebhookAuthentication.MSG_UNAUTHENTICATED_GITHUB_CALLS_DENIED; -@RunWith(MockitoJUnitRunner.class) public class GithubWebhookAuthenticationTest { - @Mock - private AuthenticationEvent authenticationEvent; + private static final String GITHUB_SIGNATURE = "sha256=1cd2d35d7bb5fc5738672bcdc959c4bc94ba308eb7008938a2f22b5713571925"; + private static final String GITHUB_PAYLOAD = "{\"action\":\"closed_by_user\",\"alert\":{\"number\":2,\"created_at\":\"2022-08-16T13:02:17Z\",\"updated_at\":\"2022-09-05T08:35:05Z\",\"url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/code-scanning/alerts/2\",\"html_url\":\"https://github.com/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/security/code-scanning/2\",\"state\":\"dismissed\",\"fixed_at\":null,\"dismissed_by\":{\"login\":\"aurelien-poscia-sonarsource\",\"id\":100427063,\"node_id\":\"U_kgDOBfxlNw\",\"avatar_url\":\"https://avatars.githubusercontent.com/u/100427063?v=4\",\"gravatar_id\":\"\",\"url\":\"https://api.github.com/users/aurelien-poscia-sonarsource\",\"html_url\":\"https://github.com/aurelien-poscia-sonarsource\",\"followers_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/followers\",\"following_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/following{/other_user}\",\"gists_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/gists{/gist_id}\",\"starred_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/starred{/owner}{/repo}\",\"subscriptions_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/subscriptions\",\"organizations_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/orgs\",\"repos_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/repos\",\"events_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/events{/privacy}\",\"received_events_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/received_events\",\"type\":\"User\",\"site_admin\":false},\"dismissed_at\":\"2022-09-05T08:35:05Z\",\"dismissed_reason\":\"false positive\",\"dismissed_comment\":\"c'est pas un buuuuug\",\"rule\":{\"id\":\"js/inconsistent-use-of-new\",\"severity\":\"warning\",\"description\":\"Inconsistent use of 'new'\",\"name\":\"js/inconsistent-use-of-new\",\"tags\":[\"correctness\",\"language-features\",\"reliability\"],\"full_description\":\"If a function is intended to be a constructor, it should always be invoked with 'new'. Otherwise, it should always be invoked as a normal function, that is, without 'new'.\",\"help\":\"\",\"help_uri\":\"\"},\"tool\":{\"name\":\"CodeQL\",\"guid\":null,\"version\":\"2.0.0\"},\"most_recent_instance\":{\"ref\":\"refs/heads/main\",\"analysis_key\":\"(default)\",\"environment\":\"{}\",\"category\":\"\",\"state\":\"dismissed\",\"commit_sha\":\"b4f17b0f7612575b70e9708e0aeda7e8067e8404\",\"message\":{\"text\":\"Function resolvingPromise is sometimes invoked as a constructor (for example here), and sometimes as a normal function (for example here).\"},\"location\":{\"path\":\"src/promiseUtils.js\",\"start_line\":2,\"end_line\":2,\"start_column\":8,\"end_column\":37},\"classifications\":[]},\"instances_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/code-scanning/alerts/2/instances\"},\"ref\":\"\",\"commit_oid\":\"\",\"repository\":{\"id\":525365935,\"node_id\":\"R_kgDOH1Byrw\",\"name\":\"mmf-2821-github-scanning-alerts\",\"full_name\":\"aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts\",\"private\":false,\"owner\":{\"login\":\"aurelien-poscia-sonarsource\",\"id\":100427063,\"node_id\":\"U_kgDOBfxlNw\",\"avatar_url\":\"https://avatars.githubusercontent.com/u/100427063?v=4\",\"gravatar_id\":\"\",\"url\":\"https://api.github.com/users/aurelien-poscia-sonarsource\",\"html_url\":\"https://github.com/aurelien-poscia-sonarsource\",\"followers_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/followers\",\"following_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/following{/other_user}\",\"gists_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/gists{/gist_id}\",\"starred_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/starred{/owner}{/repo}\",\"subscriptions_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/subscriptions\",\"organizations_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/orgs\",\"repos_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/repos\",\"events_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/events{/privacy}\",\"received_events_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/received_events\",\"type\":\"User\",\"site_admin\":false},\"html_url\":\"https://github.com/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts\",\"description\":null,\"fork\":false,\"url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts\",\"forks_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/forks\",\"keys_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/keys{/key_id}\",\"collaborators_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/collaborators{/collaborator}\",\"teams_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/teams\",\"hooks_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/hooks\",\"issue_events_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/issues/events{/number}\",\"events_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/events\",\"assignees_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/assignees{/user}\",\"branches_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/branches{/branch}\",\"tags_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/tags\",\"blobs_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/git/blobs{/sha}\",\"git_tags_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/git/tags{/sha}\",\"git_refs_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/git/refs{/sha}\",\"trees_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/git/trees{/sha}\",\"statuses_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/statuses/{sha}\",\"languages_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/languages\",\"stargazers_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/stargazers\",\"contributors_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/contributors\",\"subscribers_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/subscribers\",\"subscription_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/subscription\",\"commits_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/commits{/sha}\",\"git_commits_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/git/commits{/sha}\",\"comments_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/comments{/number}\",\"issue_comment_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/issues/comments{/number}\",\"contents_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/contents/{+path}\",\"compare_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/compare/{base}...{head}\",\"merges_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/merges\",\"archive_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/{archive_format}{/ref}\",\"downloads_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/downloads\",\"issues_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/issues{/number}\",\"pulls_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/pulls{/number}\",\"milestones_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/milestones{/number}\",\"notifications_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/notifications{?since,all,participating}\",\"labels_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/labels{/name}\",\"releases_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/releases{/id}\",\"deployments_url\":\"https://api.github.com/repos/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts/deployments\",\"created_at\":\"2022-08-16T12:16:56Z\",\"updated_at\":\"2022-08-16T12:16:56Z\",\"pushed_at\":\"2022-08-18T10:08:48Z\",\"git_url\":\"git://github.com/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts.git\",\"ssh_url\":\"git@github.com:aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts.git\",\"clone_url\":\"https://github.com/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts.git\",\"svn_url\":\"https://github.com/aurelien-poscia-sonarsource/mmf-2821-github-scanning-alerts\",\"homepage\":null,\"size\":2,\"stargazers_count\":0,\"watchers_count\":0,\"language\":null,\"has_issues\":true,\"has_projects\":true,\"has_downloads\":true,\"has_wiki\":true,\"has_pages\":false,\"forks_count\":0,\"mirror_url\":null,\"archived\":false,\"disabled\":false,\"open_issues_count\":0,\"license\":null,\"allow_forking\":true,\"is_template\":false,\"web_commit_signoff_required\":false,\"topics\":[],\"visibility\":\"public\",\"forks\":0,\"open_issues\":0,\"watchers\":0,\"default_branch\":\"main\"},\"sender\":{\"login\":\"aurelien-poscia-sonarsource\",\"id\":100427063,\"node_id\":\"U_kgDOBfxlNw\",\"avatar_url\":\"https://avatars.githubusercontent.com/u/100427063?v=4\",\"gravatar_id\":\"\",\"url\":\"https://api.github.com/users/aurelien-poscia-sonarsource\",\"html_url\":\"https://github.com/aurelien-poscia-sonarsource\",\"followers_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/followers\",\"following_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/following{/other_user}\",\"gists_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/gists{/gist_id}\",\"starred_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/starred{/owner}{/repo}\",\"subscriptions_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/subscriptions\",\"organizations_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/orgs\",\"repos_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/repos\",\"events_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/events{/privacy}\",\"received_events_url\":\"https://api.github.com/users/aurelien-poscia-sonarsource/received_events\",\"type\":\"User\",\"site_admin\":false},\"installation\":{\"id\":28299870,\"node_id\":\"MDIzOkludGVncmF0aW9uSW5zdGFsbGF0aW9uMjgyOTk4NzA=\"}}"; + private static final String APP_ID = "APP_ID"; + private static final String WEBHOOK_SECRET = "toto_secret"; + + @Rule + public LogTester logTester = new LogTester(); + + @Rule + public final DbTester db = DbTester.create(); + + private final AuthenticationEvent authenticationEvent = mock(AuthenticationEvent.class); + + private final Settings settings = mock(Settings.class); + + private final Encryption encryption = mock(Encryption.class); - @InjectMocks private GithubWebhookAuthentication githubWebhookAuthentication; + private AlmSettingDto almSettingDto; + + @Before + public void setUp() { + when(settings.getEncryption()).thenReturn(encryption); + githubWebhookAuthentication = new GithubWebhookAuthentication(authenticationEvent, db.getDbClient(), settings); + almSettingDto = db.almSettings().insertGitHubAlmSetting( + setting -> setting.setAppId(APP_ID), + setting -> setting.setWebhookSecret(WEBHOOK_SECRET)); + } + + @Test + public void authenticate_withComputedSignatureMatchingGithubSignature_returnsAuthentication() { + HttpServletRequest request = mockRequest(GITHUB_PAYLOAD, GITHUB_SIGNATURE); + + Optional authentication = githubWebhookAuthentication.authenticate(request); + assertThat(authentication).isPresent(); + + UserAuthResult userAuthResult = authentication.get(); + assertThat(userAuthResult.getUserDto()).isNull(); + assertThat(userAuthResult.getAuthType()).isEqualTo(UserAuthResult.AuthType.GITHUB_WEBHOOK); + assertThat(userAuthResult.getTokenDto()).isNull(); + + verify(authenticationEvent).loginSuccess(request, "github-webhook", AuthenticationEvent.Source.githubWebhook()); + assertThat(logTester.getLogs()).isEmpty(); + } + + @Test + public void authenticate_withoutGithubSignatureHeader_throws() { + HttpServletRequest request = mockRequest(GITHUB_PAYLOAD, null); + + String expectedMessage = format(MSG_UNAUTHENTICATED_GITHUB_CALLS_DENIED, APP_ID); + assertThatExceptionOfType(AuthenticationException.class) + .isThrownBy(() -> githubWebhookAuthentication.authenticate(request)) + .withMessage(expectedMessage); + assertThat(logTester.getLogs(LoggerLevel.WARN)).extracting(LogAndArguments::getFormattedMsg).contains(expectedMessage); + } + @Test - public void test(){ + public void authenticate_withoutBody_throws() { + HttpServletRequest request = mockRequest(null, GITHUB_SIGNATURE); - githubWebhookAuthentication.authenticate(mock(HttpServletRequest.class)); + assertThatExceptionOfType(AuthenticationException.class) + .isThrownBy(() -> githubWebhookAuthentication.authenticate(request)) + .withMessage(MSG_NO_BODY_FOUND); + assertThat(logTester.getLogs(LoggerLevel.WARN)).extracting(LogAndArguments::getFormattedMsg).contains(MSG_NO_BODY_FOUND); + } - //TODO SONAR-17266 + @Test + public void authenticate_withoutAppId_returnsEmpty() { + HttpServletRequest request = mockRequest(null, GITHUB_SIGNATURE); + when(request.getHeader(GITHUB_APP_ID_HEADER)).thenReturn(null); + + assertThat(githubWebhookAuthentication.authenticate(request)).isEmpty(); + assertThat(logTester.getLogs()).isEmpty(); + } + + @Test + public void authenticate_withWrongPayload_throws() { + HttpServletRequest request = mockRequest(GITHUB_PAYLOAD + "_", GITHUB_SIGNATURE); + + assertThatExceptionOfType(AuthenticationException.class) + .isThrownBy(() -> githubWebhookAuthentication.authenticate(request)) + .withMessage(MSG_AUTHENTICATION_FAILED); + assertThat(logTester.getLogs(LoggerLevel.WARN)).extracting(LogAndArguments::getFormattedMsg).contains(MSG_AUTHENTICATION_FAILED); + } + + @Test + public void authenticate_withWrongSignature_throws() { + HttpServletRequest request = mockRequest(GITHUB_PAYLOAD, GITHUB_SIGNATURE + "_"); + + assertThatExceptionOfType(AuthenticationException.class) + .isThrownBy(() -> githubWebhookAuthentication.authenticate(request)) + .withMessage(MSG_AUTHENTICATION_FAILED); + assertThat(logTester.getLogs(LoggerLevel.WARN)).extracting(LogAndArguments::getFormattedMsg).contains(MSG_AUTHENTICATION_FAILED); + } + + @Test + public void authenticate_whenNoWebhookSecret_throws() { + HttpServletRequest request = mockRequest(GITHUB_PAYLOAD, GITHUB_SIGNATURE); + db.getDbClient().almSettingDao().update(db.getSession(), almSettingDto.setWebhookSecret(null), true); + db.commit(); + + String expectedMessage = format(MSG_NO_WEBHOOK_SECRET_FOUND, APP_ID); + assertThatExceptionOfType(AuthenticationException.class) + .isThrownBy(() -> githubWebhookAuthentication.authenticate(request)) + .withMessage(expectedMessage); + assertThat(logTester.getLogs(LoggerLevel.WARN)).extracting(LogAndArguments::getFormattedMsg).contains(expectedMessage); + } + private static HttpServletRequest mockRequest(@Nullable String payload, @Nullable String gitHubSignature) { + HttpServletRequest request = mock(HttpServletRequest.class, Mockito.RETURNS_DEEP_STUBS); + try { + StringReader stringReader = new StringReader(requireNonNullElse(payload, "")); + BufferedReader bufferedReader = new BufferedReader(stringReader); + when(request.getReader()).thenReturn(bufferedReader); + when(request.getHeader(GITHUB_SIGNATURE_HEADER)).thenReturn(gitHubSignature); + when(request.getHeader(GITHUB_APP_ID_HEADER)).thenReturn(APP_ID); + } catch (IOException e) { + fail("mockRequest threw an exception: " + e.getMessage()); + } + return request; } } diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/GithubWebhookUserSessionTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/GithubWebhookUserSessionTest.java index 26280fbf9b2..400b3f8971d 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/GithubWebhookUserSessionTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/GithubWebhookUserSessionTest.java @@ -25,6 +25,7 @@ import org.sonar.db.permission.GlobalPermission; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalStateException; +import static org.sonar.server.user.GithubWebhookUserSession.GITHUB_WEBHOOK_USER_NAME; public class GithubWebhookUserSessionTest { @@ -32,7 +33,7 @@ public class GithubWebhookUserSessionTest { @Test public void getLogin() { - assertThatIllegalStateException().isThrownBy(() -> githubWebhookUserSession.getLogin()); + assertThat(githubWebhookUserSession.getLogin()).isEqualTo(GITHUB_WEBHOOK_USER_NAME); } @Test @@ -42,7 +43,7 @@ public class GithubWebhookUserSessionTest { @Test public void getName() { - assertThat(githubWebhookUserSession.getName()).isEqualTo("github-webhook"); + assertThat(githubWebhookUserSession.getName()).isEqualTo(GITHUB_WEBHOOK_USER_NAME); } @Test diff --git a/server/sonar-webserver-auth/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker b/server/sonar-webserver-auth/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker new file mode 100644 index 00000000000..1f0955d450f --- /dev/null +++ b/server/sonar-webserver-auth/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker @@ -0,0 +1 @@ +mock-maker-inline -- 2.39.5