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 Oct 9, 2021
1 parent caf0eee commit 8acdd69
Show file tree
Hide file tree
Showing 6 changed files with 114 additions and 27 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 @@ -59,10 +58,6 @@ public class AzureDevopsRestClient implements AzureDevopsClient {
private final ObjectMapper objectMapper;
private final Supplier<CloseableHttpClient> httpClientFactory;

public AzureDevopsRestClient(String apiUrl, String authToken, ObjectMapper objectMapper) {
this(apiUrl, authToken, objectMapper, HttpClients::createSystem);
}

AzureDevopsRestClient(String apiUrl, String authToken, ObjectMapper objectMapper, Supplier<CloseableHttpClient> httpClientFactory) {
super();
this.apiUrl = apiUrl;
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.api.ce.ComputeEngineSide;
import org.sonar.api.config.internal.Settings;
import org.sonar.api.server.ServerSide;
Expand Down Expand Up @@ -54,6 +55,6 @@ public DefaultAzureDevopsClientFactory(Settings settings) {
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.getDecryptedPersonalAccessToken(settings.getEncryption())).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 @@ -23,6 +23,7 @@
import com.github.mc1arke.sonarqube.plugin.InvalidConfigurationException;
import com.github.mc1arke.sonarqube.plugin.almclient.LinkHeaderReader;
import org.apache.commons.lang.StringUtils;
import org.apache.http.impl.client.HttpClients;
import org.sonar.api.ce.ComputeEngineSide;
import org.sonar.api.config.internal.Settings;
import org.sonar.api.server.ServerSide;
Expand Down Expand Up @@ -55,6 +56,6 @@ public GitlabClient createClient(ProjectAlmSettingDto projectAlmSettingDto, AlmS
.orElseThrow(() -> new InvalidConfigurationException(InvalidConfigurationException.Scope.GLOBAL, "ALM URL must be specified"));
String apiToken = almSettingDto.getDecryptedPersonalAccessToken(settings.getEncryption());

return new GitlabRestClient(apiURL, apiToken, linkHeaderReader, objectMapper);
return new GitlabRestClient(apiURL, apiToken, linkHeaderReader, objectMapper, HttpClients::createSystem);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,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 @@ -52,6 +51,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 @@ -61,12 +61,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 @@ -111,7 +113,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 @@ -120,7 +122,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 @@ -146,7 +148,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 @@ -169,7 +171,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 @@ -188,7 +190,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 @@ -209,21 +211,22 @@ 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);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
package com.github.mc1arke.sonarqube.plugin.almclient.gitlab;

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

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.List;

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()).usingRecursiveComparison().isEqualTo(new UrlEncodedFormEntity(List.of(new BasicNameValuePair("body", "note")), StandardCharsets.UTF_8));
}

@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()).usingRecursiveComparison().isEqualTo(new UrlEncodedFormEntity(List.of(new BasicNameValuePair("body", "Merge request note")), StandardCharsets.UTF_8));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,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 @@ -202,11 +202,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

0 comments on commit 8acdd69

Please sign in to comment.