From 02c65d9c7b3ec0344ee9f730bb31a441cc06d7d3 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 17:44:59 +0000 Subject: [PATCH 1/3] feat: keep GitHub tokens in the OS secret store [minor] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ProjectDirectorOptions carried a personal access token per configured owner, plus an account-level one, in plain fields. AppData serializes the whole object, so every debounced save wrote those PATs to an unencrypted settings file alongside window state and UI preferences. TokenStorage now holds them in the platform-native secret store through ktsu.CredentialCache, under a service name scoped to ProjectDirector. Personas are derived rather than stored: a versioned namespace plus the login or the owner name. GitHubOwners becomes a set of names — it was doing double duty as both the owner registry and the token map — so the settings file keeps tracking which owners are configured while holding no secret. LegacyGitHubToken and LegacyGitHubOwners keep the old JSON names so startup can migrate: written to the secret store first, so a store that throws cannot lose them, and only then emptied. A token already in the store wins over a stale copy, but the stale copy is still cleared. There was no way to enter a token short of editing the settings file by hand, so moving storage without adding one would have left new setups with no way at all. File > Set GitHub Owner Token fills that in, alongside the existing Add New GitHub Owner. With no usable secret store, tokens read as empty and the reason reaches the log once rather than on every owner of every scan. A throw out of a token read would take down the render loop. There is no plaintext fallback. DevDirectory defaulted to the literal C:\dev, which AbsoluteDirectoryPath rejects off Windows, so constructing the options threw there and no test could touch the type. It now picks a valid path per platform, unchanged on Windows. Fixes #411 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018AnVpPWKjVtXnAvd4bnzUL --- CLAUDE.md | 10 +- Directory.Packages.props | 1 + ProjectDirector.Test/TokenStorageTests.cs | 305 ++++++++++++++++++++++ ProjectDirector/ProjectDirector.cs | 82 +++++- ProjectDirector/ProjectDirector.csproj | 1 + ProjectDirector/ProjectDirectorOptions.cs | 63 ++++- ProjectDirector/TokenStorage.cs | 262 +++++++++++++++++++ README.md | 14 +- 8 files changed, 720 insertions(+), 18 deletions(-) create mode 100644 ProjectDirector.Test/TokenStorageTests.cs create mode 100644 ProjectDirector/TokenStorage.cs diff --git a/CLAUDE.md b/CLAUDE.md index 465e273..a4a95a7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -41,7 +41,13 @@ dotnet test --configuration Release **[ProjectDirectorOptions.cs](ProjectDirector/ProjectDirectorOptions.cs)** - Application state - Extends `AppData` from ktsu.AppDataStorage for automatic JSON persistence -- Stores: dev directory path, GitHub credentials, repo cache, UI state (divider positions, panel states) +- Stores: dev directory path, which GitHub owners are configured, repo cache, UI state (divider + positions, panel states) +- **Not** GitHub tokens. Those live in the OS secret store via + [TokenStorage.cs](ProjectDirector/TokenStorage.cs), because this file sits next to UI preferences + and is rewritten on every debounced save +- `LegacyGitHubToken` and `LegacyGitHubOwners` keep the old JSON names purely for the one-time + migration run at startup: tokens are written to the secret store first, then emptied here - Semantic string types for type-safe paths and identifiers **[GitRepository.cs](ProjectDirector/GitRepository.cs)** - Repository abstraction @@ -62,7 +68,7 @@ dotnet test --configuration Release Git LFS is a pair of filters plus a set of hooks, and all of them belong to the git command. A library that reads and writes the object database directly bypasses them: a commit stores raw bytes where a pointer belongs, and a clone or checkout lands the pointer text on disk where the file belongs. This application clones, fetches and pulls, so it is the checkout side that matters here. `ProjectDirector.Test` pins both halves down. -Authentication follows from the same decision. There are no credentials in this code, because git uses the platform credential helper, which is also what makes SSH remotes work. +Authentication follows from the same decision. There are no credentials in this code for git operations, because git uses the platform credential helper, which is also what makes SSH remotes work. The GitHub API tokens the app does hold follow the same principle: `TokenStorage` keeps them in the platform secret store (Windows Credential Manager, macOS Keychain, libsecret on Linux), derives a persona per token from a versioned namespace plus the login or owner name, and never falls back to a plain file. With no usable store, tokens read as empty and the reason reaches the log once — a throw out of a token read would take down the render loop. ### Key Dependencies diff --git a/Directory.Packages.props b/Directory.Packages.props index 0dbd93f..c52a68f 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -7,6 +7,7 @@ + diff --git a/ProjectDirector.Test/TokenStorageTests.cs b/ProjectDirector.Test/TokenStorageTests.cs new file mode 100644 index 0000000..6e7c2d3 --- /dev/null +++ b/ProjectDirector.Test/TokenStorageTests.cs @@ -0,0 +1,305 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.ProjectDirector.Test; + +using System.Text.Json; +using System.Text.Json.Serialization; + +using ktsu.CredentialCache; +using ktsu.CredentialCache.Storage; +using ktsu.RoundTripStringJsonConverter; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using CredentialCache = ktsu.CredentialCache.CredentialCache; + +/// +/// Covers where GitHub personal access tokens live. The settings file sits next to window state and +/// UI preferences and is written on every debounced save, so the contract under test is that a PAT +/// reaches the OS secret store and does not survive in that file. +/// +/// +/// Follows the repository's own rule for this codebase: the part with a decision in it is a plain +/// class, so it can be driven without a live ImGui context or a display. +/// +[TestClass] +public sealed class TokenStorageTests +{ + private CredentialCache Cache { get; set; } = null!; + + [TestInitialize] + public void SetUp() + { + Cache = new CredentialCache(new InMemoryCredentialStore()); + TokenStorage.UseCache(Cache); + } + + [TestCleanup] + public void TearDown() + { + TokenStorage.UseCache(null); + Cache.Dispose(); + } + + /// + /// A store standing in for a machine whose native secret library will not load — a Linux host + /// with no Secret Service provider, which is what a container or an SSH session usually is. + /// + private sealed class UnavailableCredentialStore : ICredentialStore + { + public string Name => "Unavailable"; + + public bool TryLoad(PersonaGUID persona, out Credential? credential) => + throw new DllNotFoundException("libsecret-1.so.0"); + + public void Save(PersonaGUID persona, Credential credential) => + throw new DllNotFoundException("libsecret-1.so.0"); + + public bool Remove(PersonaGUID persona) => throw new DllNotFoundException("libsecret-1.so.0"); + } + + /// + /// Mirrors how writes the settings file, so a token would show + /// as a plain string rather than as the char array a bare serializer produces for a semantic + /// string. + /// + private static readonly JsonSerializerOptions AppDataLike = BuildAppDataLikeOptions(); + + private static JsonSerializerOptions BuildAppDataLikeOptions() + { + JsonSerializerOptions options = new() + { + ReferenceHandler = ReferenceHandler.Preserve, + }; + options.Converters.Add(new RoundTripStringJsonConverterFactory()); + return options; + } + + private static GitHubOwnerName Owner(string name) => GitHubOwnerName.Create(name); + + private static GitHubToken Token(string value) => GitHubToken.Create(value); + + /// + /// Two owners never share a persona. + /// + [TestMethod] + public void OwnerPersonasAreDistinct() => + Assert.AreNotEqual(TokenStorage.OwnerPersona(Owner("ktsu-dev")), TokenStorage.OwnerPersona(Owner("ktsu-io"))); + + /// + /// The account-level token and an owner token of the same name are separate entries, so one + /// cannot overwrite the other. + /// + [TestMethod] + public void LoginPersonaIsNotAnOwnerPersona() => + Assert.AreNotEqual( + TokenStorage.LoginPersona(GitHubLogin.Create("ktsu-dev")), + TokenStorage.OwnerPersona(Owner("ktsu-dev"))); + + /// + /// Derivation is deterministic and GUID-shaped. Changing it would orphan every token already in + /// the store. + /// + [TestMethod] + public void PersonaDerivationIsStable() + { + Assert.AreEqual( + TokenStorage.OwnerPersona(Owner("ktsu-dev")).ToString(), + TokenStorage.OwnerPersona(Owner("ktsu-dev")).ToString()); + Assert.IsTrue(Guid.TryParse(TokenStorage.OwnerPersona(Owner("ktsu-dev")).ToString(), out _)); + } + + /// + /// An owner token written through the normal path is readable again. + /// + [TestMethod] + public void OwnerTokenRoundTripsThroughTheStore() + { + Assert.IsTrue(TokenStorage.WriteOwnerToken(Owner("ktsu-dev"), Token("ghp_owner"))); + + Assert.AreEqual("ghp_owner", TokenStorage.ReadOwnerToken(Owner("ktsu-dev")).ToString()); + Assert.IsTrue(TokenStorage.ReadOwnerToken(Owner("ktsu-io")).IsEmpty()); + } + + /// + /// The account-level token set through the options property goes to the store, keyed by the + /// login it belongs to. + /// + [TestMethod] + public void AccountTokenRoundTripsThroughTheStore() + { + using ProjectDirectorOptions options = new() + { + GitHubLogin = GitHubLogin.Create("someone"), + GitHubToken = Token("ghp_account"), + }; + + Assert.AreEqual("ghp_account", options.GitHubToken.ToString()); + Assert.IsTrue(options.LegacyGitHubToken.IsEmpty()); + } + + /// + /// Clearing a token removes the entry rather than leaving an empty one behind. + /// + [TestMethod] + public void ClearingATokenRemovesItFromTheStore() + { + _ = TokenStorage.WriteOwnerToken(Owner("ktsu-dev"), Token("ghp_owner")); + + Assert.IsTrue(TokenStorage.WriteOwnerToken(Owner("ktsu-dev"), new())); + + Assert.IsFalse(Cache.TryGet(TokenStorage.OwnerPersona(Owner("ktsu-dev")), out _)); + } + + /// + /// The settings file an earlier version wrote carries its tokens into the store, and the owners + /// it listed become the owner registry. + /// + [TestMethod] + public void MigrationMovesAccountAndOwnerTokens() + { + using ProjectDirectorOptions options = new() + { + GitHubLogin = GitHubLogin.Create("someone"), + LegacyGitHubToken = Token("ghp_account"), + LegacyGitHubOwners = + { + [Owner("ktsu-dev")] = Token("ghp_owner"), + [Owner("ktsu-io")] = new(), + }, + }; + + int migrated = TokenStorage.MigrateLegacyTokens(options); + + Assert.AreEqual(2, migrated); + Assert.AreEqual("ghp_account", options.GitHubToken.ToString()); + Assert.AreEqual("ghp_owner", TokenStorage.ReadOwnerToken(Owner("ktsu-dev")).ToString()); + Assert.IsTrue(options.GitHubOwners.Contains(Owner("ktsu-dev"))); + Assert.IsTrue(options.GitHubOwners.Contains(Owner("ktsu-io"))); + } + + /// + /// Migration also empties the old fields. Ceasing to write a secret does not remove the one + /// already on disk, so this is the half that does the security work. + /// + [TestMethod] + public void MigrationEmptiesThePlaintextFields() + { + using ProjectDirectorOptions options = new() + { + LegacyGitHubToken = Token("ghp_account"), + LegacyGitHubOwners = { [Owner("ktsu-dev")] = Token("ghp_owner") }, + }; + + _ = TokenStorage.MigrateLegacyTokens(options); + + Assert.IsTrue(options.LegacyGitHubToken.IsEmpty()); + Assert.AreEqual(0, options.LegacyGitHubOwners.Count); + } + + /// + /// The settings file written after migration carries no token. + /// + [TestMethod] + public void SerializedOptionsCarryNoToken() + { + using ProjectDirectorOptions options = new() + { + LegacyGitHubToken = Token("ghp_account_secret"), + LegacyGitHubOwners = { [Owner("ktsu-dev")] = Token("ghp_owner_secret") }, + }; + + _ = TokenStorage.MigrateLegacyTokens(options); + string json = JsonSerializer.Serialize(options, AppDataLike); + + Assert.DoesNotContain("ghp_account_secret", json, StringComparison.Ordinal); + Assert.DoesNotContain("ghp_owner_secret", json, StringComparison.Ordinal); + Assert.Contains("ktsu-dev", json, StringComparison.Ordinal); + } + + /// + /// A second start has nothing left to move. + /// + [TestMethod] + public void MigrationIsIdempotent() + { + using ProjectDirectorOptions options = new() + { + LegacyGitHubToken = Token("ghp_account"), + }; + + _ = TokenStorage.MigrateLegacyTokens(options); + + Assert.AreEqual(0, TokenStorage.MigrateLegacyTokens(options)); + Assert.AreEqual("ghp_account", options.GitHubToken.ToString()); + } + + /// + /// A stale token in the settings file must not overwrite the one currently in the store — but it + /// is still cleared. + /// + [TestMethod] + public void MigrationKeepsTheStoredTokenAndStillClearsTheStaleOne() + { + _ = TokenStorage.WriteOwnerToken(Owner("ktsu-dev"), Token("ghp_current")); + using ProjectDirectorOptions options = new() + { + LegacyGitHubOwners = { [Owner("ktsu-dev")] = Token("ghp_stale") }, + }; + + Assert.AreEqual(1, TokenStorage.MigrateLegacyTokens(options)); + Assert.AreEqual("ghp_current", TokenStorage.ReadOwnerToken(Owner("ktsu-dev")).ToString()); + Assert.AreEqual(0, options.LegacyGitHubOwners.Count); + } + + /// + /// If the secret store will not take the token, the old copy stays where it is. Clearing it would + /// destroy the only copy the user has. + /// + [TestMethod] + public void MigrationKeepsTheLegacyTokenWhenTheStoreRefuses() + { + using CredentialCache unavailable = new(new UnavailableCredentialStore()); + TokenStorage.UseCache(unavailable); + + using ProjectDirectorOptions options = new() + { + LegacyGitHubOwners = { [Owner("ktsu-dev")] = Token("ghp_owner") }, + }; + + Assert.AreEqual(0, TokenStorage.MigrateLegacyTokens(options)); + Assert.AreEqual("ghp_owner", options.LegacyGitHubOwners[Owner("ktsu-dev")].ToString()); + Assert.IsTrue(options.GitHubOwners.Contains(Owner("ktsu-dev"))); + } + + /// + /// A machine with no secret store reads as "no token" rather than throwing, and says why once. + /// ProjectDirector is a desktop application, and an exception out of a token read would take down + /// the render loop. What it must never do is fall back to a plain file. + /// + [TestMethod] + public void ReadingWithoutASecretStoreIsEmptyAndReportedOnce() + { + using CredentialCache unavailable = new(new UnavailableCredentialStore()); + TokenStorage.UseCache(unavailable); + + Assert.IsTrue(TokenStorage.ReadOwnerToken(Owner("ktsu-dev")).IsEmpty()); + Assert.IsTrue(TokenStorage.StoreUnavailable); + + string report = TokenStorage.DrainUnavailableReport(); + Assert.Contains("secret store", report, StringComparison.Ordinal); + Assert.AreEqual(string.Empty, TokenStorage.DrainUnavailableReport()); + } + + /// + /// A write to an unusable store reports failure rather than pretending it saved. + /// + [TestMethod] + public void WritingWithoutASecretStoreReportsFailure() + { + using CredentialCache unavailable = new(new UnavailableCredentialStore()); + TokenStorage.UseCache(unavailable); + + Assert.IsFalse(TokenStorage.WriteOwnerToken(Owner("ktsu-dev"), Token("ghp_owner"))); + } +} diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index 79d2af9..347302f 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -37,12 +37,19 @@ internal sealed class ProjectDirector private ConcurrentQueue LogQueue { get; } = new(); private ImGuiPopups.InputString PopupSetDevDirectory { get; } = new(); private ImGuiPopups.InputString PopupAddNewGitHubOwner { get; } = new(); + private ImGuiPopups.InputString PopupSetGitHubOwnerToken { get; } = new(); private ImGuiPopups.Prompt PopupConfirmPull { get; } = new(); private ImGuiPopups.InputString PopupCommitMessage { get; } = new(); private Collection BrowserContentsBase { get; set; } = []; private Collection BrowserContentsCompare { get; set; } = []; private PopupPropagateFile PopupPropagateFile { get; } = new(); + /// + /// The owner whose token popup should open on the next tick. Opening a popup from inside + /// BeginMenu does not work, so the menu records the owner and the tick opens it. + /// + private GitHubOwnerName? OwnerPendingTokenPopup { get; set; } + // private ChatClient ChatClient { get; init; } private static void Main(string[] _) @@ -90,11 +97,25 @@ public ProjectDirector() GitHubClient = new(new ProductHeaderValue("ktsu.ProjectDirector")); - if (!string.IsNullOrEmpty(Options.GitHubLogin) && !string.IsNullOrEmpty(Options.GitHubToken)) + int migratedTokens = TokenStorage.MigrateLegacyTokens(Options); + if (migratedTokens > 0) { - GitHubClient.Credentials = new(Options.GitHubLogin, Options.GitHubToken); + QueueSaveOptions(); } + GitHubToken startupToken = Options.GitHubToken; + if (!string.IsNullOrEmpty(Options.GitHubLogin) && !string.IsNullOrEmpty(startupToken)) + { + GitHubClient.Credentials = new(Options.GitHubLogin, startupToken); + } + + if (migratedTokens > 0) + { + QueueLog($"Moved {migratedTokens} GitHub token(s) out of the settings file into the OS secret store"); + } + + DrainSecretStoreReport(); + RefreshPage(); } @@ -161,6 +182,19 @@ private void RestoreDividerStates() private void QueueSaveOptions() => SaveOptionsQueuedTime = DateTime.UtcNow; + /// + /// Writes the secret store's complaint to the log, if it has one. + /// records the message rather than logging it, because the log is an instance member here. + /// + private void DrainSecretStoreReport() + { + string report = TokenStorage.DrainUnavailableReport(); + if (report.Length > 0) + { + QueueLog(report); + } + } + private void SaveOptionsIfRequired() { //debounce the save requests and avoid saving multiple times per frame or multiple frames in a row @@ -445,8 +479,22 @@ private void Tick(float dt) { DividerContainerCols.Tick(dt); + if (OwnerPendingTokenPopup is not null) + { + GitHubOwnerName owner = OwnerPendingTokenPopup; + OwnerPendingTokenPopup = null; + PopupSetGitHubOwnerToken.Open($"Set Token for {owner}", "Personal Access Token", string.Empty, result => + { + if (!TokenStorage.WriteOwnerToken(owner, GitHubToken.Create(result))) + { + DrainSecretStoreReport(); + } + }); + } + _ = PopupSetDevDirectory.ShowIfOpen(); _ = PopupAddNewGitHubOwner.ShowIfOpen(); + _ = PopupSetGitHubOwnerToken.ShowIfOpen(); _ = PopupConfirmPull.ShowIfOpen(); _ = PopupCommitMessage.ShowIfOpen(); @@ -662,12 +710,25 @@ private void ShowMenu() if (!string.IsNullOrEmpty(result)) { GitHubOwnerName newName = GitHubOwnerName.Create(result); - _ = Options.GitHubOwners.TryAdd(newName, GitHubToken.Create(string.Empty)); + _ = Options.GitHubOwners.Add(newName); SyncGitHubOwnerInfo(newName); } }); } + if (Options.GitHubOwners.Count > 0 && ImGui.BeginMenu("Set GitHub Owner Token")) + { + foreach (GitHubOwnerName owner in Options.GitHubOwners.OrderBy(o => o.ToString(), StringComparer.Ordinal)) + { + if (ImGui.MenuItem(owner)) + { + OwnerPendingTokenPopup = owner; + } + } + + ImGui.EndMenu(); + } + ImGui.Separator(); if (ImGui.MenuItem("Exit")) @@ -696,7 +757,7 @@ private void ShowMenu() private void ShowOwners() { - foreach ((GitHubOwnerName owner, GitHubToken pat) in Options.GitHubOwners) + foreach (GitHubOwnerName owner in Options.GitHubOwners.OrderBy(o => o.ToString(), StringComparer.Ordinal)) { ShowCollapsiblePanel(owner, () => ShowRepos(owner)); } @@ -868,7 +929,7 @@ private void ScanDevDirectoryForOwnersAndRepos() Options.Repos[repoFullName] = gitHubRepo; gitHubRepo.OwnerName = ownerName; gitHubRepo.RepoName = repoName; - _ = Options.GitHubOwners.TryAdd(gitHubRepo.OwnerName, GitHubToken.Create(string.Empty)); + _ = Options.GitHubOwners.Add(gitHubRepo.OwnerName); } } } @@ -883,17 +944,20 @@ private void ScanDevDirectoryForOwnersAndRepos() private void ScanRemoteAccountsForRepos() { - Dictionary knownOwners = Options.GitHubOwners; - foreach ((GitHubOwnerName owner, GitHubToken pat) in knownOwners) + foreach (GitHubOwnerName owner in Options.GitHubOwners.ToArray()) { - if (!string.IsNullOrEmpty(pat) || (!string.IsNullOrEmpty(Options.GitHubLogin) && !string.IsNullOrEmpty(Options.GitHubToken))) + GitHubToken pat = TokenStorage.ReadOwnerToken(owner); + GitHubToken accountToken = Options.GitHubToken; + if (!string.IsNullOrEmpty(pat) || (!string.IsNullOrEmpty(Options.GitHubLogin) && !string.IsNullOrEmpty(accountToken))) { - GitHubClient.Credentials = !string.IsNullOrEmpty(pat) ? new(owner, pat) : new(Options.GitHubLogin, Options.GitHubToken); + GitHubClient.Credentials = !string.IsNullOrEmpty(pat) ? new(owner, pat) : new(Options.GitHubLogin, accountToken); } SyncGitHubOwnerInfo(owner); } + DrainSecretStoreReport(); + UpdateClonedStatus(); } diff --git a/ProjectDirector/ProjectDirector.csproj b/ProjectDirector/ProjectDirector.csproj index 6d2ec1b..ed7d5ea 100644 --- a/ProjectDirector/ProjectDirector.csproj +++ b/ProjectDirector/ProjectDirector.csproj @@ -14,6 +14,7 @@ + diff --git a/ProjectDirector/ProjectDirectorOptions.cs b/ProjectDirector/ProjectDirectorOptions.cs index b624423..9aa2f93 100644 --- a/ProjectDirector/ProjectDirectorOptions.cs +++ b/ProjectDirector/ProjectDirectorOptions.cs @@ -18,15 +18,72 @@ public sealed record class FullyQualifiedLocalRepoPath : SemanticString { - public AbsoluteDirectoryPath DevDirectory { get; set; } = AbsoluteDirectoryPath.Create(@"C:\dev"); + public AbsoluteDirectoryPath DevDirectory { get; set; } = DefaultDevDirectory(); + + /// + /// Where to look for repositories before the user picks a directory. + /// + /// + /// This used to be the literal C:\dev, which rejects + /// off Windows — so constructing the options at all threw there, and nothing could exercise this + /// type on another platform. + /// + private static AbsoluteDirectoryPath DefaultDevDirectory() => + AbsoluteDirectoryPath.Create( + OperatingSystem.IsWindows() + ? @"C:\dev" + : Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), "dev")); public ImGuiAppWindowState WindowState { get; set; } = new(); public GitHubLogin GitHubLogin { get; set; } = new(); - public GitHubToken GitHubToken { get; set; } = new(); + + /// + /// The account-level token as earlier versions persisted it: plaintext, in the settings file. + /// + /// + /// Retained under its original JSON name only so + /// can move it into the OS secret store and blank it here. + /// + [JsonInclude] + [JsonPropertyName("GitHubToken")] + internal GitHubToken LegacyGitHubToken { get; set; } = new(); + + /// + /// The account-level token that goes with , held in the OS secret store. + /// + [JsonIgnore] + public GitHubToken GitHubToken + { + get => TokenStorage.Read(TokenStorage.LoginPersona(GitHubLogin)); + set + { + Ensure.NotNull(value); + _ = TokenStorage.Write(TokenStorage.LoginPersona(GitHubLogin), value); + } + } + public OpenAIToken OpenAIToken { get; set; } = new(); public Dictionary PanelStates { get; init; } = []; public Dictionary> DividerStates { get; init; } = []; - public Dictionary GitHubOwners { get; init; } = []; + + /// + /// The owner-to-token map as earlier versions persisted it: plaintext, in the settings file. + /// + /// + /// Retained under its original JSON name only so + /// can move the tokens into the OS secret store, register the owner names in + /// , and empty this. + /// + [JsonInclude] + [JsonPropertyName("GitHubOwners")] + internal Dictionary LegacyGitHubOwners { get; init; } = []; + + /// + /// Which GitHub owners are configured. Their tokens live in the OS secret store, so this is a + /// plain set of names rather than a map to secrets. + /// + [JsonPropertyName("GitHubOwnerNames")] + public HashSet GitHubOwners { get; init; } = []; public Dictionary GitHubOwnerInfo { get; init; } = []; public Dictionary ClonedRepos { get; init; } = []; public FullyQualifiedGitHubRepoName BaseRepo { get; set; } = new(); diff --git a/ProjectDirector/TokenStorage.cs b/ProjectDirector/TokenStorage.cs new file mode 100644 index 0000000..1b99062 --- /dev/null +++ b/ProjectDirector/TokenStorage.cs @@ -0,0 +1,262 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.ProjectDirector; + +using System.Security.Cryptography; +using System.Text; + +using ktsu.CredentialCache; +using ktsu.CredentialCache.Storage; + +using Semantics.Strings; + +using CredentialCache = ktsu.CredentialCache.CredentialCache; + +/// +/// Holds GitHub personal access tokens in the operating system's secret store — Windows Credential +/// Manager, macOS Keychain, or libsecret (Secret Service) on Linux — rather than in +/// 's settings file. +/// +/// +/// serializes the whole options object to an unencrypted file +/// alongside window state and UI preferences. A PAT is not something to keep there. +/// +internal static class TokenStorage +{ + /// + /// Scopes ProjectDirector's entries within the OS secret store so they cannot collide with + /// another ktsu tool's credentials on a shared host. + /// + internal const string CredentialServiceName = "ktsu.ProjectDirector"; + + /// + /// Prefixed to every persona seed. Versioned because changing the derivation would orphan every + /// token already in the store, so a future change has to be a deliberate, visible one. + /// + private const string PersonaNamespace = "ktsu.ProjectDirector/v1/"; + + private const string UnavailableMessage = + "No usable OS secret store was found, so GitHub tokens cannot be read or saved. On Linux this " + + "usually means no Secret Service provider (libsecret with GNOME Keyring or KWallet) is installed " + + "and unlocked. ProjectDirector will not fall back to writing tokens to a plain file."; + + private static readonly Lazy LazyDefaultCache = + new(() => new CredentialCache(CredentialStoreFactory.CreateDefault(CredentialServiceName))); + + private static int _unavailableReported; + + /// + /// The cache substituted by , or for the default. + /// + private static CredentialCache? InjectedCache { get; set; } + + /// + /// Gets the cache backing this storage. + /// + /// + /// Built directly rather than taken from so the store + /// carries ProjectDirector's own service name; the singleton can only ever use the library + /// default. + /// + internal static CredentialCache Cache => InjectedCache ?? LazyDefaultCache.Value; + + /// + /// Reports whether the last read or write found the secret store unusable. + /// + internal static bool StoreUnavailable => Volatile.Read(ref _unavailableReported) != 0; + + /// + /// Substitutes the backing cache. Test seam; pass to restore the default. + /// + internal static void UseCache(CredentialCache? cache) + { + InjectedCache = cache; + _ = Interlocked.Exchange(ref _unavailableReported, 0); + } + + /// + /// Derives the persona holding the account-level token that goes with + /// . + /// + internal static PersonaGUID LoginPersona(GitHubLogin login) => DerivePersona($"github/login/{login}"); + + /// + /// Derives the persona holding an owner's token. + /// + internal static PersonaGUID OwnerPersona(GitHubOwnerName owner) => DerivePersona($"github/owner/{owner}"); + + /// + /// Reads the token stored under , or an empty token when there is none. + /// + internal static GitHubToken Read(PersonaGUID persona) + { + try + { + return Cache.TryGet(persona, out Credential? credential) && credential is CredentialWithToken token + ? GitHubToken.Create(token.Token.ToString()) + : new(); + } + catch (Exception ex) when (IsUnavailable(ex)) + { + ReportUnavailable(ex); + return new(); + } + } + + /// + /// Stores under , removing the entry when the + /// token is empty. + /// + /// when the secret store refused the write. + internal static bool Write(PersonaGUID persona, GitHubToken token) + { + try + { + if (token.IsEmpty()) + { + _ = Cache.Remove(persona); + return true; + } + + Cache.AddOrReplace(persona, new CredentialWithToken + { + Token = SemanticString.Create(token.ToString()), + }); + return true; + } + catch (Exception ex) when (IsUnavailable(ex)) + { + ReportUnavailable(ex); + return false; + } + } + + /// + /// Reads an owner's token. + /// + internal static GitHubToken ReadOwnerToken(GitHubOwnerName owner) => Read(OwnerPersona(owner)); + + /// + /// Stores an owner's token. + /// + internal static bool WriteOwnerToken(GitHubOwnerName owner, GitHubToken token) => + Write(OwnerPersona(owner), token); + + /// + /// Moves the tokens earlier versions kept in the settings file into the secret store, registers + /// the owners they were keyed by, and blanks them where they were. + /// + /// How many tokens were moved. + /// + /// The legacy owner map did double duty: it was both the registry of configured owners and the + /// place their tokens lived. The names move to + /// — which is not credential-shaped, so it stays in the settings file — while the tokens go to + /// the secret store. + /// + internal static int MigrateLegacyTokens(ProjectDirectorOptions options) + { + Ensure.NotNull(options); + + int migrated = 0; + + if (MigrateToken( + LoginPersona(options.GitHubLogin), + options.LegacyGitHubToken, + () => options.LegacyGitHubToken = new())) + { + migrated++; + } + + foreach ((GitHubOwnerName owner, GitHubToken legacy) in options.LegacyGitHubOwners.ToArray()) + { + _ = options.GitHubOwners.Add(owner); + + if (MigrateToken(OwnerPersona(owner), legacy, () => options.LegacyGitHubOwners[owner] = new())) + { + migrated++; + } + } + + // Owners whose token moved, or who never had one, no longer need an entry here at all. + foreach ((GitHubOwnerName owner, GitHubToken remaining) in options.LegacyGitHubOwners.ToArray()) + { + if (remaining.IsEmpty()) + { + _ = options.LegacyGitHubOwners.Remove(owner); + } + } + + return migrated; + } + + /// + /// Moves one token, leaving the old copy in place if the secret store will not take it. + /// + /// + /// A token already in the store wins over a legacy one, so a stale copy in the settings file + /// cannot overwrite a current credential — but the stale copy is still cleared, because ceasing + /// to write a secret does not remove the one already on disk. + /// + private static bool MigrateToken(PersonaGUID persona, GitHubToken legacy, Action clearLegacy) + { + if (legacy.IsEmpty()) + { + return false; + } + + if (Read(persona).IsEmpty() && !Write(persona, legacy)) + { + return false; + } + + clearLegacy(); + return true; + } + + private static PersonaGUID DerivePersona(string seed) + { + byte[] hash = SHA256.HashData(Encoding.UTF8.GetBytes(PersonaNamespace + seed)); + return SemanticString.Create(new Guid(hash.AsSpan(0, 16)).ToString()); + } + + /// + /// Recognises a machine with no usable secret store: the factory refusing the platform, the + /// native library failing to resolve, or the store itself reporting a failure. + /// + private static bool IsUnavailable(Exception exception) => + exception is PlatformNotSupportedException + or DllNotFoundException + or EntryPointNotFoundException + or CredentialStoreException; + + /// + /// Records the unavailable store once per process. Tokens are read on paths that run per owner + /// on every scan, so reporting each failure would bury the log. + /// + private static void ReportUnavailable(Exception exception) + { + if (Interlocked.Exchange(ref _unavailableReported, 1) == 0) + { + UnavailableReport = $"{UnavailableMessage} ({exception.Message})"; + } + } + + /// + /// The message describing why the secret store is unusable, or empty while it is fine. + /// + /// + /// Held rather than logged directly because the log in this application is an instance member of + /// ; the caller drains this into it. + /// + internal static string UnavailableReport { get; private set; } = string.Empty; + + /// + /// Returns the pending unavailable-store message and clears it, so it is reported once. + /// + internal static string DrainUnavailableReport() + { + string report = UnavailableReport; + UnavailableReport = string.Empty; + return report; + } +} diff --git a/README.md b/README.md index b457c22..98d72c2 100644 --- a/README.md +++ b/README.md @@ -57,9 +57,14 @@ dotnet run dotnet publish --configuration Release --output ./staging ``` -On first run, set the development directory that ProjectDirector should scan and supply GitHub -credentials if you want remote repositories listed. Both are persisted to the application data -folder, so subsequent runs start where you left off. +On first run, set the development directory that ProjectDirector should scan and add the GitHub +owners you want listed (**File > Add New GitHub Owner**). Give an owner a personal access token with +**File > Set GitHub Owner Token** if its repositories are private or you want a higher rate limit. + +The development directory, the owner list and the repository cache are persisted to the application +data folder. Tokens are not: they go to the operating system's secret store — Windows Credential +Manager, macOS Keychain, or libsecret (Secret Service) on Linux. A token saved by an earlier version +is moved there on the next start and emptied where it was, so nothing needs re-entering. ### Typical Workflow @@ -75,7 +80,8 @@ folder, so subsequent runs start where you left off. | Component | Responsibility | | --- | --- | | `ProjectDirector` | Main application class: ImGui loop, three-panel layout, and the fetch/pull/diff/propagate operations. | -| `ProjectDirectorOptions` | Application state persisted as JSON — dev directory, credentials, repository cache, and UI state. | +| `ProjectDirectorOptions` | Application state persisted as JSON — dev directory, configured GitHub owners, repository cache, and UI state. Deliberately holds no tokens. | +| `TokenStorage` | GitHub tokens in the OS secret store, keyed by a persona derived from the login or owner name, plus the one-time migration off the old plaintext fields. | | `GitRepository` | Abstract repository model with polymorphic JSON serialization. | | `GitHubRepository` / `AzureDevOpsRepository` | Provider-specific repository implementations. | | `PopupPropagateFile` | Modal that drives the file propagation flow. | From 820da22a716eaa754f81a3fc72225c369dc60c69 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 17:57:33 +0000 Subject: [PATCH 2/3] refactor: pull the token rules out of the ImGui code, and test them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SonarCloud's quality gate failed the PR on new-code coverage: 62.6% against a required 80%. TokenStorage and ProjectDirectorOptions were already covered; every uncovered new line was in ProjectDirector.cs, which has no tests at all. CLAUDE.md already says what to do about that: the part with a rule in it is pulled out into a plain method so it can be driven without a live ImGui context or a display. Four were still tangled up with drawing. ResolveGitHubCredentials is the important one. An owner's own token shadowing the account-level token is what makes a private repository in another organization reachable, and it was an inline ternary in the scan loop with nothing pinning it. It now also answers null rather than building credentials around a blank secret, so a misconfigured owner produces an anonymous request instead of authenticating as nobody. ApplyOwnerToken carries the refusal case: the popup closes whether or not the secret store took the token, so a silent failure would look exactly like success. OwnersInDisplayOrder removes a duplicated sort. The owner registry became a set in this branch, and a set does not promise an order, so the owner panels and the token menu could otherwise disagree with each other and between runs. DescribeTokenMigration is the startup log line. What is left uncovered in ProjectDirector.cs is opening popups, drawing the menu and assigning to the client — drawing and wiring, with no rule left in them. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018AnVpPWKjVtXnAvd4bnzUL --- ProjectDirector.Test/TokenStorageTests.cs | 104 +++++++++++++++++++++ ProjectDirector/ProjectDirector.cs | 109 +++++++++++++++++----- 2 files changed, 188 insertions(+), 25 deletions(-) diff --git a/ProjectDirector.Test/TokenStorageTests.cs b/ProjectDirector.Test/TokenStorageTests.cs index 6e7c2d3..d5117b1 100644 --- a/ProjectDirector.Test/TokenStorageTests.cs +++ b/ProjectDirector.Test/TokenStorageTests.cs @@ -11,7 +11,10 @@ namespace ktsu.ProjectDirector.Test; using Microsoft.VisualStudio.TestTools.UnitTesting; +using Octokit; + using CredentialCache = ktsu.CredentialCache.CredentialCache; +using ICredentialStore = ktsu.CredentialCache.Storage.ICredentialStore; /// /// Covers where GitHub personal access tokens live. The settings file sits next to window state and @@ -302,4 +305,105 @@ public void WritingWithoutASecretStoreReportsFailure() Assert.IsFalse(TokenStorage.WriteOwnerToken(Owner("ktsu-dev"), Token("ghp_owner"))); } + + /// + /// An owner with its own token authenticates as that owner, which is what makes a private + /// repository in another organization reachable. + /// + [TestMethod] + public void OwnerTokenShadowsTheAccountToken() + { + Credentials? credentials = ProjectDirector.ResolveGitHubCredentials( + Owner("ktsu-dev"), + Token("ghp_owner"), + GitHubLogin.Create("someone"), + Token("ghp_account")); + + Assert.IsNotNull(credentials); + Assert.AreEqual("ktsu-dev", credentials.Login); + Assert.AreEqual("ghp_owner", credentials.Password); + } + + /// + /// Without an owner token the account-level login and token are used. + /// + [TestMethod] + public void AccountCredentialsAreUsedWhenTheOwnerHasNoToken() + { + Credentials? credentials = ProjectDirector.ResolveGitHubCredentials( + Owner("ktsu-dev"), + new(), + GitHubLogin.Create("someone"), + Token("ghp_account")); + + Assert.IsNotNull(credentials); + Assert.AreEqual("someone", credentials.Login); + Assert.AreEqual("ghp_account", credentials.Password); + } + + /// + /// With nothing usable the answer is no credentials, rather than credentials carrying a blank + /// secret — an anonymous request is a better failure than one that authenticates as nobody. + /// + [TestMethod] + public void NoCredentialsWhenNothingIsConfigured() + { + Assert.IsNull(ProjectDirector.ResolveGitHubCredentials(Owner("ktsu-dev"), new(), new(), new())); + Assert.IsNull(ProjectDirector.ResolveGitHubCredentials( + Owner("ktsu-dev"), new(), GitHubLogin.Create("someone"), new())); + Assert.IsNull(ProjectDirector.ResolveGitHubCredentials( + Owner("ktsu-dev"), new(), new(), Token("ghp_account"))); + } + + /// + /// A token typed into the popup reaches the secret store, and nothing is logged. + /// + [TestMethod] + public void ApplyOwnerTokenStoresTheTokenSilently() + { + string message = ProjectDirector.ApplyOwnerToken(Owner("ktsu-dev"), "ghp_typed"); + + Assert.AreEqual(string.Empty, message); + Assert.AreEqual("ghp_typed", TokenStorage.ReadOwnerToken(Owner("ktsu-dev")).ToString()); + } + + /// + /// When the store refuses the token the user is told. The popup closes either way, so silence + /// here would look exactly like success. + /// + [TestMethod] + public void ApplyOwnerTokenReportsARefusal() + { + using CredentialCache unavailable = new(new UnavailableCredentialStore()); + TokenStorage.UseCache(unavailable); + + string message = ProjectDirector.ApplyOwnerToken(Owner("ktsu-dev"), "ghp_typed"); + + Assert.Contains("secret store", message, StringComparison.Ordinal); + } + + /// + /// Migrating nothing says nothing; migrating something says how much. + /// + [TestMethod] + public void MigrationIsDescribedOnlyWhenSomethingMoved() + { + Assert.AreEqual(string.Empty, ProjectDirector.DescribeTokenMigration(0)); + Assert.Contains("2", ProjectDirector.DescribeTokenMigration(2), StringComparison.Ordinal); + Assert.Contains("secret store", ProjectDirector.DescribeTokenMigration(2), StringComparison.Ordinal); + } + + /// + /// Owners list in a stable order, so the owner panels and the token menu agree with each other + /// and with the previous run. The registry they come from is a set, which does not promise one. + /// + [TestMethod] + public void OwnersListInAStableOrder() + { + HashSet owners = [Owner("ktsu-io"), Owner("acme"), Owner("ktsu-dev")]; + + string ordered = string.Join(",", ProjectDirector.OwnersInDisplayOrder(owners)); + + Assert.AreEqual("acme,ktsu-dev,ktsu-io", ordered); + } } diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index 347302f..9c7ca9b 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -103,17 +103,13 @@ public ProjectDirector() QueueSaveOptions(); } - GitHubToken startupToken = Options.GitHubToken; - if (!string.IsNullOrEmpty(Options.GitHubLogin) && !string.IsNullOrEmpty(startupToken)) + Credentials? startupCredentials = ResolveGitHubCredentials(new(), new(), Options.GitHubLogin, Options.GitHubToken); + if (startupCredentials is not null) { - GitHubClient.Credentials = new(Options.GitHubLogin, startupToken); - } - - if (migratedTokens > 0) - { - QueueLog($"Moved {migratedTokens} GitHub token(s) out of the settings file into the OS secret store"); + GitHubClient.Credentials = startupCredentials; } + QueueLogIfAny(DescribeTokenMigration(migratedTokens)); DrainSecretStoreReport(); RefreshPage(); @@ -186,15 +182,80 @@ private void RestoreDividerStates() /// Writes the secret store's complaint to the log, if it has one. /// records the message rather than logging it, because the log is an instance member here. /// - private void DrainSecretStoreReport() + private void DrainSecretStoreReport() => QueueLogIfAny(TokenStorage.DrainUnavailableReport()); + + private void QueueLogIfAny(string message) + { + if (message.Length > 0) + { + QueueLog(message); + } + } + + /// + /// Picks the credentials for a request against . + /// + /// + /// The owner's own token when it has one, otherwise the account-level login and token, or + /// when neither is usable — which leaves the client unauthenticated + /// rather than authenticating with a blank secret. + /// + /// + /// The owner token shadowing the account token is the rule that makes a private repository in + /// another organization reachable, so it is here as a plain method rather than inline in the + /// scan loop. + /// + internal static Credentials? ResolveGitHubCredentials( + GitHubOwnerName owner, + GitHubToken ownerToken, + GitHubLogin login, + GitHubToken accountToken) { - string report = TokenStorage.DrainUnavailableReport(); - if (report.Length > 0) + if (!string.IsNullOrEmpty(ownerToken)) { - QueueLog(report); + return new Credentials(owner, ownerToken); } + + return !string.IsNullOrEmpty(login) && !string.IsNullOrEmpty(accountToken) + ? new Credentials(login, accountToken) + : null; + } + + /// + /// Stores the token a user typed for . + /// + /// + /// The message to log when the secret store refused the token, or empty when it took it. A + /// refusal has to reach the user: the popup closes either way, so silence would look like + /// success. + /// + internal static string ApplyOwnerToken(GitHubOwnerName owner, string typed) => + TokenStorage.WriteOwnerToken(owner, GitHubToken.Create(typed)) + ? string.Empty + : TokenStorage.DrainUnavailableReport(); + + /// + /// The configured owners in a stable display order. + /// + /// + /// The owner registry used to be a dictionary, whose enumeration order is not guaranteed, so the + /// owner panels and the token menu could disagree between runs. Ordering them once, here, is what + /// keeps both lists the same and predictable. + /// + internal static IEnumerable OwnersInDisplayOrder(IEnumerable owners) + { + Ensure.NotNull(owners); + return owners.OrderBy(owner => owner.ToString(), StringComparer.Ordinal); } + /// + /// The line to log after a startup migration, or empty when nothing moved. + /// + internal static string DescribeTokenMigration(int migratedTokens) => + migratedTokens > 0 + ? $"Moved {migratedTokens} GitHub token(s) out of the settings file into the OS secret store" + : string.Empty; + private void SaveOptionsIfRequired() { //debounce the save requests and avoid saving multiple times per frame or multiple frames in a row @@ -483,13 +544,11 @@ private void Tick(float dt) { GitHubOwnerName owner = OwnerPendingTokenPopup; OwnerPendingTokenPopup = null; - PopupSetGitHubOwnerToken.Open($"Set Token for {owner}", "Personal Access Token", string.Empty, result => - { - if (!TokenStorage.WriteOwnerToken(owner, GitHubToken.Create(result))) - { - DrainSecretStoreReport(); - } - }); + PopupSetGitHubOwnerToken.Open( + $"Set Token for {owner}", + "Personal Access Token", + string.Empty, + result => QueueLogIfAny(ApplyOwnerToken(owner, result))); } _ = PopupSetDevDirectory.ShowIfOpen(); @@ -718,7 +777,7 @@ private void ShowMenu() if (Options.GitHubOwners.Count > 0 && ImGui.BeginMenu("Set GitHub Owner Token")) { - foreach (GitHubOwnerName owner in Options.GitHubOwners.OrderBy(o => o.ToString(), StringComparer.Ordinal)) + foreach (GitHubOwnerName owner in OwnersInDisplayOrder(Options.GitHubOwners)) { if (ImGui.MenuItem(owner)) { @@ -757,7 +816,7 @@ private void ShowMenu() private void ShowOwners() { - foreach (GitHubOwnerName owner in Options.GitHubOwners.OrderBy(o => o.ToString(), StringComparer.Ordinal)) + foreach (GitHubOwnerName owner in OwnersInDisplayOrder(Options.GitHubOwners)) { ShowCollapsiblePanel(owner, () => ShowRepos(owner)); } @@ -946,11 +1005,11 @@ private void ScanRemoteAccountsForRepos() { foreach (GitHubOwnerName owner in Options.GitHubOwners.ToArray()) { - GitHubToken pat = TokenStorage.ReadOwnerToken(owner); - GitHubToken accountToken = Options.GitHubToken; - if (!string.IsNullOrEmpty(pat) || (!string.IsNullOrEmpty(Options.GitHubLogin) && !string.IsNullOrEmpty(accountToken))) + Credentials? credentials = ResolveGitHubCredentials( + owner, TokenStorage.ReadOwnerToken(owner), Options.GitHubLogin, Options.GitHubToken); + if (credentials is not null) { - GitHubClient.Credentials = !string.IsNullOrEmpty(pat) ? new(owner, pat) : new(Options.GitHubLogin, accountToken); + GitHubClient.Credentials = credentials; } SyncGitHubOwnerInfo(owner); From dc7c2384077db36d166ee679a36a97166d9d2852 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 18:09:56 +0000 Subject: [PATCH 3/3] refactor: pin the startup token ordering Migration has to run before the account token is read. For a user upgrading from a version that kept the token in the settings file, the token only exists in the secret store once migration has put it there, so resolving credentials first would start that session unauthenticated and only pick the credentials up on the next launch. That ordering was implicit in the constructor, where nothing could test it. PrepareTokens makes it one plain method with the reason written down, and StartupMigratesBeforeResolvingCredentials fails if the two are swapped. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018AnVpPWKjVtXnAvd4bnzUL --- ProjectDirector.Test/TokenStorageTests.cs | 39 +++++++++++++++++++++++ ProjectDirector/ProjectDirector.cs | 35 ++++++++++++++++---- 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/ProjectDirector.Test/TokenStorageTests.cs b/ProjectDirector.Test/TokenStorageTests.cs index d5117b1..c798fee 100644 --- a/ProjectDirector.Test/TokenStorageTests.cs +++ b/ProjectDirector.Test/TokenStorageTests.cs @@ -406,4 +406,43 @@ public void OwnersListInAStableOrder() Assert.AreEqual("acme,ktsu-dev,ktsu-io", ordered); } + + /// + /// A user upgrading from a version that kept the token in the settings file is authenticated on + /// that same launch, not the next one. The token only exists in the secret store once migration + /// has put it there, so resolving credentials before migrating would start the session + /// unauthenticated — this pins the order. + /// + [TestMethod] + public void StartupMigratesBeforeResolvingCredentials() + { + using ProjectDirectorOptions options = new() + { + GitHubLogin = GitHubLogin.Create("someone"), + LegacyGitHubToken = Token("ghp_from_settings_file"), + }; + + ProjectDirector.TokenStartup startup = ProjectDirector.PrepareTokens(options); + + Assert.AreEqual(1, startup.Migrated); + Assert.IsNotNull(startup.Credentials); + Assert.AreEqual("someone", startup.Credentials.Login); + Assert.AreEqual("ghp_from_settings_file", startup.Credentials.Password); + Assert.Contains("secret store", startup.Log, StringComparison.Ordinal); + } + + /// + /// A clean start has nothing to migrate, nothing to say, and no credentials to offer. + /// + [TestMethod] + public void StartupWithNoTokensIsSilentAndUnauthenticated() + { + using ProjectDirectorOptions options = new(); + + ProjectDirector.TokenStartup startup = ProjectDirector.PrepareTokens(options); + + Assert.AreEqual(0, startup.Migrated); + Assert.IsNull(startup.Credentials); + Assert.AreEqual(string.Empty, startup.Log); + } } diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index 9c7ca9b..695e0b8 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -97,19 +97,18 @@ public ProjectDirector() GitHubClient = new(new ProductHeaderValue("ktsu.ProjectDirector")); - int migratedTokens = TokenStorage.MigrateLegacyTokens(Options); - if (migratedTokens > 0) + TokenStartup startup = PrepareTokens(Options); + if (startup.Migrated > 0) { QueueSaveOptions(); } - Credentials? startupCredentials = ResolveGitHubCredentials(new(), new(), Options.GitHubLogin, Options.GitHubToken); - if (startupCredentials is not null) + if (startup.Credentials is not null) { - GitHubClient.Credentials = startupCredentials; + GitHubClient.Credentials = startup.Credentials; } - QueueLogIfAny(DescribeTokenMigration(migratedTokens)); + QueueLogIfAny(startup.Log); DrainSecretStoreReport(); RefreshPage(); @@ -234,6 +233,30 @@ internal static string ApplyOwnerToken(GitHubOwnerName owner, string typed) => ? string.Empty : TokenStorage.DrainUnavailableReport(); + /// + /// What the startup token pass decided: how many tokens moved out of the settings file, the + /// credentials to start with, and the line to log. + /// + internal sealed record TokenStartup(int Migrated, Credentials? Credentials, string Log); + + /// + /// Migrates any tokens left in the settings file, then works out the credentials to start with. + /// + /// + /// The order is the point. Migration has to run before the account token is read, because for a + /// user upgrading from a version that kept the token in the settings file, the token only exists + /// in the secret store once migration has put it there. Resolving first would start the session + /// unauthenticated and only pick the credentials up on the next launch. + /// + internal static TokenStartup PrepareTokens(ProjectDirectorOptions options) + { + Ensure.NotNull(options); + + int migrated = TokenStorage.MigrateLegacyTokens(options); + Credentials? credentials = ResolveGitHubCredentials(new(), new(), options.GitHubLogin, options.GitHubToken); + return new(migrated, credentials, DescribeTokenMigration(migrated)); + } + /// /// The configured owners in a stable display order. ///