Skip to content

Commit

Permalink
#445: Specify encoding when creating Gitlab decoration requests
Browse files Browse the repository at this point in the history
When calling the Gitlab API with content that contains accented or cyrillic characters, the API returns an error response and fails to complete the decoration. Explicitly setting the encoding for all body content to UTF-8 on Gitlab API requests results in the content being encoded in a way for the Gitlab API handles correctly.
  • Loading branch information
mc1arke committed Jun 19, 2022
1 parent 1ff873f commit 6963a48
Show file tree
Hide file tree
Showing 6 changed files with 116 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
import org.apache.http.entity.ContentType;
import org.apache.http.entity.StringEntity;
import org.apache.http.impl.client.CloseableHttpClient;
import org.apache.http.impl.client.HttpClients;
import org.apache.http.util.EntityUtils;
import org.sonar.api.utils.log.Logger;
import org.sonar.api.utils.log.Loggers;
Expand All @@ -46,6 +45,7 @@
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.function.Supplier;

public class AzureDevopsRestClient implements AzureDevopsClient {

Expand All @@ -56,12 +56,14 @@ public class AzureDevopsRestClient implements AzureDevopsClient {
private final String authToken;
private final String apiUrl;
private final ObjectMapper objectMapper;
private final Supplier<CloseableHttpClient> httpClientFactory;

public AzureDevopsRestClient(String apiUrl, String authToken, ObjectMapper objectMapper) {
AzureDevopsRestClient(String apiUrl, String authToken, ObjectMapper objectMapper, Supplier<CloseableHttpClient> httpClientFactory) {
super();
this.apiUrl = apiUrl;
this.authToken = authToken;
this.objectMapper = objectMapper;
this.httpClientFactory = httpClientFactory;
}

@Override
Expand Down Expand Up @@ -124,7 +126,7 @@ private <T> T execute(String url, String method, String content, Class<T> type)
Optional.ofNullable(content).ifPresent(body -> requestBuilder.setEntity(new StringEntity(body, StandardCharsets.UTF_8.name())));
Optional.ofNullable(type).ifPresent(responseType -> requestBuilder.addHeader("Accept", ContentType.APPLICATION_JSON.getMimeType()));

try (CloseableHttpClient httpClient = HttpClients.createSystem()) {
try (CloseableHttpClient httpClient = httpClientFactory.get()) {
HttpResponse httpResponse = httpClient.execute(requestBuilder.build());

validateResponse(httpResponse);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.fasterxml.jackson.databind.MapperFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.apache.commons.lang3.StringUtils;
import org.apache.http.impl.client.HttpClients;
import org.sonar.db.alm.setting.AlmSettingDto;
import org.sonar.db.alm.setting.ProjectAlmSettingDto;

Expand All @@ -47,6 +48,6 @@ public DefaultAzureDevopsClientFactory() {
public AzureDevopsClient createClient(ProjectAlmSettingDto projectAlmSettingDto, AlmSettingDto almSettingDto) {
String apiUrl = Optional.ofNullable(almSettingDto.getUrl()).map(StringUtils::trimToNull).orElseThrow(() -> new IllegalStateException("ALM URL must be provided"));
String accessToken = Optional.ofNullable(almSettingDto.getPersonalAccessToken()).map(StringUtils::trimToNull).orElseThrow(() -> new IllegalStateException("Personal Access Token must be provided"));
return new AzureDevopsRestClient(apiUrl, Base64.getEncoder().encodeToString((":" + accessToken).getBytes(StandardCharsets.UTF_8)), objectMapper);
return new AzureDevopsRestClient(apiUrl, Base64.getEncoder().encodeToString((":" + accessToken).getBytes(StandardCharsets.UTF_8)), objectMapper, HttpClients::createSystem);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.DefaultLinkHeaderReader;
import org.apache.http.impl.client.HttpClients;

public class DefaultGitlabClientFactory implements GitlabClientFactory {

Expand All @@ -36,6 +37,6 @@ public DefaultGitlabClientFactory() {

@Override
public GitlabClient createClient(String baseApiUrl, String authToken) {
return new GitlabRestClient(baseApiUrl, authToken, new DefaultLinkHeaderReader(), objectMapper);
return new GitlabRestClient(baseApiUrl, authToken, new DefaultLinkHeaderReader(), objectMapper, HttpClients::createSystem);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import org.apache.http.client.methods.HttpRequestBase;
import org.apache.http.entity.ContentType;
import org.apache.http.impl.client.CloseableHttpClient;
import org.apache.http.impl.client.HttpClients;
import org.apache.http.message.BasicNameValuePair;
import org.apache.http.util.EntityUtils;
import org.sonar.api.utils.log.Logger;
Expand All @@ -51,6 +50,7 @@
import java.util.List;
import java.util.Optional;
import java.util.function.Consumer;
import java.util.function.Supplier;

class GitlabRestClient implements GitlabClient {

Expand All @@ -60,12 +60,14 @@ class GitlabRestClient implements GitlabClient {
private final String authToken;
private final ObjectMapper objectMapper;
private final LinkHeaderReader linkHeaderReader;
private final Supplier<CloseableHttpClient> httpClientFactory;

GitlabRestClient(String baseGitlabApiUrl, String authToken, LinkHeaderReader linkHeaderReader, ObjectMapper objectMapper) {
GitlabRestClient(String baseGitlabApiUrl, String authToken, LinkHeaderReader linkHeaderReader, ObjectMapper objectMapper, Supplier<CloseableHttpClient> httpClientFactory) {
this.baseGitlabApiUrl = baseGitlabApiUrl;
this.authToken = authToken;
this.linkHeaderReader = linkHeaderReader;
this.objectMapper = objectMapper;
this.httpClientFactory = httpClientFactory;
}

@Override
Expand Down Expand Up @@ -110,7 +112,7 @@ public Discussion addMergeRequestDiscussion(long projectId, long mergeRequestIid

HttpPost httpPost = new HttpPost(targetUrl);
httpPost.addHeader("Content-type", ContentType.APPLICATION_FORM_URLENCODED.getMimeType());
httpPost.setEntity(new UrlEncodedFormEntity(requestContent));
httpPost.setEntity(new UrlEncodedFormEntity(requestContent, StandardCharsets.UTF_8));
return entity(httpPost, Discussion.class, httpResponse -> validateResponse(httpResponse, 201, "Discussion successfully created"));
}

Expand All @@ -119,7 +121,7 @@ public void addMergeRequestDiscussionNote(long projectId, long mergeRequestIid,
String targetUrl = String.format("%s/projects/%s/merge_requests/%s/discussions/%s/notes", baseGitlabApiUrl, projectId, mergeRequestIid, discussionId);

HttpPost httpPost = new HttpPost(targetUrl);
httpPost.setEntity(new UrlEncodedFormEntity(Collections.singletonList(new BasicNameValuePair("body", noteContent))));
httpPost.setEntity(new UrlEncodedFormEntity(Collections.singletonList(new BasicNameValuePair("body", noteContent)), StandardCharsets.UTF_8));
entity(httpPost, null, httpResponse -> validateResponse(httpResponse, 201, "Commit discussions note added"));
}

Expand All @@ -145,7 +147,7 @@ public void setMergeRequestPipelineStatus(long projectId, String commitRevision,

HttpPost httpPost = new HttpPost(statusUrl);
httpPost.addHeader("Content-type", ContentType.APPLICATION_FORM_URLENCODED.getMimeType());
httpPost.setEntity(new UrlEncodedFormEntity(entityFields));
httpPost.setEntity(new UrlEncodedFormEntity(entityFields, StandardCharsets.UTF_8));
entity(httpPost, null, httpResponse -> {
if (httpResponse.toString().contains("Cannot transition status")) {
// Workaround for https://gitlab.com/gitlab-org/gitlab-ce/issues/25807
Expand All @@ -163,7 +165,7 @@ private <X> X entity(HttpRequestBase httpRequest, Class<X> type) throws IOExcept
private <X> X entity(HttpRequestBase httpRequest, Class<X> type, Consumer<HttpResponse> responseValidator) throws IOException {
httpRequest.addHeader("PRIVATE-TOKEN", authToken);

try (CloseableHttpClient httpClient = HttpClients.createSystem()) {
try (CloseableHttpClient httpClient = httpClientFactory.get()) {
HttpResponse httpResponse = httpClient.execute(httpRequest);

responseValidator.accept(httpResponse);
Expand All @@ -182,7 +184,7 @@ private <X> List<X> entities(HttpGet httpRequest, Class<X> type) throws IOExcept
private <X> List<X> entities(HttpGet httpRequest, Class<X> type, Consumer<HttpResponse> responseValidator) throws IOException {
httpRequest.addHeader("PRIVATE-TOKEN", authToken);

try (CloseableHttpClient httpClient = HttpClients.createSystem()) {
try (CloseableHttpClient httpClient = httpClientFactory.get()) {
HttpResponse httpResponse = httpClient.execute(httpRequest);

responseValidator.accept(httpResponse);
Expand All @@ -203,25 +205,26 @@ private <X> List<X> entities(HttpGet httpRequest, Class<X> type, Consumer<HttpRe

private static void validateResponse(HttpResponse httpResponse, int expectedStatus, String successLogMessage) {
if (httpResponse.getStatusLine().getStatusCode() == expectedStatus) {
LOGGER.debug(Optional.ofNullable(successLogMessage).map(v -> v + System.lineSeparator()).orElse("") + httpResponse.toString());
LOGGER.debug(Optional.ofNullable(successLogMessage).map(v -> v + System.lineSeparator()).orElse("") + httpResponse);
return;
}

String responseContent;
try {
responseContent = EntityUtils.toString(httpResponse.getEntity(), StandardCharsets.UTF_8);
} catch (IOException ex) {
LOGGER.warn("Could not decode response entity", ex);
responseContent = "";
}
String responseContent = Optional.ofNullable(httpResponse.getEntity()).map(entity -> {
try {
return EntityUtils.toString(entity, StandardCharsets.UTF_8);
} catch (IOException ex) {
LOGGER.warn("Could not decode response entity", ex);
return "";
}
}).orElse("");

LOGGER.error("Gitlab response status did not match expected value. Expected: " + expectedStatus
+ System.lineSeparator()
+ httpResponse.toString()
+ httpResponse
+ System.lineSeparator()
+ responseContent);

throw new IllegalStateException("An unexpected response code was returned from the Gitlab API - Expected: " + expectedStatus + ", Got: " + httpResponse.toString());
throw new IllegalStateException("An unexpected response code was returned from the Gitlab API - Expected: " + expectedStatus + ", Got: " + httpResponse.getStatusLine().getStatusCode());

}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ private void decorateQualityGateStatus(QualityGate.Status status) {
}
when(issueVisitor.getIssues()).thenReturn(issues);
when(analysisDetails.getPostAnalysisIssueVisitor()).thenReturn(issueVisitor);
when(analysisDetails.createAnalysisSummary(any())).thenReturn("summary comment\n\n[link text]");
when(analysisDetails.createAnalysisIssueSummary(any(), any())).thenReturn("issue");
when(analysisDetails.createAnalysisSummary(any())).thenReturn("summary commént\n\n[link text]");
when(analysisDetails.createAnalysisIssueSummary(any(), any())).thenReturn("issué");
when(analysisDetails.parseIssueIdFromUrl(any())).thenCallRealMethod();

wireMockRule.stubFor(get(urlPathEqualTo("/api/v4/user")).withHeader("PRIVATE-TOKEN", equalTo("token")).willReturn(okJson("{\n" +
Expand Down Expand Up @@ -198,11 +198,11 @@ private void decorateQualityGateStatus(QualityGate.Status status) {
.willReturn(created()));

wireMockRule.stubFor(post(urlPathEqualTo("/api/v4/projects/" + sourceProjectId + "/merge_requests/" + mergeRequestIid + "/discussions"))
.withRequestBody(equalTo("body=summary+comment%0A%0A%5Blink+text%5D"))
.withRequestBody(equalTo("body=summary+comm%C3%A9nt%0A%0A%5Blink+text%5D"))
.willReturn(created().withBody(discussionPostResponseBody(discussionId, discussionNote(noteId, user, "summary comment", true, false)))));

wireMockRule.stubFor(post(urlPathEqualTo("/api/v4/projects/" + sourceProjectId + "/merge_requests/" + mergeRequestIid + "/discussions"))
.withRequestBody(equalTo("body=issue&" +
.withRequestBody(equalTo("body=issu%C3%A9&" +
urlEncode("position[base_sha]") + "=d6a420d043dfe85e7c240fd136fc6e197998b10a&" +
urlEncode("position[start_sha]") + "=d6a420d043dfe85e7c240fd136fc6e197998b10a&" +
urlEncode("position[head_sha]") + "=" + commitSHA + "&" +
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
package com.github.mc1arke.sonarqube.plugin.ce.pullrequest.gitlab;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.LinkHeaderReader;
import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.gitlab.model.MergeRequestNote;
import org.apache.http.HttpEntity;
import org.apache.http.HttpEntityEnclosingRequest;
import org.apache.http.StatusLine;
import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpUriRequest;
import org.apache.http.impl.client.CloseableHttpClient;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;

import java.io.IOException;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

class GitlabRestClientTest {

private final CloseableHttpClient closeableHttpClient = mock(CloseableHttpClient.class);
private final LinkHeaderReader linkHeaderReader = mock(LinkHeaderReader.class);
private final ObjectMapper objectMapper = mock(ObjectMapper.class);

@Test
void checkErrorThrownOnNonSuccessResponseStatus() throws IOException {
GitlabRestClient underTest = new GitlabRestClient("http://url.test/api", "token", linkHeaderReader, objectMapper, () -> closeableHttpClient);

CloseableHttpResponse closeableHttpResponse = mock(CloseableHttpResponse.class);
StatusLine statusLine = mock(StatusLine.class);
when(statusLine.getStatusCode()).thenReturn(500);
when(closeableHttpResponse.getStatusLine()).thenReturn(statusLine);
when(closeableHttpClient.execute(any())).thenReturn(closeableHttpResponse);

MergeRequestNote mergeRequestNote = mock(MergeRequestNote.class);
when(mergeRequestNote.getContent()).thenReturn("note");

assertThatThrownBy(() -> underTest.addMergeRequestDiscussion(101, 99, mergeRequestNote))
.isExactlyInstanceOf(IllegalStateException.class)
.hasMessage("An unexpected response code was returned from the Gitlab API - Expected: 201, Got: 500")
.hasNoCause();

ArgumentCaptor<HttpUriRequest> requestArgumentCaptor = ArgumentCaptor.forClass(HttpUriRequest.class);
verify(closeableHttpClient).execute(requestArgumentCaptor.capture());

HttpEntityEnclosingRequest request = (HttpEntityEnclosingRequest) requestArgumentCaptor.getValue();

assertThat(request.getRequestLine().getMethod()).isEqualTo("POST");
assertThat(request.getRequestLine().getUri()).isEqualTo("http://url.test/api/projects/101/merge_requests/99/discussions");
assertThat(request.getEntity().getContent()).hasContent("body=note");
}

@Test
void checkCorrectEncodingUsedOnMergeRequestDiscussion() throws IOException {
CloseableHttpResponse closeableHttpResponse = mock(CloseableHttpResponse.class);
StatusLine statusLine = mock(StatusLine.class);
when(statusLine.getStatusCode()).thenReturn(201);
when(closeableHttpResponse.getStatusLine()).thenReturn(statusLine);
HttpEntity httpEntity = mock(HttpEntity.class);
when(closeableHttpResponse.getEntity()).thenReturn(httpEntity);
when(closeableHttpClient.execute(any())).thenReturn(closeableHttpResponse);

MergeRequestNote mergeRequestNote = new MergeRequestNote("Merge request note");

GitlabRestClient underTest = new GitlabRestClient("http://api.url", "token", linkHeaderReader, objectMapper, () -> closeableHttpClient);
underTest.addMergeRequestDiscussion(123, 321, mergeRequestNote);

ArgumentCaptor<HttpUriRequest> requestArgumentCaptor = ArgumentCaptor.forClass(HttpUriRequest.class);
verify(closeableHttpClient).execute(requestArgumentCaptor.capture());

HttpEntityEnclosingRequest request = (HttpEntityEnclosingRequest) requestArgumentCaptor.getValue();

assertThat(request.getRequestLine().getMethod()).isEqualTo("POST");
assertThat(request.getRequestLine().getUri()).isEqualTo("http://api.url/projects/123/merge_requests/321/discussions");
assertThat(request.getEntity().getContent()).hasContent("body=Merge+request+note");
}

}

0 comments on commit 6963a48

Please sign in to comment.