Aspnetcore: Simplify clearing the JWT claims type mappings

[Tratcher]

https://github.com/aspnet/Security/pull/1636#issuecomment-374766092

JwtSecurityTokenHandler used by JwtBearer & OIDC maps the standard OIDC claim types to long namespace names to match older protocols like WsFed. We can't disable this by default without affecting existing users. Turning this off manually requires either calling JwtSecurityTokenHandler.DefaultInboundClaimTypeMap.Clear(); at the top of our Startup Class (inside the OIDC options lamda is too late), or doing the following inside your options:

                var handler = new JwtSecurityTokenHandler();
                handler.InboundClaimTypeMap.Clear();
                o.SecurityTokenValidator = handler;

Could this be shortened to a top level option or extension method?

static void UseShortClaimTypes(this OpenIdConnectOptions options)
{
                var handler = new JwtSecurityTokenHandler();
                handler.InboundClaimTypeMap.Clear();
                options.SecurityTokenValidator = handler;
}

@leastprivilege @brockallen

[brockallen]

Could this be shortened to a top level option or extension method?

One can only hope!

UseShortClaimTypes

I prefer UseOidcProtocolClaimsTypesRatherThanWsStarClaimsTypesBecauseDotNetHasConstantsForThoseLegacyClaimsTypes...

or UseOidcProtocolClaimsTypes for short if you prefer :)

or maybe some name that implies that the claim types in the token won't be adulterated.

[brentschmaltz]

@Tratcher @brockallen we are going to add a single switch to turn off mapping in 5.2.2
We wanted to get this into 5.2.0, but it dropped off the radar.

Also, when we add Async, we will have mapping OFF by default, we will address this:
https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/550

[Eilon]

Parking in the RC1 milestone to see what @brentschmaltz and company come up with.

[brockallen]

What's the timeframe for that, @brentschmaltz?

[brentschmaltz]

@brockallen @Tratcher we are targeting a release 5.2.2 in April with a static global OFF switch.
https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/milestone/24

[leastprivilege]

How is that better than clearing the default inbound claim type map?

[brentschmaltz]

@leastprivilege I was proposing the switch would turn off inbound and outbound mapping. We wouldn't have to initialize the dictionaries in the JwtSecurityTokenHandler constructor. I suppose it depends on if you care. The performance gain would be in relation to how may tokenhandlers one creates. Processing wouldn't necessarily be faster, since the bool flag could be the count as we are planning to adjust IdentityModel's code to check for the count and branch either way.

[leastprivilege]

I don't think anyone cares about perf here. Sure a "global switch" is better than clearing dictionaries.

Opting IN instead of OUT would be preferred though.

[brentschmaltz]

@leastprivilege
Sometime simple things can save 1-3%. These make a big deal to large volume services, who regularly squeak when we add %1.
We invested in running a number of perf tests and have several small improvements that will show up.

Opting IN instead of OUT would be preferred though.

I agree, for 5.2.2 we will have the BIG RED SWITCH.
Our Async work will be Opt IN.

[Eilon]

Parking in the 3.0.0 bucket because this would be a breaking change.

[brentschmaltz]

@Tratcher @Eilon we dropped IdentityModel 5.2.2 yesterday that has a the static property JwtSecurityTokenHandler.DefaultMapInboundClaims and the instance MapInboundClaims.

[Eilon]

Closing because we feel this breaking change would lead to many unexpected results in apps that would be difficult to diagnose (unlike, say, an API breaking change, which is super obvious). Considering that there are several workarounds to this, we feel it isn't worth it.

[carusology]

Closing because we feel this breaking change would lead to _many unexpected results in apps that would be difficult to diagnose_ (unlike, say, an API breaking change, which is super obvious).

The problem is _this_ is exactly what is happening today.

The way developers of new ASP.NET Core projects learn about this legacy mapping is to perform internet queries as to why our JWT Claims aren't mapping correctly. If you use the right keywords, you'll find these issues on GitHub and learn about this legacy behavior out of necessity. We see and nod to comments like this.

Now in every single web project we create (which is a lot more now because of serverless solutions) we have to plug in this boilerplate:

static Startup() {
    // By default, Microsoft has some legacy claim mapping that converts
    // standard JWT claims into proprietary ones. This removes those mappings.

    JwtSecurityTokenHandler.DefaultInboundClaimTypeMap.Clear();
    JwtSecurityTokenHandler.DefaultOutboundClaimTypeMap.Clear();
}

I appreciate that you are thinking of how to avoid breaking changes that are not identified at compile time, but the end result of this decision isn't a developer going "well, that's fine." Conversely, every time someone writes claim mapping code for the first time, they're going to spend learning about this, and they're going to say "well, that's frustrating."

[leastprivilege]

The mapping business has to stop. It is an application level concern. It's wrong to do this at the handler level.

[brentschmaltz]

@leastprivilege I agree this is a bucket of confusion. When we created our JsonWebTokenHandler, we do not map. The next step is to provide an integrate path for this handler into asp.net. This is complicated by the removal of Newtonsoft in .Net 3.0.