From 93ee0d6d9d72f3ab83cd93ea156017380f11d7f5 Mon Sep 17 00:00:00 2001 From: Bernhard Windisch Date: Fri, 18 Sep 2026 10:07:24 +0200 Subject: [PATCH 1/3] fix(oauth): AllowRememberConsent=false shows the consent screen again The flag was stored, exported and documented, but nothing read it: a remembered authorization skipped the consent screen for every client, DCR included, although the DCR docs promise the opposite. Authorize now honours it. The existing authorization is still reused, so the grant's oi_au_id survives a re-consent; the post-consent re-entry redeems its approved ticket once (?consent_ticket=) instead of leaning on the remembered-authorization shortcut. CIMD clients get the flag forced off like DCR clients. Co-Authored-By: Claude Fable 5.1 --- .../Authorization/DcrFullFlowTests.cs | 61 +++++++++++ .../Auth/OAuth/AuthorizationEndpoints.cs | 103 ++++++++++++++++-- .../Features/Auth/OAuth/ConsentEndpoints.cs | 33 +++++- .../OAuth/Consent/ConsentTicket.cs | 10 ++ .../OpenIddict/Cimd/CimdClientResolver.cs | 3 + 5 files changed, 193 insertions(+), 17 deletions(-) diff --git a/src/dotnet/Modgud.Api.Tests/Authorization/DcrFullFlowTests.cs b/src/dotnet/Modgud.Api.Tests/Authorization/DcrFullFlowTests.cs index 3da5fe47..ac3d2148 100644 --- a/src/dotnet/Modgud.Api.Tests/Authorization/DcrFullFlowTests.cs +++ b/src/dotnet/Modgud.Api.Tests/Authorization/DcrFullFlowTests.cs @@ -73,6 +73,67 @@ public async Task Happy_path_register_then_authorize_consent_token_yields_audien Assert.InRange(lifetimeMinutes, 14, 16); } + [Fact] + public async Task Second_authorize_shows_consent_again_but_keeps_the_authorization_id() + { + await SeedAsync(); + var clientId = await RegisterDcrClientAsync(scope: $"openid {ScopeName}"); + + // DriveDcrAuthCodeFlowAsync asserts the redirect to /consent, so the + // second call is itself the proof that the remembered authorization + // did not skip the screen (AllowRememberConsent=false for DCR). + var (first, _) = await DriveDcrAuthCodeFlowAsync(clientId, $"openid {ScopeName}", AllowedAudience); + var (second, _) = await DriveDcrAuthCodeFlowAsync(clientId, $"openid {ScopeName}", AllowedAudience); + + var handler = new JwtSecurityTokenHandler(); + var firstId = handler.ReadJwtToken(first).Payload["oi_au_id"].ToString(); + var secondId = handler.ReadJwtToken(second).Payload["oi_au_id"].ToString(); + Assert.False(string.IsNullOrEmpty(firstId)); + Assert.Equal(firstId, secondId); + } + + [Fact] + public async Task Approved_consent_redirect_completes_the_flow_only_once() + { + await SeedAsync(); + var clientId = await RegisterDcrClientAsync(scope: $"openid {ScopeName}"); + + var cookieClient = await CreateAuthenticatedClientAsync("tu", "TestPass1234"); + var authorizeUri = "/connect/authorize?" + string.Join("&", new[] + { + "response_type=code", + $"client_id={Uri.EscapeDataString(clientId)}", + $"redirect_uri={Uri.EscapeDataString(RedirectUri)}", + $"scope={Uri.EscapeDataString($"openid {ScopeName}")}", + $"state={Guid.NewGuid():N}", + $"code_challenge={GeneratePkceS256Challenge(GeneratePkceVerifier())}", + "code_challenge_method=S256", + $"resource={Uri.EscapeDataString(AllowedAudience)}", + }); + var authorizeResp = await cookieClient.GetAsync(authorizeUri, TestContext.Current.CancellationToken); + AssertRedirect(authorizeResp); + var ticketId = authorizeResp.Headers.Location!.ToString()["/consent?ticket=".Length..]; + + var decisionResp = await cookieClient.PostAsJsonAsync( + "/connect/consent", + new { Ticket = ticketId, Approved = true, ApprovedScopes = new[] { "openid", ScopeName } }, + TestContext.Current.CancellationToken); + Assert.True(decisionResp.IsSuccessStatusCode); + using var decisionDoc = JsonDocument.Parse( + await decisionResp.Content.ReadAsStringAsync(TestContext.Current.CancellationToken)); + var followUpUrl = decisionDoc.RootElement.GetProperty("RedirectUrl").GetString()!; + + var firstUse = await cookieClient.GetAsync(followUpUrl, TestContext.Current.CancellationToken); + AssertRedirect(firstUse); + Assert.Contains("code=", firstUse.Headers.Location!.Query); + + // The same URL again (browser back button, or a link lifted from the + // history) must not mint a second code without a new consent. + var replay = await cookieClient.GetAsync(followUpUrl, TestContext.Current.CancellationToken); + AssertRedirect(replay); + Assert.StartsWith("/consent?ticket=", replay.Headers.Location!.ToString()); + } + [Fact] public async Task Last_used_at_advances_after_first_token_issue() { diff --git a/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs b/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs index d6a5fe49..a0b09cc2 100644 --- a/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs +++ b/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs @@ -213,16 +213,22 @@ private static async Task AuthorizeAsync( var consentType = await applicationManager.GetConsentTypeAsync(application); - // CIMD: the synthesized client is ConsentType=explicit, so - // a first authorize always lands on the consent screen — which shows - // the client_id hostname + "unverified" marker as the phishing - // mitigation. Subsequent authorizes for the same user+client+scopes - // auto-approve via the remembered authorization, exactly like every - // other client (DCR included). We deliberately do NOT force consent - // on every authorize: the post-consent re-entry to /authorize relies - // on this same shortcut to complete the round-trip, so forcing it - // would loop the user back to /consent indefinitely. - if (consentType == ConsentTypes.Implicit || authorizations.Count != 0) + // A remembered authorization skips the consent screen only for clients + // that allow it. With AllowRememberConsent=false (forced for DCR and + // CIMD clients — anyone can replay a public client_id against a + // loopback redirect, RFC 8252 §8.6) every fresh authorize shows the + // screen again. The authorization itself is still REUSED, so the + // grant's `oi_au_id` stays stable across re-consents. The post-consent + // re-entry completes the round-trip by redeeming its approved ticket + // once, instead of relying on the remembered-authorization shortcut. + var skipConsent = consentType == ConsentTypes.Implicit; + if (!skipConsent && authorizations.Count != 0) + { + skipConsent = await AllowsRememberConsentAsync(applicationManager, application) + || await TryRedeemConsentTicketAsync(httpContext, request, user.Id, session); + } + + if (skipConsent) { var principal = await CreateClaimsPrincipalAsync( user, request, scopeManager, userManager: userManager, @@ -272,7 +278,7 @@ private static async Task AuthorizeAsync( Subject = user.Id, ClientId = request.ClientId!, RequestedScopes = request.GetScopes().ToArray(), - AuthorizeRequestQuery = httpContext.Request.QueryString.Value ?? string.Empty, + AuthorizeRequestQuery = WithoutTicketMarkers(httpContext.Request.Query), CreatedAt = DateTimeOffset.UtcNow, ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(5), }; @@ -282,6 +288,81 @@ private static async Task AuthorizeAsync( return Results.Redirect($"/consent?ticket={ticket.Id:N}"); } + /// + /// The locked authorize query a consent ticket replays. A spent + /// consent_ticket / deny_ticket marker from an earlier round + /// must not ride along: the consent endpoint appends a fresh one, and a + /// doubled parameter would no longer read as a single value — the re-entry + /// could never redeem it and the user would loop back to /consent. + /// + private static string WithoutTicketMarkers(IQueryCollection query) + { + var kept = query + .Where(p => p.Key is not ("consent_ticket" or "deny_ticket")) + .SelectMany(p => p.Value, (p, value) => KeyValuePair.Create(p.Key, value)); + return QueryString.Create(kept).Value ?? string.Empty; + } + + private static async Task AllowsRememberConsentAsync( + IOpenIddictApplicationManager applicationManager, + object application) + { + var properties = await applicationManager.GetPropertiesAsync(application); + return !properties.TryGetValue( + Modgud.Domain.OAuth.Applications.OAuthApplicationPropertyKeys.AllowRememberConsent, + out var element) + || element.ValueKind != System.Text.Json.JsonValueKind.False; + } + + /// + /// Redeems the ?consent_ticket={id} the consent endpoint appends on + /// approval. Genuine means: approved (consumed, not denied), bound to this + /// user and client, for exactly the scopes now requested, consumed within + /// the last two minutes, and not redeemed before. Anything else returns + /// false and the caller re-prompts — a forged marker buys nothing. + /// + private static async Task TryRedeemConsentTicketAsync( + HttpContext httpContext, + OpenIddictRequest request, + Guid userId, + IDocumentSession session) + { + // Read the raw query, not the OpenIddict request: under PAR the request + // is rebuilt from the pushed payload and never carries this marker. + if (httpContext.Request.Query["consent_ticket"] is not [{ Length: > 0 } raw] + || !Guid.TryParseExact(raw, "N", out var ticketId)) + { + return false; + } + + var ticket = await session.LoadAsync(ticketId); + var now = DateTimeOffset.UtcNow; + if (ticket is not { ConsumedAt: not null, DeniedAt: null, RedeemedAt: null } + || ticket.Subject != userId + || !string.Equals(ticket.ClientId, request.ClientId, StringComparison.Ordinal) + || ticket.ConsumedAt < now.AddMinutes(-2) + || !ticket.RequestedScopes.ToHashSet(StringComparer.Ordinal).SetEquals(request.GetScopes())) + { + return false; + } + + ticket.RedeemedAt = now; + session.Store(ticket); + try + { + await session.SaveChangesAsync(); + } + catch (JasperFx.ConcurrencyException) + { + // Lost the race to a parallel re-entry. Drop the stale write so the + // consent ticket the caller stores next can still be saved. + session.EjectAllPendingChanges(); + return false; + } + + return true; + } + private static async Task ExchangeAsync( HttpContext httpContext, IOpenIddictApplicationManager applicationManager, diff --git a/src/dotnet/Modgud.Api/Features/Auth/OAuth/ConsentEndpoints.cs b/src/dotnet/Modgud.Api/Features/Auth/OAuth/ConsentEndpoints.cs index d4603479..13906b7c 100644 --- a/src/dotnet/Modgud.Api/Features/Auth/OAuth/ConsentEndpoints.cs +++ b/src/dotnet/Modgud.Api/Features/Auth/OAuth/ConsentEndpoints.cs @@ -230,12 +230,30 @@ private static async Task SubmitConsentAsync( var principal = new ClaimsPrincipal(identity); principal.SetScopes(approvedSet); - await authorizationManager.CreateAsync( - principal: principal, - subject: await userManager.GetUserIdAsync(user), - client: await applicationManager.GetIdAsync(application) ?? string.Empty, + var subject = await userManager.GetUserIdAsync(user); + var clientPk = await applicationManager.GetIdAsync(application) ?? string.Empty; + + // A client with AllowRememberConsent=false lands here on every fresh + // authorize although its authorization already exists. Re-affirming + // must not mint a second one: consumers key their own per-connection + // state on the authorization id (`oi_au_id`), so it has to survive a + // re-consent for the same scopes. + var existing = await authorizationManager.FindAsync( + subject: subject, + client: clientPk, + status: Statuses.Valid, type: AuthorizationTypes.Permanent, - scopes: approvedSet.ToImmutableArray()); + scopes: approvedSet.ToImmutableArray()).AnyAsync(); + + if (!existing) + { + await authorizationManager.CreateAsync( + principal: principal, + subject: subject, + client: clientPk, + type: AuthorizationTypes.Permanent, + scopes: approvedSet.ToImmutableArray()); + } // The ticket was already claimed (consumed) above, before this // authorization was created — nothing more to persist here. @@ -243,9 +261,12 @@ await authorizationManager.CreateAsync( // OAUTH-08 fix: reconstruct the redirect from the SERVER-SIDE locked // query string. The SPA never sees the OAuth URL — there's no chance // for it to get tampered with between consent display and submit. + // The consent_ticket marker lets the re-entry prove this approval when + // the client's remembered authorization doesn't skip consent by itself. return Results.Ok(new ConsentResult { - RedirectUrl = "/connect/authorize" + record.AuthorizeRequestQuery, + RedirectUrl = "/connect/authorize" + record.AuthorizeRequestQuery + + "&consent_ticket=" + record.Id.ToString("N"), }); } diff --git a/src/dotnet/Modgud.Domain/OAuth/Consent/ConsentTicket.cs b/src/dotnet/Modgud.Domain/OAuth/Consent/ConsentTicket.cs index b71ba250..9014b97a 100644 --- a/src/dotnet/Modgud.Domain/OAuth/Consent/ConsentTicket.cs +++ b/src/dotnet/Modgud.Domain/OAuth/Consent/ConsentTicket.cs @@ -86,4 +86,14 @@ public class ConsentTicket /// approve path's re-entry. /// public DateTimeOffset? DeniedAt { get; set; } + + /// + /// Set when the authorize re-entry redeems an APPROVED ticket passed as + /// ?consent_ticket={Id}. Only clients with + /// AllowRememberConsent=false need it: their remembered + /// authorization no longer skips the consent screen, so the re-entry has + /// to prove that the user approved this very request a moment ago. The + /// version-checked write makes the proof single-use. + /// + public DateTimeOffset? RedeemedAt { get; set; } } diff --git a/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdClientResolver.cs b/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdClientResolver.cs index 8fbf5662..fb5222d2 100644 --- a/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdClientResolver.cs +++ b/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdClientResolver.cs @@ -259,6 +259,9 @@ private static OAuthApplicationState Synthesize(CachedCimd entry) [OAuthApplicationPropertyKeys.Enabled] = JsonSerializer.SerializeToElement(true), [OAuthApplicationPropertyKeys.DcrIsDynamicallyRegistered] = JsonSerializer.SerializeToElement(true), [OAuthApplicationPropertyKeys.CimdIsResolvedClient] = JsonSerializer.SerializeToElement(true), + // Same rule as DCR: an unverified public client never skips + // the consent screen on a remembered authorization. + [OAuthApplicationPropertyKeys.AllowRememberConsent] = JsonSerializer.SerializeToElement(false), }, AccessTokenType = AccessTokenType.Jwt, AppIds = new List(), From 6baf95159238682a650e99b5b4db0a9314b9ba3e Mon Sep 17 00:00:00 2001 From: Bernhard Windisch Date: Fri, 18 Sep 2026 10:23:13 +0200 Subject: [PATCH 2/3] feat(management-api): list and revoke an App's OAuth authorizations A consuming application that lets its users disconnect a connected system could only stop the access token on its own side; the refresh token kept living in Modgud, and RFC 7009 is out of reach because the connected system is the OAuth client, not the resource server. GET/DELETE /api/app/{id}/authorizations join the Management API behind the new oauth-authorization:read|revoke permission, bound to the client's AppIds like the scope read. An authorization is in reach when one of its scopes is app-scoped to the App or targets one of its OAuth APIs; everything else answers 404. Revoke takes the authorization, all its tokens and a native client session built on it, is idempotent, and writes security.authorization_revoked. The management-api scope example showed camelCase keys; the API emits PascalCase. Co-Authored-By: Claude Fable 5.1 --- docs/admin/client-id-metadata-documents.md | 2 + docs/admin/dynamic-client-registration.md | 2 + docs/admin/oauth-clients.md | 2 + docs/integrate/management-api.md | 78 +++- .../AppAuthorizationsApiTests.cs | 348 ++++++++++++++++++ .../Admin/Apps/AppAuthorizationsEndpoints.cs | 225 +++++++++++ src/dotnet/Modgud.Api/Program.cs | 1 + .../Api/Admin/AuthLogEndpoints.cs | 1 + .../Sessions/ClientSessionService.cs | 4 + .../Sessions/IClientSessionService.cs | 8 + .../Audit/AuditDurability.cs | 1 + .../Audit/AuditEvents.cs | 8 + .../Authorization/AppRealmSeeder.cs | 1 + .../DependencyInjection.cs | 4 + .../Audit/AuditDurabilityTests.cs | 1 + 15 files changed, 666 insertions(+), 20 deletions(-) create mode 100644 src/dotnet/Modgud.Api.Tests/Authorization/AppAuthorizationsApiTests.cs create mode 100644 src/dotnet/Modgud.Api/Features/Admin/Apps/AppAuthorizationsEndpoints.cs diff --git a/docs/admin/client-id-metadata-documents.md b/docs/admin/client-id-metadata-documents.md index 8ac96fb9..d3915e3c 100644 --- a/docs/admin/client-id-metadata-documents.md +++ b/docs/admin/client-id-metadata-documents.md @@ -100,6 +100,8 @@ A CIMD client always reaches the explicit consent screen on first authorize, wit - The **`client_id` hostname** (e.g. `claude.ai`) shown prominently — the domain that owns the document. Verify it matches the app you intended, not just the self-asserted display name. - An **`[unverified]`** marker + warning callout, the same treatment self-registered clients get. +Like a DCR client, a CIMD client never skips this screen on a remembered authorization: every fresh authorize flow shows it again. The authorization itself is reused for the same user, client and scope set, so the token's `oi_au_id` stays stable across re-consents. + ## What's NOT in v1 - **`private_key_jwt`** — confidential CIMD clients (asymmetric client auth via a `jwks_uri` in the document). v1 is public PKCE only. Deferred to v2, which will also revoke on `jwks_uri` change. diff --git a/docs/admin/dynamic-client-registration.md b/docs/admin/dynamic-client-registration.md index 84b4d91d..b21f2f6a 100644 --- a/docs/admin/dynamic-client-registration.md +++ b/docs/admin/dynamic-client-registration.md @@ -91,6 +91,8 @@ DCR-registered clients always go through the explicit consent screen, with two e `AllowRememberConsent` is forced off for DCR clients, so the AS never skips consent for a new authorization request — a fresh authorize flow always shows the consent screen again. Clients avoid repeated prompts by retaining and refreshing the authorization they already obtained (typical pattern: the agent caches its own consent decision and reuses the refresh token instead of re-authorizing). +Re-affirming does not mint a new authorization. For the same user, client and scope set Modgud reuses the existing one, so the `oi_au_id` claim in the access token stays the same across re-consents. A resource server that keys per-connection state on `sub` + `oi_au_id` keeps that state. A new authorization (and a new `oi_au_id`) appears only when the scope set changes, when the old authorization was revoked, or when the client registers again under a new `client_id`. + ## Audit log Every DCR-related event lands in the auth log with a `DCR ` prefix in its message. The [Auth Log](./auth-log) grid's category filter chips are derived from whatever event categories are present, so look for DCR events under the **operations** (and, for rejected registrations, **security-ops**) chip rather than a dedicated DCR chip. diff --git a/docs/admin/oauth-clients.md b/docs/admin/oauth-clients.md index 2a0ffa5c..579b6e1b 100644 --- a/docs/admin/oauth-clients.md +++ b/docs/admin/oauth-clients.md @@ -98,6 +98,8 @@ link are then persisted atomically by the single Create action. | **Explicit** | The user must click "Allow" once per scope set | | **External** | Consent is obtained out-of-band; Modgud doesn't intervene | +With **Explicit** consent, `AllowRememberConsent` (default `true`; settable through the API and the realm manifest) decides what "once" means. `true`: a later authorize for the same user, client and scope set skips the screen. `false`: every fresh authorize flow shows the screen again. Either way the existing authorization is reused, so the `oi_au_id` claim stays the same for that user, client and scope set. DCR and CIMD clients are always `false`. + ### Applications The **Applications** multi-select binds the client to one or more apps. Empty means realm-wide/unassigned for App-scope entitlement; it does not mean that tokens automatically receive every App's permissions. diff --git a/docs/integrate/management-api.md b/docs/integrate/management-api.md index 52fddd46..4a106a22 100644 --- a/docs/integrate/management-api.md +++ b/docs/integrate/management-api.md @@ -163,45 +163,81 @@ A group may deliberately have no roles. It then grants no permission while still assigning its members to the selected Application scopes. The admin UI marks such a group as **No permissions**. -The response includes an opaque `scopeVersion`, the contributing root groups, +The response includes an opaque `ScopeVersion`, the contributing root groups, and the typed Principal records: ```json { - "appId": "", - "appSlug": "alert-hub", - "scopeVersion": "v1-", - "rootGroups": [ + "AppId": "", + "AppSlug": "alert-hub", + "ScopeVersion": "v1-", + "RootGroups": [ { - "id": "", - "name": "AlertHub principals", - "hasPermissions": false + "Id": "", + "Name": "AlertHub principals", + "HasPermissions": false } ], - "principals": [ + "Principals": [ { - "id": "", - "type": "person", - "displayName": "AP | Alice Person", - "isActive": true, - "isScopeRoot": false, - "accountName": "alice", - "firstname": "Alice", - "lastname": "Person", - "acronym": "AP", - "email": "alice@example.com" + "Id": "", + "Type": "person", + "DisplayName": "AP | Alice Person", + "IsActive": true, + "IsScopeRoot": false, + "AccountName": "alice", + "Firstname": "Alice", + "Lastname": "Person", + "Acronym": "AP", + "Email": "alice@example.com" } ] } ``` -`scopeVersion` versions the **definition**, not every member. Adding or removing +`ScopeVersion` versions the **definition**, not every member. Adding or removing an App binding, changing the nested-group structure, or changing an automatic membership predicate changes it and tells a consumer to perform a new full read. Ordinary direct membership/profile changes leave it stable; the resumable change stream can therefore represent those as individual changes. Consumers must treat the version as opaque and compare it for equality only. +## List and revoke an Application's authorizations + +An application that shows its users their "connected systems" (an MCP host, a Home Assistant instance, a third-party app) usually stores its own per-connection state, keyed on the access token's `sub` and `oi_au_id`. Blocking a connection on the application's side stops the access token there, but the refresh token lives in Modgud. The connected system is the OAuth client, so the application cannot use RFC 7009 revocation on its behalf. These two operations close that gap. + +`GET /api/app/{appId}/authorizations` requires `oauth-authorization:read` and returns the valid authorizations that reach the Application. An authorization reaches an Application when one of its scopes is app-scoped to that Application, or lists one of the Application's [OAuth APIs](/admin/oauth-apis) as a resource. Authorizations that only concern other Applications are never returned. + +| Query parameter | Meaning | +|---|---| +| `subject` | Optional. Only this user's authorizations (the token's `sub`). | +| `limit` | Page size, default `200`, maximum `1000`. | +| `after` | The `NextAfter` value of the previous page. | + +```json +{ + "Items": [ + { + "Id": "0b6f1c0e-5d0a-4d0e-9a57-2f2f0f6f4c11", + "Subject": "5f0c8a52-7f5e-4c7b-8a39-0a3b2f1d9e77", + "ClientId": "acme-homeassistant", + "Type": "permanent", + "Scopes": ["lists.read", "offline_access", "openid"], + "CreatedAt": "2026-09-18T09:12:44Z" + } + ], + "NextAfter": "0b6f1c0e-5d0a-4d0e-9a57-2f2f0f6f4c11" +} +``` + +`Id` and `Subject` are returned exactly as they appear in the access token (`oi_au_id`, `sub`), not as ShortGuids, because the consumer joins on them. `ClientId` is absent for a [CIMD](/admin/client-id-metadata-documents) client, which Modgud never persists. `NextAfter` is absent on the last page; keep requesting while it is present. `Type` is `permanent` for a consented Authorization Code grant and `ad-hoc` for a Device Flow or native grant. + +`DELETE /api/app/{appId}/authorizations/{id}` requires `oauth-authorization:revoke`. It revokes the authorization and every token issued under it, the refresh token included, ends a native client session built on it, and writes a `security.authorization_revoked` audit event. It answers `204 No Content`, also when the authorization was revoked already. An unknown id and an authorization outside the Application's reach both answer `404`. + +An already-issued JWT access token stays cryptographically valid until it expires. The application blocks it on its side by the same `oi_au_id`; reference tokens and the next refresh fail at Modgud immediately. The next authorize flow of that client shows the consent screen and creates a new authorization with a new `oi_au_id`. + +As with the scope read, a bearer caller may only target Applications listed in its OAuth client's `AppIds`. + ## Delegated-person setup Use this flow when the consumer should act with the permissions of a signed-in @@ -249,6 +285,8 @@ scheme does not turn every cookie-only admin route into a remote API. | `GET` | `/api/position` | `position:read` | `PositionTerminals` enabled | | `GET` | `/api/position/{id}` | `position:read` | `PositionTerminals` enabled | | `GET` | `/api/app/{id}/scope` | `app-scope:read` | Full `BoundTo`-derived Principal snapshot | +| `GET` | `/api/app/{id}/authorizations` | `oauth-authorization:read` | Valid authorizations that reach the Application; paged | +| `DELETE` | `/api/app/{id}/authorizations/{authorizationId}` | `oauth-authorization:revoke` | Revokes the authorization and all its tokens; idempotent | | `GET` | `/api/app/{id}/change-feed/snapshot` | `app-scope:read` | Feed enabled; full public synchronization snapshot | | `GET` | `/api/app/{id}/change-feed` | `app-scope:read` | Feed enabled; resumable HTTP read using an opaque cursor | | `GET` | `/api/app/{id}/change-feed/stream` | `app-scope:read` | Feed enabled; bearer-only resumable SSE stream using the same cursor and envelope | diff --git a/src/dotnet/Modgud.Api.Tests/Authorization/AppAuthorizationsApiTests.cs b/src/dotnet/Modgud.Api.Tests/Authorization/AppAuthorizationsApiTests.cs new file mode 100644 index 00000000..d1e6ddfe --- /dev/null +++ b/src/dotnet/Modgud.Api.Tests/Authorization/AppAuthorizationsApiTests.cs @@ -0,0 +1,348 @@ +using System.Collections.Immutable; +using System.Net; +using System.Net.Http.Headers; +using System.Security.Claims; +using System.Text.Json; +using BuildingBlocks.Helper; +using Marten; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.DependencyInjection; +using Modgud.Api.Tests.Infrastructure; +using Modgud.Application.DTOs.OAuth; +using Modgud.Application.Services; +using Modgud.Authorization.Apps; +using Modgud.Authorization.Events; +using Modgud.Authorization.Principals; +using Modgud.Domain.OAuth.Common; +using Modgud.Domain.OAuth.Management; +using Modgud.Domain.OAuth.Storage; +using Modgud.Infrastructure.Audit; +using OpenIddict.Abstractions; +using static OpenIddict.Abstractions.OpenIddictConstants; + +namespace Modgud.Api.Tests.Authorization; + +/// +/// The Management-API pair a consuming application uses for its "connected +/// systems" screen: list the authorizations that reach its App, and end one — +/// refresh token included. +/// +[Collection(IntegrationTestCollection.Name)] +public class AppAuthorizationsApiTests : IntegrationTestBase +{ + public AppAuthorizationsApiTests(SharedPostgresFixture fixture) : base(fixture) { } + + private sealed record AuthorizationRow(string Id, string Subject, string? ClientId, string Type, List Scopes); + private sealed record AuthorizationPage(List Items, string? NextAfter); + + [Fact] + public async Task List_returns_only_authorizations_that_reach_the_callers_app() + { + var rig = await SeedAsync(); + var viaApi = await CreateAuthorizationAsync(rig.ConnectedClientPk, rig.UserId, [Scopes.OpenId, rig.ResourceScope]); + var viaAppScope = await CreateAuthorizationAsync(rig.ConnectedClientPk, rig.UserId, [rig.AppScopedScope]); + var foreign = await CreateAuthorizationAsync(rig.ConnectedClientPk, rig.UserId, [Scopes.OpenId, rig.ForeignScope]); + + var page = await ListAsync(rig.Token, rig.OwnAppId); + + Assert.Contains(page.Items, x => x.Id == viaApi.AuthorizationId); + Assert.Contains(page.Items, x => x.Id == viaAppScope.AuthorizationId); + Assert.DoesNotContain(page.Items, x => x.Id == foreign.AuthorizationId); + Assert.Null(page.NextAfter); + + var row = page.Items.Single(x => x.Id == viaApi.AuthorizationId); + Assert.Equal(rig.UserId.ToString(), row.Subject); + Assert.Equal(rig.ConnectedClientId, row.ClientId); + Assert.Equal(AuthorizationTypes.Permanent, row.Type); + } + + [Fact] + public async Task List_pages_without_dropping_or_repeating_rows() + { + var rig = await SeedAsync(); + var expected = new HashSet(); + for (var i = 0; i < 5; i++) + expected.Add((await CreateAuthorizationAsync(rig.ConnectedClientPk, rig.UserId, [rig.ResourceScope])).AuthorizationId); + + var seen = new List(); + string? after = null; + do + { + var page = await ListAsync(rig.Token, rig.OwnAppId, after: after, limit: 2); + seen.AddRange(page.Items.Select(x => x.Id)); + after = page.NextAfter; + } while (after is not null); + + Assert.Equal(seen.Count, seen.Distinct().Count()); + Assert.True(expected.IsSubsetOf(seen), "paging dropped an authorization"); + } + + [Fact] + public async Task Revoke_ends_the_authorization_and_its_refresh_token_and_is_idempotent() + { + var ct = TestContext.Current.CancellationToken; + var rig = await SeedAsync(); + var grant = await CreateAuthorizationAsync(rig.ConnectedClientPk, rig.UserId, [rig.ResourceScope]); + + using var first = await DeleteAsync(rig.Token, rig.OwnAppId, grant.AuthorizationId); + Assert.Equal(HttpStatusCode.NoContent, first.StatusCode); + + await using (var session = GetTenantedDocumentSession()) + { + var authorization = await session.LoadAsync(grant.AuthorizationId, ct); + var token = await session.LoadAsync(grant.RefreshTokenId, ct); + Assert.Equal(Statuses.Revoked, authorization!.Status); + Assert.Equal(Statuses.Revoked, token!.Status); + } + + var page = await ListAsync(rig.Token, rig.OwnAppId); + Assert.DoesNotContain(page.Items, x => x.Id == grant.AuthorizationId); + + using var second = await DeleteAsync(rig.Token, rig.OwnAppId, grant.AuthorizationId); + Assert.Equal(HttpStatusCode.NoContent, second.StatusCode); + + await using (var session = GetTenantedDocumentSession()) + { + var audited = await session.Query() + .Where(x => x.EventType == AuditEvents.AuthorizationRevoked + && x.AuthorizationId == grant.AuthorizationId) + .ToListAsync(ct); + var entry = Assert.Single(audited); // the idempotent repeat is not a second revoke + Assert.Equal(rig.UserId, entry.TargetSubjectId); + Assert.Equal(rig.ServiceAccountId, entry.ActorSubjectId); + } + } + + [Fact] + public async Task Revoke_answers_404_for_an_authorization_outside_the_app_and_leaves_it_valid() + { + var ct = TestContext.Current.CancellationToken; + var rig = await SeedAsync(); + var foreign = await CreateAuthorizationAsync(rig.ConnectedClientPk, rig.UserId, [rig.ForeignScope]); + + using var response = await DeleteAsync(rig.Token, rig.OwnAppId, foreign.AuthorizationId); + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + + await using var session = GetTenantedDocumentSession(); + var authorization = await session.LoadAsync(foreign.AuthorizationId, ct); + var token = await session.LoadAsync(foreign.RefreshTokenId, ct); + Assert.Equal(Statuses.Valid, authorization!.Status); + Assert.Equal(Statuses.Valid, token!.Status); + } + + [Fact] + public async Task Caller_cannot_target_an_app_its_client_is_not_assigned_to() + { + var rig = await SeedAsync(); + var foreign = await CreateAuthorizationAsync(rig.ConnectedClientPk, rig.UserId, [rig.ForeignScope]); + + using var list = await SendAsync(HttpMethod.Get, rig.Token, + $"/api/app/{new ShortGuid(rig.ForeignAppId)}/authorizations"); + Assert.Equal(HttpStatusCode.Forbidden, list.StatusCode); + + using var revoke = await DeleteAsync(rig.Token, rig.ForeignAppId, foreign.AuthorizationId); + Assert.Equal(HttpStatusCode.Forbidden, revoke.StatusCode); + } + + [Fact] + public async Task Read_permission_alone_does_not_allow_revoking() + { + var rig = await SeedAsync(permissions: [("oauth-authorization", "read")]); + var grant = await CreateAuthorizationAsync(rig.ConnectedClientPk, rig.UserId, [rig.ResourceScope]); + + using var response = await DeleteAsync(rig.Token, rig.OwnAppId, grant.AuthorizationId); + var body = await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken); + Assert.Equal(HttpStatusCode.Forbidden, response.StatusCode); + Assert.Contains("Management.PermissionDenied", body); + } + + // ─── Rig ───────────────────────────────────────────────────────────── + + private sealed record Rig( + Guid OwnAppId, Guid ForeignAppId, Guid UserId, Guid ServiceAccountId, string Token, + string ConnectedClientId, string ConnectedClientPk, + string ResourceScope, string AppScopedScope, string ForeignScope); + + private sealed record Grant(string AuthorizationId, string RefreshTokenId); + + private async Task SeedAsync((string Resource, string Action)[]? permissions = null) + { + var ct = TestContext.Current.CancellationToken; + var tag = Guid.NewGuid().ToString("N")[..8]; + var ownAppId = Guid.NewGuid(); + var foreignAppId = Guid.NewGuid(); + var serviceAccount = new ServiceAccount { Id = Guid.NewGuid(), AccountName = $"authz-sa-{tag}" }; + + await using (var session = GetTenantedDocumentSession()) + { + session.Events.StartStream(ownAppId, new AppCreatedEvent( + ownAppId, $"authz-own-{tag}", "Own App", null, [], IsSystem: false)); + session.Events.StartStream(foreignAppId, new AppCreatedEvent( + foreignAppId, $"authz-foreign-{tag}", "Foreign App", null, [], IsSystem: false)); + session.Store(serviceAccount); + await session.SaveChangesAsync(ct); + } + + var role = await Factory.CreateTestRoleAsync( + $"AuthzManager_{tag}", + permissions ?? [("oauth-authorization", "read"), ("oauth-authorization", "revoke")]); + await Factory.CreateTestGroupAsync($"AuthzManagers_{tag}", [serviceAccount.Id], [role.Id]); + + var apiName = $"https://authz-own-{tag}.test/"; + var resourceScope = $"own-{tag}.read"; // global scope, reaches the App via its API + var appScopedScope = $"own-{tag}.app"; // app-scoped, no resource at all + var foreignScope = $"foreign-{tag}.read"; + + using (var scope = NewSystemTenantScope()) + { + var oauth = scope.ServiceProvider.GetRequiredService(); + AssertOk(await oauth.CreateApiAsync(new CreateOAuthApiDto + { + Name = apiName, DisplayName = apiName, AppId = new ShortGuid(ownAppId).ToString(), + }, ct)); + AssertOk(await oauth.CreateScopeAsync(new CreateOAuthScopeDto + { + Name = resourceScope, DisplayName = resourceScope, Resources = [apiName], + }, ct)); + AssertOk(await oauth.CreateScopeAsync(new CreateOAuthScopeDto + { + Name = appScopedScope, DisplayName = appScopedScope, AppId = new ShortGuid(ownAppId).ToString(), + }, ct)); + AssertOk(await oauth.CreateScopeAsync(new CreateOAuthScopeDto + { + Name = foreignScope, DisplayName = foreignScope, AppId = new ShortGuid(foreignAppId).ToString(), + }, ct)); + + // The "connected system" whose grants are listed and revoked. + var connectedClientId = $"authz-connected-{tag}"; + AssertOk(await oauth.CreateClientAsync(new CreateOAuthClientDto + { + ClientId = connectedClientId, + ClientType = OAuthClientTypes.Public, + ConsentType = OAuthConsentTypes.Explicit, + DisplayName = connectedClientId, + RedirectUris = ["http://localhost/cb"], + PostLogoutRedirectUris = [], + Scopes = [Scopes.OpenId, resourceScope, appScopedScope, foreignScope], + AllowedGrantTypes = ["authorization_code", "refresh_token"], + AccessTokenType = AccessTokenType.Jwt, + }, ct)); + + var managementClientId = $"authz-mgmt-{tag}"; + AssertOk(await oauth.CreateClientAsync(new CreateOAuthClientDto + { + ClientId = managementClientId, + ClientSecret = $"{managementClientId}-secret", + ClientType = OAuthClientTypes.Confidential, + ConsentType = OAuthConsentTypes.Implicit, + DisplayName = managementClientId, + RedirectUris = [], + PostLogoutRedirectUris = [], + Scopes = [ModgudManagementApi.Scope], + AllowedGrantTypes = ["client_credentials"], + RequireConsent = false, + AccessTokenType = AccessTokenType.Jwt, + AppIds = [new ShortGuid(ownAppId).ToString()], + LinkedServiceAccountId = new ShortGuid(serviceAccount.Id).ToString(), + }, ct)); + + var applications = scope.ServiceProvider.GetRequiredService(); + var connected = await applications.FindByClientIdAsync(connectedClientId, ct); + var connectedPk = await applications.GetIdAsync(connected!, ct); + + return new Rig( + ownAppId, foreignAppId, UserId: Guid.NewGuid(), serviceAccount.Id, + await IssueManagementTokenAsync(managementClientId), + connectedClientId, connectedPk!, + resourceScope, appScopedScope, foreignScope); + } + } + + /// A Permanent authorization plus the refresh token hanging off + /// it — the state an Authorization Code flow with offline_access leaves. + private async Task CreateAuthorizationAsync(string clientPk, Guid userId, string[] scopes) + { + var ct = TestContext.Current.CancellationToken; + using var scope = NewSystemTenantScope(); + var authorizations = scope.ServiceProvider.GetRequiredService(); + var tokens = scope.ServiceProvider.GetRequiredService(); + + var identity = new ClaimsIdentity("test"); + identity.SetClaim(Claims.Subject, userId.ToString()); + var authorization = await authorizations.CreateAsync( + new ClaimsPrincipal(identity), userId.ToString(), clientPk, + AuthorizationTypes.Permanent, scopes.ToImmutableArray(), ct); + var authorizationId = (await authorizations.GetIdAsync(authorization, ct))!; + + var token = await tokens.CreateAsync(new OpenIddictTokenDescriptor + { + ApplicationId = clientPk, + AuthorizationId = authorizationId, + Subject = userId.ToString(), + Type = TokenTypeHints.RefreshToken, + Status = Statuses.Valid, + CreationDate = DateTimeOffset.UtcNow, + ExpirationDate = DateTimeOffset.UtcNow.AddDays(14), + }, ct); + + return new Grant(authorizationId, (await tokens.GetIdAsync(token, ct))!); + } + + private async Task IssueManagementTokenAsync(string clientId) + { + var form = new List> + { + new("grant_type", "client_credentials"), + new("client_id", clientId), + new("client_secret", $"{clientId}-secret"), + new("scope", ModgudManagementApi.Scope), + new("resource", ModgudManagementApi.Audience), + }; + using var tokenClient = Factory.CreateClient(); + using var response = await tokenClient.PostAsync( + "/connect/token", new FormUrlEncodedContent(form), TestContext.Current.CancellationToken); + var body = await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken); + Assert.True(response.IsSuccessStatusCode, $"client_credentials failed ({(int)response.StatusCode}): {body}"); + using var document = JsonDocument.Parse(body); + return document.RootElement.GetProperty("access_token").GetString()!; + } + + private async Task ListAsync(string token, Guid appId, string? after = null, int? limit = null) + { + var query = new List(); + if (after is not null) query.Add($"after={Uri.EscapeDataString(after)}"); + if (limit is not null) query.Add($"limit={limit}"); + var url = $"/api/app/{new ShortGuid(appId)}/authorizations" + + (query.Count > 0 ? "?" + string.Join("&", query) : string.Empty); + + using var response = await SendAsync(HttpMethod.Get, token, url); + var body = await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken); + Assert.True(response.IsSuccessStatusCode, $"list failed ({(int)response.StatusCode}): {body}"); + return JsonSerializer.Deserialize( + body, new JsonSerializerOptions { PropertyNameCaseInsensitive = true })!; + } + + private Task DeleteAsync(string token, Guid appId, string authorizationId) => + SendAsync(HttpMethod.Delete, token, + $"/api/app/{new ShortGuid(appId)}/authorizations/{Uri.EscapeDataString(authorizationId)}"); + + private async Task SendAsync(HttpMethod method, string token, string url) + { + using var request = new HttpRequestMessage(method, url); + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token); + using var client = Factory.CreateClient(); + return await client.SendAsync(request, TestContext.Current.CancellationToken); + } + + private IServiceScope NewSystemTenantScope() + { + var scope = Factory.Services.CreateScope(); + scope.ServiceProvider.GetRequiredService() + .HttpContext = new DefaultHttpContext { Items = { ["TenantId"] = "system" } }; + return scope; + } + + private static void AssertOk(ErrorOr.ErrorOr result) => + Assert.False(result.IsError, + string.Join(", ", result.ErrorsOrEmptyList.Select(e => $"{e.Code}: {e.Description}"))); +} diff --git a/src/dotnet/Modgud.Api/Features/Admin/Apps/AppAuthorizationsEndpoints.cs b/src/dotnet/Modgud.Api/Features/Admin/Apps/AppAuthorizationsEndpoints.cs new file mode 100644 index 00000000..877a3473 --- /dev/null +++ b/src/dotnet/Modgud.Api/Features/Admin/Apps/AppAuthorizationsEndpoints.cs @@ -0,0 +1,225 @@ +using System.Security.Claims; +using BuildingBlocks.Helper; +using Marten; +using Modgud.Api.Features.Management; +using Modgud.Authentication.Sessions; +using Modgud.Authorization.Apps; +using Modgud.Authorization.Principals; +using Modgud.Authorization.Services; +using Modgud.Domain.OAuth.Apis; +using Modgud.Domain.OAuth.Applications; +using Modgud.Domain.OAuth.Scopes; +using Modgud.Domain.OAuth.Storage; +using Modgud.Infrastructure.Audit; +using Modgud.Infrastructure.OpenIddict; +using OpenIddict.Abstractions; +using static OpenIddict.Abstractions.OpenIddictConstants; + +namespace Modgud.Api.Features.Admin.Apps; + +/// +/// The OAuth authorizations (one per user × client grant) that reach an +/// Application's resource servers, and the revocation of a single one. +/// +/// Built for the consuming application's own backend: it shows its users +/// their connected systems, keys its per-connection state on the token's +/// sub + oi_au_id, and needs "disconnect" to end the refresh +/// token in Modgud too — not merely to stop honouring the access token on its +/// side. The connected system is the OAuth client here, so RFC 7009 token +/// revocation is not available to the resource server. +/// +/// An authorization is in an App's reach when one of its scopes is +/// app-scoped to that App, or lists one of the App's OAuth APIs as a resource. +/// Anything else answers 404 — a caller learns nothing about grants that only +/// concern other Apps. +/// +public static class AppAuthorizationsEndpoints +{ + private const int DefaultLimit = 200; + private const int MaxLimit = 1000; + private const int ScanBatch = 500; + + public static WebApplication MapAppAuthorizationsEndpoints(this WebApplication application, string path) + { + // Mapped outside the cookie-only app group for the same reason as + // GET /app/{id}/scope: both are part of the OAuth Management API. + application.MapGet($"{path}/app/{{id}}/authorizations", ListAsync) + .WithTags("Apps") + .WithName("V2_App_GetAuthorizations") + .RequiresManagementPermission("oauth-authorization:read", clientAppRouteParameter: "id"); + + application.MapDelete($"{path}/app/{{id}}/authorizations/{{authorizationId}}", RevokeAsync) + .WithTags("Apps") + .WithName("V2_App_RevokeAuthorization") + .RequiresManagementPermission("oauth-authorization:revoke", clientAppRouteParameter: "id"); + + return application; + } + + private static async Task ListAsync( + ShortGuid id, + string? subject, + string? after, + int? limit, + IDocumentSession session, + CancellationToken ct) + { + var reach = await AppReach.LoadAsync(session, id.Guid, ct); + if (reach is null) return Results.NotFound(); + + var take = Math.Clamp(limit ?? DefaultLimit, 1, MaxLimit); + var items = new List(take); + var cursor = after; + var exhausted = false; + + // The scope→App match can't be pushed into the query (it is a set + // intersection over a JSON array), so scan valid authorizations in id + // order and filter in memory until the page is full. + while (!exhausted && items.Count < take) + { + var query = session.Query() + .Where(x => x.Status == Statuses.Valid); + if (!string.IsNullOrEmpty(subject)) query = query.Where(x => x.Subject == subject); + if (!string.IsNullOrEmpty(cursor)) query = query.Where(x => x.Id.CompareTo(cursor) > 0); + + var batch = await query.OrderBy(x => x.Id).Take(ScanBatch).ToListAsync(ct); + + var consumed = 0; + foreach (var row in batch) + { + consumed++; + cursor = row.Id; + if (reach.Covers(row)) items.Add(row); + if (items.Count == take) break; + } + + // Done only when the store had no further rows AND none of this + // batch was left unread by a full page. + exhausted = batch.Count < ScanBatch && consumed == batch.Count; + } + + var clientIds = await ResolveClientIdsAsync(session, items, ct); + return Results.Ok(new + { + Items = items.Select(x => ToDto(x, clientIds)), + NextAfter = exhausted ? null : cursor, + }); + } + + private static async Task RevokeAsync( + ShortGuid id, + string authorizationId, + HttpContext http, + IDocumentSession session, + IOAuthGrantRevoker grants, + IClientSessionService clientSessions, + IPrincipalLookupService principals, + ISecurityAuditLog audit, + CancellationToken ct) + { + var reach = await AppReach.LoadAsync(session, id.Guid, ct); + if (reach is null) return Results.NotFound(); + + var row = await session.LoadAsync(authorizationId, ct); + if (row is null || !reach.Covers(row)) return Results.NotFound(); + + // Idempotent: a repeated disconnect is a success, not an error. Tokens + // go first, so an attempt that died halfway leaves the authorization + // valid and the retry is audited as the revoke it is. The token count + // can't carry that signal — TryRevokeAsync also reports success for a + // token that was revoked already. + var wasValid = string.Equals(row.Status, Statuses.Valid, StringComparison.Ordinal); + var tokensRevoked = await grants.RevokeTokensByAuthorizationIdAsync(row.Id, ct); + await grants.RevokeAuthorizationByIdAsync(row.Id, ct); + await clientSessions.EndByAuthorizationAsync(row.Id, ct); + + if (wasValid) + { + var clientIds = await ResolveClientIdsAsync(session, [row], ct); + var actorId = Guid.TryParse( + http.User.GetClaim(Claims.Subject) ?? http.User.FindFirstValue(ClaimTypes.NameIdentifier), + out var parsedActor) + ? parsedActor + : (Guid?)null; + var actor = actorId is { } a ? await principals.GetByIdAsync(a, ct) : null; + + await audit.RecordRequiredAsync(new SecurityAuditRecord + { + EventType = AuditEvents.AuthorizationRevoked, + ActorKind = actor is ServiceAccount ? AuditActorKind.ServiceAccount : AuditActorKind.User, + ActorSubjectId = actorId, + TargetSubjectId = Guid.TryParse(row.Subject, out var target) ? target : null, + OAuthClientId = row.ApplicationId is { } appPk ? clientIds.GetValueOrDefault(appPk) : null, + AuthorizationId = row.Id, + ApplicationId = id.Guid, + OutcomeCode = AuditOutcomes.Succeeded, + OperationCode = "revoke-authorization", + Count = tokensRevoked, + }, ct); + } + + return Results.NoContent(); + } + + private static async Task> ResolveClientIdsAsync( + IDocumentSession session, + IReadOnlyCollection rows, + CancellationToken ct) + { + // CIMD clients are never persisted, so their authorizations resolve to + // no client row — the DTO then carries a null ClientId. + var pks = rows + .Select(x => Guid.TryParse(x.ApplicationId, out var pk) ? pk : (Guid?)null) + .OfType() + .Distinct() + .ToList(); + if (pks.Count == 0) return new Dictionary(); + + var clients = await session.Query() + .Where(x => pks.Contains(x.Id)) + .ToListAsync(ct); + return clients.ToDictionary(x => x.Id.ToString(), x => x.ClientId, StringComparer.OrdinalIgnoreCase); + } + + // Id and Subject are returned exactly as they appear in the access token + // (`oi_au_id`, `sub`) — not as ShortGuids — because the consumer joins on them. + private static object ToDto(OpenIddictAuthorizationDocument x, Dictionary clientIds) => new + { + x.Id, + x.Subject, + ClientId = x.ApplicationId is { } pk ? clientIds.GetValueOrDefault(pk) : null, + x.Type, + Scopes = x.Scopes.OrderBy(s => s, StringComparer.Ordinal), + CreatedAt = x.CreationDate, + }; + + private sealed class AppReach + { + private readonly HashSet _scopeNames; + + private AppReach(HashSet scopeNames) => _scopeNames = scopeNames; + + public bool Covers(OpenIddictAuthorizationDocument row) => _scopeNames.Overlaps(row.Scopes); + + public static async Task LoadAsync(IDocumentSession session, Guid appId, CancellationToken ct) + { + var app = await session.LoadAsync(appId, ct); + if (app is null) return null; + + var apiNames = (await session.Query() + .Where(x => !x.IsDeleted && x.AppId == appId) + .Select(x => x.Name) + .ToListAsync(ct)) + .ToHashSet(StringComparer.Ordinal); + + var scopes = await session.Query() + .Where(x => !x.IsDeleted) + .ToListAsync(ct); + + return new AppReach(scopes + .Where(s => s.AppId == appId || s.Resources.Any(apiNames.Contains)) + .Select(s => s.Name) + .ToHashSet(StringComparer.Ordinal)); + } + } +} diff --git a/src/dotnet/Modgud.Api/Program.cs b/src/dotnet/Modgud.Api/Program.cs index 15880313..7d619c63 100644 --- a/src/dotnet/Modgud.Api/Program.cs +++ b/src/dotnet/Modgud.Api/Program.cs @@ -1519,6 +1519,7 @@ // An App is one resource: AppsEndpoints carries the per-App ADR-0011 settings override // inline (POST/PUT/GET /api/app), so there is no separate /settings endpoint. Modgud.Api.Features.Admin.Apps.AppsEndpoints.MapAppsEndpoints(app, "api"); + Modgud.Api.Features.Admin.Apps.AppAuthorizationsEndpoints.MapAppAuthorizationsEndpoints(app, "api"); Modgud.Api.Features.ChangeFeed.AppChangeFeedEndpoints.MapAppChangeFeedEndpoints(app, "api"); // ADR-0012 — app-scoped invite codes (dual-auth: invite:write scope or invite-code:write permission). Modgud.Api.Features.InviteCodes.InviteCodeEndpoints.MapInviteCodeEndpoints(app, "api"); diff --git a/src/dotnet/Modgud.Authentication/Api/Admin/AuthLogEndpoints.cs b/src/dotnet/Modgud.Authentication/Api/Admin/AuthLogEndpoints.cs index 2e4b85c3..782c39ec 100644 --- a/src/dotnet/Modgud.Authentication/Api/Admin/AuthLogEndpoints.cs +++ b/src/dotnet/Modgud.Authentication/Api/Admin/AuthLogEndpoints.cs @@ -284,6 +284,7 @@ private static string Compose( AuditEvents.PrivilegeEscalationBlocked => "Federated privilege escalation", AuditEvents.RateLimitTriggered => "Rate limit", AuditEvents.RefreshTokenReuseDetected => "Refresh-token reuse", + AuditEvents.AuthorizationRevoked => "Authorization revoked", AuditEvents.DcrRegistrationRejected => "Dynamic client registration", AuditEvents.BootstrapInviteRejected => "Bootstrap invite", AuditEvents.SecurityRetentionChanged => "Security-log retention", diff --git a/src/dotnet/Modgud.Authentication/Sessions/ClientSessionService.cs b/src/dotnet/Modgud.Authentication/Sessions/ClientSessionService.cs index 027c8bc5..fea53044 100644 --- a/src/dotnet/Modgud.Authentication/Sessions/ClientSessionService.cs +++ b/src/dotnet/Modgud.Authentication/Sessions/ClientSessionService.cs @@ -211,7 +211,11 @@ public async Task OnReuseDetectedAsync( CancellationToken ct) { if (string.IsNullOrEmpty(authorizationId)) return; + await EndByAuthorizationAsync(authorizationId, ct); + } + public async Task EndByAuthorizationAsync(string authorizationId, CancellationToken ct = default) + { var rows = await session.Query() .Where(x => x.AuthorizationId == authorizationId) .ToListAsync(ct); diff --git a/src/dotnet/Modgud.Authentication/Sessions/IClientSessionService.cs b/src/dotnet/Modgud.Authentication/Sessions/IClientSessionService.cs index e8217325..66cfc4d6 100644 --- a/src/dotnet/Modgud.Authentication/Sessions/IClientSessionService.cs +++ b/src/dotnet/Modgud.Authentication/Sessions/IClientSessionService.cs @@ -27,4 +27,12 @@ public interface IClientSessionService Task> RevokeAsync(Guid userId, Guid sessionId, CancellationToken ct = default); Task RevokeAllAsync(Guid userId, bool revokeGrants, CancellationToken ct = default); Task PruneExpiredAsync(CancellationToken ct = default); + + /// + /// Ends the native client session(s) built on an authorization that was + /// revoked elsewhere. Does not touch the grant itself — the caller already + /// revoked it; this only keeps the session list and the session-end feed + /// in step. + /// + Task EndByAuthorizationAsync(string authorizationId, CancellationToken ct = default); } diff --git a/src/dotnet/Modgud.Infrastructure/Audit/AuditDurability.cs b/src/dotnet/Modgud.Infrastructure/Audit/AuditDurability.cs index ed667b88..f121602e 100644 --- a/src/dotnet/Modgud.Infrastructure/Audit/AuditDurability.cs +++ b/src/dotnet/Modgud.Infrastructure/Audit/AuditDurability.cs @@ -36,6 +36,7 @@ public static class AuditDurability public static AuditDurabilityClass Classify(string eventType) => eventType switch { AuditEvents.RefreshTokenReuseDetected or + AuditEvents.AuthorizationRevoked or AuditEvents.AuditLogExported or AuditEvents.SecurityRetentionChanged or AuditEvents.SigningKeyRotated or diff --git a/src/dotnet/Modgud.Infrastructure/Audit/AuditEvents.cs b/src/dotnet/Modgud.Infrastructure/Audit/AuditEvents.cs index dba99fd6..b33313c3 100644 --- a/src/dotnet/Modgud.Infrastructure/Audit/AuditEvents.cs +++ b/src/dotnet/Modgud.Infrastructure/Audit/AuditEvents.cs @@ -138,6 +138,14 @@ public static class AuditEvents /// count of sibling tokens revoked. public const string RefreshTokenReuseDetected = "security.refresh_token_reuse_detected"; + /// A single OAuth authorization was revoked on request — by the + /// consuming application's service account through the Management API. + /// Teardown covers the authorization, every token tied to it, and a native + /// client session built on it. Actor = the caller; Target = the grant's + /// user. Carries OAuthClientId, AuthorizationId, ApplicationId (the App the + /// caller acted for) and Count = tokens revoked. + public const string AuthorizationRevoked = "security.authorization_revoked"; + /// A DCR client registration was rejected (policy / validation). public const string DcrRegistrationRejected = "security.dcr_registration_rejected"; diff --git a/src/dotnet/Modgud.Infrastructure/Authorization/AppRealmSeeder.cs b/src/dotnet/Modgud.Infrastructure/Authorization/AppRealmSeeder.cs index e4380f06..3898716d 100644 --- a/src/dotnet/Modgud.Infrastructure/Authorization/AppRealmSeeder.cs +++ b/src/dotnet/Modgud.Infrastructure/Authorization/AppRealmSeeder.cs @@ -34,6 +34,7 @@ private static readonly (string Resource, string[] Actions)[] ModgudCatalog = // automatically and cannot be deleted. ("app", ["admin", "read", "write"]), ("app-scope", ["read"]), + ("oauth-authorization", ["read", "revoke"]), // Identity / directory ("user", ["read", "write"]), diff --git a/src/dotnet/Modgud.Infrastructure/DependencyInjection.cs b/src/dotnet/Modgud.Infrastructure/DependencyInjection.cs index 7d8711af..708f9924 100644 --- a/src/dotnet/Modgud.Infrastructure/DependencyInjection.cs +++ b/src/dotnet/Modgud.Infrastructure/DependencyInjection.cs @@ -263,6 +263,10 @@ public static IServiceCollection AddInfrastructure( // Kept separate from app:read because the snapshot contains // directory/profile data, not merely Application configuration. opt.RegisterResource(app, "app-scope", "read"); + // The OAuth authorizations (user × client grants) that reach one + // App's resource servers. Lets the App's own backend list them and + // end a single connection, refresh token included. + opt.RegisterResource(app, "oauth-authorization", "read", "revoke"); // Identity / directory opt.RegisterResource(app, "user", "read", "write"); diff --git a/src/dotnet/Modgud.Tests.Unit/Audit/AuditDurabilityTests.cs b/src/dotnet/Modgud.Tests.Unit/Audit/AuditDurabilityTests.cs index ef049d36..9aa638db 100644 --- a/src/dotnet/Modgud.Tests.Unit/Audit/AuditDurabilityTests.cs +++ b/src/dotnet/Modgud.Tests.Unit/Audit/AuditDurabilityTests.cs @@ -7,6 +7,7 @@ public class AuditDurabilityTests public static TheoryData ClassifiedEvents => new() { { AuditEvents.RefreshTokenReuseDetected, AuditDurabilityClass.Required }, + { AuditEvents.AuthorizationRevoked, AuditDurabilityClass.Required }, { AuditEvents.AuditLogExported, AuditDurabilityClass.Required }, { AuditEvents.SecurityRetentionChanged, AuditDurabilityClass.Required }, { AuditEvents.SigningKeyRotated, AuditDurabilityClass.Required }, From f62932ceff0e16f601a57ae49052a944c6918936 Mon Sep 17 00:00:00 2001 From: Bernhard Windisch Date: Fri, 18 Sep 2026 10:58:20 +0200 Subject: [PATCH 3/3] fix(oauth): unticking an optional scope no longer loops back to consent The consent screen lets the user drop every non-required scope, and the server stored the authorization for that subset. The authorize re-entry then looked for an authorization covering the FULL requested list, found none, and sent the user to /consent again, forever. The approved ticket now carries the scopes the user kept. The re-entry redeems it for every explicit-consent client and issues the grant for exactly those scopes. Co-Authored-By: Claude Fable 5.1 --- .../Authorization/DcrFullFlowTests.cs | 37 +++++++++++++++++ .../Auth/OAuth/AuthorizationEndpoints.cs | 41 +++++++++++-------- .../Features/Auth/OAuth/ConsentEndpoints.cs | 33 ++++++++------- .../OAuth/Consent/ConsentTicket.cs | 18 +++++--- 4 files changed, 92 insertions(+), 37 deletions(-) diff --git a/src/dotnet/Modgud.Api.Tests/Authorization/DcrFullFlowTests.cs b/src/dotnet/Modgud.Api.Tests/Authorization/DcrFullFlowTests.cs index ac3d2148..63ea73fa 100644 --- a/src/dotnet/Modgud.Api.Tests/Authorization/DcrFullFlowTests.cs +++ b/src/dotnet/Modgud.Api.Tests/Authorization/DcrFullFlowTests.cs @@ -134,6 +134,43 @@ public async Task Approved_consent_redirect_completes_the_flow_only_once() Assert.StartsWith("/consent?ticket=", replay.Headers.Location!.ToString()); } + [Fact] + public async Task Approving_only_a_subset_of_the_requested_scopes_still_completes_the_flow() + { + await SeedAsync(); + var clientId = await RegisterDcrClientAsync(scope: $"openid profile {ScopeName}"); + + var cookieClient = await CreateAuthenticatedClientAsync("tu", "TestPass1234"); + var authorizeUri = "/connect/authorize?" + string.Join("&", new[] + { + "response_type=code", + $"client_id={Uri.EscapeDataString(clientId)}", + $"redirect_uri={Uri.EscapeDataString(RedirectUri)}", + $"scope={Uri.EscapeDataString($"openid profile {ScopeName}")}", + $"state={Guid.NewGuid():N}", + $"code_challenge={GeneratePkceS256Challenge(GeneratePkceVerifier())}", + "code_challenge_method=S256", + $"resource={Uri.EscapeDataString(AllowedAudience)}", + }); + var authorizeResp = await cookieClient.GetAsync(authorizeUri, TestContext.Current.CancellationToken); + AssertRedirect(authorizeResp); + var ticketId = authorizeResp.Headers.Location!.ToString()["/consent?ticket=".Length..]; + + // The consent screen lets the user untick every non-required scope. + var decisionResp = await cookieClient.PostAsJsonAsync( + "/connect/consent", + new { Ticket = ticketId, Approved = true, ApprovedScopes = new[] { "openid", ScopeName } }, + TestContext.Current.CancellationToken); + Assert.True(decisionResp.IsSuccessStatusCode); + using var decisionDoc = JsonDocument.Parse( + await decisionResp.Content.ReadAsStringAsync(TestContext.Current.CancellationToken)); + var followUpUrl = decisionDoc.RootElement.GetProperty("RedirectUrl").GetString()!; + + var followUp = await cookieClient.GetAsync(followUpUrl, TestContext.Current.CancellationToken); + AssertRedirect(followUp); + Assert.Contains("code=", followUp.Headers.Location!.ToString()); + } + [Fact] public async Task Last_used_at_advances_after_first_token_issue() { diff --git a/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs b/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs index a0b09cc2..b74217b2 100644 --- a/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs +++ b/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs @@ -1,3 +1,4 @@ +using System.Collections.Immutable; using System.Security.Claims; using System.Text; using System.Text.Json; @@ -204,12 +205,19 @@ private static async Task AuthorizeAsync( var subject = user.Id.ToString(); var clientPk = await applicationManager.GetIdAsync(application) ?? string.Empty; + // Post-consent re-entry: the approved ticket carries the scopes the + // user kept. From here on the grant is about those, not about the + // request's full list — otherwise unticking one optional scope finds + // no matching authorization below and loops back to /consent forever. + var approvedScopes = await TryRedeemConsentTicketAsync(httpContext, request, user.Id, session); + var grantScopes = approvedScopes?.ToImmutableArray() ?? request.GetScopes(); + var authorizations = await authorizationManager.FindAsync( subject: subject, client: clientPk, status: Statuses.Valid, type: AuthorizationTypes.Permanent, - scopes: request.GetScopes()).ToListAsync(); + scopes: grantScopes).ToListAsync(); var consentType = await applicationManager.GetConsentTypeAsync(application); @@ -219,19 +227,17 @@ private static async Task AuthorizeAsync( // loopback redirect, RFC 8252 §8.6) every fresh authorize shows the // screen again. The authorization itself is still REUSED, so the // grant's `oi_au_id` stays stable across re-consents. The post-consent - // re-entry completes the round-trip by redeeming its approved ticket - // once, instead of relying on the remembered-authorization shortcut. - var skipConsent = consentType == ConsentTypes.Implicit; - if (!skipConsent && authorizations.Count != 0) - { - skipConsent = await AllowsRememberConsentAsync(applicationManager, application) - || await TryRedeemConsentTicketAsync(httpContext, request, user.Id, session); - } + // re-entry gets through on its redeemed ticket instead. + var skipConsent = consentType == ConsentTypes.Implicit + || approvedScopes is not null + || (authorizations.Count != 0 + && await AllowsRememberConsentAsync(applicationManager, application)); if (skipConsent) { var principal = await CreateClaimsPrincipalAsync( user, request, scopeManager, userManager: userManager, + scopeOverrides: approvedScopes, cookiePrincipal: authResult.Principal); var authorization = authorizations.LastOrDefault(); @@ -318,10 +324,11 @@ private static async Task AllowsRememberConsentAsync( /// Redeems the ?consent_ticket={id} the consent endpoint appends on /// approval. Genuine means: approved (consumed, not denied), bound to this /// user and client, for exactly the scopes now requested, consumed within - /// the last two minutes, and not redeemed before. Anything else returns - /// false and the caller re-prompts — a forged marker buys nothing. + /// the last two minutes, and not redeemed before. Returns the scopes the + /// user approved; anything else returns null and the caller re-prompts — a + /// forged marker buys nothing. /// - private static async Task TryRedeemConsentTicketAsync( + private static async Task?> TryRedeemConsentTicketAsync( HttpContext httpContext, OpenIddictRequest request, Guid userId, @@ -332,18 +339,18 @@ private static async Task TryRedeemConsentTicketAsync( if (httpContext.Request.Query["consent_ticket"] is not [{ Length: > 0 } raw] || !Guid.TryParseExact(raw, "N", out var ticketId)) { - return false; + return null; } var ticket = await session.LoadAsync(ticketId); var now = DateTimeOffset.UtcNow; - if (ticket is not { ConsumedAt: not null, DeniedAt: null, RedeemedAt: null } + if (ticket is not { ConsumedAt: not null, DeniedAt: null, RedeemedAt: null, ApprovedScopes: not null } || ticket.Subject != userId || !string.Equals(ticket.ClientId, request.ClientId, StringComparison.Ordinal) || ticket.ConsumedAt < now.AddMinutes(-2) || !ticket.RequestedScopes.ToHashSet(StringComparer.Ordinal).SetEquals(request.GetScopes())) { - return false; + return null; } ticket.RedeemedAt = now; @@ -357,10 +364,10 @@ private static async Task TryRedeemConsentTicketAsync( // Lost the race to a parallel re-entry. Drop the stale write so the // consent ticket the caller stores next can still be saved. session.EjectAllPendingChanges(); - return false; + return null; } - return true; + return ticket.ApprovedScopes; } private static async Task ExchangeAsync( diff --git a/src/dotnet/Modgud.Api/Features/Auth/OAuth/ConsentEndpoints.cs b/src/dotnet/Modgud.Api/Features/Auth/OAuth/ConsentEndpoints.cs index 13906b7c..e373f773 100644 --- a/src/dotnet/Modgud.Api/Features/Auth/OAuth/ConsentEndpoints.cs +++ b/src/dotnet/Modgud.Api/Features/Auth/OAuth/ConsentEndpoints.cs @@ -155,6 +155,20 @@ private static async Task SubmitConsentAsync( var (record, error) = await ResolveTicketAsync(ticketId, session, userManager, currentUserPrincipal); if (error is not null) return error; + // OAUTH-02 fix: the user-submitted ApprovedScopes are filtered against + // the requested scopes that were locked in at /authorize time. Anything + // the user "added" beyond what the RP asked for is silently dropped; + // the standard "openid is implicit" semantic is preserved. + var requestedSet = record!.RequestedScopes.ToHashSet(StringComparer.Ordinal); + var approvedSet = decision.ApprovedScopes + .Where(s => requestedSet.Contains(s)) + .ToHashSet(StringComparer.Ordinal); + + if (requestedSet.Contains(Scopes.OpenId)) + { + approvedSet.Add(Scopes.OpenId); + } + // Audit #26 — CLAIM the ticket atomically, BEFORE doing anything else. // ConsentTicket uses optimistic concurrency, so two parallel POSTs (user // double-click) both load ConsumedAt==null but only ONE can commit this @@ -162,7 +176,10 @@ private static async Task SubmitConsentAsync( // existing 409. Claiming first — before any authorization is created — // means the loser doesn't even mint a duplicate Permanent authorization // row (the previously-documented benign residual is now gone too). - record!.ConsumedAt = DateTimeOffset.UtcNow; + record.ConsumedAt = DateTimeOffset.UtcNow; + // The decision travels with the claim: the authorize re-entry issues the + // grant for what the user kept, not for everything the client asked for. + if (decision.Approved) record.ApprovedScopes = approvedSet.ToArray(); // Mark the deny atomically with the claim so the authorize re-entry // below can prove this was a genuine denial (not just any consumed // ticket) before OpenIddict emits an error to the client. @@ -201,20 +218,6 @@ private static async Task SubmitConsentAsync( }); } - // OAUTH-02 fix: the user-submitted ApprovedScopes are filtered against - // the requested scopes that were locked in at /authorize time. Anything - // the user "added" beyond what the RP asked for is silently dropped; - // the standard "openid is implicit" semantic is preserved. - var requestedSet = record.RequestedScopes.ToHashSet(StringComparer.Ordinal); - var approvedSet = decision.ApprovedScopes - .Where(s => requestedSet.Contains(s)) - .ToHashSet(StringComparer.Ordinal); - - if (requestedSet.Contains(Scopes.OpenId)) - { - approvedSet.Add(Scopes.OpenId); - } - var application = await applicationManager.FindByClientIdAsync(record.ClientId); if (application is null) return Results.NotFound(new { message = "Application not found." }); diff --git a/src/dotnet/Modgud.Domain/OAuth/Consent/ConsentTicket.cs b/src/dotnet/Modgud.Domain/OAuth/Consent/ConsentTicket.cs index 9014b97a..04fe5a8f 100644 --- a/src/dotnet/Modgud.Domain/OAuth/Consent/ConsentTicket.cs +++ b/src/dotnet/Modgud.Domain/OAuth/Consent/ConsentTicket.cs @@ -87,13 +87,21 @@ public class ConsentTicket /// public DateTimeOffset? DeniedAt { get; set; } + /// + /// The scopes the user actually approved — + /// minus whatever they unticked. Null until an approval is recorded. The + /// authorize re-entry issues the grant for exactly these, not for the + /// request's full scope list. + /// + public string[]? ApprovedScopes { get; set; } + /// /// Set when the authorize re-entry redeems an APPROVED ticket passed as - /// ?consent_ticket={Id}. Only clients with - /// AllowRememberConsent=false need it: their remembered - /// authorization no longer skips the consent screen, so the re-entry has - /// to prove that the user approved this very request a moment ago. The - /// version-checked write makes the proof single-use. + /// ?consent_ticket={Id}. The ticket is what carries the decision + /// across the redirect: it proves the user approved this very request a + /// moment ago (a client with AllowRememberConsent=false has no + /// other way past the consent screen) and says which scopes they kept. + /// The version-checked write makes the proof single-use. /// public DateTimeOffset? RedeemedAt { get; set; } }