Skip to content

Commit 89d2c2c

Browse files
committed
feat: keep provider and owner access tokens in the OS secret store [minor]
GitHub and Azure DevOps personal access tokens, usually scoped to repo and workflow, were plain [JsonInclude] fields on BuildProvider and Owner. Every QueueSaveAppData serialized the whole object graph to an unencrypted file under the user's app data directory, readable by anything running as the same user and picked up by any backup or file-sync tool covering it. TokenStorage now holds them in the platform-native secret store through ktsu.CredentialCache, under a service name scoped to BuildMonitor. Personas are derived rather than stored: a versioned namespace plus the provider name, and the owner name as well for an override. AppData is left with no credential-shaped state at all, and the owner-shadows-provider semantics stay a lookup order rather than extra persisted fields. The namespace carries a version because changing the derivation would orphan every token already in the store. LegacyToken keeps the old Token JSON name so OnStart can migrate: written to the secret store first, so a store that throws cannot lose it, and only then blanked in the file. A token already in the store wins over a stale copy, but the stale copy is still cleared — ceasing to write a secret does not remove the one already on disk. With no usable secret store, tokens read as empty and the reason is logged once rather than on every request path. A throw out of a token read would take down the render loop of a desktop app. There is no plaintext fallback. Fixes #278 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018AnVpPWKjVtXnAvd4bnzUL
1 parent 6b127ed commit 89d2c2c

9 files changed

Lines changed: 655 additions & 5 deletions

File tree

Lines changed: 357 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,357 @@
1+
// Copyright (c) 2023-2026 ktsu-dev contributors
2+
3+
namespace ktsu.BuildMonitor.Test;
4+
5+
using System.Text.Json;
6+
using System.Text.Json.Serialization;
7+
8+
using ktsu.CredentialCache;
9+
using ktsu.CredentialCache.Storage;
10+
using ktsu.RoundTripStringJsonConverter;
11+
using ktsu.Semantics.Strings;
12+
13+
using Microsoft.VisualStudio.TestTools.UnitTesting;
14+
15+
using CredentialCache = ktsu.CredentialCache.CredentialCache;
16+
17+
/// <summary>
18+
/// Covers where provider and owner access tokens live. They are GitHub and Azure DevOps personal
19+
/// access tokens, so the contract under test is that they reach the OS secret store and do not
20+
/// survive in the app data file earlier versions serialized them into.
21+
/// </summary>
22+
[TestClass]
23+
public sealed class TokenStorageTests
24+
{
25+
private CredentialCache Cache { get; set; } = null!;
26+
27+
[TestInitialize]
28+
public void SetUp()
29+
{
30+
Cache = new CredentialCache(new InMemoryCredentialStore());
31+
TokenStorage.UseCache(Cache);
32+
}
33+
34+
[TestCleanup]
35+
public void TearDown()
36+
{
37+
TokenStorage.UseCache(null);
38+
Cache.Dispose();
39+
}
40+
41+
/// <summary>
42+
/// A provider with no network behaviour, so the storage contract can be driven without a real
43+
/// GitHub or Azure DevOps client.
44+
/// </summary>
45+
private sealed class TestProvider(string name) : BuildProvider
46+
{
47+
internal override BuildProviderName Name { get; } = name.As<BuildProviderName>();
48+
49+
/// <summary>
50+
/// Writes the provider token the same way the base class's setter does. The setter itself is
51+
/// private, so this reaches the storage directly rather than widening it for a test.
52+
/// </summary>
53+
internal void SetToken(string token) =>
54+
Assert.IsTrue(TokenStorage.Write(TokenPersona, token.As<BuildProviderToken>()));
55+
56+
internal BuildProviderToken ReadToken() => Token;
57+
58+
internal override Task UpdateRepositoriesAsync(Owner owner) => Task.CompletedTask;
59+
internal override Task UpdateBuildsAsync(Repository repository) => Task.CompletedTask;
60+
internal override Task UpdateBuildAsync(Build build) => Task.CompletedTask;
61+
internal override Task UpdateRunAsync(Run run) => Task.CompletedTask;
62+
}
63+
64+
/// <summary>
65+
/// A store standing in for a machine whose native secret library will not load — a Linux host
66+
/// with no Secret Service provider, which is what a container or an SSH session usually is.
67+
/// </summary>
68+
private sealed class UnavailableCredentialStore : ICredentialStore
69+
{
70+
public string Name => "Unavailable";
71+
72+
public bool TryLoad(PersonaGUID persona, out Credential? credential) =>
73+
throw new DllNotFoundException("libsecret-1.so.0");
74+
75+
public void Save(PersonaGUID persona, Credential credential) =>
76+
throw new DllNotFoundException("libsecret-1.so.0");
77+
78+
public bool Remove(PersonaGUID persona) => throw new DllNotFoundException("libsecret-1.so.0");
79+
}
80+
81+
/// <summary>
82+
/// Mirrors how <see cref="ktsu.AppDataStorage"/> writes the app data file: references preserved,
83+
/// and semantic strings round-tripped as plain strings rather than as char arrays.
84+
/// </summary>
85+
private static readonly JsonSerializerOptions AppDataLike = BuildAppDataLikeOptions();
86+
87+
private static JsonSerializerOptions BuildAppDataLikeOptions()
88+
{
89+
JsonSerializerOptions options = new()
90+
{
91+
ReferenceHandler = ReferenceHandler.Preserve,
92+
};
93+
options.Converters.Add(new RoundTripStringJsonConverterFactory());
94+
return options;
95+
}
96+
97+
private static TestProvider NewProvider(string name = "TestHub") => new(name);
98+
99+
private static Owner AddOwner(TestProvider provider, string name)
100+
{
101+
OwnerName ownerName = name.As<OwnerName>();
102+
Owner owner = provider.CreateOwner(ownerName);
103+
Assert.IsTrue(provider.Owners.TryAdd(ownerName, owner));
104+
return owner;
105+
}
106+
107+
/// <summary>
108+
/// Two providers never share a persona, so setting one's token cannot disturb the other's.
109+
/// </summary>
110+
[TestMethod]
111+
public void ProviderPersonasAreDistinct()
112+
{
113+
PersonaGUID gitHub = TokenStorage.ProviderPersona("GitHub".As<BuildProviderName>());
114+
PersonaGUID azure = TokenStorage.ProviderPersona("AzureDevOps".As<BuildProviderName>());
115+
116+
Assert.AreNotEqual(gitHub, azure);
117+
}
118+
119+
/// <summary>
120+
/// The same organization name can exist on more than one provider, so the owner persona is scoped
121+
/// by provider too.
122+
/// </summary>
123+
[TestMethod]
124+
public void OwnerPersonasAreScopedByProvider()
125+
{
126+
OwnerName owner = "ktsu-dev".As<OwnerName>();
127+
PersonaGUID onGitHub = TokenStorage.OwnerPersona("GitHub".As<BuildProviderName>(), owner);
128+
PersonaGUID onAzure = TokenStorage.OwnerPersona("AzureDevOps".As<BuildProviderName>(), owner);
129+
130+
Assert.AreNotEqual(onGitHub, onAzure);
131+
}
132+
133+
/// <summary>
134+
/// An owner's persona is never the provider's, so an owner override cannot overwrite the
135+
/// provider-level token.
136+
/// </summary>
137+
[TestMethod]
138+
public void OwnerPersonaIsNotTheProviderPersona()
139+
{
140+
BuildProviderName provider = "GitHub".As<BuildProviderName>();
141+
142+
Assert.AreNotEqual(
143+
TokenStorage.ProviderPersona(provider),
144+
TokenStorage.OwnerPersona(provider, "ktsu-dev".As<OwnerName>()));
145+
}
146+
147+
/// <summary>
148+
/// Persona derivation is deterministic, and pinned. Changing it would orphan every token already
149+
/// in the store, so it must not drift silently.
150+
/// </summary>
151+
[TestMethod]
152+
public void PersonaDerivationIsStable()
153+
{
154+
BuildProviderName provider = "GitHub".As<BuildProviderName>();
155+
156+
Assert.AreEqual(
157+
TokenStorage.ProviderPersona(provider).ToString(),
158+
TokenStorage.ProviderPersona(provider).ToString());
159+
Assert.IsTrue(Guid.TryParse(TokenStorage.ProviderPersona(provider).ToString(), out _));
160+
}
161+
162+
/// <summary>
163+
/// A provider token set through the normal path is readable again, and came from the store.
164+
/// </summary>
165+
[TestMethod]
166+
public void ProviderTokenRoundTripsThroughTheStore()
167+
{
168+
TestProvider provider = NewProvider();
169+
170+
provider.SetToken("ghp_provider");
171+
172+
Assert.AreEqual("ghp_provider", provider.ReadToken().ToString());
173+
Assert.IsTrue(Cache.TryGet(provider.TokenPersona, out Credential? credential));
174+
Assert.IsInstanceOfType<CredentialWithToken>(credential);
175+
}
176+
177+
/// <summary>
178+
/// Setting a token leaves nothing behind in the field the app data file is built from. This is
179+
/// the whole point of the change.
180+
/// </summary>
181+
[TestMethod]
182+
public void SettingAProviderTokenWritesNothingToAppData()
183+
{
184+
TestProvider provider = NewProvider();
185+
186+
provider.SetToken("ghp_provider");
187+
188+
Assert.IsTrue(provider.LegacyToken.IsEmpty());
189+
}
190+
191+
/// <summary>
192+
/// The serialized form of a provider carries no token. A reader of the app data file learns
193+
/// nothing.
194+
/// </summary>
195+
[TestMethod]
196+
public void SerializedProviderCarriesNoToken()
197+
{
198+
TestProvider provider = NewProvider();
199+
provider.SetToken("ghp_secret_value");
200+
201+
string json = JsonSerializer.Serialize(provider, AppDataLike);
202+
203+
Assert.DoesNotContain("ghp_secret_value", json, StringComparison.Ordinal);
204+
Assert.Contains("\"Token\":\"\"", json, StringComparison.Ordinal);
205+
}
206+
207+
/// <summary>
208+
/// An owner override is stored separately from the provider token, and both survive.
209+
/// </summary>
210+
[TestMethod]
211+
public void OwnerTokenIsStoredSeparatelyFromTheProviderToken()
212+
{
213+
TestProvider provider = NewProvider();
214+
Owner owner = AddOwner(provider, "ktsu-dev");
215+
216+
provider.SetToken("ghp_provider");
217+
owner.Token = "ghp_owner".As<BuildProviderToken>();
218+
219+
Assert.AreEqual("ghp_provider", provider.ReadToken().ToString());
220+
Assert.AreEqual("ghp_owner", owner.Token.ToString());
221+
Assert.IsTrue(owner.HasToken);
222+
Assert.IsTrue(owner.LegacyToken.IsEmpty());
223+
}
224+
225+
/// <summary>
226+
/// Clearing a token removes it from the store rather than leaving an empty entry behind.
227+
/// </summary>
228+
[TestMethod]
229+
public void ClearingATokenRemovesItFromTheStore()
230+
{
231+
TestProvider provider = NewProvider();
232+
Owner owner = AddOwner(provider, "ktsu-dev");
233+
owner.Token = "ghp_owner".As<BuildProviderToken>();
234+
235+
owner.Token = new();
236+
237+
Assert.IsFalse(owner.HasToken);
238+
Assert.IsFalse(Cache.TryGet(owner.TokenPersona, out _));
239+
}
240+
241+
/// <summary>
242+
/// A provider token and an owner token left by an earlier version are both moved into the store.
243+
/// </summary>
244+
[TestMethod]
245+
public void MigrationMovesProviderAndOwnerTokens()
246+
{
247+
TestProvider provider = NewProvider();
248+
Owner owner = AddOwner(provider, "ktsu-dev");
249+
provider.LegacyToken = "ghp_old_provider".As<BuildProviderToken>();
250+
owner.LegacyToken = "ghp_old_owner".As<BuildProviderToken>();
251+
252+
int migrated = TokenStorage.MigrateLegacyTokens([provider]);
253+
254+
Assert.AreEqual(2, migrated);
255+
Assert.AreEqual("ghp_old_provider", provider.ReadToken().ToString());
256+
Assert.AreEqual("ghp_old_owner", owner.Token.ToString());
257+
}
258+
259+
/// <summary>
260+
/// Migration also blanks the old copies. Ceasing to write a secret does not remove the one
261+
/// already on disk, so this is the half that does the security work.
262+
/// </summary>
263+
[TestMethod]
264+
public void MigrationBlanksThePlaintextCopies()
265+
{
266+
TestProvider provider = NewProvider();
267+
Owner owner = AddOwner(provider, "ktsu-dev");
268+
provider.LegacyToken = "ghp_old_provider".As<BuildProviderToken>();
269+
owner.LegacyToken = "ghp_old_owner".As<BuildProviderToken>();
270+
271+
_ = TokenStorage.MigrateLegacyTokens([provider]);
272+
273+
Assert.IsTrue(provider.LegacyToken.IsEmpty());
274+
Assert.IsTrue(owner.LegacyToken.IsEmpty());
275+
}
276+
277+
/// <summary>
278+
/// With nothing left over there is nothing to migrate, so a second start is a no-op.
279+
/// </summary>
280+
[TestMethod]
281+
public void MigrationIsIdempotent()
282+
{
283+
TestProvider provider = NewProvider();
284+
provider.LegacyToken = "ghp_old_provider".As<BuildProviderToken>();
285+
286+
_ = TokenStorage.MigrateLegacyTokens([provider]);
287+
288+
Assert.AreEqual(0, TokenStorage.MigrateLegacyTokens([provider]));
289+
Assert.AreEqual("ghp_old_provider", provider.ReadToken().ToString());
290+
}
291+
292+
/// <summary>
293+
/// A stale token in the old file must not overwrite the one currently in the store — but it is
294+
/// still cleared.
295+
/// </summary>
296+
[TestMethod]
297+
public void MigrationKeepsTheStoredTokenAndStillClearsTheStaleOne()
298+
{
299+
TestProvider provider = NewProvider();
300+
provider.SetToken("ghp_current");
301+
provider.LegacyToken = "ghp_stale".As<BuildProviderToken>();
302+
303+
Assert.AreEqual(1, TokenStorage.MigrateLegacyTokens([provider]));
304+
Assert.AreEqual("ghp_current", provider.ReadToken().ToString());
305+
Assert.IsTrue(provider.LegacyToken.IsEmpty());
306+
}
307+
308+
/// <summary>
309+
/// If the secret store will not take the token, the old copy stays where it is. Clearing it would
310+
/// destroy the only copy the user has.
311+
/// </summary>
312+
[TestMethod]
313+
public void MigrationKeepsTheLegacyTokenWhenTheStoreRefuses()
314+
{
315+
using CredentialCache unavailable = new(new UnavailableCredentialStore());
316+
TokenStorage.UseCache(unavailable);
317+
318+
TestProvider provider = NewProvider();
319+
provider.LegacyToken = "ghp_old_provider".As<BuildProviderToken>();
320+
321+
Assert.AreEqual(0, TokenStorage.MigrateLegacyTokens([provider]));
322+
Assert.AreEqual("ghp_old_provider", provider.LegacyToken.ToString());
323+
}
324+
325+
/// <summary>
326+
/// A machine with no secret store reads as "no token" rather than throwing. BuildMonitor is a
327+
/// desktop application, and an exception out of a token read would take down the render loop; it
328+
/// reports the problem in the log instead. What it must never do is fall back to a plain file.
329+
/// </summary>
330+
[TestMethod]
331+
public void ReadingWithoutASecretStoreIsEmptyRatherThanFatal()
332+
{
333+
using CredentialCache unavailable = new(new UnavailableCredentialStore());
334+
TokenStorage.UseCache(unavailable);
335+
336+
TestProvider provider = NewProvider();
337+
338+
Assert.IsTrue(provider.ReadToken().IsEmpty());
339+
Assert.IsFalse(TokenStorage.Write(provider.TokenPersona, "ghp_x".As<BuildProviderToken>()));
340+
}
341+
342+
/// <summary>
343+
/// Two owners under one provider do not share a token.
344+
/// </summary>
345+
[TestMethod]
346+
public void OwnersDoNotShareTokens()
347+
{
348+
TestProvider provider = NewProvider();
349+
Owner first = AddOwner(provider, "ktsu-dev");
350+
Owner second = AddOwner(provider, "ktsu-io");
351+
352+
first.Token = "ghp_first".As<BuildProviderToken>();
353+
354+
Assert.AreEqual("ghp_first", first.Token.ToString());
355+
Assert.IsFalse(second.HasToken);
356+
}
357+
}

‎BuildMonitor/BuildMonitor.cs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,13 @@ private static void OnStart()
6666

6767
// add more providers here as needed
6868

69+
int migratedTokens = TokenStorage.MigrateLegacyTokens(AppData.BuildProviders.Values);
70+
if (migratedTokens > 0)
71+
{
72+
Log.Info($"Moved {migratedTokens} access token(s) out of the app data file into the OS secret store");
73+
needsSave = true;
74+
}
75+
6976
if (needsSave)
7077
{
7178
QueueSaveAppData();

‎BuildMonitor/BuildMonitor.csproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
<ItemGroup>
1313
<PackageReference Include="Humanizer.Core" ExcludeAssets="analyzers" />
1414
<PackageReference Include="ktsu.AppDataStorage" />
15+
<PackageReference Include="ktsu.CredentialCache" />
1516
<PackageReference Include="ktsu.Extensions" />
1617
<PackageReference Include="ktsu.Semantics.Strings" />
1718
<PackageReference Include="ktsu.TextFilter" />

0 commit comments

Comments
 (0)