IClaimsTransformation do not automagically compose

[arontsang]

Is there an existing issue for this?

• [X] I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

Multiple registrations on IClaimsTransformation should compose. For pete's sake, the signature for IClaimsTransformation is literally an async reduce function.

Describe the solution you'd like

A ReducerClaimsTransformer should be injected into AuthenticationService.Transform

public class ReducerClaimsTransformation : IClaimsTransformation
{
    public async Task<ClaimsPrincipal> TransformAsync(ClaimsPrincipal principal)
    {
        foreach (var transformation in _transformations)
        {
            principal = await transformation.TransformAsync(principal);
        }

        return principal;
    }
}

Additional context

No response

[pinkfloydx33]

I remember being surprised that you could only register one IClaimsTransformation. At the time (a few years ago) we had multiple distinct use-cases for the transformation and had to combine it all into one. Not useful either if the transformer is provided by third parties (though no idea how common that is).

We no longer need the transformation as our entire Auth flow has changed, just throwing out my 2cp that at the very least it was surprising and hard to reason about without looking at the source that you could only have one.

[davidfowl]

I think this is a reasonable change to make. @HaoK do you see any problems with this? It is a breaking change that we should document though (if we do make it).

[arontsang]

@pinkfloydx33 You could have raised this issue back then...

[HaoK]

The breaking change aspect is probably the biggest deal, but since we don't do any claims transformation by default, this is just providing new behavior when multiple IClaimsTransformations have been registered. That said, given that historically claims transformation is not a side effect free operation, and in the past we've had a lot of historical issues merging ClaimsPrincipals with multiple auth schemes, and this scenario already has a straight forward workaround, how about we keep this issue open in the backlog and see how much desire there is for this behavior before making any decisions?

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[mkArtakMSFT]

Hi. Thanks for contacting us. We're closing this issue as there was not much community interest in this ask for quite a while now. You can learn more about our triage process and how we handle issues by reading our Triage Process writeup.

[sliekens]

@mkArtakMSFT I vote to reopen. After working extensively with IAuthorizationHandler, where multiple registrations are the intended design, I was really confused why only one of my IClaimsTransformation was activated. (In my mind, they are closely related because one is used by IAuthorizationService, the other by IAuthenticationService, so they should have similar behavior.) I also agree with the OP that the reducer-like signature (mis)leads you to believe that multiple registrations are supported.

It's not clear to me how I can add ReducerClaimsTransformation as shown in the first post to my service collection, without creating a circular dependency.

Currently, the last registered transformer is used, which could also lead to programming errors, since you might not even realize that adding a new transformer will also render any previous transformer ineffective.

What you see:

builder.Services.AddTransient<IClaimsTransformation, AuthScheme1Transformer>();
+builder.Services.AddTransient<IClaimsTransformation, AuthScheme2Transformer>();

What you get:

-builder.Services.AddTransient<IClaimsTransformation, AuthScheme1Transformer>();
+builder.Services.AddTransient<IClaimsTransformation, AuthScheme2Transformer>();

That is to say, I support making this breaking change, because supporting multiple registrations will eliminate an entire category of programming mistakes in a security-sensitive context.

Lastly, I would not let this depend on community interest. This is one of those problems that don't bite often, but do bite hard.

[SHAGGAR]

Not sure why this was closed without a fix or alternative. The current design is real bad.

[voroninp]

+1 for the feature.

[voroninp]

I've recently filed an issue to EF Core: If same migrations history table is reused between different db contexts (default behavior) then List Applied Migrations shows all migrations and not only those relevant for particular db context. Funny enough, you will face it immediately if you develop a modular monolith.

Same with the current issue. I started with the docs looking for the mechanism to extend authenticated ClaimsPrincipal with the custom claims. — Oh, nice, they have IClaimsTransformation. Wait, but what if my modules need their custom transofrmation. Let's see the source of AuthenticationService. Hm... only one instance of ``IClaimsTransformation`. Not good. Let's look for an Issue on github.

And here I am...

It took me 5 minutes to discover a missing feature. Ok, I'll probably use Scrutor.

But do you know what this means?

there was not much community interest in this ask for quite a while now

This effectively means that all those advertisers of best practices, modular monoliths and similar stuff are effectively...bullshitters because they never tried to do it from start to end. Otherwise, there would be an interest in this feature.

P.S. ofc, isolation of transformer in one module from the one in another module is what really needed here, but anyway.

[sliekens]

RE:

The breaking change aspect is probably the biggest deal

At this point I would probably introduce a new interface and mark the old one Obsolete. That should take care of backwards compatibility.

+[Obsolete("IClaimsTransformation does not support multiple registrations")]
interface IClaimsTransformation
{
    Task<ClaimsPrincipal> TransformAsync (ClaimsPrincipal principal);
}

+interface IClaimsReducer
+{
+    Task<ClaimsPrincipal> TransformAsync (ClaimsPrincipal principal);
+}

I don't like that this was closed for lack of community interest. Most applications don't need claims transformation, of course this won't get thousands of views. It's also not an obvious problem, there is no error when you register multiple implementations. I suspect many applications have multiple implementations of the interface because the developer doesn't even realize that it doesn't actually work. Someone please reopen this and re-triage.

[voroninp]

You are mean, gentlemen =)

Tried to decorate it with Scrutor, but did not expect it's registered as a singleton.

[steverukuts]

I think this could be resolved in a non-breaking way.

A no-op claims transformer is already defined, so this could be replaced with this implementation:

public class ClaimsTransformationReducer(IEnumerable<IClaimsSubTransformation> subTransformations) : IClaimsTransformation
{
    public async Task<ClaimsPrincipal> TransformAsync(ClaimsPrincipal principal)
    {
        foreach (var transformation in subTransformations)
            principal = await transformation.TransformAsync(principal);

        return principal;
    }
}

This is effectively a no-op transformer when no IClaimsSubTransformations are defined. The interface is defined as so - the same as the IClaimsTransformation interface:

public interface IClaimsSubTransformation
{
    public Task<ClaimsPrincipal> TransformAsync(ClaimsPrincipal principal);
}

Anyone who would like the new behaviour can simply replace all IClaimsTransformation implementations with this same interface, and any apps using a single IClaimsTransformation would continue to work as normal.

Note: I am not sure about the IClaimsSubTransformation name so perhaps someone could iterate on that. Aside from that I'm happy with the result and it's largely transparent to my app, which has multiple authentication pathways, handlers and so on.

For the record, I also arrived at this issue because I was confused about why multiple IClaimsTransformation registrations were not working, as the API design did seem to suggest to me that this would work. I appreciate that this is a niche concern but it would be nice to not have to repeat this pattern, and it feels like it wouldn't be too hard to solve it.

[robertcoltheart]

Also greatly surprised that this wasn't supported. Registering my interest for the ability to process multiple claims transformations.