SONAR-17271 Request body can be read multiple time & implement support of comments coming from GitHub webhooks

server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/GithubWebhookAuthentication.java

@@ -20,7 +20,6 @@
 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;
@@ -97,28 +96,18 @@ public class GithubWebhookAuthentication {
 
   private static String getGithubSignature(HttpServletRequest request, String githubAppId) {
     String githubSignature = request.getHeader(GITHUB_SIGNATURE_HEADER);
-    if (isEmpty(githubSignature) ) {
+    if (isEmpty(githubSignature)) {
       logAuthenticationProblemAndThrow(format(MSG_UNAUTHENTICATED_GITHUB_CALLS_DENIED, githubAppId));
     }
     return githubSignature;
   }
 
   private static String getBody(HttpServletRequest request) {
-    Optional<String> body = getBodyInternal(request);
-    if (body.isEmpty() ||  isEmpty(body.get())) {
-      logAuthenticationProblemAndThrow(MSG_NO_BODY_FOUND);
-    }
-    return body.get();
-  }
-
-
-  private static Optional<String> 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();
+      return request.getReader().lines().collect(joining(System.lineSeparator()));
+    } catch (Exception e) {
+      logAuthenticationProblemAndThrow(MSG_NO_BODY_FOUND);
+      return "";
     }
   }
 

server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/GithubWebhookAuthenticationTest.java

@@ -117,6 +117,17 @@ public class GithubWebhookAuthenticationTest {
   public void authenticate_withoutBody_throws() {
     HttpServletRequest request = mockRequest(null, 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_withExceptionWhileReadingBody_throws() throws IOException {
+    HttpServletRequest request = mockRequest(GITHUB_PAYLOAD, GITHUB_SIGNATURE);
+    when(request.getReader()).thenThrow(new IOException());
+
     assertThatExceptionOfType(AuthenticationException.class)
       .isThrownBy(() -> githubWebhookAuthentication.authenticate(request))
       .withMessage(MSG_NO_BODY_FOUND);

server/sonar-webserver-core/src/main/java/org/sonar/server/platform/web/RootFilter.java

@@ -20,7 +20,9 @@
 package org.sonar.server.platform.web;
 
 import com.google.common.annotations.VisibleForTesting;
+import java.io.BufferedReader;
 import java.io.IOException;
+import java.io.StringReader;
 import javax.servlet.Filter;
 import javax.servlet.FilterChain;
 import javax.servlet.FilterConfig;
@@ -34,6 +36,7 @@ import javax.servlet.http.HttpSession;
 import org.sonar.api.utils.log.Loggers;
 
 import static java.lang.String.format;
+import static java.util.stream.Collectors.joining;
 
 /**
  * <p>Profile HTTP requests using platform profiling utility.</p>
@@ -91,6 +94,7 @@ public class RootFilter implements Filter {
 
   @VisibleForTesting
   static class ServletRequestWrapper extends HttpServletRequestWrapper {
+    private String body;
 
     ServletRequestWrapper(HttpServletRequest request) {
       super(request);
@@ -112,5 +116,18 @@ public class RootFilter implements Filter {
     private static UnsupportedOperationException notSupported() {
       return new UnsupportedOperationException("Sessions are disabled so that web server is stateless");
     }
+
+    @Override
+    public BufferedReader getReader() throws IOException {
+      if (body == null) {
+        body = getBodyInternal((HttpServletRequest) getRequest());
+      }
+      return new BufferedReader(new StringReader(body));
+    }
+
+    private static String getBodyInternal(HttpServletRequest request) throws IOException {
+      return request.getReader().lines().collect(joining(System.lineSeparator()));
+    }
+
   }
 }

server/sonar-webserver/src/test/java/org/sonar/server/platform/web/RootFilterTest.java → server/sonar-webserver-core/src/test/java/org/sonar/server/platform/web/RootFilterTest.java

@@ -19,7 +19,11 @@
  */
 package org.sonar.server.platform.web;
 
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.StringReader;
 import java.util.List;
+import java.util.stream.IntStream;
 import javax.servlet.FilterChain;
 import javax.servlet.FilterConfig;
 import javax.servlet.ServletContext;
@@ -34,8 +38,11 @@ import org.mockito.ArgumentCaptor;
 import org.sonar.api.utils.log.LogTester;
 import org.sonar.api.utils.log.LoggerLevel;
 
+import static java.util.stream.Collectors.joining;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.assertj.core.api.Assertions.fail;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
@@ -45,6 +52,8 @@ import static org.mockito.Mockito.when;
 
 public class RootFilterTest {
 
+  private static final String PAYLOAD = "payload";
+
   @Rule
   public LogTester logTester = new LogTester();
 
@@ -117,8 +126,8 @@ public class RootFilterTest {
     ArgumentCaptor<ServletRequest> requestArgumentCaptor = ArgumentCaptor.forClass(ServletRequest.class);
     verify(chain).doFilter(requestArgumentCaptor.capture(), any(ServletResponse.class));
 
-
-    assertThatThrownBy(() -> ((HttpServletRequest) requestArgumentCaptor.getValue()).getSession())
+    ServletRequest actualServletRequest = requestArgumentCaptor.getValue();
+    assertThatThrownBy(() -> ((HttpServletRequest) actualServletRequest).getSession())
       .isInstanceOf(UnsupportedOperationException.class);
   }
 
@@ -128,7 +137,8 @@ public class RootFilterTest {
     ArgumentCaptor<ServletRequest> requestArgumentCaptor = ArgumentCaptor.forClass(ServletRequest.class);
     verify(chain).doFilter(requestArgumentCaptor.capture(), any(ServletResponse.class));
 
-    assertThatThrownBy(() -> ((HttpServletRequest) requestArgumentCaptor.getValue()).getSession(true))
+    ServletRequest actualServletRequest = requestArgumentCaptor.getValue();
+    assertThatThrownBy(() -> ((HttpServletRequest) actualServletRequest).getSession(true))
       .isInstanceOf(UnsupportedOperationException.class);
   }
 
@@ -145,4 +155,45 @@ public class RootFilterTest {
     when(response.isCommitted()).thenReturn(committed);
     return response;
   }
+
+  @Test
+  public void body_can_be_read_several_times() {
+    HttpServletRequest request = mockRequestWithBody();
+    RootFilter.ServletRequestWrapper servletRequestWrapper = new RootFilter.ServletRequestWrapper(request);
+
+    IntStream.range(0,3).forEach(i -> assertThat(readBody(servletRequestWrapper)).isEqualTo(PAYLOAD));
+  }
+
+  @Test
+  public void getReader_whenIoExceptionThrown_rethrows() throws IOException {
+    HttpServletRequest request = mockRequestWithBody();
+    IOException ioException = new IOException();
+    when(request.getReader()).thenThrow(ioException);
+
+    RootFilter.ServletRequestWrapper servletRequestWrapper = new RootFilter.ServletRequestWrapper(request);
+
+    assertThatExceptionOfType(RuntimeException.class)
+      .isThrownBy(() -> readBody(servletRequestWrapper))
+      .withCause(ioException);
+  }
+
+  private static HttpServletRequest mockRequestWithBody() {
+    HttpServletRequest httpServletRequest = mock(HttpServletRequest.class);
+    try {
+      StringReader stringReader = new StringReader(PAYLOAD);
+      BufferedReader bufferedReader = new BufferedReader(stringReader);
+      when(httpServletRequest.getReader()).thenReturn(bufferedReader);
+    } catch (IOException e) {
+      fail("mockRequest threw an exception: " + e.getMessage());
+    }
+    return httpServletRequest;
+  }
+
+  private static String readBody(HttpServletRequest request) {
+    try {
+      return request.getReader().lines().collect(joining(System.lineSeparator()));
+    } catch (IOException e) {
+      throw new RuntimeException("unexpected failure", e);
+    }
+  }
 }

server/sonar-webserver-webapi/src/main/java/org/sonar/server/issue/ws/IssueUpdater.java

@@ -112,7 +112,7 @@ public class IssueUpdater {
         || ruleDto == null
         // notification are not supported on PRs
         || !hasNotificationSupport(branchDto)
-        || context.fromAlm()) {
+        || context.getWebhookSource() != null) {
       return issueDto;
     }
 

server/sonar-webserver-webapi/src/main/java/org/sonar/server/issue/ws/SearchResponseLoader.java

@@ -186,12 +186,16 @@ public class SearchResponseLoader {
     if (fields.contains(COMMENTS)) {
       List<IssueChangeDto> comments = dbClient.issueChangeDao().selectByTypeAndIssueKeys(dbSession, collector.getIssueKeys(), IssueChangeDto.TYPE_COMMENT);
       result.setComments(comments);
-      for (IssueChangeDto comment : comments) {
-        collector.addUserUuids(singletonList(comment.getUserUuid()));
-        if (canEditOrDelete(comment)) {
-          result.addUpdatableComment(comment.getKey());
-        }
-      }
+      comments.stream()
+        .filter(c -> c.getUserUuid() != null)
+        .forEach(comment -> loadComment(collector, result, comment));
+    }
+  }
+
+  private void loadComment(Collector collector, SearchResponseData result, IssueChangeDto comment) {
+    collector.addUserUuids(singletonList(comment.getUserUuid()));
+    if (canEditOrDelete(comment)) {
+      result.addUpdatableComment(comment.getKey());
     }
   }
 

server/sonar-webserver-webapi/src/test/java/org/sonar/server/issue/ws/SearchActionTest.java

@@ -369,6 +369,15 @@ public class SearchActionTest {
         .setUserUuid(fabrice.getUuid())
         .setProjectUuid(project.projectUuid())
         .setIssueChangeCreationDate(parseDateTime("2014-09-10T12:00:00+0000").getTime()));
+    dbClient.issueChangeDao().insert(session,
+      new IssueChangeDto()
+        .setUuid(Uuids.createFast())
+        .setIssueKey(issue.getKey())
+        .setKey("COMMENT-NO-USER")
+        .setChangeData("Another comment without user")
+        .setChangeType(IssueChangeDto.TYPE_COMMENT)
+        .setProjectUuid(project.projectUuid())
+        .setIssueChangeCreationDate(parseDateTime("2022-09-10T12:00:00+0000").getTime()));
     session.commit();
     indexIssues();
     userSession.logIn(john);

server/sonar-webserver-webapi/src/test/resources/org/sonar/server/issue/ws/SearchActionTest/issue_with_comments.json

@@ -9,7 +9,7 @@
           "htmlText": "\u003cstrong\u003eMy comment\u003c/strong\u003e",
           "markdown": "*My comment*",
           "updatable": true,
-          "createdAt": "2014-09-09T12:00:00+0000"
+          "createdAt": "2014-09-09T14:00:00+0200"
         },
         {
           "key": "COMMENT-ABCE",
@@ -17,7 +17,14 @@
           "htmlText": "Another comment",
           "markdown": "Another comment",
           "updatable": false,
-          "createdAt": "2014-09-10T12:00:00+0000"
+          "createdAt": "2014-09-10T14:00:00+0200"
+        },
+        {
+          "key": "COMMENT-NO-USER",
+          "htmlText": "Another comment without user",
+          "markdown": "Another comment without user",
+          "updatable": false,
+          "createdAt": "2022-09-10T14:00:00+0200"
         }
       ]
     }