From 06e14c854dc7ac1ec4bd56a4ed20441b0a1691c8 Mon Sep 17 00:00:00 2001 From: Igor Vinokur Date: Wed, 10 Jan 2024 10:46:02 +0200 Subject: [PATCH] Pass status code to Exception on Unauthorised to GitHub Error (#618) Pass status code to Exception on Unauthorised to GitHub Error in order to be able to recognise GitHub Server url if oAuth is not configured and no PAT is present. --- .../github/AbstractGithubURLParser.java | 13 +++-- .../server/github/GithubApiClient.java | 11 ++-- .../server/github/GithubApiClientTest.java | 3 +- .../server/github/GithubURLParserTest.java | 55 ++++++++++++++++++- 4 files changed, 70 insertions(+), 12 deletions(-) diff --git a/wsmaster/che-core-api-factory-github-common/src/main/java/org/eclipse/che/api/factory/server/github/AbstractGithubURLParser.java b/wsmaster/che-core-api-factory-github-common/src/main/java/org/eclipse/che/api/factory/server/github/AbstractGithubURLParser.java index 008df378a7..5c8f934462 100644 --- a/wsmaster/che-core-api-factory-github-common/src/main/java/org/eclipse/che/api/factory/server/github/AbstractGithubURLParser.java +++ b/wsmaster/che-core-api-factory-github-common/src/main/java/org/eclipse/che/api/factory/server/github/AbstractGithubURLParser.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2012-2023 Red Hat, Inc. + * Copyright (c) 2012-2024 Red Hat, Inc. * This program and the accompanying materials are made * available under the terms of the Eclipse Public License 2.0 * which is available at https://www.eclipse.org/legal/epl-2.0/ @@ -120,13 +120,18 @@ public abstract class AbstractGithubURLParser { private boolean isApiRequestRelevant(String repositoryUrl) { Optional serverUrlOptional = getServerUrl(repositoryUrl); if (serverUrlOptional.isPresent()) { - GithubApiClient GithubApiClient = new GithubApiClient(serverUrlOptional.get()); + GithubApiClient githubApiClient = new GithubApiClient(serverUrlOptional.get()); try { // If the user request catches the unauthorised error, it means that the provided url // belongs to GitHub. - GithubApiClient.getUser(""); + githubApiClient.getUser(""); } catch (ScmCommunicationException e) { - return e.getStatusCode() == HTTP_UNAUTHORIZED; + return e.getStatusCode() == HTTP_UNAUTHORIZED + // Check the error message as well, because other providers might also return 401 + // for such requests. + && e.getMessage().contains("Requires authentication") + || // for older GitHub Enterprise versions + e.getMessage().contains("Must authenticate to access this API."); } catch (ScmItemNotFoundException | ScmBadRequestException | IllegalArgumentException e) { return false; } diff --git a/wsmaster/che-core-api-factory-github-common/src/main/java/org/eclipse/che/api/factory/server/github/GithubApiClient.java b/wsmaster/che-core-api-factory-github-common/src/main/java/org/eclipse/che/api/factory/server/github/GithubApiClient.java index 4aebfe1440..17e43024fe 100644 --- a/wsmaster/che-core-api-factory-github-common/src/main/java/org/eclipse/che/api/factory/server/github/GithubApiClient.java +++ b/wsmaster/che-core-api-factory-github-common/src/main/java/org/eclipse/che/api/factory/server/github/GithubApiClient.java @@ -269,24 +269,25 @@ public class GithubApiClient { try { HttpResponse response = httpClient.send(request, HttpResponse.BodyHandlers.ofInputStream()); - LOG.trace("executeRequest={} response {}", request, response.statusCode()); - if (response.statusCode() == HTTP_OK) { + int statusCode = response.statusCode(); + LOG.trace("executeRequest={} response {}", request, statusCode); + if (statusCode == HTTP_OK) { return responseConverter.apply(response); - } else if (response.statusCode() == HTTP_NO_CONTENT) { + } else if (statusCode == HTTP_NO_CONTENT) { return null; } else { String body = response.body() == null ? "Unrecognised error" : CharStreams.toString(new InputStreamReader(response.body(), Charsets.UTF_8)); - switch (response.statusCode()) { + switch (statusCode) { case HTTP_BAD_REQUEST: throw new ScmBadRequestException(body); case HTTP_NOT_FOUND: throw new ScmItemNotFoundException(body); default: throw new ScmCommunicationException( - "Unexpected status code " + response.statusCode() + " " + response.toString()); + "Unexpected status code " + statusCode + " " + body, statusCode); } } } catch (IOException | InterruptedException | UncheckedIOException e) { diff --git a/wsmaster/che-core-api-factory-github/src/test/java/org/eclipse/che/api/factory/server/github/GithubApiClientTest.java b/wsmaster/che-core-api-factory-github/src/test/java/org/eclipse/che/api/factory/server/github/GithubApiClientTest.java index 7949f1cdf6..da2456f562 100644 --- a/wsmaster/che-core-api-factory-github/src/test/java/org/eclipse/che/api/factory/server/github/GithubApiClientTest.java +++ b/wsmaster/che-core-api-factory-github/src/test/java/org/eclipse/che/api/factory/server/github/GithubApiClientTest.java @@ -285,8 +285,7 @@ public class GithubApiClientTest { @Test( expectedExceptions = ScmCommunicationException.class, - expectedExceptionsMessageRegExp = - "Unexpected status code 502 \\(GET http://localhost:\\d*/api/v3/user\\) 502") + expectedExceptionsMessageRegExp = "Unexpected status code 502 item not found") public void shouldThrowExceptionOnOtherError() throws Exception { // given stubFor( diff --git a/wsmaster/che-core-api-factory-github/src/test/java/org/eclipse/che/api/factory/server/github/GithubURLParserTest.java b/wsmaster/che-core-api-factory-github/src/test/java/org/eclipse/che/api/factory/server/github/GithubURLParserTest.java index 50cb3623f3..de6f728e63 100644 --- a/wsmaster/che-core-api-factory-github/src/test/java/org/eclipse/che/api/factory/server/github/GithubURLParserTest.java +++ b/wsmaster/che-core-api-factory-github/src/test/java/org/eclipse/che/api/factory/server/github/GithubURLParserTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2012-2023 Red Hat, Inc. + * Copyright (c) 2012-2024 Red Hat, Inc. * This program and the accompanying materials are made * available under the terms of the Eclipse Public License 2.0 * which is available at https://www.eclipse.org/legal/epl-2.0/ @@ -11,6 +11,12 @@ */ package org.eclipse.che.api.factory.server.github; +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.get; +import static com.github.tomakehurst.wiremock.client.WireMock.stubFor; +import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static java.net.HttpURLConnection.HTTP_UNAUTHORIZED; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; @@ -22,6 +28,9 @@ import static org.mockito.Mockito.when; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertTrue; +import com.github.tomakehurst.wiremock.WireMockServer; +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.common.Slf4jNotifier; import java.util.Optional; import org.eclipse.che.api.core.ApiException; import org.eclipse.che.api.factory.server.scm.PersonalAccessToken; @@ -49,9 +58,17 @@ public class GithubURLParserTest { /** Instance of component that will be tested. */ private GithubURLParser githubUrlParser; + WireMockServer wireMockServer; + WireMock wireMock; + /** Setup objects/ */ @BeforeMethod protected void start() throws ApiException { + wireMockServer = + new WireMockServer(wireMockConfig().notifier(new Slf4jNotifier(false)).dynamicPort()); + wireMockServer.start(); + WireMock.configureFor("localhost", wireMockServer.port()); + wireMock = new WireMock("localhost", wireMockServer.port()); this.personalAccessTokenManager = mock(PersonalAccessTokenManager.class); this.githubApiClient = mock(GithubApiClient.class); @@ -315,4 +332,40 @@ public class GithubURLParserTest { verify(personalAccessTokenManager, times(2)) .get(any(Subject.class), eq("https://github-server.com")); } + + @Test + public void shouldValidateOldVersionGitHubServerUrl() throws Exception { + // given + String url = wireMockServer.url("/user/repo"); + stubFor( + get(urlEqualTo("/api/v3/user")) + .willReturn( + aResponse() + .withStatus(HTTP_UNAUTHORIZED) + .withBody("{\"message\": \"Must authenticate to access this API.\",\n}"))); + + // when + boolean valid = githubUrlParser.isValid(url); + + // then + assertTrue(valid); + } + + @Test + public void shouldValidateGitHubServerUrl() throws Exception { + // given + String url = wireMockServer.url("/user/repo"); + stubFor( + get(urlEqualTo("/api/v3/user")) + .willReturn( + aResponse() + .withStatus(HTTP_UNAUTHORIZED) + .withBody("{\"message\": \"Requires authentication\",\n}"))); + + // when + boolean valid = githubUrlParser.isValid(url); + + // then + assertTrue(valid); + } }