diff --git a/modules/openiddict/Volo.Abp.OpenIddict.slnx b/modules/openiddict/Volo.Abp.OpenIddict.slnx index a67c234a01..3cf219591a 100644 --- a/modules/openiddict/Volo.Abp.OpenIddict.slnx +++ b/modules/openiddict/Volo.Abp.OpenIddict.slnx @@ -16,6 +16,7 @@ + diff --git a/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictAspNetCoreModule.cs b/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictAspNetCoreModule.cs index 48496874d0..26729669e7 100644 --- a/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictAspNetCoreModule.cs +++ b/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictAspNetCoreModule.cs @@ -1,4 +1,5 @@ -using Microsoft.AspNetCore.Mvc.Razor; +using Microsoft.AspNetCore.Identity; +using Microsoft.AspNetCore.Mvc.Razor; using Microsoft.Extensions.DependencyInjection; using OpenIddict.Abstractions; using OpenIddict.Server; @@ -39,6 +40,11 @@ public class AbpOpenIddictAspNetCoreModule : AbpModule { options.ViewLocationFormats.Add("/Volo/Abp/OpenIddict/Views/{1}/{0}.cshtml"); }); + + Configure(options => + { + options.RemoveClientIdClaim(); + }); } private void AddOpenIddictServer(IServiceCollection services) diff --git a/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions.cs b/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions.cs new file mode 100644 index 0000000000..fa4fde8a71 --- /dev/null +++ b/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions.cs @@ -0,0 +1,48 @@ +using System.Linq; +using Microsoft.AspNetCore.Identity; +using Volo.Abp.Security.Claims; + +namespace Volo.Abp.OpenIddict; + +public static class AbpOpenIddictSecurityStampValidatorOptionsExtensions +{ + public static SecurityStampValidatorOptions RemoveClientIdClaim(this SecurityStampValidatorOptions options) + { + // OpenIddictClaimsPrincipalContributor stamps the ambient /connect/authorize request's client_id + // onto every principal built by CreateUserPrincipalAsync. That is meant for the access_token, but the + // cookie security-stamp validator rebuilds the interactive cookie through the same method, so a refresh + // that lands on /connect/authorize leaks client_id into the cookie and corrupts ICurrentClient. + // OnRefreshingPrincipal is the only place the cookie is re-written and never runs for token issuance. + var previousOnRefreshingPrincipal = options.OnRefreshingPrincipal; + options.OnRefreshingPrincipal = async context => + { + // Run the previous callback first: ABP Identity's UpdatePrincipal copies claims that are on the + // current cookie but not on the new principal forward, re-introducing client_id from an + // already-corrupted cookie. Removing it afterwards lets such a cookie self-heal on its next refresh. + if (previousOnRefreshingPrincipal != null) + { + await previousOnRefreshingPrincipal.Invoke(context); + } + + RemoveClientIdClaimsFromPrincipal(context); + }; + + return options; + } + + private static void RemoveClientIdClaimsFromPrincipal(SecurityStampRefreshingPrincipalContext context) + { + if (context.NewPrincipal == null) + { + return; + } + + foreach (var identity in context.NewPrincipal.Identities) + { + foreach (var claim in identity.FindAll(AbpClaimTypes.ClientId).ToArray()) + { + identity.RemoveClaim(claim); + } + } + } +} diff --git a/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo.Abp.OpenIddict.AspNetCore.Tests.csproj b/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo.Abp.OpenIddict.AspNetCore.Tests.csproj new file mode 100644 index 0000000000..f4c6815fd6 --- /dev/null +++ b/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo.Abp.OpenIddict.AspNetCore.Tests.csproj @@ -0,0 +1,21 @@ + + + + + + net10.0 + + + + + + + + + + + + + + + diff --git a/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions_Tests.cs b/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions_Tests.cs new file mode 100644 index 0000000000..4231e23b33 --- /dev/null +++ b/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions_Tests.cs @@ -0,0 +1,154 @@ +using System; +using System.Linq; +using System.Security.Claims; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Identity; +using Shouldly; +using Volo.Abp.Security.Claims; +using Xunit; + +namespace Volo.Abp.OpenIddict; + +public class AbpOpenIddictSecurityStampValidatorOptionsExtensions_Tests +{ + [Fact] + public async Task Should_Remove_ClientId_From_Refreshed_Principal() + { + var context = new SecurityStampRefreshingPrincipalContext + { + NewPrincipal = CreateCookiePrincipal( + new Claim(AbpClaimTypes.UserId, "user-1"), + new Claim(AbpClaimTypes.ClientId, "MyClient")) + }; + + await RefreshPrincipalAsync(context); + + context.NewPrincipal.FindAll(AbpClaimTypes.ClientId).ShouldBeEmpty(); + context.NewPrincipal.FindFirst(AbpClaimTypes.UserId).Value.ShouldBe("user-1"); + } + + [Fact] + public async Task Should_Not_Touch_A_Principal_Without_ClientId() + { + var context = new SecurityStampRefreshingPrincipalContext + { + NewPrincipal = CreateCookiePrincipal(new Claim(AbpClaimTypes.UserId, "user-1")) + }; + + await RefreshPrincipalAsync(context); + + context.NewPrincipal.Claims.Count().ShouldBe(1); + context.NewPrincipal.FindFirst(AbpClaimTypes.UserId).Value.ShouldBe("user-1"); + } + + [Fact] + public async Task Should_Remove_Every_ClientId_Claim_From_Every_Identity() + { + var principal = new ClaimsPrincipal(); + principal.AddIdentity(new ClaimsIdentity( + new[] { new Claim(AbpClaimTypes.UserId, "user-1"), new Claim(AbpClaimTypes.ClientId, "Client-A") }, + IdentityConstants.ApplicationScheme)); + principal.AddIdentity(new ClaimsIdentity( + new[] { new Claim(AbpClaimTypes.ClientId, "Client-B"), new Claim(AbpClaimTypes.ClientId, "Client-C") }, + IdentityConstants.ApplicationScheme)); + + var context = new SecurityStampRefreshingPrincipalContext { NewPrincipal = principal }; + + await RefreshPrincipalAsync(context); + + context.NewPrincipal.FindAll(AbpClaimTypes.ClientId).ShouldBeEmpty(); + context.NewPrincipal.FindFirst(AbpClaimTypes.UserId).Value.ShouldBe("user-1"); + } + + [Fact] + public async Task Should_Not_Throw_When_New_Principal_Is_Null() + { + var context = new SecurityStampRefreshingPrincipalContext { NewPrincipal = null }; + + await Should.NotThrowAsync(() => RefreshPrincipalAsync(context)); + } + + [Fact] + public async Task Should_Remove_ClientId_After_Running_The_Previously_Registered_Callback() + { + // The real module order: ABP Identity's callback is registered first, the removal after it. Identity's + // SecurityStampValidatorCallback.UpdatePrincipal copies client_id from an already-corrupted cookie onto + // the refreshed principal; the removal still strips it, so the cookie self-heals on its next refresh. + var previousCallbackRan = false; + Task PreviousCallback(SecurityStampRefreshingPrincipalContext context) + { + previousCallbackRan = true; + CopyClientIdForward(context); + return Task.CompletedTask; + } + + var refreshingContext = CreateCorruptedCookieRefreshContext(); + + await RefreshPrincipalAsync(refreshingContext, PreviousCallback); + + previousCallbackRan.ShouldBeTrue(); + refreshingContext.NewPrincipal.FindAll(AbpClaimTypes.ClientId).ShouldBeEmpty(); + } + + [Fact] + public async Task Should_Remove_ClientId_When_A_Callback_Is_Registered_After_It() + { + // The reverse order: the removal is registered first and an Identity-style callback (which runs its own + // copy-forward before invoking the previous callback) is registered after it. Because the two wrappers + // chain in opposite directions, the removal still runs last, so the order the modules load does not matter. + var options = new SecurityStampValidatorOptions(); + options.RemoveClientIdClaim(); + + var previousOnRefreshingPrincipal = options.OnRefreshingPrincipal; + options.OnRefreshingPrincipal = async context => + { + CopyClientIdForward(context); + if (previousOnRefreshingPrincipal != null) + { + await previousOnRefreshingPrincipal.Invoke(context); + } + }; + + var refreshingContext = CreateCorruptedCookieRefreshContext(); + + await options.OnRefreshingPrincipal(refreshingContext); + + refreshingContext.NewPrincipal.FindAll(AbpClaimTypes.ClientId).ShouldBeEmpty(); + } + + private static async Task RefreshPrincipalAsync( + SecurityStampRefreshingPrincipalContext context, + Func previousCallback = null) + { + var options = new SecurityStampValidatorOptions { OnRefreshingPrincipal = previousCallback }; + + options.RemoveClientIdClaim().ShouldBeSameAs(options); + + await options.OnRefreshingPrincipal(context); + } + + private static void CopyClientIdForward(SecurityStampRefreshingPrincipalContext context) + { + var clientId = context.CurrentPrincipal.FindFirst(AbpClaimTypes.ClientId); + if (clientId != null) + { + context.NewPrincipal.Identities.First().AddClaim(clientId); + } + } + + private static SecurityStampRefreshingPrincipalContext CreateCorruptedCookieRefreshContext() + { + return new SecurityStampRefreshingPrincipalContext + { + CurrentPrincipal = CreateCookiePrincipal( + new Claim(AbpClaimTypes.UserId, "user-1"), + new Claim(AbpClaimTypes.ClientId, "MyClient")), + NewPrincipal = CreateCookiePrincipal(new Claim(AbpClaimTypes.UserId, "user-1")) + }; + } + + private static ClaimsPrincipal CreateCookiePrincipal(params Claim[] claims) + { + return new ClaimsPrincipal(new ClaimsIdentity(claims, IdentityConstants.ApplicationScheme)); + } +}