From d903830b5f72a4113eac479ffd9763b8ed1c939d Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 21:27:45 +0000 Subject: [PATCH 1/2] fix: scan each owner with its own credentials, not the previous owner's [patch] GitHubClient.Credentials is one mutable property on a client shared by every owner in the scan, and ScanRemoteAccountsForRepos only assigned it when the owner had a PAT or a global login existed. An owner with neither left the previous owner's credentials in place and was scanned as them. The concrete case: a personal PAT is configured for owner A, and owner B is added with no credentials to browse publicly. B is then scanned as A, so B's own private repositories are missed, and anything that genuinely needed B's auth answers with an ApiException that the caller swallows -- leaving the repositories missing with no indication why. Pull the choice out into ChooseCredentials, which is total: the owner's own token, else the global login, else Credentials.Anonymous. The caller assigns its result for every owner, so nothing can carry over. A global login missing its token, or a token missing its login, now falls through to anonymous rather than reaching Octokit, which rejects an empty half. Fixes #426 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EdV5iCFkUxFqkLZQAGLAVT --- ProjectDirector.Test/ScanCredentialTests.cs | 91 +++++++++++++++++++++ ProjectDirector/ProjectDirector.cs | 39 ++++++++- 2 files changed, 126 insertions(+), 4 deletions(-) create mode 100644 ProjectDirector.Test/ScanCredentialTests.cs diff --git a/ProjectDirector.Test/ScanCredentialTests.cs b/ProjectDirector.Test/ScanCredentialTests.cs new file mode 100644 index 0000000..738a690 --- /dev/null +++ b/ProjectDirector.Test/ScanCredentialTests.cs @@ -0,0 +1,91 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.ProjectDirector.Test; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Octokit; + +/// +/// Tests the rule that decides which credentials one owner is scanned with. +/// +/// +/// is one mutable property on a client shared by +/// every owner in the scan, so the rule that sets it has to answer for every owner rather than only +/// for the ones that have credentials. It used to be a condition wrapped around the assignment, +/// which meant an owner with no credentials left the previous owner's in place and was scanned as +/// them. exists separately so that rule can be +/// driven without a live ImGui context or a GitHub account, the way +/// drives the pull rule. +/// +/// Getting it wrong is quiet: the owner's own private repositories go missing, and anything that +/// genuinely needed its auth answers with an ApiException the caller swallows, so the user is given +/// no reason for the gap. +/// +[TestClass] +public sealed class ScanCredentialTests +{ + private static GitHubOwnerName Owner(string value) => GitHubOwnerName.Create(value); + private static GitHubToken Token(string value) => GitHubToken.Create(value); + private static GitHubLogin Login(string value) => GitHubLogin.Create(value); + + [TestMethod] + public void AnOwnerWithItsOwnTokenIsScannedAsItself() + { + Credentials chosen = ProjectDirector.ChooseCredentials(Owner("alpha"), Token("alpha-pat"), Login("global-login"), Token("global-token")); + + Assert.AreEqual(AuthenticationType.Basic, chosen.AuthenticationType); + Assert.AreEqual("alpha", chosen.Login, "An owner's own token should win over the global login."); + Assert.AreEqual("alpha-pat", chosen.Password); + } + + [TestMethod] + public void AnOwnerWithoutATokenFallsBackToTheGlobalLogin() + { + Credentials chosen = ProjectDirector.ChooseCredentials(Owner("beta"), Token(string.Empty), Login("global-login"), Token("global-token")); + + Assert.AreEqual(AuthenticationType.Basic, chosen.AuthenticationType); + Assert.AreEqual("global-login", chosen.Login); + Assert.AreEqual("global-token", chosen.Password); + } + + /// + /// The regression this file exists for. + /// + /// + /// Owner A has a PAT and owner B has nothing. B must come back anonymous, because the caller + /// assigns whatever this returns to the shared client, and anything short of an answer for B + /// leaves A's identity in place. + /// + [TestMethod] + public void AnOwnerWithNoCredentialsAnywhereIsScannedAnonymously() + { + Credentials forOwnerWithPat = ProjectDirector.ChooseCredentials(Owner("alpha"), Token("alpha-pat"), Login(string.Empty), Token(string.Empty)); + Credentials forOwnerWithout = ProjectDirector.ChooseCredentials(Owner("beta"), Token(string.Empty), Login(string.Empty), Token(string.Empty)); + + Assert.AreEqual("alpha", forOwnerWithPat.Login, "The first owner should still be scanned as itself."); + + Assert.AreEqual(AuthenticationType.Anonymous, forOwnerWithout.AuthenticationType, + "An owner with no credentials of its own and no global login must be scanned anonymously, " + + "not as whichever owner was scanned before it."); + Assert.AreNotEqual("alpha", forOwnerWithout.Login, "The previous owner's login must not carry over."); + } + + [TestMethod] + public void AGlobalLoginMissingItsTokenIsNotUsed() + { + // Octokit rejects an empty password, so a half-configured global login has to be treated as + // no login rather than passed through. + Credentials chosen = ProjectDirector.ChooseCredentials(Owner("beta"), Token(string.Empty), Login("global-login"), Token(string.Empty)); + + Assert.AreEqual(AuthenticationType.Anonymous, chosen.AuthenticationType); + } + + [TestMethod] + public void AGlobalTokenMissingItsLoginIsNotUsed() + { + Credentials chosen = ProjectDirector.ChooseCredentials(Owner("beta"), Token(string.Empty), Login(string.Empty), Token("global-token")); + + Assert.AreEqual(AuthenticationType.Anonymous, chosen.AuthenticationType); + } +} diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index 79d2af9..e6a1a74 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -881,15 +881,46 @@ private void ScanDevDirectoryForOwnersAndRepos() UpdateClonedStatus(); } + /// + /// Chooses the credentials one owner is scanned with. + /// + /// The owner about to be scanned. + /// That owner's own personal access token, empty if it has none. + /// The globally configured login, empty if there is none. + /// The globally configured token, empty if there is none. + /// + /// The owner's own token where it has one, otherwise the global login where there is one, + /// otherwise . + /// + /// + /// The answer has to be total. is one mutable + /// property on a client shared by every owner in the scan, so an owner that leaves it alone is + /// not scanned anonymously -- it is scanned as whoever was set last. A PAT configured for one + /// owner therefore carried into the next owner that had none, which misses that owner's own + /// private repositories and answers anything needing its auth with an + /// that the caller swallows, leaving the repositories missing with + /// no indication why. + /// + internal static Credentials ChooseCredentials(GitHubOwnerName owner, GitHubToken pat, GitHubLogin login, GitHubToken token) + { + if (!string.IsNullOrEmpty(pat)) + { + return new Credentials(owner, pat); + } + + return !string.IsNullOrEmpty(login) && !string.IsNullOrEmpty(token) + ? new Credentials(login, token) + : Credentials.Anonymous; + } + private void ScanRemoteAccountsForRepos() { Dictionary knownOwners = Options.GitHubOwners; foreach ((GitHubOwnerName owner, GitHubToken pat) in knownOwners) { - if (!string.IsNullOrEmpty(pat) || (!string.IsNullOrEmpty(Options.GitHubLogin) && !string.IsNullOrEmpty(Options.GitHubToken))) - { - GitHubClient.Credentials = !string.IsNullOrEmpty(pat) ? new(owner, pat) : new(Options.GitHubLogin, Options.GitHubToken); - } + // Assigned for every owner, including one with no credentials of its own, so that the + // previous owner's identity cannot carry into this one. + GitHubClient.Credentials = ChooseCredentials(owner, pat, Options.GitHubLogin, Options.GitHubToken); SyncGitHubOwnerInfo(owner); } From dc27f7fd41c961ef3ad8256dbe8757acf84fb7cc Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 21:41:18 +0000 Subject: [PATCH 2/2] test: cover applying an owner's credentials to the shared client [patch] The SonarCloud gate failed at 75% coverage on new code, against a required 80%. The uncovered line was the one that mattered most: the assignment in ScanRemoteAccountsForRepos, which is what actually stops the previous owner's identity carrying over. ChooseCredentials was fully covered, but the rule only helps if the caller applies it for every owner, and nothing exercised that. Move the assignment into ApplyCredentials so a test can watch one client across two owners -- the shape the defect actually had. Scanning A with a PAT and then B with nothing now asserts against a real GitHubClient that B is anonymous rather than still authenticated as A. Verified by restoring the conditional assignment in ApplyCredentials: ScanningASecondOwnerDoesNotInheritTheFirstOwnersCredentials fails against it and passes against the fix. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EdV5iCFkUxFqkLZQAGLAVT --- ProjectDirector.Test/ScanCredentialTests.cs | 35 +++++++++++++++++++++ ProjectDirector/ProjectDirector.cs | 24 ++++++++++++-- 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/ProjectDirector.Test/ScanCredentialTests.cs b/ProjectDirector.Test/ScanCredentialTests.cs index 738a690..5d9a1cd 100644 --- a/ProjectDirector.Test/ScanCredentialTests.cs +++ b/ProjectDirector.Test/ScanCredentialTests.cs @@ -71,6 +71,41 @@ public void AnOwnerWithNoCredentialsAnywhereIsScannedAnonymously() Assert.AreNotEqual("alpha", forOwnerWithout.Login, "The previous owner's login must not carry over."); } + /// + /// The defect in the shape it actually had: one client, reused across owners. + /// + /// + /// pins the rule, but the rule + /// only helps if the caller applies it for every owner. Scanning A and then B against a single + /// client is what the loop does, so this is what catches a caller that skips the assignment. + /// + [TestMethod] + public void ScanningASecondOwnerDoesNotInheritTheFirstOwnersCredentials() + { + GitHubClient client = new(new ProductHeaderValue("ktsu.ProjectDirector.Test")); + + ProjectDirector.ApplyCredentials(client, Owner("alpha"), Token("alpha-pat"), Login(string.Empty), Token(string.Empty)); + + Assert.AreEqual("alpha", client.Credentials.Login, "The first owner should be scanned as itself."); + + ProjectDirector.ApplyCredentials(client, Owner("beta"), Token(string.Empty), Login(string.Empty), Token(string.Empty)); + + Assert.AreEqual(AuthenticationType.Anonymous, client.Credentials.AuthenticationType, + "The client must not still be authenticated as the previous owner when scanning one with no credentials."); + Assert.AreNotEqual("alpha", client.Credentials.Login); + } + + [TestMethod] + public void ApplyingAGlobalLoginPointsTheClientAtIt() + { + GitHubClient client = new(new ProductHeaderValue("ktsu.ProjectDirector.Test")); + + ProjectDirector.ApplyCredentials(client, Owner("beta"), Token(string.Empty), Login("global-login"), Token("global-token")); + + Assert.AreEqual("global-login", client.Credentials.Login); + Assert.AreEqual("global-token", client.Credentials.Password); + } + [TestMethod] public void AGlobalLoginMissingItsTokenIsNotUsed() { diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index e6a1a74..d46138c 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -913,14 +913,32 @@ internal static Credentials ChooseCredentials(GitHubOwnerName owner, GitHubToken : Credentials.Anonymous; } + /// + /// Points the shared client at the credentials one owner is scanned with. + /// + /// The client every owner in the scan shares. + /// The owner about to be scanned. + /// That owner's own personal access token, empty if it has none. + /// The globally configured login, empty if there is none. + /// The globally configured token, empty if there is none. + /// + /// Assigned for every owner, including one with no credentials of its own, so that the previous + /// owner's identity cannot carry into this one. That is the whole of the rule, and it is here + /// rather than inline in the loop so a test can watch one client across two owners, which is the + /// shape the defect actually had. + /// + internal static void ApplyCredentials(GitHubClient client, GitHubOwnerName owner, GitHubToken pat, GitHubLogin login, GitHubToken token) + { + Ensure.NotNull(client); + client.Credentials = ChooseCredentials(owner, pat, login, token); + } + private void ScanRemoteAccountsForRepos() { Dictionary knownOwners = Options.GitHubOwners; foreach ((GitHubOwnerName owner, GitHubToken pat) in knownOwners) { - // Assigned for every owner, including one with no credentials of its own, so that the - // previous owner's identity cannot carry into this one. - GitHubClient.Credentials = ChooseCredentials(owner, pat, Options.GitHubLogin, Options.GitHubToken); + ApplyCredentials(GitHubClient, owner, pat, Options.GitHubLogin, Options.GitHubToken); SyncGitHubOwnerInfo(owner); }