Compose multiple IClaimsTransformation registrations sequentially#65585
Open
kubaflo wants to merge 2 commits intodotnet:mainfrom
Open
Compose multiple IClaimsTransformation registrations sequentially#65585kubaflo wants to merge 2 commits intodotnet:mainfrom
kubaflo wants to merge 2 commits intodotnet:mainfrom
Conversation
When multiple IClaimsTransformation implementations are registered in DI, they now all execute sequentially in registration order during authentication. Previously, only the last registered transformation was used. Introduces an internal CompositeClaimsTransformation that wraps all registered IClaimsTransformation instances and iterates them in AuthenticateAsync. Fixes dotnet#46120 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Thanks for your PR, @@kubaflo. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the long-standing DI behavior where registering multiple IClaimsTransformation implementations results in only the last registration being used, by composing all registered transformations and running them sequentially.
Changes:
- Add an internal
CompositeClaimsTransformationthat executes allIClaimsTransformations in sequence. - Update
AuthenticationServiceImplto receiveIEnumerable<IClaimsTransformation>and pass a composite to the baseAuthenticationService. - Add tests validating multi-transform composition and registration order.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Http/Authentication.Core/src/CompositeClaimsTransformation.cs | Introduces an internal composite implementation to execute multiple transformations sequentially. |
| src/Http/Authentication.Core/src/AuthenticationServiceImpl.cs | Switches DI constructor dependency from single transform to IEnumerable and wraps with the composite. |
| src/Http/Authentication.Core/test/AuthenticationServiceTests.cs | Adds coverage for composition, ordering, and single-transform compatibility. |
src/Http/Authentication.Core/test/AuthenticationServiceTests.cs
Outdated
Show resolved
Hide resolved
- ClaimsAdder: attach new ClaimsIdentity to principal when Identity is not a ClaimsIdentity (prevents silently lost claims) - AuthenticationMetricsTest: wrap single IClaimsTransformation mock in array to match new IEnumerable<IClaimsTransformation> constructor parameter Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI Summary
🔍 Automated Fix Report
🔍 Pre-Flight — Context & Validation
Issue: #46120 - IClaimsTransformation do not automagically compose
Area: area-auth (
src/Http/Authentication.Core/)PR: None — will create
Key Findings
AuthenticationServicetakes a singleIClaimsTransformationvia DI constructor injectionNoopClaimsTransformationas default singleton viaTryAddSingletonIClaimsTransformationregistrations exist, DI only resolves the LAST one registeredRoot Cause
Standard DI resolves
IClaimsTransformationas a single service. OnlyIEnumerable<IClaimsTransformation>would resolve all. TheAuthenticationServiceconstructor takes a singleIClaimsTransformation, so only the last registration wins.Fix Approach
NoopClaimsTransformationas itself (not asIClaimsTransformation)CompositeClaimsTransformationclass that:IEnumerable<IClaimsTransformation>in constructorTransformAsyncCompositeClaimsTransformationasIClaimsTransformation(scoped, to support DbContext-using transformations)services.AddTransient<IClaimsTransformation, MyTransform>()Test Command
dotnet test src/Http/Authentication.Core/test/Microsoft.AspNetCore.Authentication.Core.Test.csprojFix Candidates
🧪 Test — Bug Reproduction
Test Result: ✅ TESTS CREATED
Test Command:
dotnet test src/Http/Authentication.Core/test/Microsoft.AspNetCore.Authentication.Core.Test.csproj --filter "MultipleClaimsTransformations|SingleClaimsTransformationStillWorks"Tests Created:
src/Http/Authentication.Core/test/AuthenticationServiceTests.csTests Written
MultipleClaimsTransformationsAreComposed— Registers 2 IClaimsTransformation, verifies both run and add claimsMultipleClaimsTransformationsRunInRegistrationOrder— Registers 3, verifies execution order matches registration orderSingleClaimsTransformationStillWorks— Backward compat: single registration still worksConclusion
Tests reproduce the bug: multiple registrations only run the last one (DI resolves single service).
🚦 Gate — Test Verification & Regression
Gate Result: ✅ PASSED
Test Command:
dotnet test src/Http/Authentication.Core/test/Microsoft.AspNetCore.Authentication.Core.Test.csproj --filter "MultipleClaimsTransformations|SingleClaimsTransformationStillWorks"New Tests vs Buggy Code
Regression Check
Conclusion
Tests are properly written and correctly detect the missing composite behavior.
🔧 Fix — Analysis & Comparison (✅ 4 passed, ❌ 1 failed)
Fix Exploration Summary
Total Attempts: 5
Passing Candidates: 5
Selected Fix: Attempt 1 — Internal
CompositeClaimsTransformation+AuthenticationServiceImplmodificationAttempt Results
Cross-Pollination
All 5 approaches converge on the same insight: compose IEnumerable into a single IClaimsTransformation. The variation is WHERE the composition happens.
Exhausted: Yes
Comparison
Recommendation
Attempt 1 is the best: internal
CompositeClaimsTransformationwraps all registeredIClaimsTransformationinstances,AuthenticationServiceImpltakesIEnumerable<IClaimsTransformation>and passes the composite to the baseAuthenticationServiceconstructor. Zero public API changes, minimal code, leverages existing DIIEnumerable<T>resolution.✅ Attempt 0: PASS
Attempt 0 — Baseline (claude-opus-4.6-fast)
Approach: Add IEnumerable constructor overload to AuthenticationService
Changes:
AuthenticationService.cs— Added new constructor acceptingIEnumerable<IClaimsTransformation>, modifiedAuthenticateAsyncto iterate all transforms when enumerable is presentAuthenticationServiceImpl.cs— Changed to useIEnumerable<IClaimsTransformation>constructorPublicAPI.Unshipped.txt— Added new constructor to public API surfaceKey Design Decisions:
IClaimsTransformationconstructor for backward compatibility_transformsenumerable, iterates sequentially inAuthenticateAsyncAuthenticationServiceImpl(the default DI-registered impl) uses the new constructorservices.AddTransient<IClaimsTransformation, MyTransform>()TryAddSingleton<IClaimsTransformation, NoopClaimsTransformation>kept for backward compat📄 Diff
✅ Attempt 1: PASS
Approach: CompositeClaimsTransformation in AuthenticationServiceImpl
Strategy
Create an internal
CompositeClaimsTransformationclass that sequences all registeredIClaimsTransformationinstances, then modifyAuthenticationServiceImpl(not the publicAuthenticationService) to acceptIEnumerable<IClaimsTransformation>and compose them into a single composite before passing to the base class.Key Differences from Attempt 0
AuthenticationService— the public API (Transformproperty, constructor signature) remains unchangedCompositeClaimsTransformationis internalAuthenticationServiceImplnow takesIEnumerable<IClaimsTransformation>and wraps them inCompositeClaimsTransformationTryAddSingleton<IClaimsTransformation, NoopClaimsTransformation>inAuthenticationCoreServiceCollectionExtensionsstays as-is; DI's built-inIEnumerable<T>resolution collects all registrationsFiles Changed
src/Http/Authentication.Core/src/CompositeClaimsTransformation.cs— NEW internal classsrc/Http/Authentication.Core/src/AuthenticationServiceImpl.cs— constructor changed fromIClaimsTransformationtoIEnumerable<IClaimsTransformation>src/Http/Authentication.Core/test/AuthenticationServiceTests.cs— added 3 new testsHow It Works
AuthenticationServiceImpl, it injectsIEnumerable<IClaimsTransformation>which includes all registeredIClaimsTransformationservicesCompositeClaimsTransformation.TransformAsynciterates them sequentiallyIClaimsTransformationregistrations and all will run in registration order📄 Diff
✅ Attempt 2: PASS
Attempt 2: Compose claims transformations via IAuthenticationService factory
Idea
Avoid changing
AuthenticationService/AuthenticationServiceImplconstructors by changing DI registration: registerIAuthenticationServicewith a scoped factory that resolves allIClaimsTransformationinstances (GetServices<IClaimsTransformation>()) and wraps them into a composite that runs sequentially.Why this is different from prior attempts
AuthenticationService).AuthenticationServiceImplconstructor/signature; composition happens only in the DI setup.Expected behavior
IClaimsTransformationare registered, they run in registration order.📄 Diff
⚪ Attempt 3: UNKNOWN
Approach: override AuthenticationServiceImpl.AuthenticateAsync to apply all registered IClaimsTransformation instances in sequence.
Why this is different:
Implementation details:
📄 Diff
⚪ Attempt 4: UNKNOWN
Implement a fix by registering AuthenticationServiceImpl via a factory that uses ActivatorUtilities to inject a CompositeClaimsTransformation. The CompositeClaimsTransformation wraps all registered IClaimsTransformation services.