fix: scan each owner with its own credentials, not the previous owner's [patch]

ProjectDirector.Test/ScanCredentialTests.cs

@@ -0,0 +1,126 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.ProjectDirector.Test;
+
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+using Octokit;
+
+/// <summary>
+/// Tests the rule that decides which credentials one owner is scanned with.
+/// </summary>
+/// <remarks>
+/// <see cref="Octokit.GitHubClient.Credentials"/> 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. <see cref="ProjectDirector.ChooseCredentials"/> exists separately so that rule can be
+/// driven without a live ImGui context or a GitHub account, the way <see cref="PullDecisionTests"/>
+/// 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.
+/// </remarks>
+[TestClass]
+public sealed class ScanCredentialTests
+{
+	private static GitHubOwnerName Owner(string value) => GitHubOwnerName.Create<GitHubOwnerName>(value);
+	private static GitHubToken Token(string value) => GitHubToken.Create<GitHubToken>(value);
+	private static GitHubLogin Login(string value) => GitHubLogin.Create<GitHubLogin>(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);
+	}
+
+	/// <summary>
+	/// The regression this file exists for.
+	/// </summary>
+	/// <remarks>
+	/// 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.
+	/// </remarks>
+	[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.");
+	}
+
+	/// <summary>
+	/// The defect in the shape it actually had: one client, reused across owners.
+	/// </summary>
+	/// <remarks>
+	/// <see cref="AnOwnerWithNoCredentialsAnywhereIsScannedAnonymously"/> 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.
+	/// </remarks>
+	[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()
+	{
+		// 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);
+	}
+}

ProjectDirector/ProjectDirector.cs

@@ -18,7 +18,7 @@
 using ktsu.ImGui.Widgets;
 using ktsu.ImGui.Styler;
 using Octokit;
 // using OpenAI.Chat;
 using Semantics.Paths;
 
 #pragma warning disable CA1506
@@ -43,7 +43,7 @@
 	private Collection<RelativePath> BrowserContentsCompare { get; set; } = [];
 	private PopupPropagateFile PopupPropagateFile { get; } = new();
 
 	// private ChatClient ChatClient { get; init; }
 
 	private static void Main(string[] _)
 	{
@@ -63,7 +63,7 @@
 	{
 		Options = ProjectDirectorOptions.LoadOrCreate();
 		Options.Save();
 		// ChatClient = new(model: "gpt-4o", new ApiKeyCredential(Options.OpenAIToken));
 		DividerDiff = new("DiffDivider", DividerResized, ImGuiWidgets.DividerLayout.Columns);
 		DividerContainerCols = new("VerticalDivider", DividerResized, ImGuiWidgets.DividerLayout.Columns);
 		DividerContainerRows = new("HorizontalDivider", DividerResized, ImGuiWidgets.DividerLayout.Rows);
@@ -501,7 +501,7 @@
 						});
 					}
 
 					//int fetchInterval = repo.MinFetchIntervalSeconds;
 					//if (ImGuiWidgets.Knob("Min Fetch Interval", ref fetchInterval, 0, 300, 150))
 					//{
 					//	repo.MinFetchIntervalSeconds = fetchInterval;
@@ -881,15 +881,64 @@
 		UpdateClonedStatus();
 	}
 
+	/// <summary>
+	/// Chooses the credentials one owner is scanned with.
+	/// </summary>
+	/// <param name="owner">The owner about to be scanned.</param>
+	/// <param name="pat">That owner's own personal access token, empty if it has none.</param>
+	/// <param name="login">The globally configured login, empty if there is none.</param>
+	/// <param name="token">The globally configured token, empty if there is none.</param>
+	/// <returns>
+	/// The owner's own token where it has one, otherwise the global login where there is one,
+	/// otherwise <see cref="Credentials.Anonymous"/>.
+	/// </returns>
+	/// <remarks>
+	/// The answer has to be total. <see cref="Octokit.GitHubClient.Credentials"/> 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
+	/// <see cref="ApiException"/> that the caller swallows, leaving the repositories missing with
+	/// no indication why.
+	/// </remarks>
+	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;
+	}
+
+	/// <summary>
+	/// Points the shared client at the credentials one owner is scanned with.
+	/// </summary>
+	/// <param name="client">The client every owner in the scan shares.</param>
+	/// <param name="owner">The owner about to be scanned.</param>
+	/// <param name="pat">That owner's own personal access token, empty if it has none.</param>
+	/// <param name="login">The globally configured login, empty if there is none.</param>
+	/// <param name="token">The globally configured token, empty if there is none.</param>
+	/// <remarks>
+	/// 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.
+	/// </remarks>
+	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<GitHubOwnerName, GitHubToken> 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);
-			}
+			ApplyCredentials(GitHubClient, owner, pat, Options.GitHubLogin, Options.GitHubToken);
 
 			SyncGitHubOwnerInfo(owner);
 		}
@@ -1614,7 +1663,7 @@
 				if (ImGui.TableNextColumn())
 				{
 					//if (ImGui.Button($"Propagate Directory###Propagate{path.Replace(Path.DirectorySeparatorChar, '.').Replace(Path.AltDirectorySeparatorChar, '.')}"))
 					//{
 					//	shouldOpenPopup |= true;
 					//	Options.PropagatePath = path;
 					//}