New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify clearing the JWT claims type mappings #4660

Closed
Tratcher opened this Issue Mar 21, 2018 · 14 comments

Comments

Projects
None yet
7 participants
@Tratcher
Copy link
Member

Tratcher commented Mar 21, 2018

aspnet/Security#1636 (comment)

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

This comment has been minimized.

Copy link

brockallen commented Mar 22, 2018

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

This comment has been minimized.

Copy link

brentschmaltz commented Mar 22, 2018

@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:
AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#550

@Eilon

This comment has been minimized.

Copy link
Member

Eilon commented Mar 29, 2018

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

@brockallen

This comment has been minimized.

Copy link

brockallen commented Mar 29, 2018

What's the timeframe for that, @brentschmaltz?

@brentschmaltz

This comment has been minimized.

Copy link

brentschmaltz commented Mar 31, 2018

@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

This comment has been minimized.

Copy link

leastprivilege commented Apr 1, 2018

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

@brentschmaltz

This comment has been minimized.

Copy link

brentschmaltz commented Apr 3, 2018

@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

This comment has been minimized.

Copy link

leastprivilege commented Apr 3, 2018

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

This comment has been minimized.

Copy link

brentschmaltz commented Apr 3, 2018

@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

This comment has been minimized.

Copy link
Member

Eilon commented Apr 6, 2018

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

@brentschmaltz

This comment has been minimized.

Copy link

brentschmaltz commented Apr 25, 2018

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

@aspnet-hello aspnet-hello transferred this issue from aspnet/Security Dec 13, 2018

@aspnet-hello aspnet-hello added this to the 3.0.0-preview2 milestone Dec 13, 2018

@Eilon

This comment has been minimized.

Copy link
Member

Eilon commented Dec 13, 2018

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.

@Eilon Eilon closed this Dec 13, 2018

@carusology

This comment has been minimized.

Copy link

carusology commented Dec 21, 2018

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

This comment has been minimized.

Copy link

leastprivilege commented Dec 22, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment