From edadc4979d6d053ff6f2157819e5f55a0b20eee4 Mon Sep 17 00:00:00 2001 From: Bernhard Windisch Date: Wed, 22 Jul 2026 06:55:46 +0200 Subject: [PATCH] fix(oauth): remove the implicit and password grants entirely MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admin UI listed 'implicit' and 'password' as selectable grant types (marked deprecated) and the backend silently persisted them as gt:implicit / gt:password OpenIddict permissions on the client. They never actually worked — the server enables neither AllowImplicitFlow nor AllowPasswordFlow, so any real request is rejected at the token/authorize endpoint and discovery never advertises them — but a client could be configured with them, and the security-model doc's 'deliberately rejected' claim was only true by omission at the endpoint, not at config time. OAuth 2.1 removes both grants. Remove them for good: - Frontend: drop the two options from the client grant-type picker (the whole 'Deprecated' group goes with them). - Backend: remove the gt:implicit / gt:password constants and both mapping directions. A client that somehow still carries the permission maps to null on read (dropped from the DTO, stripped on next save). - Add OAuthAdminMapping.ValidateGrantTypes: any grant that doesn't map to a permission — implicit, password, or a typo — is now rejected with OAuth.UnsupportedGrantType on both create and update, instead of being silently dropped while building permissions. The rejection is now explicit and at config time. No seed or provisioning manifest used either grant (demo-seed's 'implicit' is a consent type, not a grant). Tests: unit ValidateGrantTypes accept/reject + the two removed grants map to null; integration OAuthGrantTypeAllowlistTests drives the real OAuthAdminService and asserts create + update reject implicit/password with a positive control. Unit 1402/1402, integration 537/538 (the one failure is the known last-dependent UserView catch-up flake — DcrRegistrationEndpointTests, green in isolation and logically unrelated to grant validation). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Security/OAuthGrantTypeAllowlistTests.cs | 97 +++++++++++++++++++ .../Modgud.Application/Errors/OAuthErrors.cs | 9 +- .../Services/OAuthAdminMapping.cs | 33 ++++++- .../Services/OAuthAdminService.cs | 11 +++ .../OAuth/Common/OAuthConstants.cs | 6 +- .../Application/OAuthAdminMappingTests.cs | 51 +++++++++- .../OAuth/OAuthConstantsTests.cs | 12 --- .../src/views/admin/oauth/ClientDetails.vue | 8 +- 8 files changed, 198 insertions(+), 29 deletions(-) create mode 100644 src/dotnet/Modgud.Api.Tests/Security/OAuthGrantTypeAllowlistTests.cs 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