diff --git a/docs/admin/client-id-metadata-documents.md b/docs/admin/client-id-metadata-documents.md index ca9e4699..c08d8456 100644 --- a/docs/admin/client-id-metadata-documents.md +++ b/docs/admin/client-id-metadata-documents.md @@ -39,7 +39,7 @@ When a client presents an HTTPS-URL `client_id` that isn't a known stored client 1. Validates the URL shape — `https` scheme, has a path, no fragment, no userinfo, no dot-segments. 2. Fetches it over an **SSRF-hardened** transport (see below), capped at **5 KB**, `Accept: application/json`, ~5 s timeout, **no redirects**. 3. Validates the document (table below). -4. Synthesizes an in-memory public PKCE client with a deterministic id derived from the URL, JWT access tokens, and the redirect URIs / scopes the document declares. +4. Synthesizes an in-memory public PKCE client with a deterministic id derived from the URL, JWT access tokens, the redirect URIs the document declares, and the realm's **dynamic-client scopes** (see below) — narrowed to the document's `scope` when it has one. 5. Caches the validated document per URL, respecting `Cache-Control` (clamped to between 5 minutes and 24 hours; failures are never cached). No database row is created. A refresh after the cache expires re-fetches and re-validates the live document — if the URL becomes unreachable, refresh fails and the client must re-authenticate. @@ -58,7 +58,9 @@ CIMD shares DCR's resource/scope opt-in surface — flipping the master toggle d | Per-API allow-list | [OAuth APIs](./oauth-apis) → **Allow DCR** checkbox per row | Off | | Per-Scope allow-list | [OAuth Scopes](./oauth-scopes) → **Allow DCR Clients** checkbox per row | Off | -A CIMD client must send a `resource=` parameter, and the target resource server must have **Allow DCR** enabled — otherwise the token endpoint rejects with `invalid_target`. App-scoped scopes are reachable only when **Allow DCR Clients** is ticked, exactly as for DCR clients (a CIMD client is realm-wide and has no App link of its own). Global scopes (`openid`, `email`, `profile`, …) are always reachable. +A CIMD client must send a `resource=` parameter, and the target resource server must have **Allow DCR** enabled — otherwise the token endpoint rejects with `invalid_target`. The scopes it can hold and request are the realm's **dynamic-client scopes**: every scope with **Allow DCR Clients** ticked (app-scoped or not — a CIMD client is realm-wide and has no App link of its own) plus the standard scopes other than `modgud.management` (`openid`, `profile`, `email`, `phone`, `address`, `offline_access`, `roles`, `permissions`). Same rule as for [DCR clients](./dynamic-client-registration#how-the-per-scope-flag-interacts-with-app-scoped-scopes). + +A document **without** `scope` — the normal case: Claude Code's document is one static file for every MCP server in the world and cannot know one server's scopes, the client reads them from the server's protected-resource metadata at run time — holds that whole set. A document **with** `scope` holds the intersection; it can narrow what its client may ask for, never widen it. The set is read live on every resolve, so ticking **Allow DCR Clients** on a scope takes effect at once, even for an already-cached document. Requesting a scope outside the set fails at `/connect/authorize` with `invalid_scope` and a description naming the scope and the flag. ## Enabling CIMD for a realm @@ -67,7 +69,7 @@ A CIMD client must send a `resource=` parameter, and the target resource server - **Access-token lifetime** (default 15 min) — shorter than admin-created clients on purpose; a leaked token from an unverified, domain-bound client has a smaller blast radius. - **Refresh-token lifetime** (default 7 d). 3. **OAuth APIs → your MCP-server API** → tick **Allow DCR**. -4. **OAuth Scopes → the scope(s) the MCP server gates** → tick **Allow DCR Clients** (for app-scoped scopes). +4. **OAuth Scopes → the scope(s) the MCP server gates** → tick **Allow DCR Clients**. ## What's accepted in the metadata document @@ -79,7 +81,7 @@ A CIMD client must send a `resource=` parameter, and the target resource server | `token_endpoint_auth_method` | `none` or omitted. **v1 is public-only** — a `client_secret*` method or any `client_secret` field is rejected. | | `grant_types` | Subset of `{authorization_code, refresh_token}`; must include `authorization_code`. | | `response_types` | Subset of `{code}`. | -| `scope` | Optional, space-delimited. The scopes the client may request (still subject to the opt-in gates above). | +| `scope` | Optional, space-delimited. An **upper bound**: the client holds these scopes intersected with the realm's dynamic-client scopes. Omitted (as every static MCP-client document does), the client holds the whole set. | | `client_name` | Optional. Used as the display name; the consent screen also shows the URL hostname regardless. | A document that fails any rule is rejected and never cached; the authorize request fails as "unknown client". diff --git a/docs/admin/dynamic-client-registration.md b/docs/admin/dynamic-client-registration.md index 9f814230..7f666603 100644 --- a/docs/admin/dynamic-client-registration.md +++ b/docs/admin/dynamic-client-registration.md @@ -48,13 +48,18 @@ The master toggle just turns the registration endpoint on. The per-API flag cont ### How the per-Scope flag interacts with app-scoped scopes -Most scopes you create in Modgud are **app-scoped** — they belong to one [Application](./applications) (`Scope.AppId` is set). Non-DCR clients are restricted to scopes whose `AppId` matches one of their own linked Apps. **DCR clients have no `AppId`** by design (they're realm-wide public PKCE clients), so the per-Scope `Allow DCR Clients` flag replaces the app-link check for them: +Most scopes you create in Modgud are **app-scoped** — they belong to one [Application](./applications) (`Scope.AppId` is set). Non-DCR clients are restricted to scopes whose `AppId` matches one of their own linked Apps. **DCR clients have no `AppId`** by design (they're realm-wide public PKCE clients), so the per-Scope `Allow DCR Clients` flag replaces the app-link check for them. The scopes a DCR client can hold and request — its **dynamic-client scopes** — are: -- **Global scopes** (`AppId = null` — the OIDC standards `openid`, `email`, `profile`, … plus any cross-app scope you create): always reachable by DCR clients. -- **App-scoped scope with `Allow DCR Clients = true`**: reachable by DCR clients. The realm-admin has explicitly opted this scope in for anonymous-registrant access. -- **App-scoped scope with `Allow DCR Clients = false`** (default): `/connect/authorize` rejects the request with `invalid_scope` before the user ever sees the consent screen. The agent gets a clear error description. +- **Any scope with `Allow DCR Clients = true`**, app-scoped or global. The realm-admin has explicitly opted this scope in for anonymous-registrant access. +- **The standard scopes other than `modgud.management`**: `openid`, `profile`, `email`, `phone`, `address`, `offline_access`, `roles`, `permissions`. They cannot carry the flag (standard scopes are immutable) and name no resource privilege of their own. The management-API selector is never available to a dynamic client. -The combined effect: enabling DCR safely requires you to walk through your existing scopes once and decide which ones agents are allowed to ask for. Until you tick `Allow DCR Clients` on at least one app-scoped scope (or create a fresh global scope), DCR clients can only request the OIDC standard scopes. +A custom scope **without** the flag (default) — app-scoped or global — is not: `/connect/authorize` rejects the request with `invalid_scope` before the user ever sees the consent screen, with a description naming the scope and the flag to enable. + +The combined effect: enabling DCR safely requires you to walk through your existing scopes once and decide which ones agents are allowed to ask for. Until you tick `Allow DCR Clients` on at least one scope, DCR clients can only request the standard scopes. + +### What a registration gets when it omits `scope` + +RFC 7591 leaves it to the server ("a default set of scopes"). Modgud's default is the realm's dynamic-client scopes as they are at registration time; a registration **with** `scope` gets the intersection of what it asked for and that set — its own list is an upper bound, never a grant. The `201` response echoes the registered set in `scope`, so an agent can see what it holds. A client keeps the set it was registered with; a scope opted in later needs a re-registration (or a CIMD document, which is resolved live). ## Enabling DCR for a realm @@ -80,6 +85,7 @@ After these four steps, an agent that POSTs to `/connect/register` with a valid | `token_endpoint_auth_method` | Must be `none` (or omitted). Public PKCE only — no secret-storage. | | `grant_types` | Subset of `{authorization_code, refresh_token}`. | | `response_types` | Subset of `{code}`. No implicit / hybrid flows. | +| `scope` | Optional, space-delimited. An upper bound intersected with the realm's dynamic-client scopes; omitted, the client gets the whole set. The response echoes what was registered. | On success the endpoint returns `201 Created` with the assigned `client_id` per RFC 7591 §3.2.1. On rejection it returns `400 Bad Request` with `{ error, error_description }` per §3.2.2. Hitting the rate-limit returns `429`. diff --git a/src/dotnet/Modgud.Api.Tests/Authorization/CimdFullFlowTests.cs b/src/dotnet/Modgud.Api.Tests/Authorization/CimdFullFlowTests.cs index 85d1f08e..ef90b38c 100644 --- a/src/dotnet/Modgud.Api.Tests/Authorization/CimdFullFlowTests.cs +++ b/src/dotnet/Modgud.Api.Tests/Authorization/CimdFullFlowTests.cs @@ -112,6 +112,66 @@ public async Task Loopback_redirect_with_an_ephemeral_port_is_accepted_for_a_cim Assert.Contains(AllowedAudience, new JwtSecurityTokenHandler().ReadJwtToken(accessToken).Audiences); } + /// + /// A static CIMD document has no scope — Claude Code's is one document + /// for every MCP server in the world and cannot know one server's scopes; the + /// client learns them from the server's protected-resource metadata at run + /// time. Such a client must hold the realm's dynamic-client scope set (the + /// opted-in scopes plus the open standard scopes), so the per-scope opt-in is + /// reachable at all. It used to hold nothing, and OpenIddict refused every + /// scope but openid/offline_access with ID2051 before the opt-in + /// was ever consulted (reported from the field). A scope the admin did not + /// opt in stays refused — with Modgud's own message, not the opaque one — and + /// a document that does declare scope cannot name its way past that. + /// + [Fact] + public async Task A_document_without_scope_holds_the_realms_dynamic_client_scopes() + { + await SeedAsync(); + const string notOptedIn = "cimd-not-opted-in-scope"; + await CreateScopeAsync(notOptedIn, allowDynamicClients: false); + Factory.CimdDocuments[_clientIdUrl] = JsonSerializer.Serialize(new Dictionary + { + ["client_id"] = _clientIdUrl, + ["client_name"] = "Claude Code", + ["redirect_uris"] = new[] { "http://localhost/callback", "http://127.0.0.1/callback" }, + ["grant_types"] = new[] { "authorization_code", "refresh_token" }, + ["response_types"] = new[] { "code" }, + ["token_endpoint_auth_method"] = "none", + }); + const string callback = "http://localhost:40489/callback"; + + // The opted-in scope and the OIDC identity scopes reach consent. + Assert.StartsWith("/consent?ticket=", await DriveAuthorizeAsync(_clientIdUrl, $"openid {ScopeName}", callback)); + Assert.StartsWith("/consent?ticket=", await DriveAuthorizeAsync(_clientIdUrl, "openid profile email offline_access", callback)); + + // A scope without the opt-in is refused — by Modgud, naming the flag. Request + // validation errors are rendered by OpenIddict as a page, not redirected. + await AssertRefusedScopeAsync(_clientIdUrl, $"openid {notOptedIn}", callback, "AllowDynamicRegistrationClients"); + + // The management selector is never a dynamic client's to ask for. + await AssertRefusedScopeAsync(_clientIdUrl, "openid modgud.management", callback, "not available to dynamically registered clients"); + + // And the whole flow completes on the default set. + var (accessToken, _) = await DriveCimdAuthCodeFlowAsync(_clientIdUrl, $"openid {ScopeName}", AllowedAudience, redirectUri: callback); + Assert.Contains(AllowedAudience, new JwtSecurityTokenHandler().ReadJwtToken(accessToken).Audiences); + + // A document that declares scope gets the intersection, never more. + var declaring = $"https://cimd-app.test/oauth/{Guid.NewGuid():N}/declaring.json"; + Factory.CimdDocuments[declaring] = BuildDocument(declaring, $"openid {ScopeName} {notOptedIn}"); + Assert.StartsWith("/consent?ticket=", await DriveAuthorizeAsync(declaring, $"openid {ScopeName}")); + await AssertRefusedScopeAsync(declaring, $"openid {notOptedIn}", RedirectUri, "AllowDynamicRegistrationClients"); + } + + private async Task AssertRefusedScopeAsync(string clientId, string scope, string redirectUri, string expectedDescriptionPart) + { + var resp = await DriveAuthorizeResponseAsync(clientId, scope, redirectUri); + var body = await resp.Content.ReadAsStringAsync(TestContext.Current.CancellationToken); + Assert.True(resp.StatusCode == HttpStatusCode.BadRequest, $"expected 400, got {(int)resp.StatusCode} {resp.Headers.Location}\n{body}"); + Assert.Contains("invalid_scope", body); + Assert.Contains(expectedDescriptionPart, body); + } + [Fact] public async Task Consent_surfaces_hostname_and_unverified_marker() { @@ -237,16 +297,18 @@ private async Task CreateAllowedApiAsync() Assert.False(result.IsError, DescribeErrors(result.Errors)); } - private async Task CreateAllowedScopeAsync() + private Task CreateAllowedScopeAsync() => CreateScopeAsync(ScopeName, allowDynamicClients: true); + + private async Task CreateScopeAsync(string name, bool allowDynamicClients) { using var scope = NewSystemTenantScope(); var oauthAdmin = scope.ServiceProvider.GetRequiredService(); var result = await oauthAdmin.CreateScopeAsync(new CreateOAuthScopeDto { - Name = ScopeName, - DisplayName = ScopeName, + Name = name, + DisplayName = name, Resources = new List { AllowedAudience }, - AllowDynamicRegistrationClients = true, + AllowDynamicRegistrationClients = allowDynamicClients, }, TestContext.Current.CancellationToken); Assert.False(result.IsError, DescribeErrors(result.Errors)); } @@ -372,13 +434,15 @@ private async Task DriveCimdFlowThroughToTokenAsync( /// Drives just the authorize GET; returns the redirect Location /// (or null when the response isn't a redirect). private async Task DriveAuthorizeAsync(string clientId, string scope, string? redirectUri = null) + => (await DriveAuthorizeResponseAsync(clientId, scope, redirectUri)).Headers.Location?.ToString(); + + private async Task DriveAuthorizeResponseAsync(string clientId, string scope, string? redirectUri = null) { var challenge = GeneratePkceS256Challenge(GeneratePkceVerifier()); var cookieClient = await CreateAuthenticatedClientAsync("tu", "TestPass1234"); - var resp = await cookieClient.GetAsync( + return await cookieClient.GetAsync( BuildAuthorizeUri(clientId, scope, challenge, AllowedAudience, redirectUri ?? RedirectUri), TestContext.Current.CancellationToken); - return resp.Headers.Location?.ToString(); } private async Task DiscoveryHasCimdFlagAsync() diff --git a/src/dotnet/Modgud.Api.Tests/Authorization/DcrFullFlowTests.cs b/src/dotnet/Modgud.Api.Tests/Authorization/DcrFullFlowTests.cs index 02197c8d..36fa168f 100644 --- a/src/dotnet/Modgud.Api.Tests/Authorization/DcrFullFlowTests.cs +++ b/src/dotnet/Modgud.Api.Tests/Authorization/DcrFullFlowTests.cs @@ -130,7 +130,73 @@ public async Task Loopback_redirect_with_an_ephemeral_port_is_accepted_for_a_dcr Assert.False(doc.RootElement.TryGetProperty("application_type", out _)); } - private async Task AuthorizeLocationAsync(string clientId, string redirectUri) + /// + /// RFC 7591 §2: a registration without scope gets "a default set of + /// scopes" — Modgud's is the realm's dynamic-client set (opted-in scopes plus + /// the open standard scopes), the same one a scope-less CIMD document holds. + /// It used to be the empty set, which left such a client unable to request + /// anything but openid/offline_access. A declared scope + /// is an upper bound intersected with that set, and the response echoes what + /// was actually registered (§3.2.1). + /// + [Fact] + public async Task Registration_without_scope_gets_the_realms_dynamic_client_scopes() + { + await SeedAsync(); + var ct = TestContext.Current.CancellationToken; + var http = Factory.CreateClient(); + var notOptedIn = $"dcr-not-opted-in-{Guid.NewGuid():N}"; + await CreateScopeAsync(notOptedIn, allowDynamicClients: false); + + var reg = await http.PostAsync("/connect/register", JsonContent.Create(new + { + client_name = "Scope-less MCP Client", + redirect_uris = new[] { "http://localhost/callback" }, + grant_types = new[] { "authorization_code", "refresh_token" }, + }), ct); + var regBody = await reg.Content.ReadAsStringAsync(ct); + Assert.True(reg.StatusCode == HttpStatusCode.Created, regBody); + string clientId; + string[] registered; + using (var doc = JsonDocument.Parse(regBody)) + { + clientId = doc.RootElement.GetProperty("client_id").GetString()!; + registered = doc.RootElement.GetProperty("scope").GetString()!.Split(' '); + } + Assert.Contains(ScopeName, registered); + Assert.Contains("openid", registered); + Assert.Contains("profile", registered); + Assert.Contains("email", registered); + Assert.Contains("offline_access", registered); + Assert.DoesNotContain(notOptedIn, registered); + Assert.DoesNotContain("modgud.management", registered); + Assert.Equal(registered, (await LoadStoredAsync(clientId)).Permissions + .Where(p => p.StartsWith("scp:")).Select(p => p["scp:".Length..]).ToArray()); + + Assert.StartsWith("/consent?ticket=", await AuthorizeLocationAsync(clientId, "http://localhost:43210/callback")); + var refused = await AuthorizeResponseAsync(clientId, "http://localhost:43210/callback", $"openid {notOptedIn}"); + var refusedBody = await refused.Content.ReadAsStringAsync(ct); + Assert.Equal(HttpStatusCode.BadRequest, refused.StatusCode); + Assert.Contains("invalid_scope", refusedBody); + Assert.Contains("AllowDynamicRegistrationClients", refusedBody); + + // Declared scope: the intersection is registered and echoed, nothing more. + var declaring = await http.PostAsync("/connect/register", JsonContent.Create(new + { + client_name = "Declaring MCP Client", + redirect_uris = new[] { RedirectUri }, + scope = $"openid {ScopeName} {notOptedIn} no-such-scope", + }), ct); + var declaringBody = await declaring.Content.ReadAsStringAsync(ct); + Assert.True(declaring.StatusCode == HttpStatusCode.Created, declaringBody); + using (var doc = JsonDocument.Parse(declaringBody)) + Assert.Equal(new[] { "openid", ScopeName }, doc.RootElement.GetProperty("scope").GetString()!.Split(' ')); + } + + private async Task AuthorizeLocationAsync(string clientId, string redirectUri, string? scope = null) + => (await AuthorizeResponseAsync(clientId, redirectUri, scope)).Headers.Location?.ToString(); + + private async Task AuthorizeResponseAsync(string clientId, string redirectUri, string? scope = null) { var challenge = GeneratePkceS256Challenge(GeneratePkceVerifier()); var cookieClient = await CreateAuthenticatedClientAsync("tu", "TestPass1234"); @@ -139,14 +205,13 @@ public async Task Loopback_redirect_with_an_ephemeral_port_is_accepted_for_a_dcr "response_type=code", $"client_id={Uri.EscapeDataString(clientId)}", $"redirect_uri={Uri.EscapeDataString(redirectUri)}", - $"scope={Uri.EscapeDataString($"openid {ScopeName}")}", + $"scope={Uri.EscapeDataString(scope ?? $"openid {ScopeName}")}", $"state={Guid.NewGuid():N}", $"code_challenge={challenge}", "code_challenge_method=S256", $"resource={Uri.EscapeDataString(AllowedAudience)}", }); - var resp = await cookieClient.GetAsync(uri, TestContext.Current.CancellationToken); - return resp.Headers.Location?.ToString(); + return await cookieClient.GetAsync(uri, TestContext.Current.CancellationToken); } [Fact] @@ -312,7 +377,12 @@ public async Task App_scoped_scope_without_AllowDcrClients_rejects_authorize_for { // Negative twin: the same setup but AllowDynamicRegistrationClients=false // on the scope. /connect/authorize must short-circuit with - // invalid_scope before the user ever lands on the consent screen. + // invalid_scope before the user ever lands on the consent screen. The + // scope is not even registered on the client any more (declared scopes + // are intersected with the opted-in set), so the refusal comes from + // DynamicClientScopeHandler inside OpenIddict's request validation — + // rendered as an error page like every validation failure, naming the + // scope and the flag, instead of OpenIddict's generic ID2051. await SeedAsync(); var app = await CreateAppAsync($"dcr-app-{Guid.NewGuid():N}", "DCR Test App (negative)"); var apiName = "https://dcr-appscoped-negative.example/"; @@ -339,11 +409,11 @@ public async Task App_scoped_scope_without_AllowDcrClients_rejects_authorize_for }); var resp = await cookieClient.GetAsync(authorizeUri, TestContext.Current.CancellationToken); - // The endpoint redirects back to redirect_uri with error=invalid_scope. - AssertRedirect(resp); - var loc = resp.Headers.Location!.ToString(); - Assert.Contains("error=invalid_scope", loc); - Assert.Contains("error_description=", loc); + var body = await resp.Content.ReadAsStringAsync(TestContext.Current.CancellationToken); + Assert.True(resp.StatusCode == HttpStatusCode.BadRequest, $"expected 400, got {(int)resp.StatusCode} {resp.Headers.Location}\n{body}"); + Assert.Contains("invalid_scope", body); + Assert.Contains(appScopedScope, body); + Assert.Contains("AllowDynamicRegistrationClients", body); } [Fact] @@ -466,17 +536,19 @@ private async Task CreateDisallowedApiAsync() /// Creates an OAuthScope whose Resources include the allowed /// API name (so principals granted this scope receive that audience) /// AND whose Properties dict carries AllowDynamicRegistrationClients=true. - private async Task CreateAllowedScopeAsync() + private Task CreateAllowedScopeAsync() => CreateScopeAsync(ScopeName, allowDynamicClients: true); + + private async Task CreateScopeAsync(string name, bool allowDynamicClients) { using var scope = NewSystemTenantScope(); var oauthAdmin = scope.ServiceProvider.GetRequiredService(); var createResult = await oauthAdmin.CreateScopeAsync(new CreateOAuthScopeDto { - Name = ScopeName, - DisplayName = ScopeName, + Name = name, + DisplayName = name, Resources = new List { AllowedAudience }, - AllowDynamicRegistrationClients = true, + AllowDynamicRegistrationClients = allowDynamicClients, }, TestContext.Current.CancellationToken); Assert.False(createResult.IsError, diff --git a/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs b/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs index b74217b2..17bf6110 100644 --- a/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs +++ b/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs @@ -5,6 +5,7 @@ using BuildingBlocks.Helper; using Modgud.Api.Features.Auth.PositionTerminals; using Modgud.Api.Features.Auth.Staffing; +using Modgud.Application.Dcr; using Modgud.Authentication.Applications; using Modgud.Authentication.Sessions; using Modgud.Authentication.Domain; @@ -2770,6 +2771,9 @@ private static async Task IsApplicationEnabledAsync( /// (standard OIDC scopes, cross-app utility scopes). /// Scope registered with a non-null AppId → only allowed /// when the calling client's AppId matches. + /// Dynamic client (DCR / CIMD) → every registered scope must pass + /// instead: the per-scope + /// opt-in, or an open standard scope. /// /// private static async Task ValidateScopeRestrictionAsync( @@ -2784,9 +2788,12 @@ private static async Task IsApplicationEnabledAsync( .Where(s => scopeNames.Contains(s.Name) && !s.IsDeleted) .ToListAsync(cancellationToken); - // If no requested scope is app-scoped, there's nothing to restrict. + // Nothing to restrict when every requested scope is one any client may + // hold — an open standard scope — so the common `openid profile email` + // request never pays for a client lookup. App-scoped and custom global + // scopes need the client to decide. var appScoped = scopes.Where(s => s.AppId.HasValue).ToList(); - if (appScoped.Count == 0) return null; + if (scopes.All(s => DynamicClientScopePolicy.IsOpenStandardScope(s.Name))) return null; // Resolve the calling client. We need it twice: once for the // app-link set, once for the IsDynamicallyRegistered flag. @@ -2816,18 +2823,19 @@ private static async Task IsApplicationEnabledAsync( if (isDcrClient) { - var notOptedIn = appScoped.FirstOrDefault(s => - !ReadDcrFlag(s.Properties, ScopePropertyKeys.AllowDynamicRegistrationClients)); - if (notOptedIn is not null) + // Same verdict as DynamicClientScopeHandler in the OpenIddict pipeline; + // this copy also covers the consent continuation, where that pipeline + // does not run. Global custom scopes need the opt-in as much as + // app-scoped ones — the flag is the boundary, not the App link. + var refused = scopes.FirstOrDefault(s => !DynamicClientScopePolicy.IsRequestable(s)); + if (refused is not null) { return Results.Forbid( new AuthenticationProperties(new Dictionary { [OpenIddictServerAspNetCoreConstants.Properties.Error] = Errors.InvalidScope, [OpenIddictServerAspNetCoreConstants.Properties.ErrorDescription] = - $"Scope '{notOptedIn.Name}' is not opted in for Dynamic Client Registration clients. " + - "Ask the realm admin to enable AllowDynamicRegistrationClients on the scope, " + - "or use a global (cross-app) scope.", + DynamicClientScopeHandler.DescribeRefusal(refused), }), new[] { OpenIddictServerAspNetCoreDefaults.AuthenticationScheme }); } @@ -2835,8 +2843,9 @@ private static async Task IsApplicationEnabledAsync( } // Non-DCR client: app-scoped scope must intersect the client's - // own App set. (Global scopes — AppId == null — were filtered - // out above.) + // own App set. (Global scopes — AppId == null — are not restricted + // for registered clients.) + if (appScoped.Count == 0) return null; var clientAppIds = client?.AppIds.ToHashSet() ?? new HashSet(); var bad = appScoped.FirstOrDefault(s => !clientAppIds.Contains(s.AppId!.Value)); if (bad is not null) diff --git a/src/dotnet/Modgud.Api/Features/Auth/OAuth/DcrRegistrationEndpoints.cs b/src/dotnet/Modgud.Api/Features/Auth/OAuth/DcrRegistrationEndpoints.cs index 116dcf69..2cc16150 100644 --- a/src/dotnet/Modgud.Api/Features/Auth/OAuth/DcrRegistrationEndpoints.cs +++ b/src/dotnet/Modgud.Api/Features/Auth/OAuth/DcrRegistrationEndpoints.cs @@ -191,7 +191,10 @@ private static async Task RegisterAsync( ClientName = created.DisplayName, ClientUri = request.ClientUri, LogoUri = request.LogoUri, - Scope = normalized.Scopes.Count == 0 ? null : string.Join(' ', normalized.Scopes), + // The REGISTERED set (RFC 7591 §3.2.1): the request's `scope` was an upper + // bound intersected with the realm's dynamic-client scopes, and a request + // without one got that whole set — the response tells the client which. + Scope = RegisteredScope(created.Permissions), Contacts = request.Contacts, TosUri = request.TosUri, PolicyUri = request.PolicyUri, @@ -200,6 +203,15 @@ private static async Task RegisterAsync( }); } + private static string? RegisteredScope(IEnumerable permissions) + { + var scopes = permissions + .Where(p => p.StartsWith(OAuthPermissions.Prefixes.Scope, StringComparison.Ordinal)) + .Select(p => p[OAuthPermissions.Prefixes.Scope.Length..]) + .ToList(); + return scopes.Count == 0 ? null : string.Join(' ', scopes); + } + private static string ResolveSourceIp(HttpContext ctx) { // CDN / reverse-proxy headers are honoured upstream by diff --git a/src/dotnet/Modgud.Application/Dcr/DynamicClientScopePolicy.cs b/src/dotnet/Modgud.Application/Dcr/DynamicClientScopePolicy.cs new file mode 100644 index 00000000..70c60266 --- /dev/null +++ b/src/dotnet/Modgud.Application/Dcr/DynamicClientScopePolicy.cs @@ -0,0 +1,88 @@ +using System.Text.Json; +using Marten; +using Modgud.Domain.OAuth.Scopes; + +namespace Modgud.Application.Dcr; + +/// +/// Which scopes a dynamic client — one minted via Dynamic Client +/// Registration or synthesized from a CIMD document — may hold and request. +/// The one rule behind three seams: the CIMD resolver (permissions on the +/// synthesized client), the DCR registration (permissions on the stored +/// client) and the authorize-time check. +/// +/// A scope is requestable by dynamic clients when it is enabled and +/// either carries +/// — the admin's per-scope opt-in, the flag's whole meaning — or is one of the +/// standard scopes other than modgud.management. The standard scopes +/// cannot carry the flag (they are immutable), so they need a built-in +/// verdict: the identity and claim-gate scopes name no resource privilege of +/// their own, an unverified client may ask for them; the management selector +/// names the IdP's own management API and is exactly the "tenant.admin.*" +/// case the flag exists to keep away from a habit-click consent (the realm +/// seeder strips the flag from it for the same reason). +/// +/// The client's own scope declaration is an upper bound, never +/// a grant: declared scopes are intersected with this set, and a client that +/// declares none gets the whole set (RFC 7591 §2 — "a default set of +/// scopes"). A static CIMD document published for every MCP server in the +/// world cannot know one server's scopes; the set makes such a client work +/// without letting it invent permissions. +/// +public static class DynamicClientScopePolicy +{ + /// The scope may be held and requested by dynamic clients. A + /// standard scope is judged by the built-in verdict alone — a flag that + /// found its way onto one (the seeder strips it) changes nothing. + public static bool IsRequestable(OAuthScopeState scope) + => scope is { IsDeleted: false, Enabled: true } + && (StandardScopes.IsStandard(scope.Name) ? IsOpenStandardScope(scope.Name) : HasOptIn(scope.Properties)); + + /// A standard scope that dynamic clients may always request — + /// every standard scope except the management-API selector. + public static bool IsOpenStandardScope(string? name) + => StandardScopes.IsStandard(name) && name != StandardScopes.Management; + + /// Reads the per-scope opt-in flag (plain bool or JsonElement, + /// depending on which serializer wrote the document). + public static bool HasOptIn(IDictionary properties) + { + if (!properties.TryGetValue(ScopePropertyKeys.AllowDynamicRegistrationClients, out var raw) || raw is null) + return false; + return raw switch + { + bool b => b, + JsonElement e when e.ValueKind is JsonValueKind.True => true, + _ => false, + }; + } + + /// + /// The scope names a dynamic client ends up holding: the declared names + /// intersected with the requestable set, or the whole requestable set when + /// nothing was declared. Order is preserved from + /// so the result is stable across resolves. + /// + public static List Resolve(IReadOnlyCollection declared, IReadOnlyList requestable) + { + if (declared.Count == 0) return requestable.ToList(); + var wanted = declared.ToHashSet(StringComparer.Ordinal); + return requestable.Where(wanted.Contains).ToList(); + } + + /// + /// Loads the requestable scope names of the current realm: standard scopes + /// first in their canonical order, then the opted-in ones by name. + /// + public static async Task> LoadRequestableNamesAsync(IQuerySession session, CancellationToken ct) + { + var scopes = await session.Query() + .Where(s => !s.IsDeleted && s.Enabled) + .ToListAsync(ct); + var requestable = scopes.Where(IsRequestable).Select(s => s.Name).ToHashSet(StringComparer.Ordinal); + + var ordered = StandardScopes.All.Where(requestable.Contains).ToList(); + ordered.AddRange(requestable.Except(StandardScopes.All, StringComparer.Ordinal).Order(StringComparer.Ordinal)); + return ordered; + } +} diff --git a/src/dotnet/Modgud.Application/Services/OAuthAdminService.cs b/src/dotnet/Modgud.Application/Services/OAuthAdminService.cs index 4b3afe40..4105184d 100644 --- a/src/dotnet/Modgud.Application/Services/OAuthAdminService.cs +++ b/src/dotnet/Modgud.Application/Services/OAuthAdminService.cs @@ -237,8 +237,17 @@ public async Task> CreateClientAsync( clientSecret = dto.ClientSecret ?? (createJwks is null ? GenerateSecret() : null); } + // A dynamically registered client holds what the realm lets dynamic clients + // hold: its own `scope` is an upper bound intersected with the opted-in set, + // and a registration without one gets the whole set (RFC 7591 §2 — "a + // default set of scopes"). Registering the declared list verbatim used to + // leave a scope-less registration with no scope permission at all. + var scopes = dcrMetadata is null + ? dto.Scopes + : DynamicClientScopePolicy.Resolve(dto.Scopes, await DynamicClientScopePolicy.LoadRequestableNamesAsync(_session, ct)); + // Build permissions list (endpoints + grant types + scopes). - var permissions = BuildClientPermissions(dto.AllowedGrantTypes, dto.Scopes, dto.ClientType, dto.Capabilities); + var permissions = BuildClientPermissions(dto.AllowedGrantTypes, scopes, dto.ClientType, dto.Capabilities); var pinnedClientId = await PinnedEntityId.ResolveAsync( _session, dto.Id, "OAuthClient", s => s.IsDeleted, ct); diff --git a/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdClientResolver.cs b/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdClientResolver.cs index af6da8ab..b98ab442 100644 --- a/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdClientResolver.cs +++ b/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdClientResolver.cs @@ -3,6 +3,7 @@ using System.Security.Cryptography; using System.Text; using System.Text.Json; +using Modgud.Application.Dcr; using Modgud.Domain.Applications; using Modgud.Domain.OAuth.Applications; using Modgud.Domain.OAuth.Common; @@ -98,6 +99,7 @@ public CimdClientResolver( // session (NOT the resolver) to preserve this resolver's no-eager-tenant- // session design (it must not throw on a realm without a physical DB). CimdSettings? settings; + IReadOnlyList requestableScopes; await using (var session = _sessionFactory.OpenQuerySession()) { var realmCimd = (await session.LoadAsync(RealmSettingsDoc.SingletonId, cancellationToken))?.Cimd; @@ -117,12 +119,19 @@ public CimdClientResolver( { settings = realmCimd; } + if (settings is null || !settings.Enabled) return null; + + // The scope permissions are NOT part of the cached document: they are + // the realm's live opt-in set (DynamicClientScopePolicy) intersected with + // whatever the document declares, recomputed on every resolve so a scope + // opted in or out by an admin takes effect at once — same liveness rule + // as the Enabled check above. The document itself stays memoized. + requestableScopes = await LoadRequestableScopesAsync(session, cancellationToken); } - if (settings is null || !settings.Enabled) return null; var cacheKey = $"cimd:doc:{TenantContext.Current}:{clientId}"; if (_cache.TryGetValue(cacheKey, out var cached) && cached is not null) - return Synthesize(cached); + return Synthesize(cached, requestableScopes); if (!CimdClientId.TryValidateUrl(clientId, out var uri, out var urlError) || uri is null) { @@ -137,7 +146,26 @@ public CimdClientResolver( if (ttl > TimeSpan.Zero) _cache.Set(cacheKey, entry, ttl); - return Synthesize(entry); + return Synthesize(entry, requestableScopes); + } + + private const string RequestableScopesItemKey = "Modgud.Cimd.RequestableScopes"; + + /// + /// One realm-wide scope query per request, not per resolve: the store, the + /// audience-containment handler and the authorize endpoint each resolve the + /// same client once per request, so the set is memoized on HttpContext.Items + /// (a background resolve without a request context simply queries). + /// + private async Task> LoadRequestableScopesAsync(IQuerySession session, CancellationToken cancellationToken) + { + var items = _httpContextAccessor.HttpContext?.Items; + if (items is not null && items.TryGetValue(RequestableScopesItemKey, out var memo) && memo is IReadOnlyList known) + return known; + + var loaded = await DynamicClientScopePolicy.LoadRequestableNamesAsync(session, cancellationToken); + if (items is not null) items[RequestableScopesItemKey] = loaded; + return loaded; } private async Task<(CimdMetadata? Metadata, TimeSpan Ttl)> FetchAndValidateAsync( @@ -223,7 +251,7 @@ private static TimeSpan ResolveTtl(CacheControlHeaderValue? cacheControl) return ttl; } - private static OAuthApplicationState Synthesize(CachedCimd entry) + private static OAuthApplicationState Synthesize(CachedCimd entry, IReadOnlyList requestableScopes) { var meta = entry.Metadata; return new OAuthApplicationState @@ -245,7 +273,7 @@ private static OAuthApplicationState Synthesize(CachedCimd entry) ?? OAuthApplicationTypes.Web, RedirectUris = meta.RedirectUris.ToList(), PostLogoutRedirectUris = new List(), - Permissions = BuildPermissions(meta), + Permissions = BuildPermissions(meta, requestableScopes), Requirements = new List(), // global RequireProofKeyForCodeExchange enforces PKCE Settings = new Dictionary { @@ -273,7 +301,7 @@ private static OAuthApplicationState Synthesize(CachedCimd entry) }; } - private static List BuildPermissions(CimdMetadata meta) + private static List BuildPermissions(CimdMetadata meta, IReadOnlyList requestableScopes) { var permissions = new List { @@ -298,7 +326,13 @@ private static List BuildPermissions(CimdMetadata meta) // authorization_code is guaranteed present (the parser requires it). permissions.Add(OAuthPermissions.ResponseTypes.Code); - foreach (var scope in meta.Scopes) + // The document's `scope` is an upper bound, not a grant; a document without + // one (every static MCP-client document — it cannot know one server's + // scopes) holds the realm's whole dynamic-client set. Granting only the + // declared scopes used to leave such a client with no scope permission at + // all, so OpenIddict refused everything but openid/offline_access (ID2051) + // before the per-scope opt-in was ever consulted. + foreach (var scope in DynamicClientScopePolicy.Resolve(meta.Scopes, requestableScopes)) permissions.Add(OAuthPermissions.Prefixes.Scope + scope); return permissions; diff --git a/src/dotnet/Modgud.Infrastructure/OpenIddict/DynamicClientScopeHandler.cs b/src/dotnet/Modgud.Infrastructure/OpenIddict/DynamicClientScopeHandler.cs new file mode 100644 index 00000000..b363066e --- /dev/null +++ b/src/dotnet/Modgud.Infrastructure/OpenIddict/DynamicClientScopeHandler.cs @@ -0,0 +1,100 @@ +using System.Text.Json; +using Marten; +using Modgud.Application.Dcr; +using Modgud.Domain.OAuth.Applications; +using Modgud.Domain.OAuth.Scopes; +using OpenIddict.Abstractions; +using OpenIddict.Server; +using static OpenIddict.Abstractions.OpenIddictConstants; +using static OpenIddict.Server.OpenIddictServerEvents; + +namespace Modgud.Infrastructure.OpenIddict; + +/// +/// Authorize-time half of : a dynamic +/// client (DCR-minted or CIMD-synthesized) that requests a scope the realm has +/// not opted in for dynamic clients is refused with invalid_scope and a +/// description that names the scope and the flag to flip. +/// +/// Ordered just BEFORE OpenIddict's ValidateScopePermissions on +/// purpose. A dynamic client's permissions are derived from the same policy, +/// so a scope that fails here also has no scp: permission — and +/// OpenIddict's check would reject it first with the generic ID2051 ("not +/// allowed to use the specified scope"), which tells an admin nothing about +/// AllowDynamicRegistrationClients. Pre-compute, not override: the +/// reject stops the pipeline, so Order - 1 is the right side. +/// +/// Runs after OpenIddict's ValidateScopes, so an unregistered +/// scope name still gets OpenIddict's own error; this handler only judges +/// scopes the realm knows. +/// +public sealed class DynamicClientScopeHandler : IOpenIddictServerHandler +{ + public static OpenIddictServerHandlerDescriptor Descriptor { get; } + = OpenIddictServerHandlerDescriptor.CreateBuilder() + .UseScopedHandler() + .SetOrder(OpenIddictServerHandlers.Authentication.ValidateScopePermissions.Descriptor.Order - 1) + .SetType(OpenIddictServerHandlerType.Custom) + .Build(); + + private readonly IDocumentSession _session; + private readonly IOpenIddictApplicationManager _applications; + + public DynamicClientScopeHandler(IDocumentSession session, IOpenIddictApplicationManager applications) + { + _session = session; + _applications = applications; + } + + public async ValueTask HandleAsync(ValidateAuthorizationRequestContext context) + { + var clientId = context.ClientId; + if (string.IsNullOrEmpty(clientId)) return; + + var requested = context.Request.GetScopes(); + if (requested.Length == 0) return; + + // Through the manager, not a direct query: ValidateClientId just loaded this + // client, so this is a cache hit for every registered client, and the store + // already falls back to the CIMD resolver for a synthesized one. + if (await _applications.FindByClientIdAsync(clientId, context.CancellationToken) is not OAuthApplicationState application + || !IsDynamicClient(application.Properties)) + return; + + var names = requested.ToArray(); + var scopes = await _session.Query() + .Where(s => names.Contains(s.Name) && !s.IsDeleted) + .ToListAsync(context.CancellationToken); + + var refused = scopes.FirstOrDefault(s => !DynamicClientScopePolicy.IsRequestable(s)); + if (refused is null) return; + + context.Reject( + error: Errors.InvalidScope, + description: DescribeRefusal(refused)); + } + + /// The same wording the authorize endpoint's own check uses, so an + /// agent sees one message whichever layer catches it. + public static string DescribeRefusal(OAuthScopeState scope) + { + if (!scope.Enabled) + return $"Scope '{scope.Name}' is disabled on this realm."; + if (StandardScopes.IsStandard(scope.Name)) + return $"Scope '{scope.Name}' is not available to dynamically registered clients."; + return $"Scope '{scope.Name}' is not opted in for Dynamic Client Registration clients. " + + "Ask the realm admin to enable AllowDynamicRegistrationClients on the scope."; + } + + private static bool IsDynamicClient(IDictionary props) + { + if (!props.TryGetValue(OAuthApplicationPropertyKeys.DcrIsDynamicallyRegistered, out var raw) || raw is null) + return false; + return raw switch + { + bool b => b, + JsonElement e when e.ValueKind is JsonValueKind.True => true, + _ => false, + }; + } +} diff --git a/src/dotnet/Modgud.Infrastructure/OpenIddict/OpenIddictExtensions.cs b/src/dotnet/Modgud.Infrastructure/OpenIddict/OpenIddictExtensions.cs index 959de56c..61af4a10 100644 --- a/src/dotnet/Modgud.Infrastructure/OpenIddict/OpenIddictExtensions.cs +++ b/src/dotnet/Modgud.Infrastructure/OpenIddict/OpenIddictExtensions.cs @@ -356,6 +356,7 @@ public static IServiceCollection AddOpenIddictWithMarten( options.AddEventHandler(AccessTokenTypeHandler.Descriptor); options.AddEventHandler(TokenMintMetricHandler.Descriptor); options.AddEventHandler(ResourceIndicatorHandler.Descriptor); + options.AddEventHandler(DynamicClientScopeHandler.Descriptor); options.AddEventHandler(DcrAudienceContainmentHandler.Descriptor); options.AddEventHandler(DcrLastUsedTrackerHandler.Descriptor); options.AddEventHandler(RealmSigningKeyHandler.Descriptor); diff --git a/src/dotnet/Modgud.Tests.Unit/OAuth/Dcr/DynamicClientScopePolicyTests.cs b/src/dotnet/Modgud.Tests.Unit/OAuth/Dcr/DynamicClientScopePolicyTests.cs new file mode 100644 index 00000000..0eff6b21 --- /dev/null +++ b/src/dotnet/Modgud.Tests.Unit/OAuth/Dcr/DynamicClientScopePolicyTests.cs @@ -0,0 +1,104 @@ +using System.Text.Json; +using Modgud.Application.Dcr; +using Modgud.Domain.OAuth.Scopes; + +namespace Modgud.Tests.Unit.OAuth.Dcr; + +/// +/// Pins the one rule behind the CIMD permissions, the DCR registration and the +/// authorize-time check: which scopes a dynamic client may hold, and how a +/// declared scope is reconciled with that set. +/// +public class DynamicClientScopePolicyTests +{ + [Theory] + [InlineData("openid")] + [InlineData("profile")] + [InlineData("email")] + [InlineData("phone")] + [InlineData("address")] + [InlineData("offline_access")] + [InlineData("roles")] + [InlineData("permissions")] + public void Every_standard_scope_but_the_management_selector_is_open(string name) + { + Assert.True(DynamicClientScopePolicy.IsOpenStandardScope(name)); + Assert.True(DynamicClientScopePolicy.IsRequestable(Scope(name))); + } + + [Fact] + public void The_management_selector_is_never_requestable_by_dynamic_clients() + { + Assert.False(DynamicClientScopePolicy.IsOpenStandardScope(StandardScopes.Management)); + Assert.False(DynamicClientScopePolicy.IsRequestable(Scope(StandardScopes.Management))); + // Not even with the flag — the seeder strips it, the policy ignores it. + Assert.False(DynamicClientScopePolicy.IsRequestable(Scope(StandardScopes.Management, optIn: true))); + } + + [Fact] + public void A_custom_scope_needs_the_opt_in_whether_app_scoped_or_global() + { + Assert.False(DynamicClientScopePolicy.IsRequestable(Scope("lists.read"))); + Assert.True(DynamicClientScopePolicy.IsRequestable(Scope("lists.read", optIn: true))); + Assert.False(DynamicClientScopePolicy.IsRequestable(Scope("lists.read", appId: Guid.NewGuid()))); + Assert.True(DynamicClientScopePolicy.IsRequestable(Scope("lists.read", optIn: true, appId: Guid.NewGuid()))); + } + + [Fact] + public void The_opt_in_reads_as_plain_bool_or_json_element() + { + var plain = Scope("a"); + plain.Properties[ScopePropertyKeys.AllowDynamicRegistrationClients] = true; + Assert.True(DynamicClientScopePolicy.HasOptIn(plain.Properties)); + + var element = Scope("b"); + element.Properties[ScopePropertyKeys.AllowDynamicRegistrationClients] = JsonSerializer.SerializeToElement(true); + Assert.True(DynamicClientScopePolicy.HasOptIn(element.Properties)); + + var off = Scope("c"); + off.Properties[ScopePropertyKeys.AllowDynamicRegistrationClients] = JsonSerializer.SerializeToElement(false); + Assert.False(DynamicClientScopePolicy.HasOptIn(off.Properties)); + Assert.False(DynamicClientScopePolicy.HasOptIn(new Dictionary())); + } + + [Fact] + public void A_disabled_or_deleted_scope_is_not_requestable_even_with_the_opt_in() + { + var disabled = Scope("x", optIn: true); + disabled.Enabled = false; + Assert.False(DynamicClientScopePolicy.IsRequestable(disabled)); + + var deleted = Scope("y", optIn: true); + deleted.IsDeleted = true; + Assert.False(DynamicClientScopePolicy.IsRequestable(deleted)); + } + + [Fact] + public void Nothing_declared_means_the_whole_requestable_set() + { + var requestable = new[] { "openid", "profile", "lists.read" }; + Assert.Equal(requestable, DynamicClientScopePolicy.Resolve(Array.Empty(), requestable)); + } + + [Fact] + public void A_declaration_is_an_upper_bound_intersected_in_requestable_order() + { + var requestable = new[] { "openid", "profile", "lists.read", "lists.write" }; + var declared = new[] { "lists.write", "admin", "openid", "no-such-scope" }; + Assert.Equal(new[] { "openid", "lists.write" }, DynamicClientScopePolicy.Resolve(declared, requestable)); + } + + [Fact] + public void A_declaration_with_nothing_requestable_in_it_yields_the_empty_set() + { + Assert.Empty(DynamicClientScopePolicy.Resolve(new[] { "admin" }, new[] { "openid", "lists.read" })); + } + + private static OAuthScopeState Scope(string name, bool optIn = false, Guid? appId = null) + { + var scope = new OAuthScopeState { Id = Guid.NewGuid(), Name = name, AppId = appId }; + if (optIn) + scope.Properties[ScopePropertyKeys.AllowDynamicRegistrationClients] = JsonSerializer.SerializeToElement(true); + return scope; + } +}