diff --git a/.gitignore b/.gitignore index dc0470a..e043c9f 100644 --- a/.gitignore +++ b/.gitignore @@ -203,6 +203,11 @@ PublishScripts/ **/[Pp]ackages/* # except build/, which is used as an MSBuild target. !**/[Pp]ackages/build/ +# and except a Unity project's Packages/, which is source: Unity's package manifest and its +# resolved lock file are both meant to be committed, and a NuGet restore folder never contains +# a file by either name. +!**/[Pp]ackages/manifest.json +!**/[Pp]ackages/packages-lock.json # Uncomment if necessary however generally it will be regenerated when needed #!**/[Pp]ackages/repositories.config # NuGet v3's project.json files produces more ignorable files @@ -651,3 +656,16 @@ Temporary Items # ImGui.ini files imgui.ini + +# Game engine projects +# +# Godot: the import cache, and the mono/temp bin+obj a C# build writes. +.godot/ + +# Unity: .meta files are source, not the Visual Studio C++ build artifact that the `*.meta` rule +# further up targets. Unity generates one per asset and it carries the GUID that scenes, prefabs +# and serialized references point at, so ignoring them gives every clone fresh GUIDs and silently +# breaks those references - including for a plug-in whose .dll is itself a build output. This +# negation has to come after that rule to win, and is scoped to the asset tree so the Visual +# Studio artifact stays ignored everywhere else. +!**/[Aa]ssets/**/*.meta diff --git a/BuildMonitor.Test/GitHubRequestOutcomeTests.cs b/BuildMonitor.Test/GitHubRequestOutcomeTests.cs index 178ca2a..02c86cf 100644 --- a/BuildMonitor.Test/GitHubRequestOutcomeTests.cs +++ b/BuildMonitor.Test/GitHubRequestOutcomeTests.cs @@ -104,7 +104,7 @@ public async Task ARateLimited403ReportsFailure() } /// - /// The other headline case: a plain 403, which is what a just-revoked token looks like. + /// The other headline case: a plain 403, a request the token is not permitted to make. /// [TestMethod] public async Task APlain403ReportsFailure() @@ -114,9 +114,76 @@ public async Task APlain403ReportsFailure() bool succeeded = await RunFailing(provider, ApiFailure(HttpStatusCode.Forbidden)).ConfigureAwait(false); Assert.IsFalse(succeeded, "A request refused for lack of authorization did not happen, and must not report success."); + Assert.AreEqual(ProviderStatus.Error, provider.Status); + } + + /// + /// A 403 that is not a rate limit refuses one request -- a workflow dispatch on a repository the + /// token lacks actions: write for, say. The token still works for everything else, so the + /// refusal must not erase it along with every owner that depends on it. + /// + [TestMethod] + public async Task APlain403LeavesTheCredentialsInPlace() + { + GitHub provider = new(); + SetProviderToken(provider, "provider-pat"); + Owner owner = OwnerWithToken(provider); + + bool succeeded = await provider.MakeGitHubRequestAsync( + RequestName, () => Task.FromException(ApiFailure(HttpStatusCode.Forbidden)), owner).ConfigureAwait(false); + + Assert.IsFalse(succeeded); + Assert.AreEqual("provider-pat", ProviderToken(provider)); + Assert.AreEqual("alpha-pat", owner.Token.ToString()); + } + + /// + /// A request sent with an owner's override token that GitHub rejects condemns that token, not + /// the provider's. Clearing the provider token took every other owner offline, while the owner + /// token that actually failed stayed in place and was rejected again every cycle. + /// + [TestMethod] + public async Task ARejectedOwnerTokenClearsOnlyThatOwnersToken() + { + GitHub provider = new(); + SetProviderToken(provider, "provider-pat"); + Owner owner = OwnerWithToken(provider); + AuthorizationException unauthorized = new(new FakeResponse(HttpStatusCode.Unauthorized, new Dictionary())); + + bool succeeded = await provider.MakeGitHubRequestAsync( + RequestName, () => Task.FromException(unauthorized), owner).ConfigureAwait(false); + + Assert.IsFalse(succeeded); + Assert.IsFalse(owner.HasToken, "The owner token GitHub rejected must not be sent again."); + Assert.AreEqual("provider-pat", ProviderToken(provider), "The provider token was not the one rejected."); + Assert.AreEqual(ProviderStatus.AuthFailed, provider.Status); + } + + /// + /// An owner without its own token uses the provider's, so a rejection there still clears the + /// provider credentials as before. + /// + [TestMethod] + public async Task ARejectedProviderTokenStillClearsTheProviderCredentials() + { + GitHub provider = new(); + SetProviderToken(provider, "provider-pat"); + Owner owner = provider.CreateOwner(OwnerName.Create("beta")); + AuthorizationException unauthorized = new(new FakeResponse(HttpStatusCode.Unauthorized, new Dictionary())); + + bool succeeded = await provider.MakeGitHubRequestAsync( + RequestName, () => Task.FromException(unauthorized), owner).ConfigureAwait(false); + + Assert.IsFalse(succeeded); + Assert.AreEqual(string.Empty, ProviderToken(provider)); Assert.AreEqual(ProviderStatus.AuthFailed, provider.Status); } + private static void SetProviderToken(GitHub provider, string token) => + Assert.IsTrue(TokenStorage.Write(provider.TokenPersona, BuildProviderToken.Create(token))); + + private static string ProviderToken(GitHub provider) => TokenStorage.Read(provider.TokenPersona).ToString(); + [TestMethod] public async Task A429ReportsFailure() { diff --git a/BuildMonitor/Providers/GitHub.cs b/BuildMonitor/Providers/GitHub.cs index b0dd2ab..1f88bfa 100644 --- a/BuildMonitor/Providers/GitHub.cs +++ b/BuildMonitor/Providers/GitHub.cs @@ -680,6 +680,9 @@ internal async Task MakeGitHubRequestAsync(string name, Func action, // concurrent requests use different owner tokens SetCurrentClient(owner); + // Which credential this request is sent with decides which one a rejection condemns. + Owner? tokenOwner = owner?.HasToken == true ? owner : null; + // Use smart waiting: if rate limited with a known reset time, wait until reset TimeSpan waitTime = GetRateLimitWaitTime(); if (waitTime > BaseRequestDelay) @@ -701,7 +704,15 @@ internal async Task MakeGitHubRequestAsync(string name, Func action, catch (AuthorizationException) { Log.Error($"{Name}: AuthorizationException for request '{name}'"); - OnAuthenticationFailure(); + if (tokenOwner is not null) + { + OnOwnerAuthenticationFailure(tokenOwner); + } + else + { + OnAuthenticationFailure(); + } + return false; } catch (ApiException e) @@ -718,8 +729,11 @@ internal async Task MakeGitHubRequestAsync(string name, Func action, } else { + // A token that is not allowed to make this one request -- a workflow + // dispatch without actions: write, say -- is still a working token. + // The refusal belongs to this request, not to the credentials. Log.Error($"{Name}: 403 Forbidden for request '{name}' - {e.Message}"); - OnAuthenticationFailure(); + SetStatus(ProviderStatus.Error, $"{Strings.Forbidden}: {name}"); } break; case System.Net.HttpStatusCode.TooManyRequests: @@ -746,6 +760,23 @@ internal async Task MakeGitHubRequestAsync(string name, Func action, } } + /// + /// Clears an owner's override token after GitHub rejected it. + /// + /// The owner whose token the rejected request was sent with. + /// + /// The provider credentials were not used for the request, so they are left alone: clearing them + /// took every other owner offline while the token that actually failed stayed in place and kept + /// being sent, and rejected, every cycle. Clearing the owner token instead lets that owner fall + /// back to the provider token until a new one is set. + /// + private void OnOwnerAuthenticationFailure(Owner owner) + { + Log.Error($"{Name}: Authentication failed for owner '{owner.Name}' - owner token cleared"); + owner.Token = new(); + SetStatus(ProviderStatus.AuthFailed, $"{Strings.AuthFailedMessage} ({owner.Name})"); + } + /// /// Updates the rate limit budget from the last API response. /// Uses Octokit's GetLastApiInfo() to retrieve rate limit headers. diff --git a/BuildMonitor/Strings.cs b/BuildMonitor/Strings.cs index 5a161f1..36c64f1 100644 --- a/BuildMonitor/Strings.cs +++ b/BuildMonitor/Strings.cs @@ -44,6 +44,7 @@ internal static class Strings internal static string RateLimitedMessage { get; } = "Rate limited."; internal static string AuthFailed { get; } = nameof(AuthFailed).Titleize(); internal static string AuthFailedMessage { get; } = "Authentication failed. Please update credentials."; + internal static string Forbidden { get; } = "Forbidden"; internal static string ConnectionError { get; } = nameof(ConnectionError).Titleize(); internal static string ConnectionErrorMessage { get; } = "Connection error."; internal static string Delay { get; } = nameof(Delay).Titleize();