From 257ae6f5c8b1849956b9ea8b540d5a23650c49c6 Mon Sep 17 00:00:00 2001 From: Philipp0205 <36603238+Philipp0205@users.noreply.github.com> Date: Mon, 7 Sep 2026 12:06:38 +0000 Subject: [PATCH] Skip GitHub owner scopes the access token may not search Listing pull requests across every accessible repository searches one owner scope at a time, but a single rejected scope aborted the whole listing. GitHub answers GET /search/issues with HTTP 422 "The listed users and repositories cannot be searched" as soon as one qualifier names something the token may not search, which happens for organizations the token was never SSO-authorized for, for private repositories when the token lacks the 'repo' scope, and for repositories renamed or deleted after they were listed. The connection test then reported "FAILED Read pull requests" even though every other scope would have returned results. Run the per-scope searches through GitHubPullRequestSearch, which skips a scope GitHub refuses and only fails when no scope is searchable at all, in which case the message names the scopes and how to fix the token. Failures that are not scope rejections still propagate, so a server or network error is never mistaken for partial results. Organizations and collaborator repositories that cannot be listed no longer hide the user's own pull requests either, and the connection test reports the skipped scopes as a warning. Co-authored-by: Kurrle, Philipp --- .../internal/github/GitHubConnectionTest.java | 238 ++++++++++++++++++ .../github/GitHubPullRequestSearchTest.java | 216 ++++++++++++++++ .../internal/github/StubGitHubServer.java | 172 +++++++++++++ .../internal/github/GitHubClient.java | 144 +++++++---- .../internal/github/GitHubJsonParser.java | 2 +- .../github/GitHubPullRequestSearch.java | 186 ++++++++++++++ .../internal/github/GitHubSearchQueries.java | 23 ++ .../ui/PullRequestPreferencePage.java | 7 +- 8 files changed, 941 insertions(+), 47 deletions(-) create mode 100644 org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubConnectionTest.java create mode 100644 org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearchTest.java create mode 100644 org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/StubGitHubServer.java create mode 100644 org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearch.java diff --git a/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubConnectionTest.java b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubConnectionTest.java new file mode 100644 index 0000000..69d522a --- /dev/null +++ b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubConnectionTest.java @@ -0,0 +1,238 @@ +/******************************************************************************* + * Copyright (C) 2026, Philipp0205 and others + * + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.egit.pullrequest.internal.github; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; + +import java.io.IOException; +import java.util.List; + +import org.eclipse.egit.pullrequest.internal.client.ConnectionDiagnostics; +import org.eclipse.egit.pullrequest.internal.client.ConnectionDiagnostics.Outcome; +import org.eclipse.egit.pullrequest.internal.client.ConnectionDiagnostics.Step; +import org.eclipse.egit.pullrequest.internal.github.StubGitHubServer.Response; +import org.eclipse.egit.pullrequest.internal.model.PullRequest; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +/** + * Tests how {@link GitHubClient} lists pull requests across every accessible + * owner scope when GitHub refuses to search some of them. + */ +public class GitHubConnectionTest { + + private static final String VALIDATION_FAILED = "{\"message\":" //$NON-NLS-1$ + + "\"Validation Failed\",\"errors\":[{\"message\":\"The listed " //$NON-NLS-1$ + + "users and repositories cannot be searched either because " //$NON-NLS-1$ + + "the resources do not exist or you do not have permission to " //$NON-NLS-1$ + + "view them.\",\"resource\":\"Search\",\"field\":\"q\"," //$NON-NLS-1$ + + "\"code\":\"invalid\"}],\"status\":\"422\"}"; //$NON-NLS-1$ + + private StubGitHubServer server; + + @Before + public void startServer() throws IOException { + server = new StubGitHubServer(); + server.on("/user", ok("{\"login\":\"Philipp0205\"}")); //$NON-NLS-1$ //$NON-NLS-2$ + server.on("/user/orgs", ok("[{\"login\":\"acme\"}]")); //$NON-NLS-1$ //$NON-NLS-2$ + server.on("/user/repos", ok("[]")); //$NON-NLS-1$ //$NON-NLS-2$ + server.on("/repos/Philipp0205/notes/pulls/1", //$NON-NLS-1$ + ok(pullRequest(1, "Fix the parser"))); //$NON-NLS-1$ + server.on("/repos/acme/app/pulls/4", //$NON-NLS-1$ + ok(pullRequest(4, "Bump the target platform"))); //$NON-NLS-1$ + } + + @After + public void stopServer() throws IOException { + server.close(); + } + + @Test + public void testScopeGitHubRefusesToSearchIsSkipped() throws Exception { + searchReturns("user%3APhilipp0205", //$NON-NLS-1$ + "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ + searchFailsWithValidationError("org%3Aacme"); //$NON-NLS-1$ + + List pullRequests = client() + .getPullRequests(null, null, null, 25, 0); + + assertThat(pullRequests, hasSize(1)); + assertThat(pullRequests.get(0).getTitle(), + equalTo("Fix the parser")); //$NON-NLS-1$ + } + + @Test + public void testDiagnosticsReportSkippedScopeAsWarningNotFailure() + throws Exception { + searchReturns("user%3APhilipp0205", //$NON-NLS-1$ + "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ + searchFailsWithValidationError("org%3Aacme"); //$NON-NLS-1$ + + ConnectionDiagnostics report = client().diagnoseConnection(); + + assertThat(report.toReport(), report.isSuccessful(), is(true)); + assertThat(step(report, "Read pull requests").getDetail(), //$NON-NLS-1$ + equalTo("Returned 1 pull request(s)")); //$NON-NLS-1$ + Step scopes = step(report, "Search scopes"); //$NON-NLS-1$ + assertThat(scopes.getOutcome(), equalTo(Outcome.WARNING)); + assertThat(scopes.getDetail(), containsString("org:acme")); //$NON-NLS-1$ + assertThat(scopes.getDetail(), containsString("read:org")); //$NON-NLS-1$ + } + + @Test + public void testDiagnosticsStaySilentWhenEveryScopeIsSearchable() + throws Exception { + searchReturns("user%3APhilipp0205", //$NON-NLS-1$ + "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ + searchReturns("org%3Aacme", "/repos/acme/app/pulls/4"); //$NON-NLS-1$ //$NON-NLS-2$ + + ConnectionDiagnostics report = client().diagnoseConnection(); + + assertThat(report.toReport(), report.isSuccessful(), is(true)); + assertThat(report.toReport(), not(containsString("Search scopes"))); //$NON-NLS-1$ + } + + @Test + public void testEveryScopeRefusedIsReportedWithTokenAdvice() { + searchFailsWithValidationError("user%3APhilipp0205"); //$NON-NLS-1$ + searchFailsWithValidationError("org%3Aacme"); //$NON-NLS-1$ + + ConnectionDiagnostics report = client().diagnoseConnection(); + + assertThat(report.isSuccessful(), is(false)); + Step read = step(report, "Read pull requests"); //$NON-NLS-1$ + assertThat(read.getOutcome(), equalTo(Outcome.FAILED)); + assertThat(read.getDetail(), containsString("user:Philipp0205")); //$NON-NLS-1$ + assertThat(read.getDetail(), containsString("org:acme")); //$NON-NLS-1$ + assertThat(read.getDetail(), containsString("read:org")); //$NON-NLS-1$ + } + + @Test + public void testOrganizationsThatCannotBeListedDoNotHideOwnPullRequests() + throws Exception { + server.on("/user/orgs", new Response(403, "Forbidden", //$NON-NLS-1$ //$NON-NLS-2$ + "{\"message\":\"Resource not accessible by integration\"}")); //$NON-NLS-1$ + searchReturns("user%3APhilipp0205", //$NON-NLS-1$ + "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ + + List pullRequests = client() + .getPullRequests(null, null, null, 25, 0); + + assertThat(pullRequests, hasSize(1)); + assertThat(pullRequests.get(0).getTitle(), + equalTo("Fix the parser")); //$NON-NLS-1$ + } + + @Test + public void testSearchFailuresOtherThanRefusedScopesAreReported() { + searchReturns("user%3APhilipp0205", //$NON-NLS-1$ + "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ + server.onQueryContaining("org%3Aacme", //$NON-NLS-1$ + new Response(503, "Service Unavailable", //$NON-NLS-1$ + "{\"message\":\"No server is currently available\"}")); //$NON-NLS-1$ + + ConnectionDiagnostics report = client().diagnoseConnection(); + + Step read = step(report, "Read pull requests"); //$NON-NLS-1$ + assertThat(read.getOutcome(), equalTo(Outcome.FAILED)); + assertThat(read.getDetail(), containsString("HTTP 503")); //$NON-NLS-1$ + } + + @Test + public void testEveryOwnerScopeIsSearchedSeparately() throws Exception { + server.on("/user/repos", //$NON-NLS-1$ + ok("[{\"full_name\":\"bob/tool\"}]")); //$NON-NLS-1$ + searchReturns("user%3APhilipp0205", //$NON-NLS-1$ + "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ + searchReturns("org%3Aacme", "/repos/acme/app/pulls/4"); //$NON-NLS-1$ //$NON-NLS-2$ + searchFailsWithValidationError("repo%3Abob%2Ftool"); //$NON-NLS-1$ + + List pullRequests = client() + .getPullRequests(null, null, null, 25, 0); + + assertThat(pullRequests, hasSize(2)); + assertThat(searchTargets(), hasSize(3)); + } + + @Test + public void testPullRequestFoundInTwoScopesIsListedOnce() + throws Exception { + server.on("/user/repos", //$NON-NLS-1$ + ok("[{\"full_name\":\"acme/app\"}]")); //$NON-NLS-1$ + searchReturns("user%3APhilipp0205", //$NON-NLS-1$ + "/repos/acme/app/pulls/4"); //$NON-NLS-1$ + searchReturns("org%3Aacme", "/repos/acme/app/pulls/4"); //$NON-NLS-1$ //$NON-NLS-2$ + + List pullRequests = client() + .getPullRequests(null, null, null, 25, 0); + + assertThat(pullRequests, hasSize(1)); + assertThat(pullRequests.get(0).getTitle(), + equalTo("Bump the target platform")); //$NON-NLS-1$ + } + + private GitHubClient client() { + return new GitHubClient(null, null, "token", server.url()); //$NON-NLS-1$ + } + + private void searchReturns(String encodedScope, String... pullPaths) { + StringBuilder items = new StringBuilder(); + for (String pullPath : pullPaths) { + if (items.length() > 0) { + items.append(','); + } + items.append("{\"pull_request\":{\"url\":\"") //$NON-NLS-1$ + .append(server.url()).append(pullPath).append("\"}}"); //$NON-NLS-1$ + } + server.onQueryContaining(encodedScope, + ok("{\"total_count\":" + pullPaths.length + ",\"items\":[" //$NON-NLS-1$ //$NON-NLS-2$ + + items + "]}")); //$NON-NLS-1$ + } + + private void searchFailsWithValidationError(String encodedScope) { + server.onQueryContaining(encodedScope, new Response(422, + "Unprocessable Entity", VALIDATION_FAILED)); //$NON-NLS-1$ + } + + private List searchTargets() { + return server.requestedTargets().stream() + .filter(target -> target.startsWith("/search/issues")) //$NON-NLS-1$ + .toList(); + } + + private static Response ok(String body) { + return new Response(200, "OK", body); //$NON-NLS-1$ + } + + private static String pullRequest(int number, String title) { + return "{\"number\":" + number + ",\"title\":\"" + title //$NON-NLS-1$ //$NON-NLS-2$ + + "\",\"state\":\"open\",\"body\":\"\"," //$NON-NLS-1$ + + "\"created_at\":\"2026-09-01T10:00:00Z\"," //$NON-NLS-1$ + + "\"updated_at\":\"2026-09-0" + number //$NON-NLS-1$ + + "T10:00:00Z\",\"user\":{\"login\":\"Philipp0205\"}}"; //$NON-NLS-1$ + } + + private static Step step(ConnectionDiagnostics report, String name) { + for (Step step : report.getSteps()) { + if (step.getName().equals(name)) { + return step; + } + } + throw new AssertionError( + "No step " + name + " in\n" + report.toReport()); //$NON-NLS-1$ //$NON-NLS-2$ + } +} diff --git a/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearchTest.java b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearchTest.java new file mode 100644 index 0000000..57e5979 --- /dev/null +++ b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearchTest.java @@ -0,0 +1,216 @@ +/******************************************************************************* + * Copyright (C) 2026, Philipp0205 and others + * + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.egit.pullrequest.internal.github; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; +import static org.junit.Assert.fail; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import org.eclipse.egit.pullrequest.internal.github.GitHubPullRequestSearch.PageReader; +import org.eclipse.egit.pullrequest.internal.github.GitHubPullRequestSearch.Result; +import org.junit.Test; + +/** + * Tests for {@link GitHubPullRequestSearch} + */ +public class GitHubPullRequestSearchTest { + + private static final String VALIDATION_FAILED = "GitHub API request " //$NON-NLS-1$ + + "failed: HTTP 422 - {\"message\":\"Validation Failed\"," //$NON-NLS-1$ + + "\"errors\":[{\"message\":\"The listed users and " //$NON-NLS-1$ + + "repositories cannot be searched either because the " //$NON-NLS-1$ + + "resources do not exist or you do not have permission to " //$NON-NLS-1$ + + "view them.\"}]}"; //$NON-NLS-1$ + + private final List requestedQueries = new ArrayList<>(); + + /** + * A reader that answers each query from a canned list of paths and fails + * every query that was registered as rejected. + */ + private class StubReader implements PageReader { + + private final Map> results = new HashMap<>(); + + private final Map failures = new HashMap<>(); + + StubReader returning(String query, String... paths) { + results.put(query, Arrays.asList(paths)); + return this; + } + + StubReader failingWith(String query, String message) { + failures.put(query, new IOException(message)); + return this; + } + + @Override + public List read(String query, int page, int pageSize) + throws IOException { + requestedQueries.add(query); + IOException failure = failures.get(query); + if (failure != null) { + throw failure; + } + if (page > 1) { + return Collections.emptyList(); + } + return results.getOrDefault(query, Collections.emptyList()); + } + } + + @Test + public void testRejectedScopeIsSkippedAndOthersStillReturnResults() + throws Exception { + StubReader reader = new StubReader() + .returning("is:pr user:alice is:open", //$NON-NLS-1$ + "/repos/alice/notes/pulls/1") //$NON-NLS-1$ + .failingWith("is:pr org:acme is:open", VALIDATION_FAILED) //$NON-NLS-1$ + .returning("is:pr repo:bob/tool is:open", //$NON-NLS-1$ + "/repos/bob/tool/pulls/9"); //$NON-NLS-1$ + + Result result = GitHubPullRequestSearch.run( + Arrays.asList("is:pr user:alice is:open", //$NON-NLS-1$ + "is:pr org:acme is:open", //$NON-NLS-1$ + "is:pr repo:bob/tool is:open"), //$NON-NLS-1$ + 1, 1, reader); + + assertThat(result.getPullRequestPaths(), + contains("/repos/alice/notes/pulls/1", //$NON-NLS-1$ + "/repos/bob/tool/pulls/9")); //$NON-NLS-1$ + assertThat(result.getUnsearchableScopes(), contains("org:acme")); //$NON-NLS-1$ + } + + @Test + public void testRejectedScopeDoesNotStopLaterQueries() throws Exception { + StubReader reader = new StubReader() + .failingWith("is:pr user:alice is:open", VALIDATION_FAILED) //$NON-NLS-1$ + .returning("is:pr org:acme is:open", //$NON-NLS-1$ + "/repos/acme/app/pulls/3"); //$NON-NLS-1$ + + GitHubPullRequestSearch.run( + Arrays.asList("is:pr user:alice is:open", //$NON-NLS-1$ + "is:pr org:acme is:open"), //$NON-NLS-1$ + 1, 1, reader); + + assertThat(requestedQueries, hasSize(2)); + } + + @Test + public void testAllScopesRejectedFailsWithActionableMessage() { + StubReader reader = new StubReader() + .failingWith("is:pr user:alice is:open", VALIDATION_FAILED) //$NON-NLS-1$ + .failingWith("is:pr org:acme is:open", VALIDATION_FAILED); //$NON-NLS-1$ + + try { + GitHubPullRequestSearch.run( + Arrays.asList("is:pr user:alice is:open", //$NON-NLS-1$ + "is:pr org:acme is:open"), //$NON-NLS-1$ + 1, 1, reader); + fail("Expected the search to fail"); //$NON-NLS-1$ + } catch (IOException e) { + assertThat(e.getMessage(), containsString("user:alice")); //$NON-NLS-1$ + assertThat(e.getMessage(), containsString("org:acme")); //$NON-NLS-1$ + assertThat(e.getMessage(), containsString("read:org")); //$NON-NLS-1$ + assertThat(e.getCause().getMessage(), + containsString("HTTP 422")); //$NON-NLS-1$ + } + } + + @Test + public void testFailuresOtherThanRejectedScopesArePropagated() { + StubReader reader = new StubReader() + .returning("is:pr user:alice is:open", //$NON-NLS-1$ + "/repos/alice/notes/pulls/1") //$NON-NLS-1$ + .failingWith("is:pr org:acme is:open", //$NON-NLS-1$ + "GitHub API request failed: HTTP 503 - unavailable"); //$NON-NLS-1$ + + try { + GitHubPullRequestSearch.run( + Arrays.asList("is:pr user:alice is:open", //$NON-NLS-1$ + "is:pr org:acme is:open"), //$NON-NLS-1$ + 1, 1, reader); + fail("Expected the search to fail"); //$NON-NLS-1$ + } catch (IOException e) { + assertThat(e.getMessage(), containsString("HTTP 503")); //$NON-NLS-1$ + } + } + + @Test + public void testPullRequestFoundByTwoScopesIsReportedOnce() + throws Exception { + StubReader reader = new StubReader() + .returning("is:pr user:alice is:open", //$NON-NLS-1$ + "/repos/alice/notes/pulls/1") //$NON-NLS-1$ + .returning("is:pr repo:alice/notes is:open", //$NON-NLS-1$ + "/repos/alice/notes/pulls/1"); //$NON-NLS-1$ + + Result result = GitHubPullRequestSearch.run( + Arrays.asList("is:pr user:alice is:open", //$NON-NLS-1$ + "is:pr repo:alice/notes is:open"), //$NON-NLS-1$ + 1, 1, reader); + + assertThat(result.getPullRequestPaths(), + contains("/repos/alice/notes/pulls/1")); //$NON-NLS-1$ + } + + @Test + public void testSuccessfulSearchReportsNoUnsearchableScopes() + throws Exception { + StubReader reader = new StubReader().returning( + "is:pr user:alice is:open", "/repos/alice/notes/pulls/1"); //$NON-NLS-1$ //$NON-NLS-2$ + + Result result = GitHubPullRequestSearch.run( + Arrays.asList("is:pr user:alice is:open"), 1, 1, reader); //$NON-NLS-1$ + + assertThat(result.getUnsearchableScopes(), empty()); + } + + @Test + public void testValidationFailureIsRecognizedAsUnsearchableScope() { + assertThat( + GitHubPullRequestSearch.isUnsearchableScope( + new IOException(VALIDATION_FAILED)), + equalTo(true)); + assertThat( + GitHubPullRequestSearch.isUnsearchableScope(new IOException( + "GitHub API request failed: HTTP 401 - Bad token")), //$NON-NLS-1$ + equalTo(false)); + assertThat(GitHubPullRequestSearch + .isUnsearchableScope(new IOException()), equalTo(false)); + } + + @Test + public void testScopeOfExtractsTheOwnerQualifier() { + assertThat(GitHubSearchQueries.scopeOf("is:pr user:alice is:open"), //$NON-NLS-1$ + equalTo("user:alice")); //$NON-NLS-1$ + assertThat( + GitHubSearchQueries.scopeOf( + "is:pr org:acme author:carol is:merged"), //$NON-NLS-1$ + equalTo("org:acme")); //$NON-NLS-1$ + assertThat(GitHubSearchQueries + .scopeOf("is:pr repo:bob/tool is:closed is:unmerged"), //$NON-NLS-1$ + equalTo("repo:bob/tool")); //$NON-NLS-1$ + assertThat(GitHubSearchQueries.scopeOf("is:pr"), equalTo("is:pr")); //$NON-NLS-1$ //$NON-NLS-2$ + } +} diff --git a/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/StubGitHubServer.java b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/StubGitHubServer.java new file mode 100644 index 0000000..441fc4f --- /dev/null +++ b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/StubGitHubServer.java @@ -0,0 +1,172 @@ +/******************************************************************************* + * Copyright (C) 2026, Philipp0205 and others + * + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.egit.pullrequest.internal.github; + +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStreamReader; +import java.io.OutputStream; +import java.net.InetAddress; +import java.net.ServerSocket; +import java.net.Socket; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; + +/** + * Minimal HTTP server on the loopback interface that answers canned responses, + * used to exercise {@link GitHubClient} without talking to github.com. + *

+ * Responses are registered either for a request path or for a fragment of the + * query string, so that the different searches sent to + * {@code /search/issues} can be answered individually. + */ +class StubGitHubServer implements AutoCloseable { + + /** A canned response. */ + static class Response { + + private final int status; + + private final String reason; + + private final String body; + + Response(int status, String reason, String body) { + this.status = status; + this.reason = reason; + this.body = body; + } + } + + private final ServerSocket serverSocket; + + private final Map byPath = new ConcurrentHashMap<>(); + + private final Map byQuery = new ConcurrentHashMap<>(); + + private final List requestedTargets = new CopyOnWriteArrayList<>(); + + private volatile boolean running = true; + + StubGitHubServer() throws IOException { + serverSocket = new ServerSocket(0, 0, InetAddress.getLoopbackAddress()); + Thread thread = new Thread(this::serve, "stub-github"); //$NON-NLS-1$ + thread.setDaemon(true); + thread.start(); + } + + /** + * @return the API base URL of this server + */ + String url() { + return "http://" //$NON-NLS-1$ + + serverSocket.getInetAddress().getHostAddress() + ':' + + serverSocket.getLocalPort(); + } + + /** + * Registers the response for a path, ignoring any query string. + * + * @param path + * the request path + * @param response + * the response to send + */ + void on(String path, Response response) { + byPath.put(path, response); + } + + /** + * Registers the response for every request whose target contains the given + * fragment. Checked before the path responses. + * + * @param fragment + * a fragment of the encoded request target + * @param response + * the response to send + */ + void onQueryContaining(String fragment, Response response) { + byQuery.put(fragment, response); + } + + /** + * @return the request targets that were received, in order + */ + List requestedTargets() { + return requestedTargets; + } + + @Override + public void close() throws IOException { + running = false; + serverSocket.close(); + } + + private void serve() { + while (running) { + try (Socket client = serverSocket.accept()) { + handle(client); + } catch (IOException e) { + return; + } + } + } + + private void handle(Socket client) throws IOException { + BufferedReader in = new BufferedReader(new InputStreamReader( + client.getInputStream(), StandardCharsets.UTF_8)); + String requestLine = in.readLine(); + if (requestLine == null) { + return; + } + String line; + while ((line = in.readLine()) != null && !line.isEmpty()) { + // Headers are not inspected, but must be drained. + } + String[] parts = requestLine.split(" "); //$NON-NLS-1$ + String target = parts.length > 1 ? parts[1] : ""; //$NON-NLS-1$ + requestedTargets.add(target); + write(client.getOutputStream(), responseFor(target)); + } + + private Response responseFor(String target) { + for (Map.Entry entry : byQuery.entrySet()) { + if (target.contains(entry.getKey())) { + return entry.getValue(); + } + } + int query = target.indexOf('?'); + String path = query < 0 ? target : target.substring(0, query); + Response response = byPath.get(path); + if (response != null) { + return response; + } + return new Response(404, "Not Found", //$NON-NLS-1$ + "{\"message\":\"Not Found\"}"); //$NON-NLS-1$ + } + + private void write(OutputStream out, Response response) + throws IOException { + byte[] body = response.body.getBytes(StandardCharsets.UTF_8); + StringBuilder head = new StringBuilder(); + head.append("HTTP/1.1 ").append(response.status).append(' ') //$NON-NLS-1$ + .append(response.reason).append("\r\n"); //$NON-NLS-1$ + head.append("Content-Type: application/json\r\n"); //$NON-NLS-1$ + head.append("Content-Length: ").append(body.length).append("\r\n"); //$NON-NLS-1$ //$NON-NLS-2$ + head.append("Connection: close\r\n\r\n"); //$NON-NLS-1$ + out.write(head.toString().getBytes(StandardCharsets.UTF_8)); + out.write(body); + out.flush(); + } +} diff --git a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubClient.java b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubClient.java index 3280fea..66837c4 100644 --- a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubClient.java +++ b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubClient.java @@ -11,7 +11,6 @@ import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Comparator; -import java.util.LinkedHashSet; import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -20,6 +19,8 @@ import org.eclipse.egit.pullrequest.internal.model.PullRequest; import org.eclipse.egit.pullrequest.internal.model.PullRequestComment; import org.eclipse.egit.pullrequest.internal.model.PullRequestCommit; +import org.eclipse.egit.pullrequest.internal.client.ConnectionDiagnostics; +import org.eclipse.egit.pullrequest.internal.client.ConnectionDiagnostics.Outcome; import org.eclipse.egit.pullrequest.internal.client.IPullRequestClient; import org.eclipse.egit.pullrequest.internal.client.PullRequestProviderCapabilities; import org.eclipse.egit.pullrequest.Activator; @@ -34,8 +35,6 @@ public class GitHubClient implements IPullRequestClient { private static final String API_BASE_URL = "https://api.github.com"; //$NON-NLS-1$ - private static final String GRAPHQL_URL = "https://api.github.com/graphql"; //$NON-NLS-1$ - private static final int DEFAULT_TIMEOUT = 30000; // 30 seconds private String owner; @@ -44,8 +43,16 @@ public class GitHubClient implements IPullRequestClient { private final String token; + private final String apiBaseUrl; + private final PullRequestProviderCapabilities capabilities; + /** + * Owner scopes the last token-scoped listing had to skip because GitHub + * refused to search them. Reported by {@link #diagnoseConnection()}. + */ + private List unsearchableScopes = List.of(); + /** * Creates a new GitHub client * @@ -58,11 +65,7 @@ public class GitHubClient implements IPullRequestClient { */ public GitHubClient(@NonNull String owner, @NonNull String repo, @NonNull String token) { - this.owner = owner; - this.repo = repo; - this.token = token; - this.capabilities = PullRequestProviderCapabilities - .forProvider(PullRequestProviderType.GITHUB); + this(owner, repo, token, API_BASE_URL); } /** @@ -73,9 +76,28 @@ public GitHubClient(@NonNull String owner, @NonNull String repo, * the GitHub access token */ public GitHubClient(@NonNull String token) { - this.owner = null; - this.repo = null; + this(null, null, token, API_BASE_URL); + } + + /** + * Creates a client against a specific GitHub REST API endpoint. + * + * @param owner + * the repository owner, or {@code null} to list pull requests + * from every accessible repository + * @param repo + * the repository name, or {@code null} + * @param token + * the GitHub access token + * @param apiBaseUrl + * the REST API base URL, without a trailing slash + */ + GitHubClient(String owner, String repo, @NonNull String token, + @NonNull String apiBaseUrl) { + this.owner = owner; + this.repo = repo; this.token = token; + this.apiBaseUrl = apiBaseUrl; this.capabilities = PullRequestProviderCapabilities .forProvider(PullRequestProviderType.GITHUB); } @@ -205,32 +227,15 @@ private List getUserPullRequests(String state, int needed = Math.max(1, start + Math.max(1, limit)); int pageSize = Math.min(100, needed); - LinkedHashSet pullRequestPaths = new LinkedHashSet<>(); - for (String query : queries) { - int collected = 0; - int page = 1; - while (collected < needed) { - String path = "/search/issues?q=" //$NON-NLS-1$ - + URLEncoder.encode(query, StandardCharsets.UTF_8) - + "&per_page=" + pageSize //$NON-NLS-1$ - + "&page=" + page //$NON-NLS-1$ - + "&sort=updated&order=desc"; //$NON-NLS-1$ - List pagePaths = GitHubJsonParser - .parseSearchPullRequestPaths(doGet(path)); - if (pagePaths.isEmpty()) { - break; - } - pullRequestPaths.addAll(pagePaths); - collected += pagePaths.size(); - if (pagePaths.size() < pageSize) { - break; - } - page++; - } - } + GitHubPullRequestSearch.Result search = GitHubPullRequestSearch.run( + queries, needed, pageSize, + (query, page, size) -> GitHubJsonParser + .parseSearchPullRequestPaths( + doGet(searchPath(query, page, size)))); + unsearchableScopes = search.getUnsearchableScopes(); List result = new ArrayList<>(); - for (String pullRequestPath : pullRequestPaths) { + for (String pullRequestPath : search.getPullRequestPaths()) { PullRequest pullRequest = GitHubJsonParser .parseSinglePullRequest(doGet(pullRequestPath)); if (pullRequest != null) { @@ -244,19 +249,56 @@ private List getUserPullRequests(String state, return new ArrayList<>(result.subList(from, to)); } - private List listOrganizationLogins() throws IOException { + private static String searchPath(String query, int page, int pageSize) { + return "/search/issues?q=" //$NON-NLS-1$ + + URLEncoder.encode(query, StandardCharsets.UTF_8) + + "&per_page=" + pageSize //$NON-NLS-1$ + + "&page=" + page //$NON-NLS-1$ + + "&sort=updated&order=desc"; //$NON-NLS-1$ + } + + /** + * Lists the organizations of the authenticated user. + *

+ * A token without the {@code read:org} scope cannot read them. That only + * narrows the set of searched scopes, so it is reported as a warning + * instead of failing the whole listing. + * + * @return the organization logins, empty if they cannot be read + */ + private List listOrganizationLogins() { List logins = new ArrayList<>(); - for (String page : doGetAllPages("/user/orgs?per_page=100")) { //$NON-NLS-1$ - logins.addAll(GitHubJsonParser.parseLogins(page)); + try { + for (String page : doGetAllPages("/user/orgs?per_page=100")) { //$NON-NLS-1$ + logins.addAll(GitHubJsonParser.parseLogins(page)); + } + } catch (IOException e) { + Activator.logWarning( + "Cannot list GitHub organizations; their pull requests " //$NON-NLS-1$ + + "are not listed. " + e.getMessage()); //$NON-NLS-1$ } return logins; } - private List listCollaboratorRepositories() throws IOException { + /** + * Lists the repositories the authenticated user collaborates on. + * + * @return the {@code owner/name} repositories, empty if they cannot be + * read + */ + private List listCollaboratorRepositories() { List fullNames = new ArrayList<>(); - for (String page : doGetAllPages( - "/user/repos?affiliation=collaborator&per_page=100")) { //$NON-NLS-1$ - fullNames.addAll(GitHubJsonParser.parseRepositoryFullNames(page)); + try { + for (String page : doGetAllPages( + "/user/repos?affiliation=collaborator&per_page=100")) { //$NON-NLS-1$ + fullNames.addAll( + GitHubJsonParser.parseRepositoryFullNames(page)); + } + } catch (IOException e) { + Activator.logWarning( + "Cannot list GitHub collaborator repositories; their " //$NON-NLS-1$ + + "pull requests are not listed. " //$NON-NLS-1$ + + e.getMessage()); } return fullNames; } @@ -816,6 +858,20 @@ public boolean testConnection() { return GitHubJsonParser.parseCurrentUser(json); } + @Override + public @NonNull ConnectionDiagnostics diagnoseConnection() { + ConnectionDiagnostics report = IPullRequestClient.super + .diagnoseConnection(); + if (!unsearchableScopes.isEmpty()) { + report.add("Search scopes", Outcome.WARNING, //$NON-NLS-1$ + "GitHub refused to search " //$NON-NLS-1$ + + String.join(", ", unsearchableScopes) //$NON-NLS-1$ + + ".\n" //$NON-NLS-1$ + + GitHubPullRequestSearch.TOKEN_SCOPE_HINT); + } + return report; + } + @Override public @NonNull List getPullRequestCommits( long pullRequestId) throws IOException { @@ -881,7 +937,7 @@ private String doGet(String path) throws IOException { */ private List doGetAllPages(String path) throws IOException { List pages = new ArrayList<>(); - String nextUrl = API_BASE_URL + path; + String nextUrl = apiBaseUrl + path; while (nextUrl != null) { HttpURLConnection conn = null; @@ -1134,7 +1190,7 @@ private String executeRequest(String path, String method, private String executeGraphQL(String query) throws IOException { HttpURLConnection conn = null; try { - URL url = new URL(GRAPHQL_URL); + URL url = new URL(apiBaseUrl + "/graphql"); //$NON-NLS-1$ conn = (HttpURLConnection) url.openConnection(); conn.setRequestMethod("POST"); //$NON-NLS-1$ conn.setConnectTimeout(DEFAULT_TIMEOUT); @@ -1184,7 +1240,7 @@ private String executeGraphQL(String query) throws IOException { */ private HttpURLConnection createConnection(String path, String method) throws IOException { - URL url = new URL(API_BASE_URL + path); + URL url = new URL(apiBaseUrl + path); HttpURLConnection conn = (HttpURLConnection) url.openConnection(); // HttpURLConnection doesn't support PATCH by default in Java diff --git a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubJsonParser.java b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubJsonParser.java index fbd8305..e00e653 100644 --- a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubJsonParser.java +++ b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubJsonParser.java @@ -26,7 +26,7 @@ class GitHubJsonParser { private static final Pattern SEARCH_PULL_URL = Pattern.compile( "\"pull_request\"\\s*:\\s*\\{[^}]*\"url\"\\s*:\\s*" //$NON-NLS-1$ - + "\"https://api\\.github\\.com([^\"?]+)\"", //$NON-NLS-1$ + + "\"https?://[^\"]+?(/repos/[^\"?]+)\"", //$NON-NLS-1$ Pattern.DOTALL); /** diff --git a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearch.java b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearch.java new file mode 100644 index 0000000..4e01e50 --- /dev/null +++ b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearch.java @@ -0,0 +1,186 @@ +/******************************************************************************* + * Copyright (C) 2026, Philipp0205 and others + * + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.egit.pullrequest.internal.github; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashSet; +import java.util.List; + +import org.eclipse.egit.pullrequest.Activator; + +/** + * Runs one GitHub issue search per owner scope and merges the results. + *

+ * GitHub answers {@code GET /search/issues} with HTTP 422 as soon as a single + * qualifier in the query names a user, organization or repository the access + * token may not search. That happens for organizations the token was never + * SSO-authorized for, for private repositories when the token lacks the + * {@code repo} scope, and for repositories that were renamed or deleted after + * they were listed. Because every scope is searched with its own query, such a + * scope is skipped instead of failing the whole listing; only when GitHub + * rejects every scope is the failure reported to the caller. + */ +final class GitHubPullRequestSearch { + + /** Advice shown whenever GitHub refuses to search an owner scope. */ + static final String TOKEN_SCOPE_HINT = "Use a classic personal access " //$NON-NLS-1$ + + "token with the 'repo' and 'read:org' scopes, and authorize " //$NON-NLS-1$ + + "it for every organization that enforces SAML single " //$NON-NLS-1$ + + "sign-on. Fine-grained tokens can only search the " //$NON-NLS-1$ + + "repositories they were granted."; //$NON-NLS-1$ + + private GitHubPullRequestSearch() { + // No instantiation + } + + /** + * Reads one page of pull request paths for a single search query. + */ + @FunctionalInterface + interface PageReader { + + /** + * Runs one search request. + * + * @param query + * the search query + * @param page + * the one-based page number + * @param pageSize + * the number of results per page + * @return REST pull request paths in result order + * @throws IOException + * if the request fails + */ + List read(String query, int page, int pageSize) + throws IOException; + } + + /** Merged outcome of searching every owner scope. */ + static final class Result { + + private final List pullRequestPaths; + + private final List unsearchableScopes; + + Result(List pullRequestPaths, + List unsearchableScopes) { + this.pullRequestPaths = pullRequestPaths; + this.unsearchableScopes = unsearchableScopes; + } + + /** + * @return REST pull request paths without duplicates + */ + List getPullRequestPaths() { + return pullRequestPaths; + } + + /** + * @return the owner qualifiers GitHub refused to search + */ + List getUnsearchableScopes() { + return unsearchableScopes; + } + } + + /** + * Searches every query and merges the pull requests they return. + * + * @param queries + * one search query per owner scope + * @param needed + * how many results are needed per scope + * @param pageSize + * how many results to request per page + * @param reader + * runs the individual search requests + * @return the merged result + * @throws IOException + * if a search fails for any reason other than an unsearchable + * scope, or if GitHub refuses to search every scope + */ + static Result run(List queries, int needed, int pageSize, + PageReader reader) throws IOException { + LinkedHashSet paths = new LinkedHashSet<>(); + List unsearchable = new ArrayList<>(); + IOException lastRejection = null; + for (String query : queries) { + try { + collect(query, needed, pageSize, reader, paths); + } catch (IOException e) { + if (!isUnsearchableScope(e)) { + throw e; + } + String scope = GitHubSearchQueries.scopeOf(query); + unsearchable.add(scope); + lastRejection = e; + Activator.logWarning( + "GitHub cannot search " + scope //$NON-NLS-1$ + + "; its pull requests are not listed. " //$NON-NLS-1$ + + e.getMessage()); + } + } + if (lastRejection != null && unsearchable.size() == queries.size()) { + throw new IOException(allScopesRejected(unsearchable), + lastRejection); + } + return new Result(new ArrayList<>(paths), + Collections.unmodifiableList(unsearchable)); + } + + private static void collect(String query, int needed, int pageSize, + PageReader reader, LinkedHashSet paths) + throws IOException { + int collected = 0; + int page = 1; + while (collected < needed) { + List pagePaths = reader.read(query, page, pageSize); + if (pagePaths.isEmpty()) { + return; + } + paths.addAll(pagePaths); + collected += pagePaths.size(); + if (pagePaths.size() < pageSize) { + return; + } + page++; + } + } + + /** + * Tells whether GitHub rejected the query because one of its qualifiers + * names something the token may not search. + * + * @param e + * the failure reported by the search request + * @return true if the scope should be skipped rather than propagated + */ + static boolean isUnsearchableScope(IOException e) { + String message = e.getMessage(); + return message != null && message.contains("HTTP 422"); //$NON-NLS-1$ + } + + /** + * Builds the message for the case where no scope could be searched at all. + * + * @param unsearchableScopes + * the owner qualifiers GitHub refused to search + * @return an actionable error message + */ + static String allScopesRejected(List unsearchableScopes) { + return "GitHub refused to search " //$NON-NLS-1$ + + String.join(", ", unsearchableScopes) //$NON-NLS-1$ + + ". " + TOKEN_SCOPE_HINT; //$NON-NLS-1$ + } +} diff --git a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchQueries.java b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchQueries.java index 8708de8..55bdcdc 100644 --- a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchQueries.java +++ b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchQueries.java @@ -96,6 +96,29 @@ static String pullRequestQuery(String ownerQualifier, String state, return query.toString(); } + /** + * Returns the owner qualifier a query was built for. + * + * @param query + * a query produced by + * {@link #pullRequestQuery(String, String, String)} + * @return the {@code user:}, {@code org:} or {@code repo:} qualifier, or + * the whole query if it carries none + */ + static String scopeOf(String query) { + if (query == null) { + return ""; //$NON-NLS-1$ + } + for (String token : query.split(" ")) { //$NON-NLS-1$ + if (token.startsWith("user:") //$NON-NLS-1$ + || token.startsWith("org:") //$NON-NLS-1$ + || token.startsWith("repo:")) { //$NON-NLS-1$ + return token; + } + } + return query; + } + static boolean isCoveredByUserOrOrg(String fullName, String login, Set organizations) { if (fullName == null || fullName.isBlank()) { diff --git a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/ui/PullRequestPreferencePage.java b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/ui/PullRequestPreferencePage.java index 2c0c404..6e4739b 100644 --- a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/ui/PullRequestPreferencePage.java +++ b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/ui/PullRequestPreferencePage.java @@ -230,10 +230,13 @@ private Composite createGitHubConfiguration(Composite parent) { Label infoLabel = new Label(group, SWT.WRAP); infoLabel.setText( "Authenticate as a GitHub user with a personal access token " //$NON-NLS-1$ - + "(classic) that has the 'repo' scope:\n" //$NON-NLS-1$ + + "(classic) that has the 'repo' and 'read:org' " //$NON-NLS-1$ + + "scopes:\n" //$NON-NLS-1$ + "https://github.com/settings/tokens\n\n" //$NON-NLS-1$ + "The plugin lists pull requests from every repository " //$NON-NLS-1$ - + "you own or belong to."); //$NON-NLS-1$ + + "you own or belong to. Organizations that enforce " //$NON-NLS-1$ + + "SAML single sign-on are only searched once the token " //$NON-NLS-1$ + + "is authorized for them."); //$NON-NLS-1$ GridDataFactory.fillDefaults().span(2, 1).hint(400, SWT.DEFAULT) .indent(0, 5).applyTo(infoLabel);