diff --git a/GitIntegration.Test/Hosting/GitHubProviderTests.cs b/GitIntegration.Test/Hosting/GitHubProviderTests.cs index e27d013..4f2d311 100644 --- a/GitIntegration.Test/Hosting/GitHubProviderTests.cs +++ b/GitIntegration.Test/Hosting/GitHubProviderTests.cs @@ -85,13 +85,34 @@ private static string SingleRepositoryArray(string name) => [{"id":90000001,"name":"{{name}}","html_url":"https://github.com/example-user/example-repo","clone_url":"https://github.com/example-user/example-repo.git"}] """; + /// + /// Builds the response GET /users/{login} and GET /user return, carrying only the + /// two fields 's route selection reads. + /// + /// + /// Written inline for the reason is: the routing decision + /// reads type and login and nothing else, and a captured account payload would + /// carry thirty fields whose presence no assertion depends on, inviting a later reader to wonder + /// which of them mattered. + /// + /// The value to send as the account's login field. + /// The value to send as the account's type field — GitHub sends User or Organization. + private static string AccountPayload(string login, string type) => + $$""" + {"id":90000002,"login":"{{login}}","type":"{{type}}"} + """; + [TestMethod] public async Task AppliesATokenCredentialToTheRequestAsync() { PersonaGUID persona = CredentialCache.CreatePersonaGUID(); CredentialCache.Instance.AddOrReplace(persona, new CredentialWithToken { Token = "ghp_abc123".As() }); + // Two responses because a credential is supplied: an authenticated enumeration establishes the + // owner's type before choosing a route. The assertion is on the first request either way — the + // credential has to reach every request this provider issues, probe included. using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .Respond(HttpStatusCode.OK, AccountPayload("contoso", "Organization"), ("Content-Type", "application/json")) .Respond(HttpStatusCode.OK, Fixture("github-repositories.json"), ("Content-Type", "application/json")); GitHubProvider provider = new() { Owner = "contoso".As(), PersonaGUID = persona, Handler = handler }; @@ -109,6 +130,7 @@ public async Task SendsBearerAuthForABearerTokenCredentialAsync() // scheme for each: "Token" for a PAT, "Bearer" for a JWT such as a GitHub App token. Proves // the BearerToken kind reaches the Octokit client as Bearer rather than collapsing to Token. using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .Respond(HttpStatusCode.OK, AccountPayload("contoso", "Organization"), ("Content-Type", "application/json")) .Respond(HttpStatusCode.OK, Fixture("github-repositories.json"), ("Content-Type", "application/json")); GitHubProvider provider = new() { @@ -133,9 +155,9 @@ public async Task EnumeratesRepositoriesForTheOwnerAsync() await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); // The route this provider calls is a documented part of its contract, not an Octokit detail: - // GET /users/{login}/repos is exactly why GetRepositoriesAsync returns public repositories - // only, and GET /user/repos would silently return the token's own repositories instead of the - // configured owner's. Nothing else in the suite pins it. + // GET /users/{login}/repos is the owner-honouring route that needs no credential, and + // GET /user/repos would silently return the token's own repositories instead of the configured + // owner's. Nothing else in the suite pins it for the unauthenticated case. Assert.AreEqual("/users/contoso/repos", handler.Requests[0].Uri.AbsolutePath); // Asserted on the parsed fields of the first two entries, against the fixture's real @@ -151,6 +173,151 @@ public async Task EnumeratesRepositoriesForTheOwnerAsync() Assert.AreEqual("https://github.com/example-user/example-repo-2".As(), repositories[1].WebURI); } + [TestMethod] + public async Task IssuesNoOwnerTypeProbeWithoutACredentialAsync() + { + // The unauthenticated enumeration is one request, not three. Every route this provider can + // choose between collapses to the same public answer without a credential, so probing which + // one to ask for would spend two requests distinguishing identical answers. Asserted as an + // exact count rather than on the one path, because a probe that ran and was ignored would + // leave that path assertion passing. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .RespondToPath("/users/contoso/repos", HttpStatusCode.OK, SingleRepositoryArray("public-repo"), ("Content-Type", "application/json")); + GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + + IReadOnlyList repositories = + await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.HasCount(1, handler.Requests); + Assert.AreEqual("public-repo".As(), repositories[0].Name); + } + + [TestMethod] + public async Task EnumeratesAnOrganisationOnTheOrgRouteWhenAuthenticatedAsync() + { + // The case this whole route selection exists for. GET /users/{login}/repos is public-only even + // with a token, so an organisation's private repositories were invisible to a credential that + // could plainly see them — the divergence from AzureDevOpsProvider, which reports everything + // its token reaches, under one interface that promises the same coverage of both. + // + // RespondToPath rather than an assertion on the recorded URI: a scripted success returned + // regardless of route would let the wrong route parse and map a body GitHub would never have + // sent it, and the test would pass on a provider that still asks the public endpoint. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .RespondToPath("/users/contoso", HttpStatusCode.OK, AccountPayload("contoso", "Organization"), ("Content-Type", "application/json")) + .RespondToPath("/orgs/contoso/repos", HttpStatusCode.OK, SingleRepositoryArray("private-repo"), ("Content-Type", "application/json")); + GitHubProvider provider = new() + { + Owner = "contoso".As(), + Handler = handler, + CredentialSource = () => HostingCredential.FromToken("ghp_abc123"), + }; + + IReadOnlyList repositories = + await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.HasCount(2, handler.Requests); + Assert.AreEqual("/users/contoso", handler.Requests[0].Uri.AbsolutePath); + Assert.AreEqual("/orgs/contoso/repos", handler.Requests[1].Uri.AbsolutePath); + + // The repository is named for the route that produced it: the public route is scripted to + // answer nothing here, so this name can only have arrived through /orgs/contoso/repos. + Assert.AreEqual("private-repo".As(), repositories[0].Name); + } + + [TestMethod] + public async Task EnumeratesTheCredentialsOwnAccountOnTheCurrentUserRouteAsync() + { + // A user owner that is the credential's own account. GET /user/repos is the only route that + // reveals that account's private repositories, and it is reachable only once GET /user has + // confirmed the configured owner IS that account — the endpoint describes the token's own + // repositories regardless of which owner was configured, so reaching it any earlier would + // stop honouring Owner. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + // Scripted at the owner's own casing, because that is the spelling this provider sends: + // GitHub resolves a login case-insensitively, the recorded path is compared exactly, and + // nothing here canonicalises Owner before putting it in a URI. + .RespondToPath("/users/Octocat", HttpStatusCode.OK, AccountPayload("octocat", "User"), ("Content-Type", "application/json")) + .RespondToPath("/user", HttpStatusCode.OK, AccountPayload("octocat", "User"), ("Content-Type", "application/json")) + .RespondToPath("/user/repos", HttpStatusCode.OK, SingleRepositoryArray("private-repo"), ("Content-Type", "application/json")); + GitHubProvider provider = new() + { + // Deliberately cased differently from the login GET /user reports. GitHub logins are + // unique case-insensitively, so "Octocat" and "octocat" name one account, and an ordinal + // comparison here would demote the owner's own repositories to the public-only route. + Owner = "Octocat".As(), + Handler = handler, + CredentialSource = () => HostingCredential.FromToken("ghp_abc123"), + }; + + IReadOnlyList repositories = + await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.HasCount(3, handler.Requests); + Assert.AreEqual("/user/repos", handler.Requests[2].Uri.AbsolutePath); + + // Owner affiliation, not the unfiltered default: GET /user/repos with no affiliation also + // returns repositories the account merely collaborates on or reaches through an organisation, + // which would report another owner's work under this owner's name. + Assert.AreEqual("?affiliation=owner", handler.Requests[2].Uri.Query); + + Assert.AreEqual("private-repo".As(), repositories[0].Name); + } + + [TestMethod] + public async Task FallsBackToThePublicRouteForAUserOtherThanTheCredentialsOwnAsync() + { + // The one case where the public-only limit is real rather than a defect: GitHub publishes no + // authenticated route that honours an arbitrary user owner. Worth pinning, because the + // tempting shortcut — sending GET /user/repos anyway — would answer with the token holder's + // own repositories under someone else's name. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .RespondToPath("/users/someone-else", HttpStatusCode.OK, AccountPayload("someone-else", "User"), ("Content-Type", "application/json")) + .RespondToPath("/user", HttpStatusCode.OK, AccountPayload("octocat", "User"), ("Content-Type", "application/json")) + .RespondToPath("/users/someone-else/repos", HttpStatusCode.OK, SingleRepositoryArray("public-repo"), ("Content-Type", "application/json")); + GitHubProvider provider = new() + { + Owner = "someone-else".As(), + Handler = handler, + CredentialSource = () => HostingCredential.FromToken("ghp_abc123"), + }; + + IReadOnlyList repositories = + await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.HasCount(3, handler.Requests); + Assert.AreEqual("/users/someone-else/repos", handler.Requests[2].Uri.AbsolutePath); + Assert.AreEqual("public-repo".As(), repositories[0].Name); + } + + [TestMethod] + public async Task FallsBackToThePublicRouteWhenTheCredentialHasNoUserIdentityAsync() + { + // A GitHub App installation token authenticates but belongs to no user, and GitHub answers its + // GET /user with 403. That must not become a thrown GitHostingAuthenticationException: such a + // credential enumerated a user owner's public repositories perfectly well before any of this + // routing existed, and turning a working call into a failure is a worse regression than the + // under-reporting the routing was added to fix. A 401 is deliberately not covered here — + // a credential GitHub rejects outright still propagates, and + // TranslatesAnUnauthorizedResponseToGitHostingAuthenticationExceptionAsync pins that. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .RespondToPath("/users/octocat", HttpStatusCode.OK, AccountPayload("octocat", "User"), ("Content-Type", "application/json")) + .RespondToPath("/user", HttpStatusCode.Forbidden, "{\"message\":\"Resource not accessible by integration\"}", ("Content-Type", "application/json")) + .RespondToPath("/users/octocat/repos", HttpStatusCode.OK, SingleRepositoryArray("public-repo"), ("Content-Type", "application/json")); + GitHubProvider provider = new() + { + Owner = "octocat".As(), + Handler = handler, + CredentialSource = () => HostingCredential.FromBearerToken("eyJ0eXAiOiJKV1Qi"), + }; + + IReadOnlyList repositories = + await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual("/users/octocat/repos", handler.Requests[2].Uri.AbsolutePath); + Assert.AreEqual("public-repo".As(), repositories[0].Name); + } + [TestMethod] public async Task ReportsNoLocalPathForAnEnumeratedRepositoryAsync() { diff --git a/GitIntegration/GitHubProvider.cs b/GitIntegration/GitHubProvider.cs index ade82c1..0f03d18 100644 --- a/GitIntegration/GitHubProvider.cs +++ b/GitIntegration/GitHubProvider.cs @@ -66,21 +66,24 @@ public sealed class GitHubProvider : GitProvider /// /// /// Adds to the interface's remarks rather than restating them. - /// says only that implementations differ - /// in coverage and points here for this host's specifics, so the two texts have one job each and - /// neither is a copy of the other. An edit that moves this explanation must leave that pointer - /// aimed somewhere real. + /// states the coverage every provider + /// promises and points here for the routes this host reaches it by, so the two texts have one + /// job each and neither is a copy of the other. An edit that moves this explanation must leave + /// that pointer aimed somewhere real. /// - /// Calls GitHub's GET /users/{login}/repos, which returns only 's - /// public repositories. Supplying a token does not widen this: that endpoint does not - /// honour authentication to reveal private repositories the way GET /user/repos would for - /// the token's own account, and switching to that endpoint would silently stop honouring - /// — it always describes the token's own repositories, regardless - /// of which owner was configured, which would break callers who name someone else's owner on - /// purpose. A caller that needs private repositories for a specific owner has no equivalent - /// through this provider today; do not assume this method's coverage matches an - /// AzureDevOpsProvider equivalent, whose token can see everything it has access to under - /// the same interface. + /// GitHub has no single endpoint that both honours and reveals + /// the private repositories a credential can see, so the route is chosen from what the owner is. + /// GET /orgs/{org}/repos does both for an organisation. GET /user/repos does both + /// for the credential's own account, but only for that account — it always describes the token's + /// own repositories regardless of which owner was configured, so it is reached only once the + /// configured owner has been confirmed to be that account. GET /users/{login}/repos, + /// which this method used to be alone in calling, honours any owner but is public-only, and + /// remains the answer for a user who is not the credential's own: that is genuinely all GitHub + /// offers there. + /// + /// + /// Which one applies is established by , whose remarks + /// carry the per-branch reasoning and the cost each branch pays in requests. /// /// public override async Task> GetRepositoriesAsync(CancellationToken cancellationToken = default) @@ -92,7 +95,7 @@ public override async Task> GetRepositoriesAsync(Ca try { - IReadOnlyList repositories = await client.Repository.GetAllForUser(Owner.WeakString).ConfigureAwait(false); + IReadOnlyList repositories = await GetOwnerRepositoriesAsync(client).ConfigureAwait(false); return [.. repositories.Select(ToGitRepository)]; } catch (ApiException exception) @@ -101,6 +104,98 @@ public override async Task> GetRepositoriesAsync(Ca } } + /// + /// Lists 's repositories on whichever route reaches the widest + /// set this provider's credential is entitled to. + /// + /// + /// + /// An unauthenticated provider skips both probes below and calls the public route directly. This + /// is not a shortcut taken for speed: without a credential every route collapses to the same + /// public answer — GET /orgs/{org}/repos and GET /user/repos reveal a private + /// repository only to a credential entitled to it — so the probes would spend requests + /// establishing which of three identical answers to ask for. + /// + /// + /// Authenticated, the owner's type decides the route, and it is read from + /// GET /users/{login} rather than inferred from GET /orgs/{login}/repos answering + /// 404. The inference is the cheaper probe and the wrong one: a token without + /// read:org, or one not authorised for an organisation that enforces SSO, is answered + /// 404 by that route for an organisation that plainly exists, and the inference would + /// quietly demote it to the public-only user route — reinstating the exact under-reporting this + /// method exists to remove, under a condition nothing would report. GET /users/{login} is + /// a public endpoint whose type no credential's scope can change, and its 404 says + /// the owner does not exist, which surfaces as instead + /// of an empty list. + /// + /// + /// A user owner then costs a second probe, GET /user, because GET /user/repos is + /// correct only when the configured owner is the credential's own account, and nothing + /// short of asking establishes that. A credential that has no user to report — a GitHub App + /// installation token, whose GET /user is answered 403 — is not a failure to + /// surface: it means only that this branch cannot apply, and the public route is what such a + /// credential could reach anyway, so the enumeration continues there rather than throwing where + /// it previously succeeded. A 401 is left to propagate, because a credential GitHub + /// rejects outright is a failure the caller has to see. + /// + /// + /// The client this call's transport is wired to. + /// The repositories GitHub reported on the chosen route. + private async Task> GetOwnerRepositoriesAsync(GitHubClient client) + { + string owner = Owner.WeakString; + + if (!IsAuthenticated) + { + return await client.Repository.GetAllForUser(owner).ConfigureAwait(false); + } + + User account = await client.User.Get(owner).ConfigureAwait(false); + + if (IsOrganisation(account)) + { + return await client.Repository.GetAllForOrg(owner).ConfigureAwait(false); + } + + string? authenticatedLogin; + + try + { + authenticatedLogin = (await client.User.Current().ConfigureAwait(false)).Login; + } + catch (ForbiddenException) + { + authenticatedLogin = null; + } + + // Ordinal-ignore-case, because GitHub logins are unique case-insensitively and a caller who + // configured "Octocat" for the account GitHub reports as "octocat" named the same account. + return string.Equals(authenticatedLogin, owner, StringComparison.OrdinalIgnoreCase) + // Affiliation rather than the default: GetAllForCurrent() unfiltered also returns + // repositories the account merely collaborates on or reaches through an organisation, + // which are not Owner's repositories and would report a different owner's work under + // this owner's name. Owner is the affiliation that makes this route mean what + // GET /users/{login}/repos means, minus the public-only limit. + ? await client.Repository.GetAllForCurrent(new RepositoryRequest { Affiliation = RepositoryAffiliation.Owner }).ConfigureAwait(false) + : await client.Repository.GetAllForUser(owner).ConfigureAwait(false); + } + + /// + /// Reports whether an account GitHub described is an organisation. + /// + /// + /// Asks whether GitHub said "organisation" rather than whether it said "user", so every other + /// answer routes to the user branches: is nullable and + /// already carries and + /// alongside the two this decision is really about. An + /// omitted type, and an account kind GitHub adds later, both land on the route this + /// method's caller took before any of these branches existed, which is the answer that cannot be + /// wrong about coverage — only conservative about it. + /// + /// The account GET /users/{login} reported. + /// when GitHub called the account an organisation; otherwise, . + private static bool IsOrganisation(User account) => account.Type == AccountType.Organization; + /// internal override async Task> GetPullRequestsCoreAsync(GitRepositoryAddress repositoryAddress, CancellationToken cancellationToken) { diff --git a/GitIntegration/Hosting/IGitHostingProvider.cs b/GitIntegration/Hosting/IGitHostingProvider.cs index efc3242..077ac29 100644 --- a/GitIntegration/Hosting/IGitHostingProvider.cs +++ b/GitIntegration/Hosting/IGitHostingProvider.cs @@ -55,12 +55,23 @@ public interface IGitHostingProvider /// Retrieves the repositories this provider's owner has, from the remote service. /// /// - /// Implementations are not guaranteed to agree on exactly which of the owner's repositories this - /// returns — each host's own API shapes that. GitHub's implementation, in particular, returns - /// only the owner's public repositories, even when an authenticated credential is - /// supplied; see for why. A caller that needs a - /// specific host's exact coverage should consult that provider's own remarks rather than assume - /// parity across hosts. + /// + /// Every implementation returns the owner's repositories this provider's credential is entitled + /// to see, private ones included, and falls back to the owner's public repositories only where + /// the host publishes no owner-scoped route that a credential widens. That is the contract + /// callers may rely on, and it is stated here rather than left to each provider to describe its + /// own behaviour: a caller who has to read two providers' remarks and work out what they have in + /// common is a caller who will get it wrong. + /// + /// + /// The fallback is not hypothetical, and where it applies is worth naming. + /// never takes it — its enumeration is entitlement-scoped + /// throughout. takes it in exactly one case: an owner that is a + /// GitHub user other than the credential's own account, for which GitHub publishes no + /// authenticated owner-scoped route at all. An organisation owner, and the credential's own user + /// account, both reach the full entitled set. See + /// for the routes that produces. + /// /// /// /// A token to cancel the request. Every provider checks it before issuing a request. Whether it