From c190d7a4747dd6df3fba641d307ea265b399b6d7 Mon Sep 17 00:00:00 2001 From: Bernhard Windisch Date: Wed, 22 Jul 2026 18:29:38 +0200 Subject: [PATCH] fix(oauth): backfill ept:pushed_authorization on pre-existing clients MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since Modgud advertises the RFC-9126 /connect/par endpoint, current .NET OIDC clients call it by default (PushedAuthorizationBehavior.UseIfAvailable). BuildClientPermissions() grants ept:pushed_authorization as a baseline, but OAuthAdminService only recalculates permissions when a grant/scope change (or an unrelated re-save) comes through — so clients persisted before the baseline kept their old permission list and failed the PAR challenge with unauthorized_client / OpenIddict ID2183 until an admin re-saved them. Add PushedAuthorizationPermissionBackfill, a one-time idempotent boot migration (same cold-start-walks-every-realm pattern as LegacyAdminGroupRenameBootstrap): for every active realm it enters the tenant context and appends a SetPermissions event adding the missing PAR endpoint permission to each non-deleted client that lacks it. Add-only — grants, scopes, response types and other endpoints are carried through unchanged, and the separate ft:par requirement (RequirePushedAuthorizationRequests) is never touched, so this grants "may use PAR", never "must". The projection carries the authoritative permission set, so only streams that actually need it are aggregated + appended; a repeat boot finds none and appends nothing. Tests (Modgud.Api.Tests/Authorization/PushedAuthorizationPermissionBackfillTests): adds the permission while preserving the rest + ft:par and is idempotent (one append, second boot none); a client that already has it is untouched; a stripped legacy client is refused by /connect/par (ID2183) before the backfill and accepted (201) after — no manual re-save required. Ref: Atlas modgud/engineering/par-endpoint-permission-backfill-missing (DevOrbs incident 2026-07-22). Co-Authored-By: Claude Opus 4.8 (1M context) --- ...hedAuthorizationPermissionBackfillTests.cs | 212 ++++++++++++++++++ src/dotnet/Modgud.Api/Program.cs | 5 + .../PushedAuthorizationPermissionBackfill.cs | 108 +++++++++ 3 files changed, 325 insertions(+) create mode 100644 src/dotnet/Modgud.Api.Tests/Authorization/PushedAuthorizationPermissionBackfillTests.cs create mode 100644 src/dotnet/Modgud.Authentication/Setup/PushedAuthorizationPermissionBackfill.cs diff --git a/src/dotnet/Modgud.Api.Tests/Authorization/PushedAuthorizationPermissionBackfillTests.cs b/src/dotnet/Modgud.Api.Tests/Authorization/PushedAuthorizationPermissionBackfillTests.cs new file mode 100644 index 00000000..51190d16 --- /dev/null +++ b/src/dotnet/Modgud.Api.Tests/Authorization/PushedAuthorizationPermissionBackfillTests.cs @@ -0,0 +1,212 @@ +using System.Net; +using System.Net.Http.Headers; +using System.Security.Cryptography; +using System.Text; +using Marten; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Modgud.Api.Tests.Infrastructure; +using Modgud.Application.DTOs.OAuth; +using Modgud.Application.Services; +using Modgud.Authentication.Setup; +using Modgud.Domain.OAuth.Applications; +using Modgud.Domain.OAuth.Common; + +namespace Modgud.Api.Tests.Authorization; + +/// +/// Verifies the one-time PAR-permission boot backfill +/// (): existing OAuth clients +/// that predate the ept:pushed_authorization baseline get it added on the +/// next boot, without a manual re-save and without touching anything else — in +/// particular the separate ft:par requirement. +/// +[Collection(IntegrationTestCollection.Name)] +public class PushedAuthorizationPermissionBackfillTests : IntegrationTestBase +{ + public PushedAuthorizationPermissionBackfillTests(SharedPostgresFixture fixture) : base(fixture) { } + + private const string Par = OAuthPermissions.Endpoints.PushedAuthorization; + private const string ParRequirement = OAuthPermissions.Requirements.PushedAuthorizationRequests; + + [Fact] + public async Task Backfill_adds_par_permission_preserves_the_rest_and_is_idempotent() + { + var ct = TestContext.Current.CancellationToken; + var clientId = $"legacy-{Guid.NewGuid():N}"; + + // A client that also *requires* PAR (ft:par), so we can prove the + // requirement is left untouched by the permission backfill. + await CreateConfidentialClientAsync(clientId, "S3cret!Value", "https://app.example/cb", + ["openid"], requirePar: true); + var streamId = await StreamIdForAsync(clientId, ct); + + // Simulate a pre-migration client: strip the PAR endpoint permission. + var (permsBefore, reqsBefore) = await StripParPermissionAsync(streamId, ct); + Assert.DoesNotContain(Par, permsBefore); + Assert.Contains(ParRequirement, reqsBefore); // ft:par is on and must stay on + + var versionBefore = await StreamVersionAsync(streamId, ct); + + // Run the boot migration. + await RunBackfillAsync(ct); + + var agg = await LoadAggregateAsync(streamId, ct); + // Exactly the previous permissions plus PAR — nothing else added or dropped. + Assert.Equal( + permsBefore.Append(Par).OrderBy(p => p, StringComparer.Ordinal), + agg.Permissions.OrderBy(p => p, StringComparer.Ordinal)); + // The ft:par requirement (and any others) are byte-for-byte unchanged. + Assert.Equal(reqsBefore, agg.Requirements); + + // One append happened. + var versionAfter = await StreamVersionAsync(streamId, ct); + Assert.Equal(versionBefore + 1, versionAfter); + + // Idempotent: a second boot appends nothing. + await RunBackfillAsync(ct); + Assert.Equal(versionAfter, await StreamVersionAsync(streamId, ct)); + } + + [Fact] + public async Task A_client_that_already_has_the_permission_is_left_untouched() + { + var ct = TestContext.Current.CancellationToken; + var clientId = $"fresh-{Guid.NewGuid():N}"; + + // A freshly-created client already carries the PAR permission. + await CreateConfidentialClientAsync(clientId, "S3cret!Value", "https://app.example/cb", ["openid"]); + var streamId = await StreamIdForAsync(clientId, ct); + Assert.Contains(Par, (await LoadAggregateAsync(streamId, ct)).Permissions); + + var versionBefore = await StreamVersionAsync(streamId, ct); + await RunBackfillAsync(ct); + Assert.Equal(versionBefore, await StreamVersionAsync(streamId, ct)); // no new event + } + + [Fact] + public async Task Backfilled_client_can_reach_the_par_endpoint() + { + var ct = TestContext.Current.CancellationToken; + var clientId = $"legacy-par-{Guid.NewGuid():N}"; + const string secret = "S3cret!Value"; + + await CreateConfidentialClientAsync(clientId, secret, "https://app.example/cb", ["openid"]); + var streamId = await StreamIdForAsync(clientId, ct); + await StripParPermissionAsync(streamId, ct); + + // Before the backfill the advertised endpoint refuses the client (ID2183). + Assert.NotEqual(HttpStatusCode.Created, await PostParAsync(clientId, secret)); + + await RunBackfillAsync(ct); + + // After the backfill the same client is accepted. + Assert.Equal(HttpStatusCode.Created, await PostParAsync(clientId, secret)); + } + + // ── helpers ── + + private async Task RunBackfillAsync(CancellationToken ct) + { + var migration = Factory.Services.GetServices() + .OfType().Single(); + await migration.StartAsync(ct); + } + + private async Task StreamIdForAsync(string clientId, CancellationToken ct) + { + using var session = GetTenantedSession(); + var state = await session.Query() + .FirstOrDefaultAsync(x => x.ClientId == clientId, ct); + Assert.NotNull(state); + return state!.Id; + } + + private async Task LoadAggregateAsync(Guid streamId, CancellationToken ct) + { + using var session = GetTenantedSession(); + var agg = await session.Events.AggregateStreamAsync(streamId, token: ct); + Assert.NotNull(agg); + return agg!; + } + + private async Task StreamVersionAsync(Guid streamId, CancellationToken ct) + { + using var session = GetTenantedSession(); + var events = await session.Events.FetchStreamAsync(streamId, token: ct); + return events.Count == 0 ? 0 : events[^1].Version; + } + + private async Task<(List perms, List reqs)> StripParPermissionAsync(Guid streamId, CancellationToken ct) + { + using var session = GetTenantedDocumentSession(); + var agg = await session.Events.AggregateStreamAsync(streamId, token: ct); + Assert.NotNull(agg); + var stripped = agg!.Permissions.Where(p => p != Par).ToList(); + session.Events.Append(streamId, agg.SetPermissions(stripped)); + await session.SaveChangesAsync(ct); + return (stripped, agg.Requirements.ToList()); + } + + private async Task PostParAsync(string clientId, string clientSecret) + { + var verifier = GeneratePkceVerifier(); + var form = new Dictionary + { + ["response_type"] = "code", + ["client_id"] = clientId, + ["redirect_uri"] = "https://app.example/cb", + ["scope"] = "openid", + ["code_challenge"] = GeneratePkceS256Challenge(verifier), + ["code_challenge_method"] = "S256", + }; + var client = Factory.CreateClient(); + using var req = new HttpRequestMessage(HttpMethod.Post, "/connect/par") + { + Content = new FormUrlEncodedContent(form), + }; + req.Headers.Authorization = new AuthenticationHeaderValue( + "Basic", Convert.ToBase64String(Encoding.ASCII.GetBytes($"{clientId}:{clientSecret}"))); + var resp = await client.SendAsync(req, TestContext.Current.CancellationToken); + return resp.StatusCode; + } + + private async Task CreateConfidentialClientAsync( + string clientId, string clientSecret, string redirectUri, List scopes, bool requirePar = false) + { + using var scope = Factory.Services.CreateScope(); + var oauthAdmin = scope.ServiceProvider.GetRequiredService(); + var dto = new CreateOAuthClientDto + { + ClientId = clientId, + ClientSecret = clientSecret, + ClientType = OAuthClientTypes.Confidential, + ConsentType = OAuthConsentTypes.Implicit, + DisplayName = clientId, + RedirectUris = [redirectUri], + PostLogoutRedirectUris = [], + Scopes = scopes, + AllowedGrantTypes = ["authorization_code", "refresh_token"], + RequireConsent = false, + AccessTokenType = AccessTokenType.Jwt, + RequirePushedAuthorizationRequests = requirePar, + }; + var result = await oauthAdmin.CreateClientAsync(dto, TestContext.Current.CancellationToken); + if (result.IsError) + throw new InvalidOperationException( + $"CreateClientAsync failed: {string.Join(", ", result.Errors.Select(e => $"{e.Code}: {e.Description}"))}"); + } + + private static string GeneratePkceVerifier() + { + Span bytes = stackalloc byte[32]; + RandomNumberGenerator.Fill(bytes); + return Base64UrlEncode(bytes); + } + + private static string GeneratePkceS256Challenge(string verifier) + => Base64UrlEncode(SHA256.HashData(Encoding.ASCII.GetBytes(verifier))); + + private static string Base64UrlEncode(ReadOnlySpan bytes) + => Convert.ToBase64String(bytes).TrimEnd('=').Replace('+', '-').Replace('/', '_'); +} diff --git a/src/dotnet/Modgud.Api/Program.cs b/src/dotnet/Modgud.Api/Program.cs index 739473a8..f1fd0554 100644 --- a/src/dotnet/Modgud.Api/Program.cs +++ b/src/dotnet/Modgud.Api/Program.cs @@ -659,6 +659,11 @@ // renames a realm's legacy "Administratoren" admin group to "Administrators". // Same cold-start-walks-every-realm pattern as OidcSchemeBootstrap below. builder.Services.AddHostedService(); + // RFC 9126 (2026-07) — one-time, idempotent boot migration that backfills the + // ept:pushed_authorization endpoint permission onto pre-existing OAuth clients + // so they can use the advertised /connect/par endpoint (fixes unauthorized_client + // / ID2183 without a manual re-save). Same cold-start-walks-every-realm pattern. + builder.Services.AddHostedService(); builder.Services.AddSingleton(); builder.Services.AddSingleton(); builder.Services.AddSingleton(); diff --git a/src/dotnet/Modgud.Authentication/Setup/PushedAuthorizationPermissionBackfill.cs b/src/dotnet/Modgud.Authentication/Setup/PushedAuthorizationPermissionBackfill.cs new file mode 100644 index 00000000..be5b9854 --- /dev/null +++ b/src/dotnet/Modgud.Authentication/Setup/PushedAuthorizationPermissionBackfill.cs @@ -0,0 +1,108 @@ +using Marten; +using Modgud.Domain.OAuth.Applications; +using Modgud.Domain.OAuth.Common; +using Modgud.Infrastructure.Persistence.Tenancy; +using Modgud.Infrastructure.Realms; + +namespace Modgud.Authentication.Setup; + +/// +/// One-time, idempotent boot migration (2026-07): backfills the RFC-9126 PAR +/// endpoint permission ept:pushed_authorization onto every existing +/// OAuth client that predates it. +/// +/// Since Modgud advertises /connect/par in discovery, current +/// .NET OIDC clients call it by default (PushedAuthorizationBehavior.UseIfAvailable). +/// OAuthAdminMapping.BuildClientPermissions() grants the permission as a +/// baseline, but only to clients that are created or whose permissions are +/// recalculated (a grant/scope change, or an unrelated re-save). Clients +/// persisted before that change kept their old permission list and failed the +/// PAR challenge with unauthorized_client / OpenIddict ID2183 +/// until an admin re-saved them. This closes that upgrade gap. +/// +/// Same cold-start-walks-every-realm shape as +/// : re-enter +/// per realm so the tenant-scoped reads/writes the +/// correct database, and append directly to the event stream (no Wolverine bus, +/// so tenant DBs never need the durable-inbox tables). Naturally idempotent — a +/// repeat boot finds every client already carrying the permission and appends +/// nothing. +/// +/// Only is touched, and +/// only by adding the one missing entry. The PAR requirement +/// (ft:par / RequirePushedAuthorizationRequests) lives in the +/// separate list and is left +/// exactly as-is — this grants "may use PAR", never "must use PAR". +/// +public class PushedAuthorizationPermissionBackfill( + IServiceScopeFactory scopeFactory, + IRealmCache realmCache, + ILogger logger) : IHostedService +{ + private const string ParPermission = OAuthPermissions.Endpoints.PushedAuthorization; + + public async Task StartAsync(CancellationToken cancellationToken) + { + var realms = await realmCache.GetAllActiveAsync(); + var totalBackfilled = 0; + + foreach (var realm in realms) + { + try + { + using var _ = TenantContext.Enter(realm.Slug); + using var scope = scopeFactory.CreateScope(); + var session = scope.ServiceProvider.GetRequiredService(); + + // The projection carries the authoritative permission set, so we + // only aggregate + append for streams that actually lack it — a + // repeat boot returns zero candidates and does no stream work. + var candidates = await session.Query() + .Where(x => !x.IsDeleted) + .ToListAsync(cancellationToken); + + var backfilled = 0; + foreach (var state in candidates) + { + if (state.Permissions.Contains(ParPermission)) continue; + + var aggregate = await session.Events + .AggregateStreamAsync(state.Id, token: cancellationToken); + // Re-check on the authoritative aggregate; skip deleted streams. + if (aggregate is null || aggregate.IsDeleted) continue; + if (aggregate.Permissions.Contains(ParPermission)) continue; + + // Add-only: every existing permission (grants, scopes, response + // types, other endpoints) is carried through unchanged. + var permissions = new List(aggregate.Permissions) { ParPermission }; + session.Events.Append(state.Id, aggregate.SetPermissions(permissions)); + backfilled++; + } + + if (backfilled > 0) + { + await session.SaveChangesAsync(cancellationToken); + totalBackfilled += backfilled; + logger.LogInformation( + "Backfilled '{Perm}' on {Count} OAuth client(s) in realm {Realm}", + ParPermission, backfilled, realm.Slug); + } + } + catch (Exception ex) + { + logger.LogError(ex, + "PAR permission backfill failed for realm {Realm} — it self-heals on the next boot", + realm.Slug); + } + } + + if (totalBackfilled > 0) + { + logger.LogInformation( + "PushedAuthorizationPermissionBackfill added '{Perm}' to {Count} client(s) across {Realms} realm(s)", + ParPermission, totalBackfilled, realms.Count); + } + } + + public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask; +}