diff --git a/CHANGELOG.md b/CHANGELOG.md index 8bf1a7022..a22a7b03e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## 0.22.1 + +### Patch Changes + +- 7dd046c: Harden GitHub Copilot platform-connection handling so malformed saved configuration does not block recovery, platform/default and project-scoped bindings safely clean up or preserve shared credentials, and SQLite-to-Postgres migration carries the platform-default binding forward reliably. + ## 0.22.0 ### Minor Changes diff --git a/VERSION b/VERSION index 215740905..a723ece79 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.22.0 +0.22.1 diff --git a/apps/Agentweaver.Api/Auth/ByokProviderConfigurationService.cs b/apps/Agentweaver.Api/Auth/ByokProviderConfigurationService.cs index 3a11a16c8..0c2193550 100644 --- a/apps/Agentweaver.Api/Auth/ByokProviderConfigurationService.cs +++ b/apps/Agentweaver.Api/Auth/ByokProviderConfigurationService.cs @@ -1,47 +1,73 @@ -using System.Text.Json; -using Agentweaver.Domain; - -namespace Agentweaver.Api.Auth; - -public sealed class ByokProviderConfigurationService(ISecretStore secretStore) - : IByokProviderConfigurationProvider -{ - private const string SecretName = "byok-provider-configuration"; - - public async Task GetAsync(CancellationToken ct) - { - var secret = await secretStore.GetSecretAsync(SecretName, ct).ConfigureAwait(false); - return secret.Found && !string.IsNullOrWhiteSpace(secret.Value) - ? JsonSerializer.Deserialize(secret.Value) - : null; - } - - public async Task SetAsync(ByokProviderConfiguration configuration, CancellationToken ct) - { - Validate(configuration); - await secretStore.SetSecretAsync( - SecretName, - JsonSerializer.Serialize(configuration), - ct: ct).ConfigureAwait(false); - } - - public Task ClearAsync(CancellationToken ct) => - secretStore.DeleteSecretAsync(SecretName, ct); - - private static void Validate(ByokProviderConfiguration configuration) - { - ArgumentNullException.ThrowIfNull(configuration); - if (configuration.Type is not ("openai" or "azure" or "anthropic")) - throw new ArgumentException("Provider type must be openai, azure, or anthropic."); - if (!Uri.TryCreate(configuration.BaseUrl, UriKind.Absolute, out var baseUri) || - baseUri.Scheme != Uri.UriSchemeHttps) - throw new ArgumentException("Provider base URL must be an HTTPS URL."); - if (configuration.Type == "azure" && - (!string.IsNullOrEmpty(baseUri.PathAndQuery.Trim('/')) || !string.IsNullOrEmpty(baseUri.Fragment))) - throw new ArgumentException("Azure provider base URL must be its HTTPS host without an API path."); - if (string.IsNullOrWhiteSpace(configuration.Model)) - throw new ArgumentException("Provider model is required."); - if (string.IsNullOrWhiteSpace(configuration.ApiKey)) - throw new ArgumentException("Provider API key is required."); - } -} +using System.Text.Json; +using Agentweaver.Domain; + +namespace Agentweaver.Api.Auth; + +public sealed class ByokProviderConfigurationService(ISecretStore secretStore) + : IByokProviderConfigurationProvider +{ + private const string SecretName = "byok-provider-configuration"; + private static readonly JsonSerializerOptions ReadJsonOptions = new() { PropertyNameCaseInsensitive = true }; + + public async Task GetAsync(CancellationToken ct) + { + var secret = await secretStore.GetSecretAsync(SecretName, ct).ConfigureAwait(false); + if (!secret.Found || string.IsNullOrWhiteSpace(secret.Value)) + return null; + try + { + var configuration = JsonSerializer.Deserialize(secret.Value, ReadJsonOptions); + return IsValid(configuration) ? configuration : null; + } + catch (JsonException) + { + return null; + } + } + + public async Task SetAsync(ByokProviderConfiguration configuration, CancellationToken ct) + { + Validate(configuration); + await secretStore.SetSecretAsync( + SecretName, + JsonSerializer.Serialize(configuration), + ct: ct).ConfigureAwait(false); + } + + public Task ClearAsync(CancellationToken ct) => + secretStore.DeleteSecretAsync(SecretName, ct); + + private static void Validate(ByokProviderConfiguration configuration) + { + ArgumentNullException.ThrowIfNull(configuration); + if (configuration.Type is not ("openai" or "azure" or "anthropic")) + throw new ArgumentException("Provider type must be openai, azure, or anthropic."); + if (!Uri.TryCreate(configuration.BaseUrl, UriKind.Absolute, out var baseUri) || + baseUri.Scheme != Uri.UriSchemeHttps) + throw new ArgumentException("Provider base URL must be an HTTPS URL."); + if (configuration.Type == "azure" && + (!string.IsNullOrEmpty(baseUri.PathAndQuery.Trim('/')) || !string.IsNullOrEmpty(baseUri.Fragment))) + throw new ArgumentException("Azure provider base URL must be its HTTPS host without an API path."); + if (string.IsNullOrWhiteSpace(configuration.Model)) + throw new ArgumentException("Provider model is required."); + if (string.IsNullOrWhiteSpace(configuration.ApiKey)) + throw new ArgumentException("Provider API key is required."); + } + + private static bool IsValid(ByokProviderConfiguration? configuration) + { + try + { + Validate(configuration!); + return true; + } + catch (ArgumentNullException) + { + return false; + } + catch (ArgumentException) + { + return false; + } + } +} diff --git a/apps/Agentweaver.Api/Auth/GitHubConnectionsCredentialVault.cs b/apps/Agentweaver.Api/Auth/GitHubConnectionsCredentialVault.cs index 4c3a2c6ed..c80bb0140 100644 --- a/apps/Agentweaver.Api/Auth/GitHubConnectionsCredentialVault.cs +++ b/apps/Agentweaver.Api/Auth/GitHubConnectionsCredentialVault.cs @@ -1,85 +1,87 @@ -using System.Text.Json; - -namespace Agentweaver.Api.Auth; - -/// Opaque, vault-owned locator for reserved GitHub connections credential material. -internal sealed record GitHubConnectionsCredentialLocator -{ - private GitHubConnectionsCredentialLocator(string key) => Key = key; - - internal string Key { get; } - - internal static GitHubConnectionsCredentialLocator ForRepoAppUser(string credentialReference) => - Create(credentialReference, "repo-app-user-credential-"); - - internal static GitHubConnectionsCredentialLocator ForCopilotProject(string credentialReference) => - Create(credentialReference, "copilot-app-project-"); - - internal static GitHubConnectionsCredentialLocator ForCopilotBinding(string credentialReference) - { - if (string.IsNullOrWhiteSpace(credentialReference) || - (!credentialReference.StartsWith("copilot-app-project-", StringComparison.Ordinal) && - !credentialReference.StartsWith("copilot-app-platform-default-", StringComparison.Ordinal))) - throw new ArgumentException("Credential reference is not a reserved GitHub connections locator.", nameof(credentialReference)); - return new(credentialReference); - } - - private static GitHubConnectionsCredentialLocator Create(string credentialReference, string requiredPrefix) - { - if (string.IsNullOrWhiteSpace(credentialReference) || - !credentialReference.StartsWith(requiredPrefix, StringComparison.Ordinal)) - throw new ArgumentException("Credential reference is not a reserved GitHub connections locator.", nameof(credentialReference)); - return new(credentialReference); - } -} - -internal interface IGitHubConnectionsCredentialVault -{ - Task ReadCurrentAsync(GitHubConnectionsCredentialLocator locator, CancellationToken ct = default); - Task WriteAsync(GitHubConnectionsCredentialLocator locator, string value, CancellationToken ct = default); - Task TombstoneAndDeleteAsync(GitHubConnectionsCredentialLocator locator, CancellationToken ct = default); -} - -/// -/// The sole GitHub connections authority allowed to bridge reserved credential locators to generic secret -/// storage. Reads are current-version only and tombstones cannot be treated as a credential. -/// -internal sealed class GitHubConnectionsCredentialVault(ISecretStore secretStore) : IGitHubConnectionsCredentialVault -{ - private const string Tombstone = """{"status":"revoked"}"""; - - public async Task ReadCurrentAsync(GitHubConnectionsCredentialLocator locator, CancellationToken ct = default) - { - var result = await secretStore.GetSecretAsync(locator.Key, ct).ConfigureAwait(false); - return !result.Found || IsTombstone(result.Value) ? SecretGetResult.NotFound : result; - } - - public async Task WriteAsync(GitHubConnectionsCredentialLocator locator, string value, CancellationToken ct = default) - { - if (string.IsNullOrWhiteSpace(value) || IsTombstone(value)) - throw new ArgumentException("The vault cannot write empty or tombstone credential material.", nameof(value)); - await secretStore.SetSecretAsync(locator.Key, value, ct: ct).ConfigureAwait(false); - } - - public async Task TombstoneAndDeleteAsync(GitHubConnectionsCredentialLocator locator, CancellationToken ct = default) - { - await secretStore.SetSecretAsync(locator.Key, Tombstone, ct: ct).ConfigureAwait(false); - await secretStore.DeleteSecretAsync(locator.Key, ct).ConfigureAwait(false); - } - - private static bool IsTombstone(string? value) - { - if (string.IsNullOrWhiteSpace(value)) - return false; - try - { - using var document = JsonDocument.Parse(value); - return document.RootElement.TryGetProperty("status", out var status) && - string.Equals(status.GetString(), "revoked", StringComparison.Ordinal); - } - catch (JsonException) - { - return false; - } - } -} +using System.Text.Json; + +namespace Agentweaver.Api.Auth; + +/// Opaque, vault-owned locator for reserved GitHub connections credential material. +internal sealed record GitHubConnectionsCredentialLocator +{ + private GitHubConnectionsCredentialLocator(string key) => Key = key; + + internal string Key { get; } + + internal static GitHubConnectionsCredentialLocator ForRepoAppUser(string credentialReference) => + Create(credentialReference, "repo-app-user-credential-"); + + internal static GitHubConnectionsCredentialLocator ForCopilotProject(string credentialReference) => + Create(credentialReference, "copilot-app-project-"); + + internal static GitHubConnectionsCredentialLocator ForCopilotBinding(string credentialReference) + { + if (string.IsNullOrWhiteSpace(credentialReference) || + (!credentialReference.StartsWith("copilot-app-project-", StringComparison.Ordinal) && + !credentialReference.StartsWith("copilot-app-platform-default-", StringComparison.Ordinal))) + throw new ArgumentException("Credential reference is not a reserved GitHub connections locator.", nameof(credentialReference)); + return new(credentialReference); + } + + private static GitHubConnectionsCredentialLocator Create(string credentialReference, string requiredPrefix) + { + if (string.IsNullOrWhiteSpace(credentialReference) || + !credentialReference.StartsWith(requiredPrefix, StringComparison.Ordinal)) + throw new ArgumentException("Credential reference is not a reserved GitHub connections locator.", nameof(credentialReference)); + return new(credentialReference); + } +} + +internal interface IGitHubConnectionsCredentialVault +{ + Task ReadCurrentAsync(GitHubConnectionsCredentialLocator locator, CancellationToken ct = default); + Task WriteAsync(GitHubConnectionsCredentialLocator locator, string value, CancellationToken ct = default); + Task TombstoneAndDeleteAsync(GitHubConnectionsCredentialLocator locator, CancellationToken ct = default); +} + +/// +/// The sole GitHub connections authority allowed to bridge reserved credential locators to generic secret +/// storage. Reads are current-version only and tombstones cannot be treated as a credential. +/// +internal sealed class GitHubConnectionsCredentialVault(ISecretStore secretStore) : IGitHubConnectionsCredentialVault +{ + private const string Tombstone = """{"status":"revoked"}"""; + + public async Task ReadCurrentAsync(GitHubConnectionsCredentialLocator locator, CancellationToken ct = default) + { + var result = await secretStore.GetSecretAsync(locator.Key, ct).ConfigureAwait(false); + return !result.Found || IsTombstone(result.Value) ? SecretGetResult.NotFound : result; + } + + public async Task WriteAsync(GitHubConnectionsCredentialLocator locator, string value, CancellationToken ct = default) + { + if (string.IsNullOrWhiteSpace(value) || IsTombstone(value)) + throw new ArgumentException("The vault cannot write empty or tombstone credential material.", nameof(value)); + await secretStore.SetSecretAsync(locator.Key, value, ct: ct).ConfigureAwait(false); + } + + public async Task TombstoneAndDeleteAsync(GitHubConnectionsCredentialLocator locator, CancellationToken ct = default) + { + await secretStore.SetSecretAsync(locator.Key, Tombstone, ct: ct).ConfigureAwait(false); + await secretStore.DeleteSecretAsync(locator.Key, ct).ConfigureAwait(false); + } + + private static bool IsTombstone(string? value) + { + if (string.IsNullOrWhiteSpace(value)) + return false; + try + { + using var document = JsonDocument.Parse(value); + return document.RootElement.ValueKind == JsonValueKind.Object && + document.RootElement.TryGetProperty("status", out var status) && + status.ValueKind == JsonValueKind.String && + string.Equals(status.GetString(), "revoked", StringComparison.Ordinal); + } + catch (Exception ex) when (ex is JsonException or InvalidOperationException) + { + return false; + } + } +} diff --git a/apps/Agentweaver.Api/Auth/PlatformDefaultCopilotBindingService.cs b/apps/Agentweaver.Api/Auth/PlatformDefaultCopilotBindingService.cs index 2f934ca44..02ee0f7e4 100644 --- a/apps/Agentweaver.Api/Auth/PlatformDefaultCopilotBindingService.cs +++ b/apps/Agentweaver.Api/Auth/PlatformDefaultCopilotBindingService.cs @@ -50,6 +50,7 @@ internal sealed class PlatformDefaultCopilotBindingService( private static readonly TimeSpan ProviderTimeout = TimeSpan.FromSeconds(10); private readonly string _baseUrl = configuration["Auth:CopilotApp:BaseUrl"] ?? "https://github.com"; + private readonly string _apiUrl = configuration["Auth:CopilotApp:ApiUrl"] ?? "https://api.github.com"; private readonly string? _clientId = configuration["Auth:CopilotApp:ClientId"]; private readonly string? _clientSecret = configuration["Auth:CopilotApp:ClientSecret"]; private readonly string? _configuredCallbackUrl = configuration["Auth:CopilotApp:CallbackUrl"]; @@ -173,7 +174,7 @@ public async Task DisconnectAsync( { var secret = await secretStore.GetSecretAsync(reference.CredentialReference, ct).ConfigureAwait(false); var credential = secret.Found ? DeserializeCredential(secret.Value) : null; - if (await ShouldRevokeCredentialAsync(reference.Id, credential, null, ct).ConfigureAwait(false)) + if (!await IsTokenStillInUseAsync(reference.Id, credential?.AccessToken, ct).ConfigureAwait(false)) await RevokeWithProviderAsync(credential?.AccessToken, ct).ConfigureAwait(false); await DeleteCredentialAsync(reference.CredentialReference, ct).ConfigureAwait(false); } @@ -218,7 +219,9 @@ private async Task GetConnectionC var secret = await secretStore.GetSecretAsync(binding.CredentialReference, ct).ConfigureAwait(false); var credential = secret.Found ? DeserializeCredential(secret.Value) : null; - if (credential is null || !string.Equals(credential.Status, "signed-in", StringComparison.Ordinal)) + if (credential is null || + !string.Equals(credential.Status, "signed-in", StringComparison.Ordinal) || + string.IsNullOrWhiteSpace(credential.AccessToken)) { logger.LogWarning( "Platform-default Copilot connection has an active binding record but its credential secret is {SecretState}.", @@ -296,7 +299,7 @@ await credentialVault.WriteAsync( if (completed.ReplacedCredential is not null) await RevokeReplacedCredentialAsync( completed.ReplacedCredential, - credential.GitHubLogin, + credential.AccessToken, CancellationToken.None).ConfigureAwait(false); return PlatformDefaultCopilotBindingOutcome.Success; } @@ -421,7 +424,7 @@ private async Task RevokeWithProviderAsync(string? accessToken, CancellationToke using var timeout = CancellationTokenSource.CreateLinkedTokenSource(ct); timeout.CancelAfter(ProviderTimeout); using var request = new HttpRequestMessage(HttpMethod.Delete, - $"{_baseUrl.TrimEnd('/')}/applications/{Uri.EscapeDataString(_clientId!)}/grant") + $"{_apiUrl.TrimEnd('/')}/applications/{Uri.EscapeDataString(_clientId!)}/token") { Content = new StringContent(JsonSerializer.Serialize(new { access_token = accessToken }), Encoding.UTF8, "application/json"), }; @@ -467,6 +470,25 @@ await credentialVault.TombstoneAndDeleteAsync( catch (JsonException) { return null; } } + private async Task IsTokenStillInUseAsync( + string bindingId, + string? accessToken, + CancellationToken ct) + { + if (string.IsNullOrWhiteSpace(accessToken)) + return false; + var otherBindings = await persistence.ListActiveCopilotBindingsAsync(bindingId, ct).ConfigureAwait(false); + foreach (var otherBinding in otherBindings) + { + var otherSecret = await secretStore.GetSecretAsync(otherBinding.CredentialReference, ct).ConfigureAwait(false); + var otherCredential = otherSecret.Found ? DeserializeCredential(otherSecret.Value) : null; + if (string.Equals(otherCredential?.AccessToken, accessToken, StringComparison.Ordinal)) + return true; + } + + return false; + } + private static GitHubAuditRecord CreateAudit( string entraObjectId, GitHubAuditOutcome outcome, @@ -503,7 +525,7 @@ private static async Task ReadBoundedAsync(HttpContent content, Cancella private async Task RevokeReplacedCredentialAsync( RepoAppCredentialReference reference, - string? replacementGitHubLogin, + string? replacementAccessToken, CancellationToken ct) { try @@ -512,7 +534,8 @@ private async Task RevokeReplacedCredentialAsync( var replacedCredential = secret.Found && !string.IsNullOrWhiteSpace(secret.Value) ? DeserializeCredential(secret.Value) : null; - if (await ShouldRevokeCredentialAsync(reference.Id, replacedCredential, replacementGitHubLogin, ct).ConfigureAwait(false)) + if (!string.Equals(replacedCredential?.AccessToken, replacementAccessToken, StringComparison.Ordinal) && + !await IsTokenStillInUseAsync(reference.Id, replacedCredential?.AccessToken, ct).ConfigureAwait(false)) await RevokeWithProviderAsync(replacedCredential?.AccessToken, ct).ConfigureAwait(false); await DeleteCredentialAsync(reference.CredentialReference, ct).ConfigureAwait(false); } @@ -525,32 +548,6 @@ private async Task RevokeReplacedCredentialAsync( } } - private async Task ShouldRevokeCredentialAsync( - string bindingId, - CopilotCredential? credential, - string? replacementGitHubLogin, - CancellationToken ct) - { - if (credential is null || - string.IsNullOrWhiteSpace(credential.AccessToken) || - string.IsNullOrWhiteSpace(credential.GitHubLogin)) - return false; - if (!string.IsNullOrWhiteSpace(replacementGitHubLogin) && - string.Equals(credential.GitHubLogin, replacementGitHubLogin, StringComparison.OrdinalIgnoreCase)) - return false; - var otherBindings = await persistence.ListActiveCopilotBindingsAsync(bindingId, ct).ConfigureAwait(false); - foreach (var otherBinding in otherBindings) - { - var otherSecret = await secretStore.GetSecretAsync(otherBinding.CredentialReference, ct).ConfigureAwait(false); - var otherCredential = otherSecret.Found && !string.IsNullOrWhiteSpace(otherSecret.Value) - ? DeserializeCredential(otherSecret.Value) - : null; - if (string.Equals(credential.GitHubLogin, otherCredential?.GitHubLogin, StringComparison.OrdinalIgnoreCase)) - return false; - } - - return true; - } private static bool IsGitHubLogin(string? value) => !string.IsNullOrWhiteSpace(value) && diff --git a/apps/Agentweaver.Api/Auth/ProjectCopilotBindingService.cs b/apps/Agentweaver.Api/Auth/ProjectCopilotBindingService.cs index b99e68c52..2ad7ec5e0 100644 --- a/apps/Agentweaver.Api/Auth/ProjectCopilotBindingService.cs +++ b/apps/Agentweaver.Api/Auth/ProjectCopilotBindingService.cs @@ -67,6 +67,7 @@ public sealed class ProjectCopilotBindingService( new Dictionary(StringComparer.Ordinal) { ["projects"] = "/projects" }; private readonly string _baseUrl = configuration["Auth:CopilotApp:BaseUrl"] ?? "https://github.com"; + private readonly string _apiUrl = configuration["Auth:CopilotApp:ApiUrl"] ?? "https://api.github.com"; private readonly string? _clientId = configuration["Auth:CopilotApp:ClientId"]; private readonly string? _clientSecret = configuration["Auth:CopilotApp:ClientSecret"]; private readonly string? _callbackUrl = configuration["Auth:CopilotApp:CallbackUrl"]; @@ -326,8 +327,9 @@ public async Task DisconnectAsync( try { var secret = await secretStore.GetSecretAsync(reference.CredentialReference, ct).ConfigureAwait(false); - if (secret.Found) - await RevokeWithProviderAsync(DeserializeCredential(secret.Value)?.AccessToken, ct).ConfigureAwait(false); + var credential = secret.Found ? DeserializeCredential(secret.Value) : null; + if (!await IsTokenStillInUseAsync(reference.Id, credential?.AccessToken, ct).ConfigureAwait(false)) + await RevokeWithProviderAsync(credential?.AccessToken, ct).ConfigureAwait(false); await WriteTombstoneAsync(reference.CredentialReference, ct).ConfigureAwait(false); } catch @@ -583,7 +585,7 @@ private async Task RevokeWithProviderAsync(string? accessToken, CancellationToke using var timeout = CancellationTokenSource.CreateLinkedTokenSource(ct); timeout.CancelAfter(ProviderTimeout); using var request = new HttpRequestMessage(HttpMethod.Delete, - $"{_baseUrl.TrimEnd('/')}/applications/{Uri.EscapeDataString(_clientId!)}/grant") + $"{_apiUrl.TrimEnd('/')}/applications/{Uri.EscapeDataString(_clientId!)}/token") { Content = new StringContent(JsonSerializer.Serialize(new { access_token = accessToken }), Encoding.UTF8, "application/json"), }; @@ -622,6 +624,26 @@ private async Task WriteTombstoneAsync(string reference, CancellationToken ct) = try { return string.IsNullOrWhiteSpace(value) ? null : JsonSerializer.Deserialize(value); } catch (JsonException) { return null; } } + + private async Task IsTokenStillInUseAsync( + string bindingId, + string? accessToken, + CancellationToken ct) + { + if (string.IsNullOrWhiteSpace(accessToken)) + return false; + var otherBindings = await persistence.ListActiveCopilotBindingsAsync(bindingId, ct).ConfigureAwait(false); + foreach (var otherBinding in otherBindings) + { + var otherSecret = await secretStore.GetSecretAsync(otherBinding.CredentialReference, ct).ConfigureAwait(false); + var otherCredential = otherSecret.Found ? DeserializeCredential(otherSecret.Value) : null; + if (string.Equals(otherCredential?.AccessToken, accessToken, StringComparison.Ordinal)) + return true; + } + + return false; + } + private static GitHubAuditRecord CreateAudit( string entraObjectId, ProjectId projectId, GitHubAuditOutcome outcome, GitHubAuditReasonCode reason, string? version) => new() diff --git a/apps/Agentweaver.Api/Endpoints/AuthEndpoints.cs b/apps/Agentweaver.Api/Endpoints/AuthEndpoints.cs index d26fe1ecf..5c6f5835a 100644 --- a/apps/Agentweaver.Api/Endpoints/AuthEndpoints.cs +++ b/apps/Agentweaver.Api/Endpoints/AuthEndpoints.cs @@ -313,7 +313,7 @@ private static async Task HasUsablePlatformDefaultCopilotBindingAsync( if (document.RootElement.ValueKind != JsonValueKind.Object) return false; var status = GetJsonString(document.RootElement, "status"); - var accessToken = GetJsonString(document.RootElement, "access_token", "accessToken"); + var accessToken = GetJsonString(document.RootElement, "accessToken"); return string.Equals(status, "signed-in", StringComparison.Ordinal) && !string.IsNullOrWhiteSpace(accessToken); } diff --git a/apps/Agentweaver.Api/Tools/SqliteToPostgresMigrator.cs b/apps/Agentweaver.Api/Tools/SqliteToPostgresMigrator.cs index 5b72d5f88..d62b98308 100644 --- a/apps/Agentweaver.Api/Tools/SqliteToPostgresMigrator.cs +++ b/apps/Agentweaver.Api/Tools/SqliteToPostgresMigrator.cs @@ -84,7 +84,9 @@ private async Task MigrateGitHubConnectionsRecordsAsync(string memoryDbPath, Mem installations = await source.GitHubInstallations.AsNoTracking().ToListAsync(ct).ConfigureAwait(false); grants = await source.GitHubRepositoryGrants.AsNoTracking().ToListAsync(ct).ConfigureAwait(false); bindings = await source.ProjectCopilotBindings.AsNoTracking().ToListAsync(ct).ConfigureAwait(false); - platformBindings = await source.PlatformDefaultCopilotBindings.AsNoTracking().ToListAsync(ct).ConfigureAwait(false); + platformBindings = await HasTableAsync(source, "platform_default_copilot_bindings", ct).ConfigureAwait(false) + ? await source.PlatformDefaultCopilotBindings.AsNoTracking().ToListAsync(ct).ConfigureAwait(false) + : []; activations = await source.AutomationActivations.AsNoTracking().ToListAsync(ct).ConfigureAwait(false); invocations = await source.AutomationInvocations.AsNoTracking().ToListAsync(ct).ConfigureAwait(false); lifecycleDeliveries = await source.GitHubLifecycleDeliveries.AsNoTracking().ToListAsync(ct).ConfigureAwait(false); @@ -142,8 +144,20 @@ private async Task MigrateGitHubConnectionsRecordsAsync(string memoryDbPath, Mem if (!await destination.ProjectCopilotBindings.AnyAsync(x => x.Id == item.Id, ct).ConfigureAwait(false)) destination.ProjectCopilotBindings.Add(item); foreach (var item in platformBindings) - if (!await destination.PlatformDefaultCopilotBindings.AnyAsync(x => x.Id == item.Id, ct).ConfigureAwait(false)) + { + var existing = await destination.PlatformDefaultCopilotBindings.AsNoTracking() + .SingleOrDefaultAsync(x => x.Id == item.Id, ct) + .ConfigureAwait(false); + if (existing is null) + { destination.PlatformDefaultCopilotBindings.Add(item); + continue; + } + + if (!PlatformBindingsMatch(item, existing)) + throw new InvalidOperationException( + "GitHub connections persistence transfer aborted: immutable platform-default Copilot binding conflict."); + } foreach (var item in activations) if (!await destination.AutomationActivations.AnyAsync(x => x.Id == item.Id, ct).ConfigureAwait(false)) destination.AutomationActivations.Add(item); @@ -219,6 +233,26 @@ destination is not null && source.CapturedAt == destination.CapturedAt && source.SnapshotExpiresAt == destination.SnapshotExpiresAt; + private static bool PlatformBindingsMatch( + PlatformDefaultCopilotBindingRecord source, + PlatformDefaultCopilotBindingRecord destination) => + source.Id == destination.Id && + source.EntraObjectId == destination.EntraObjectId && + source.CredentialReference == destination.CredentialReference && + source.CredentialVersion == destination.CredentialVersion && + source.GrantDigest == destination.GrantDigest && + source.Status == destination.Status && + NormalizeTimestamp(source.BoundAt) == NormalizeTimestamp(destination.BoundAt) && + NormalizeTimestamp(source.DeactivatedAt) == NormalizeTimestamp(destination.DeactivatedAt); + + private static DateTimeOffset? NormalizeTimestamp(DateTimeOffset? value) + { + if (value is null) + return null; + var ticks = value.Value.ToUniversalTime().Ticks; + return new DateTimeOffset(ticks - (ticks % 10), TimeSpan.Zero); + } + private static async Task PrepareGitHubConnectionsSourceSchemaAsync(MemoryDbContext source, CancellationToken ct) { if (await HasMigrationHistoryAsync(source, ct).ConfigureAwait(false)) diff --git a/package-lock.json b/package-lock.json index 725b8a1ca..87882306e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "agentweaver", - "version": "0.22.0", + "version": "0.22.1", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "agentweaver", - "version": "0.22.0", + "version": "0.22.1", "devDependencies": { "@changesets/cli": "^3.0.0", "@types/node": "^26.2.0", diff --git a/package.json b/package.json index 6d943cb93..404799f3e 100644 --- a/package.json +++ b/package.json @@ -41,5 +41,5 @@ "@types/node": "^26.2.0", "playwright": "^1.62.1" }, - "version": "0.22.0" + "version": "0.22.1" } diff --git a/tests/Agentweaver.Tests/Auth/GitHubConnectionsCredentialVaultTests.cs b/tests/Agentweaver.Tests/Auth/GitHubConnectionsCredentialVaultTests.cs index 684b1bdb7..e0fb85e93 100644 --- a/tests/Agentweaver.Tests/Auth/GitHubConnectionsCredentialVaultTests.cs +++ b/tests/Agentweaver.Tests/Auth/GitHubConnectionsCredentialVaultTests.cs @@ -26,4 +26,18 @@ public void Locator_RejectsCallerSuppliedPrefixesOutsideItsTypedPurpose() action.Should().Throw(); } + + [Fact] + public async Task ReadCurrent_TreatsMalformedJsonShapesAsMissingInsteadOfThrowing() + { + var store = new InMemorySecretStore(); + var vault = new GitHubConnectionsCredentialVault(store); + var locator = GitHubConnectionsCredentialLocator.ForCopilotBinding("copilot-app-platform-default-bad-shape"); + + await store.SetSecretAsync(locator.Key, "\"signed-in\""); + (await vault.ReadCurrentAsync(locator)).Found.Should().BeTrue(); + + await store.SetSecretAsync(locator.Key, """{"status":{}}"""); + (await vault.ReadCurrentAsync(locator)).Found.Should().BeTrue(); + } } diff --git a/tests/Agentweaver.Tests/Auth/PlatformDefaultCopilotBindingServiceTests.cs b/tests/Agentweaver.Tests/Auth/PlatformDefaultCopilotBindingServiceTests.cs index 345bfd0ba..f025efb6d 100644 --- a/tests/Agentweaver.Tests/Auth/PlatformDefaultCopilotBindingServiceTests.cs +++ b/tests/Agentweaver.Tests/Auth/PlatformDefaultCopilotBindingServiceTests.cs @@ -94,6 +94,46 @@ public async Task Disconnect_RevokesOnlyTheSingletonBinding() (await secrets.GetSecretAsync("copilot-app-platform-default-existing")).Should().Be(SecretGetResult.NotFound); } + [Fact] + public async Task Disconnect_DoesNotRevokeATokenThatIsStillUsedByAProjectBinding() + { + await using var db = await OpenDatabaseAsync(); + var secrets = new InMemorySecretStore(); + var httpClientFactory = new StubHttpClientFactory(); + db.Projects.Add(new ProjectRecord { ProjectId = "project" }); + await new GitHubConnectionsPersistenceStore(db).ReplacePlatformDefaultCopilotBindingAsync(new PlatformDefaultCopilotBindingRecord + { + Id = PlatformDefaultCopilotBindingRecord.SingletonId, + EntraObjectId = "platform-admin", + CredentialReference = "copilot-app-platform-default-existing", + CredentialVersion = "version-one", + GrantDigest = "digest", + Status = GitHubBindingStatus.Active, + BoundAt = DateTimeOffset.UtcNow, + }); + await new GitHubConnectionsPersistenceStore(db).ReplaceCopilotBindingAsync(new ProjectCopilotBindingRecord + { + Id = "project-binding", + ProjectId = "project", + EntraObjectId = "owner", + CredentialReference = "copilot-app-project-project-version-two", + CredentialVersion = "version-two", + GrantDigest = "digest-project", + Status = GitHubBindingStatus.Active, + BoundAt = DateTimeOffset.UtcNow, + }); + await secrets.SetSecretAsync("copilot-app-platform-default-existing", """{"Status":"signed-in","AccessToken":"ghu_shared","GitHubLogin":"octocat"}"""); + await secrets.SetSecretAsync("copilot-app-project-project-version-two", """{"Status":"signed-in","AccessToken":"ghu_shared","GitHubLogin":"octocat"}"""); + await db.SaveChangesAsync(); + var service = CreateService(db, secrets, httpClientFactory: httpClientFactory); + + (await service.DisconnectAsync(Admin("platform-admin"), HumanPrincipal())) + .Should().Be(PlatformDefaultCopilotBindingOutcome.Success); + + httpClientFactory.ProviderGrantRevocations.Should().Be(0); + (await secrets.GetSecretAsync("copilot-app-project-project-version-two")).Value.Should().Contain("ghu_shared"); + } + [Fact] public async Task CompleteBrowserCallback_RevokeAndTombstonesReplacedCredentialAfterRebind() { @@ -123,7 +163,7 @@ public async Task CompleteBrowserCallback_RevokeAndTombstonesReplacedCredentialA } [Fact] - public async Task CompleteBrowserCallback_DoesNotRevokeTheReplacementGrantWhenTheSameGitHubLoginReconnects() + public async Task CompleteBrowserCallback_RevokesOnlyTheRemovedTokenWhenTheSameGitHubLoginReconnects() { await using var db = await OpenDatabaseAsync(); var secrets = new InMemorySecretStore(); @@ -138,7 +178,34 @@ public async Task CompleteBrowserCallback_DoesNotRevokeTheReplacementGrantWhenTh Status = GitHubBindingStatus.Active, BoundAt = DateTimeOffset.UtcNow.AddMinutes(-5), }); - await secrets.SetSecretAsync("copilot-app-platform-default-old", """{"access_token":"ghu_old","refresh_token":"refresh-old","status":"signed-in","github_login":"octocat"}"""); + await secrets.SetSecretAsync("copilot-app-platform-default-old", """{"Status":"signed-in","AccessToken":"ghu_old","RefreshToken":"refresh-old","GitHubLogin":"octocat"}"""); + var service = CreateService(db, secrets, httpClientFactory: httpClientFactory); + var begin = await service.BeginAsync(Admin("platform-admin"), HumanPrincipal()); + + (await service.CompleteBrowserCallbackAsync(null, null, Query(begin.AuthorizationUrl!, "state"), "code", begin.CallbackCookie)) + .Should().Be(PlatformDefaultCopilotBindingOutcome.Success); + + httpClientFactory.ProviderGrantRevocations.Should().Be(1); + (await secrets.GetSecretAsync("copilot-app-platform-default-old")).Should().Be(SecretGetResult.NotFound); + } + + [Fact] + public async Task CompleteBrowserCallback_DoesNotRevokeTheActiveBindingWhenGitHubReturnsTheSameAccessToken() + { + await using var db = await OpenDatabaseAsync(); + var secrets = new InMemorySecretStore(); + var httpClientFactory = new StubHttpClientFactory("""{"access_token":"ghu_same","refresh_token":"refresh-secret"}"""); + await new GitHubConnectionsPersistenceStore(db).ReplacePlatformDefaultCopilotBindingAsync(new PlatformDefaultCopilotBindingRecord + { + Id = PlatformDefaultCopilotBindingRecord.SingletonId, + EntraObjectId = "first-admin", + CredentialReference = "copilot-app-platform-default-old", + CredentialVersion = "version-old", + GrantDigest = "digest-old", + Status = GitHubBindingStatus.Active, + BoundAt = DateTimeOffset.UtcNow.AddMinutes(-5), + }); + await secrets.SetSecretAsync("copilot-app-platform-default-old", """{"Status":"signed-in","AccessToken":"ghu_same","RefreshToken":"refresh-old","GitHubLogin":"octocat"}"""); var service = CreateService(db, secrets, httpClientFactory: httpClientFactory); var begin = await service.BeginAsync(Admin("platform-admin"), HumanPrincipal()); @@ -146,6 +213,10 @@ public async Task CompleteBrowserCallback_DoesNotRevokeTheReplacementGrantWhenTh .Should().Be(PlatformDefaultCopilotBindingOutcome.Success); httpClientFactory.ProviderGrantRevocations.Should().Be(0); + var binding = await db.PlatformDefaultCopilotBindings.SingleAsync(); + var activeSecret = await secrets.GetSecretAsync(binding.CredentialReference); + activeSecret.Found.Should().BeTrue(); + activeSecret.Value.Should().Contain("ghu_same"); (await secrets.GetSecretAsync("copilot-app-platform-default-old")).Should().Be(SecretGetResult.NotFound); } @@ -189,7 +260,7 @@ private static async Task OpenDatabaseAsync() private static string Query(string url, string name) => Uri.UnescapeDataString(new Uri(url).Query.TrimStart('?').Split('&').Single(x => x.StartsWith($"{name}=", StringComparison.Ordinal)).Split('=', 2)[1]); - private sealed class StubHttpClientFactory(string? response) : IHttpClientFactory + private sealed class StubHttpClientFactory(string? response = null) : IHttpClientFactory { public int ProviderGrantRevocations { get; private set; } diff --git a/tests/Agentweaver.Tests/Auth/ProjectCopilotBindingServiceTests.cs b/tests/Agentweaver.Tests/Auth/ProjectCopilotBindingServiceTests.cs index c61935662..fea4a1bb4 100644 --- a/tests/Agentweaver.Tests/Auth/ProjectCopilotBindingServiceTests.cs +++ b/tests/Agentweaver.Tests/Auth/ProjectCopilotBindingServiceTests.cs @@ -139,6 +139,52 @@ public async Task Disconnect_AllowsHumanAdminButTombstonesOnlyThatProjectsBindin (await secrets.GetSecretAsync("other-secret")).Value.Should().Contain("ghu_other"); } + [Fact] + public async Task Disconnect_RevokesOnlyTheRemovedTokenWhenAnotherBindingUsesTheSameGitHubLogin() + { + await using var db = await OpenDatabaseAsync(); + var roles = new MutableRoles(); + var secrets = new InMemorySecretStore(); + var httpClientFactory = new StubHttpClientFactory(); + var project = ProjectId.New(); + var other = ProjectId.New(); + await SeedProjectAsync(db, project, other); + await new GitHubConnectionsPersistenceStore(db).ReplaceCopilotBindingAsync(Binding(project, "project-secret", "version-one")); + await new GitHubConnectionsPersistenceStore(db).ReplaceCopilotBindingAsync(Binding(other, "other-secret", "version-two")); + await secrets.SetSecretAsync("project-secret", """{"Status":"signed-in","AccessToken":"ghu_shared_1","GitHubLogin":"octocat"}"""); + await secrets.SetSecretAsync("other-secret", """{"Status":"signed-in","AccessToken":"ghu_shared_2","GitHubLogin":"octocat"}"""); + var service = CreateService(db, roles, secrets, httpClientFactory: httpClientFactory); + var admin = new CallerContext { User = "admin", EntraObjectId = "admin", PlatformRoles = [PlatformRoles.PlatformAdmin] }; + + (await service.DisconnectAsync(admin, HumanPrincipal(), project)).Should().Be(CopilotBindingOutcome.Success); + + httpClientFactory.ProviderGrantRevocations.Should().Be(1); + (await secrets.GetSecretAsync("other-secret")).Value.Should().Contain("ghu_shared_2"); + } + + [Fact] + public async Task Disconnect_DoesNotRevokeATokenThatIsStillUsedByAnotherBinding() + { + await using var db = await OpenDatabaseAsync(); + var roles = new MutableRoles(); + var secrets = new InMemorySecretStore(); + var httpClientFactory = new StubHttpClientFactory(); + var project = ProjectId.New(); + var other = ProjectId.New(); + await SeedProjectAsync(db, project, other); + await new GitHubConnectionsPersistenceStore(db).ReplaceCopilotBindingAsync(Binding(project, "project-secret", "version-one")); + await new GitHubConnectionsPersistenceStore(db).ReplaceCopilotBindingAsync(Binding(other, "other-secret", "version-two")); + await secrets.SetSecretAsync("project-secret", """{"Status":"signed-in","AccessToken":"ghu_shared","GitHubLogin":"octocat"}"""); + await secrets.SetSecretAsync("other-secret", """{"Status":"signed-in","AccessToken":"ghu_shared","GitHubLogin":"octocat"}"""); + var service = CreateService(db, roles, secrets, httpClientFactory: httpClientFactory); + var admin = new CallerContext { User = "admin", EntraObjectId = "admin", PlatformRoles = [PlatformRoles.PlatformAdmin] }; + + (await service.DisconnectAsync(admin, HumanPrincipal(), project)).Should().Be(CopilotBindingOutcome.Success); + + httpClientFactory.ProviderGrantRevocations.Should().Be(0); + (await secrets.GetSecretAsync("other-secret")).Value.Should().Contain("ghu_shared"); + } + [Fact] public async Task BindingAndAuditSerialization_RedactsProviderCredential() { @@ -204,7 +250,12 @@ public async Task ConnectionStatus_WithoutABindingReportsNotConnected() connection.GitHubLogin.Should().BeNull(); } - private static ProjectCopilotBindingService CreateService(MemoryDbContext db, MutableRoles roles, ISecretStore secrets, string? provider = null) + private static ProjectCopilotBindingService CreateService( + MemoryDbContext db, + MutableRoles roles, + ISecretStore secrets, + string? provider = null, + StubHttpClientFactory? httpClientFactory = null) { var configuration = new ConfigurationBuilder().AddInMemoryCollection(new Dictionary { @@ -212,7 +263,7 @@ private static ProjectCopilotBindingService CreateService(MemoryDbContext db, Mu ["Auth:CopilotApp:CallbackUrl"] = "https://agentweaver.test/auth/github/copilot-app/callback", ["Auth:CopilotApp:Slug"] = "agentweaver-copilot", }).Build(); - var httpClientFactory = new StubHttpClientFactory(provider); + httpClientFactory ??= new StubHttpClientFactory(provider); return new(configuration, new GitHubConnectionsPersistenceStore(db), secrets, httpClientFactory, roles, new CopilotAppRegistrationService(configuration, httpClientFactory), NullLogger.Instance); @@ -249,22 +300,31 @@ private sealed class MutableRoles : IProjectRoleAssignmentStore public Task DeleteAsync(ProjectId p, string s, CancellationToken ct = default) => throw new NotSupportedException(); public Task DeleteEnsuringOwnerInvariantAsync(ProjectId p, string s, CancellationToken ct = default) => throw new NotSupportedException(); } - private sealed class StubHttpClientFactory(string? response) : IHttpClientFactory + private sealed class StubHttpClientFactory(string? response = null) : IHttpClientFactory { - public HttpClient CreateClient(string name) => new(new Handler(response ?? """{"access_token":"ghu_token"}""")); - private sealed class Handler(string body) : HttpMessageHandler + public int ProviderGrantRevocations { get; private set; } + + public HttpClient CreateClient(string name) => new(new Handler(response ?? """{"access_token":"ghu_token"}""", this)); + private sealed class Handler(string body, StubHttpClientFactory owner) : HttpMessageHandler { protected override Task SendAsync(HttpRequestMessage request, CancellationToken ct) => - Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) + Task.FromResult(CreateResponse(request, owner, body)); + } + + private static HttpResponseMessage CreateResponse(HttpRequestMessage request, StubHttpClientFactory owner, string body) + { + if (request.RequestUri!.AbsolutePath.Contains("/applications/", StringComparison.Ordinal)) + owner.ProviderGrantRevocations++; + return new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent(request.RequestUri.AbsolutePath switch { - Content = new StringContent(request.RequestUri!.AbsolutePath switch - { - var path when path.StartsWith("/apps/", StringComparison.Ordinal) => - """{"permissions":{"metadata":"read"}}""", - "/user" => """{"login":"octocat"}""", - _ => body, - }), - }); + var path when path.StartsWith("/apps/", StringComparison.Ordinal) => + """{"permissions":{"metadata":"read"}}""", + "/user" => """{"login":"octocat"}""", + _ => body, + }), + }; } } }