diff --git a/src/dotnet/Modgud.Api.Tests/Security/OAuthGrantTypeAllowlistTests.cs b/src/dotnet/Modgud.Api.Tests/Security/OAuthGrantTypeAllowlistTests.cs new file mode 100644 index 00000000..524c883b --- /dev/null +++ b/src/dotnet/Modgud.Api.Tests/Security/OAuthGrantTypeAllowlistTests.cs @@ -0,0 +1,97 @@ +using Microsoft.Extensions.DependencyInjection; +using Modgud.Api.Tests.Infrastructure; +using Modgud.Application.DTOs.OAuth; +using Modgud.Application.Services; +using Modgud.Domain.OAuth.Common; + +namespace Modgud.Api.Tests.Security; + +/// +/// Pins that the OAuth-2.1-removed implicit and password grants — +/// and any other unsupported grant — are rejected at client configuration time, +/// not merely inert at the token endpoint. +/// +/// +/// They used to be selectable in the admin UI and were silently persisted as +/// gt:implicit / gt:password permissions (they never functioned, +/// because the server enables neither flow). They are now gone from the UI and +/// refused by on both create and update, so a +/// client can never carry them — the "deliberately rejected" claim in the +/// security-model doc is now true at the config layer, not just by omission. +/// +/// +[Collection(IntegrationTestCollection.Name)] +public class OAuthGrantTypeAllowlistTests : IntegrationTestBase +{ + public OAuthGrantTypeAllowlistTests(SharedPostgresFixture fixture) : base(fixture) { } + + private OAuthAdminService Admin(IServiceScope scope) => + scope.ServiceProvider.GetRequiredService(); + + private static CreateOAuthClientDto NewClient(string clientId, List grants) => new() + { + ClientId = clientId, + ClientType = OAuthClientTypes.Public, + ConsentType = OAuthConsentTypes.Explicit, + DisplayName = clientId, + RedirectUris = [], + PostLogoutRedirectUris = [], + Scopes = ["openid"], + AllowedGrantTypes = grants, + RequireConsent = false, + }; + + [Theory] + [InlineData("implicit")] + [InlineData("password")] + public async Task Create_with_a_removed_grant_is_rejected(string removedGrant) + { + var ct = TestContext.Current.CancellationToken; + using var scope = Factory.Services.CreateScope(); + + var result = await Admin(scope).CreateClientAsync( + NewClient($"reject-{removedGrant}-{Guid.NewGuid():N}", + ["authorization_code", removedGrant]), ct); + + Assert.True(result.IsError); + Assert.Equal("OAuth.UnsupportedGrantType", result.FirstError.Code); + Assert.Contains(removedGrant, result.FirstError.Description); + } + + [Fact] + public async Task Update_to_add_a_removed_grant_is_rejected() + { + var ct = TestContext.Current.CancellationToken; + using var scope = Factory.Services.CreateScope(); + var admin = Admin(scope); + + // A valid client first. + var created = await admin.CreateClientAsync( + NewClient($"upd-{Guid.NewGuid():N}", ["authorization_code", "refresh_token"]), ct); + Assert.False(created.IsError, + created.IsError ? created.FirstError.Description : ""); + + // Now try to sneak the removed grant in via update. + var updated = await admin.UpdateClientAsync(created.Value.Client.Id, new UpdateOAuthClientDto + { + AllowedGrantTypes = ["authorization_code", "password"], + }, ct); + + Assert.True(updated.IsError); + Assert.Equal("OAuth.UnsupportedGrantType", updated.FirstError.Code); + } + + [Fact] + public async Task Create_with_only_supported_grants_still_succeeds() + { + var ct = TestContext.Current.CancellationToken; + using var scope = Factory.Services.CreateScope(); + + // Positive control: the guard must not break the supported set. + var result = await Admin(scope).CreateClientAsync( + NewClient($"ok-{Guid.NewGuid():N}", ["authorization_code", "refresh_token"]), ct); + + Assert.False(result.IsError, + result.IsError ? result.FirstError.Description : ""); + } +} diff --git a/src/dotnet/Modgud.Application/Errors/OAuthErrors.cs b/src/dotnet/Modgud.Application/Errors/OAuthErrors.cs index b1b027e4..ebb696f5 100644 --- a/src/dotnet/Modgud.Application/Errors/OAuthErrors.cs +++ b/src/dotnet/Modgud.Application/Errors/OAuthErrors.cs @@ -20,6 +20,13 @@ public static Error InvalidConsentType(string consentType) => Error.Validation( code: "OAuth.InvalidConsentType", description: $"Invalid consent type '{consentType}'. Must be 'explicit', 'implicit', or 'external'."); + public static Error UnsupportedGrantType(string grantType) => Error.Validation( + code: "OAuth.UnsupportedGrantType", + description: $"Grant type '{grantType}' is not supported. Allowed: authorization_code, " + + "client_credentials, refresh_token, urn:ietf:params:oauth:grant-type:device_code, " + + "and the native urn:cocoar:* grants. The OAuth 2.1-removed 'implicit' and 'password' " + + "grants are rejected."); + public static Error InvalidWebAuthnRpId(string value) => Error.Validation( code: "OAuth.InvalidWebAuthnRpId", description: $"Invalid WebAuthn RP ID '{value}'. Must be a bare hostname (e.g. 'app.example.com') " @@ -67,7 +74,7 @@ public static Error ServiceAccountNotFound(string id) => Error.Validation( public static Error ServiceAccountLinkRequiresClientCredentialsOnly => Error.Validation( code: "OAuth.ServiceAccountLinkRequiresClientCredentialsOnly", - description: "A ServiceAccount-linked client must use only the 'client_credentials' grant. User-flow grants (authorization_code, implicit, password, refresh_token, device_code) are forbidden — strict separation between user-flow and machine-to-machine clients."); + description: "A ServiceAccount-linked client must use only the 'client_credentials' grant. User-flow grants (authorization_code, refresh_token, device_code) are forbidden — strict separation between user-flow and machine-to-machine clients."); public static Error CannotMutateServiceAccountManagedClient(string clientId) => Error.Validation( code: "OAuth.CannotMutateServiceAccountManagedClient", diff --git a/src/dotnet/Modgud.Application/Services/OAuthAdminMapping.cs b/src/dotnet/Modgud.Application/Services/OAuthAdminMapping.cs index 0eb3f4bd..0af0ae9f 100644 --- a/src/dotnet/Modgud.Application/Services/OAuthAdminMapping.cs +++ b/src/dotnet/Modgud.Application/Services/OAuthAdminMapping.cs @@ -106,8 +106,9 @@ internal static List ExtractScopes(IReadOnlyList permissions) => "authorization_code" => OAuthPermissions.GrantTypes.AuthorizationCode, "client_credentials" => OAuthPermissions.GrantTypes.ClientCredentials, "refresh_token" => OAuthPermissions.GrantTypes.RefreshToken, - "implicit" => OAuthPermissions.GrantTypes.Implicit, - "password" => OAuthPermissions.GrantTypes.Password, + // No "implicit"/"password" cases: OAuth 2.1 removes both and they are + // rejected up front by ValidateGrantTypes. An unmapped grant returns null, + // which ValidateGrantTypes turns into a hard error. "urn:ietf:params:oauth:grant-type:device_code" => OAuthPermissions.GrantTypes.DeviceCode, // ADR-0010 — native (cookieless) passwordless grants. Admin-set per-client // opt-in surfaces here so the OAuth-client admin CRUD can grant them. @@ -122,8 +123,9 @@ internal static List ExtractScopes(IReadOnlyList permissions) => OAuthPermissions.GrantTypes.AuthorizationCode => "authorization_code", OAuthPermissions.GrantTypes.ClientCredentials => "client_credentials", OAuthPermissions.GrantTypes.RefreshToken => "refresh_token", - OAuthPermissions.GrantTypes.Implicit => "implicit", - OAuthPermissions.GrantTypes.Password => "password", + // A legacy client that somehow carries gt:implicit / gt:password (they + // were never functional — no server flow) maps to null here, so it is + // dropped from the DTO and stripped on the next save. OAuthPermissions.GrantTypes.DeviceCode => "urn:ietf:params:oauth:grant-type:device_code", OAuthPermissions.GrantTypes.CocoarOtp => CocoarGrantTypes.Otp, OAuthPermissions.GrantTypes.CocoarMagic => CocoarGrantTypes.Magic, @@ -131,6 +133,29 @@ internal static List ExtractScopes(IReadOnlyList permissions) => _ => null, }; + /// + /// Rejects any grant type the server does not support, so a client can never + /// be configured with a dead or forbidden grant. The supported set is exactly + /// what maps to a permission; anything + /// unmapped — implicit and password (removed per OAuth 2.1), a + /// typo, or a future grant we haven't wired — returns + /// . Previously an unknown grant + /// was silently dropped when building permissions, so the admin got a client + /// that quietly lacked what they asked for; this makes the rejection explicit + /// and moves it to config time instead of the token endpoint. Null/empty input + /// is valid (a client with no token-flow grants — see BuildClientPermissions). + /// + internal static Error? ValidateGrantTypes(IReadOnlyList? grantTypes) + { + if (grantTypes is null) return null; + foreach (var g in grantTypes) + { + if (MapGrantTypeToPermission(g) is null) + return OAuthErrors.UnsupportedGrantType(g); + } + return null; + } + // ───────────────────────────────────────────── Settings / Properties ────── internal static Dictionary BuildClientSettings(CreateOAuthClientDto dto) diff --git a/src/dotnet/Modgud.Application/Services/OAuthAdminService.cs b/src/dotnet/Modgud.Application/Services/OAuthAdminService.cs index d10ec71e..7da404ee 100644 --- a/src/dotnet/Modgud.Application/Services/OAuthAdminService.cs +++ b/src/dotnet/Modgud.Application/Services/OAuthAdminService.cs @@ -133,6 +133,11 @@ public async Task> CreateClientAsync( if (ValidateServiceAccountLinkInvariant(dto.AllowedGrantTypes, linkedServiceAccountId) is { } createLinkErr) return createLinkErr; + // Reject unsupported / removed grant types (implicit, password, typos) + // up front rather than silently dropping them while building permissions. + if (ValidateGrantTypes(dto.AllowedGrantTypes) is { } createGrantErr) + return createGrantErr; + // Confidential clients must have a secret (generated if not supplied). string? clientSecret = null; if (dto.ClientType == OAuthClientTypes.Confidential) @@ -287,6 +292,12 @@ public async Task> UpdateClientAsync( ValidateServiceAccountLinkInvariant(dto.AllowedGrantTypes, linkedServiceAccountId: null) is { } updLinkErr) return updLinkErr; + // Reject unsupported / removed grant types (implicit, password, typos) + // on update too — the guard is only meaningful if it can't be bypassed + // by editing an existing client. + if (ValidateGrantTypes(dto.AllowedGrantTypes) is { } updGrantErr) + return updGrantErr; + if (ValidateWebAuthnRpId(dto.WebAuthnRpId) is { } updRpIdErr) return updRpIdErr; diff --git a/src/dotnet/Modgud.Domain/OAuth/Common/OAuthConstants.cs b/src/dotnet/Modgud.Domain/OAuth/Common/OAuthConstants.cs index 8849215a..4d4ec868 100644 --- a/src/dotnet/Modgud.Domain/OAuth/Common/OAuthConstants.cs +++ b/src/dotnet/Modgud.Domain/OAuth/Common/OAuthConstants.cs @@ -45,8 +45,10 @@ public static class GrantTypes public const string AuthorizationCode = "gt:authorization_code"; public const string ClientCredentials = "gt:client_credentials"; public const string RefreshToken = "gt:refresh_token"; - public const string Implicit = "gt:implicit"; - public const string Password = "gt:password"; + // NB: no Implicit or Password grant. OAuth 2.1 removes both, and the + // OpenIddict server never enables their flows — they were also removed + // from the admin surface so a client can't even be configured with them + // (rejected by OAuthAdminMapping.ValidateGrantTypes). public const string DeviceCode = "gt:urn:ietf:params:oauth:grant-type:device_code"; // ADR-0010 — native (cookieless) passwordless token grants. The per-client diff --git a/src/dotnet/Modgud.Tests.Unit/Application/OAuthAdminMappingTests.cs b/src/dotnet/Modgud.Tests.Unit/Application/OAuthAdminMappingTests.cs index 14f48691..e308d6ff 100644 --- a/src/dotnet/Modgud.Tests.Unit/Application/OAuthAdminMappingTests.cs +++ b/src/dotnet/Modgud.Tests.Unit/Application/OAuthAdminMappingTests.cs @@ -129,8 +129,6 @@ public class GrantTypeRoundTrip [InlineData("authorization_code")] [InlineData("client_credentials")] [InlineData("refresh_token")] - [InlineData("implicit")] - [InlineData("password")] [InlineData("urn:ietf:params:oauth:grant-type:device_code")] public void Maps_grant_type_to_permission_and_back(string grantType) { @@ -141,6 +139,16 @@ public void Maps_grant_type_to_permission_and_back(string grantType) Assert.Equal(grantType, roundTripped); } + [Theory] + [InlineData("implicit")] + [InlineData("password")] + public void Implicit_and_password_are_removed_and_map_to_null(string removedGrant) + { + // OAuth 2.1 removes both grants. They must not map to a permission, so + // ValidateGrantTypes rejects them and BuildClientPermissions can't add them. + Assert.Null(OAuthAdminMapping.MapGrantTypeToPermission(removedGrant)); + } + [Fact] public void Unknown_grant_type_maps_to_null() { @@ -154,6 +162,43 @@ public void Unknown_permission_maps_to_null() } } + public class ValidateGrantTypes + { + [Theory] + [InlineData("authorization_code")] + [InlineData("client_credentials")] + [InlineData("refresh_token")] + [InlineData("urn:ietf:params:oauth:grant-type:device_code")] + [InlineData("urn:cocoar:otp")] + [InlineData("urn:cocoar:magic")] + [InlineData("urn:cocoar:passkey")] + public void Supported_grant_types_are_accepted(string grant) + { + Assert.Null(OAuthAdminMapping.ValidateGrantTypes(new[] { grant })); + } + + [Theory] + [InlineData("implicit")] + [InlineData("password")] + [InlineData("xyz")] + public void Unsupported_grant_types_are_rejected(string grant) + { + var err = OAuthAdminMapping.ValidateGrantTypes(new[] { "authorization_code", grant }); + Assert.NotNull(err); + Assert.Equal("OAuth.UnsupportedGrantType", err!.Value.Code); + Assert.Contains(grant, err.Value.Description); + } + + [Fact] + public void Null_and_empty_are_valid() + { + // A client with no token-flow grants is a legitimate (if useless) state + // — BuildClientPermissions leaves it unable to mint tokens. + Assert.Null(OAuthAdminMapping.ValidateGrantTypes(null)); + Assert.Null(OAuthAdminMapping.ValidateGrantTypes(System.Array.Empty())); + } + } + public class ExtractGrantTypes { [Fact] @@ -799,8 +844,6 @@ public void Link_without_client_credentials_is_rejected_R2() [Theory] [InlineData("authorization_code")] - [InlineData("implicit")] - [InlineData("password")] [InlineData("refresh_token")] [InlineData("urn:ietf:params:oauth:grant-type:device_code")] public void All_user_flow_grants_violate_R3_when_link_set(string userFlowGrant) diff --git a/src/dotnet/Modgud.Tests.Unit/OAuth/OAuthConstantsTests.cs b/src/dotnet/Modgud.Tests.Unit/OAuth/OAuthConstantsTests.cs index bcf3c7c9..aa54a450 100644 --- a/src/dotnet/Modgud.Tests.Unit/OAuth/OAuthConstantsTests.cs +++ b/src/dotnet/Modgud.Tests.Unit/OAuth/OAuthConstantsTests.cs @@ -152,14 +152,6 @@ public void ClientCredentials_value_is_pinned() => public void RefreshToken_value_is_pinned() => Assert.Equal("gt:refresh_token", OAuthPermissions.GrantTypes.RefreshToken); - [Fact] - public void Implicit_value_is_pinned() => - Assert.Equal("gt:implicit", OAuthPermissions.GrantTypes.Implicit); - - [Fact] - public void Password_value_is_pinned() => - Assert.Equal("gt:password", OAuthPermissions.GrantTypes.Password); - [Fact] public void DeviceCode_value_is_the_full_oauth_urn() { @@ -177,8 +169,6 @@ public void All_grant_type_constants_use_the_grant_type_prefix() OAuthPermissions.GrantTypes.AuthorizationCode, OAuthPermissions.GrantTypes.ClientCredentials, OAuthPermissions.GrantTypes.RefreshToken, - OAuthPermissions.GrantTypes.Implicit, - OAuthPermissions.GrantTypes.Password, OAuthPermissions.GrantTypes.DeviceCode, }; @@ -194,8 +184,6 @@ public void All_grant_type_values_are_unique() OAuthPermissions.GrantTypes.AuthorizationCode, OAuthPermissions.GrantTypes.ClientCredentials, OAuthPermissions.GrantTypes.RefreshToken, - OAuthPermissions.GrantTypes.Implicit, - OAuthPermissions.GrantTypes.Password, OAuthPermissions.GrantTypes.DeviceCode, }; Assert.Equal(grants.Length, grants.Distinct().Count()); diff --git a/src/frontend-vue/src/views/admin/oauth/ClientDetails.vue b/src/frontend-vue/src/views/admin/oauth/ClientDetails.vue index ee817d94..98762082 100644 --- a/src/frontend-vue/src/views/admin/oauth/ClientDetails.vue +++ b/src/frontend-vue/src/views/admin/oauth/ClientDetails.vue @@ -85,12 +85,8 @@ const standardGrantTypeOptions = [ { value: 'urn:ietf:params:oauth:grant-type:device_code', label: 'device_code', subtitle: 'TVs, CLIs, anything without a browser', icon: 'monitor', group: 'Standard flows' }, - { value: 'implicit', label: 'implicit', - subtitle: 'DEPRECATED — superseded by authorization_code+PKCE', - icon: 'circle-alert', group: 'Deprecated' }, - { value: 'password', label: 'password', - subtitle: 'DEPRECATED — resource-owner password credentials', - icon: 'circle-alert', group: 'Deprecated' }, + // No implicit / password grants: OAuth 2.1 removes both and the backend + // rejects them (OAuth.UnsupportedGrantType), so they are not offered here. ] // Native passwordless grants (ADR-0010) — cookieless proofs exchanged