From 3601435b31bfdeaee7a7e2251d275e4b1a9e254e Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Mon, 2 Oct 2017 14:48:30 +0200 Subject: [PATCH] SONAR-9896 Webhook must support basic authentication --- .../webhook/WebhookCallerImpl.java | 13 ++++- .../webhook/WebhookCallerImplTest.java | 48 +++++++++++++++++-- .../sonar/core/config/WebhookProperties.java | 6 +++ .../tests/user/LocalAuthenticationTest.java | 4 +- .../sonarqube/tests/webhook/WebhooksTest.java | 3 +- 5 files changed, 64 insertions(+), 10 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/webhook/WebhookCallerImpl.java b/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/webhook/WebhookCallerImpl.java index 40cb2b128d8..3c14782e586 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/webhook/WebhookCallerImpl.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/webhook/WebhookCallerImpl.java @@ -20,6 +20,7 @@ package org.sonar.server.computation.task.projectanalysis.webhook; import java.io.IOException; +import okhttp3.Credentials; import okhttp3.HttpUrl; import okhttp3.MediaType; import okhttp3.OkHttpClient; @@ -32,8 +33,10 @@ import org.sonar.api.utils.System2; import static java.lang.String.format; import static java.net.HttpURLConnection.HTTP_MOVED_PERM; import static java.net.HttpURLConnection.HTTP_MOVED_TEMP; +import static java.nio.charset.StandardCharsets.UTF_8; import static okhttp3.internal.http.StatusLine.HTTP_PERM_REDIRECT; import static okhttp3.internal.http.StatusLine.HTTP_TEMP_REDIRECT; +import static org.apache.commons.lang.StringUtils.isNotEmpty; @ComputeEngineSide public class WebhookCallerImpl implements WebhookCaller { @@ -71,9 +74,17 @@ public class WebhookCallerImpl implements WebhookCaller { } private static Request buildHttpRequest(Webhook webhook, WebhookPayload payload) { + HttpUrl url = HttpUrl.parse(webhook.getUrl()); + if (url == null) { + throw new IllegalArgumentException("Webhook URL is not valid: " + webhook.getUrl()); + } Request.Builder request = new Request.Builder(); - request.url(webhook.getUrl()); + request.url(url); request.header(PROJECT_KEY_HEADER, payload.getProjectKey()); + if (isNotEmpty(url.username())) { + request.header("Authorization", Credentials.basic(url.username(), url.password(), UTF_8)); + } + RequestBody body = RequestBody.create(JSON, payload.getJson()); request.post(body); return request.build(); diff --git a/server/sonar-server/src/test/java/org/sonar/server/computation/task/projectanalysis/webhook/WebhookCallerImplTest.java b/server/sonar-server/src/test/java/org/sonar/server/computation/task/projectanalysis/webhook/WebhookCallerImplTest.java index aff151adb48..155ec647963 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/computation/task/projectanalysis/webhook/WebhookCallerImplTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/computation/task/projectanalysis/webhook/WebhookCallerImplTest.java @@ -19,6 +19,7 @@ */ package org.sonar.server.computation.task.projectanalysis.webhook; +import okhttp3.Credentials; import okhttp3.HttpUrl; import okhttp3.mockwebserver.MockResponse; import okhttp3.mockwebserver.MockWebServer; @@ -103,7 +104,7 @@ public class WebhookCallerImplTest { assertThat(delivery.getHttpStatus()).isEmpty(); assertThat(delivery.getDurationInMs()).isEmpty(); assertThat(delivery.getError().get()).isInstanceOf(IllegalArgumentException.class); - assertThat(delivery.getErrorMessage().get()).isEqualTo("unexpected url: this_is_not_an_url"); + assertThat(delivery.getErrorMessage().get()).isEqualTo("Webhook URL is not valid: this_is_not_an_url"); assertThat(delivery.getAt()).isEqualTo(NOW); assertThat(delivery.getWebhook()).isSameAs(webhook); assertThat(delivery.getPayload()).isSameAs(PAYLOAD); @@ -133,6 +134,26 @@ public class WebhookCallerImplTest { takeAndVerifyPostRequest("/target"); } + @Test + public void credentials_are_propagated_to_POST_redirects() throws Exception { + HttpUrl url = server.url("/redirect").newBuilder().username("theLogin").password("thePassword").build(); + Webhook webhook = new Webhook(PROJECT_UUID, CE_TASK_UUID, "my-webhook", url.toString()); + + // /redirect redirects to /target + server.enqueue(new MockResponse().setResponseCode(307).setHeader("Location", server.url("target"))); + server.enqueue(new MockResponse().setResponseCode(200)); + + WebhookDelivery delivery = newSender().call(webhook, PAYLOAD); + + assertThat(delivery.getHttpStatus().get()).isEqualTo(200); + + RecordedRequest redirectedRequest = takeAndVerifyPostRequest("/redirect"); + assertThat(redirectedRequest.getHeader("Authorization")).isEqualTo(Credentials.basic(url.username(), url.password())); + + RecordedRequest targetRequest = takeAndVerifyPostRequest("/target"); + assertThat(targetRequest.getHeader("Authorization")).isEqualTo(Credentials.basic(url.username(), url.password())); + } + @Test public void redirects_throws_ISE_if_header_Location_is_missing() throws Exception { HttpUrl url = server.url("/redirect"); @@ -163,11 +184,28 @@ public class WebhookCallerImplTest { .hasMessage("Unsupported protocol in redirect of " + url + " to ftp://foo"); } - private void takeAndVerifyPostRequest(String expectedPath) throws Exception { - RecordedRequest redirectedRequest = server.takeRequest(); + @Test + public void send_basic_authentication_header_if_url_contains_credentials() throws Exception { + HttpUrl url = server.url("/ping").newBuilder().username("theLogin").password("thePassword").build(); + Webhook webhook = new Webhook(PROJECT_UUID, CE_TASK_UUID, "my-webhook", url.toString()); + server.enqueue(new MockResponse().setBody("pong")); + + WebhookDelivery delivery = newSender().call(webhook, PAYLOAD); + + assertThat(delivery.getWebhook().getUrl()) + .isEqualTo(url.toString()) + .contains("://theLogin:thePassword@"); + RecordedRequest recordedRequest = takeAndVerifyPostRequest("/ping"); + assertThat(recordedRequest.getHeader("Authorization")).isEqualTo(Credentials.basic(url.username(), url.password())); + } + + private RecordedRequest takeAndVerifyPostRequest(String expectedPath) throws Exception { + RecordedRequest request = server.takeRequest(); - assertThat(redirectedRequest.getMethod()).isEqualTo("POST"); - assertThat(redirectedRequest.getPath()).isEqualTo(expectedPath); + assertThat(request.getMethod()).isEqualTo("POST"); + assertThat(request.getPath()).isEqualTo(expectedPath); + assertThat(request.getHeader("User-Agent")).isEqualTo("SonarQube/6.2"); + return request; } private WebhookCaller newSender() { diff --git a/sonar-core/src/main/java/org/sonar/core/config/WebhookProperties.java b/sonar-core/src/main/java/org/sonar/core/config/WebhookProperties.java index 91fb4c141e8..f3e3b98742d 100644 --- a/sonar-core/src/main/java/org/sonar/core/config/WebhookProperties.java +++ b/sonar-core/src/main/java/org/sonar/core/config/WebhookProperties.java @@ -51,6 +51,9 @@ public class WebhookProperties { private static final String DESCRIPTION = "Webhooks are used to notify external services when a project analysis is done. " + "An HTTP POST request including a JSON payload is sent to each of the first ten provided URLs.
" + "Learn more in the Webhooks documentation."; + private static final String URL_DESCRIPTION = "Server endpoint that will receive the webhook payload, for example 'http://my_server/foo'. " + + "If HTTP Basic authentication is used, HTTPS is recommended to avoid man in the middle attacks. " + + "Example: 'https://myLogin:myPassword@my_server/foo'"; private WebhookProperties() { // only static stuff @@ -70,6 +73,7 @@ public class WebhookProperties { PropertyFieldDefinition.build(URL_FIELD) .name("URL") .type(PropertyType.STRING) + .description(URL_DESCRIPTION) .build()) .build(), @@ -86,7 +90,9 @@ public class WebhookProperties { PropertyFieldDefinition.build(URL_FIELD) .name("URL") .type(PropertyType.STRING) + .description(URL_DESCRIPTION) .build()) .build()); } + } diff --git a/tests/src/test/java/org/sonarqube/tests/user/LocalAuthenticationTest.java b/tests/src/test/java/org/sonarqube/tests/user/LocalAuthenticationTest.java index d5b8a50240c..7c17b06599a 100644 --- a/tests/src/test/java/org/sonarqube/tests/user/LocalAuthenticationTest.java +++ b/tests/src/test/java/org/sonarqube/tests/user/LocalAuthenticationTest.java @@ -139,7 +139,7 @@ public class LocalAuthenticationTest { } @Test - public void basic_authentication_does_not_support_utf8_passwords() { + public void basic_authentication_supports_utf8_passwords() { String login = "user_with_utf8_password"; // see http://www.cl.cam.ac.uk/~mgk25/ucs/examples/UTF-8-test.txt String password = "κόσμε"; @@ -148,7 +148,7 @@ public class LocalAuthenticationTest { tester.users().generate(u -> u.setLogin(login).setPassword(password)); // authenticate - assertThat(checkAuthenticationWithAuthenticateWebService(login, password)).isFalse(); + assertThat(checkAuthenticationWithAuthenticateWebService(login, password)).isTrue(); } @Test diff --git a/tests/src/test/java/org/sonarqube/tests/webhook/WebhooksTest.java b/tests/src/test/java/org/sonarqube/tests/webhook/WebhooksTest.java index 975a5dc9366..2539ab6bb6d 100644 --- a/tests/src/test/java/org/sonarqube/tests/webhook/WebhooksTest.java +++ b/tests/src/test/java/org/sonarqube/tests/webhook/WebhooksTest.java @@ -195,8 +195,7 @@ public class WebhooksTest { assertThat(detail.getPayload()).isNotEmpty(); assertThat(detail.getErrorStacktrace()) .contains("java.lang.IllegalArgumentException") - .contains("unexpected url") - .contains("this_is_not_an_url"); + .contains("Webhook URL is not valid: this_is_not_an_url"); } @Test -- 2.39.5