Make claim mapping more configurable #1124

Tratcher · 2017-02-15T00:30:59Z

Thoughts?

dnfclas · 2017-02-15T00:31:02Z

Hi @Tratcher, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

kevinchalet · 2017-02-15T11:31:30Z

Thoughts?

Not sure I like it, but I can't think of a better approach, so...

Tratcher · 2017-02-15T17:32:29Z

I was looking at how this would apply to OIDC and realized that right now all possible data from the user-info is being added as claims. No wonder OIDC has cookie size issues. Would it make sense to replace that system with this new explicit mapping? It would likely require a little more setup for devs, and I don't know if there are any default mappings it would make sense to include.

IdentityModel already includes all of the claims from the JWTs, right?

/cc: @brentschmaltz

HaoK · 2017-02-15T18:47:56Z

Not in love with the resolver name, I'll chew on this a bit

HaoK · 2017-02-15T18:55:31Z

src/Microsoft.AspNetCore.Authentication.Google/GoogleOptions.cs

+            ClaimResolvers.Add(ClaimTypes.NameIdentifier, "id");
+            ClaimResolvers.Add(ClaimTypes.Name, "displayName");
+            ClaimResolvers.AddNested(ClaimTypes.GivenName, "name", "givenName");
+            ClaimResolvers.AddNested(ClaimTypes.Surname, "name", "familyName");


What does nested do exactly?

Just brainstorming some different ways to express this:

JObjectTransform.Map(ClaimTypes.NameIdentifier, "id"); JObjectTransform.Map(ClaimTypes.Name, "displayName"); JObjectTransform.MapNested(ClaimTypes.GivenName, "name", "givenName"); JObjectTransform.MapNeste(ClaimTypes.Surname, "name", "familyName");

JsonClaimsMap.Map[Nested]

ClaimsResolver.Map

HaoK · 2017-02-15T19:00:14Z

Yeah so my initial instinct is that this feels pretty heavy, but maybe if we just tweak a few names it won't feel so complicated.

kevinchalet · 2017-02-15T19:03:28Z

IdentityModel already includes all of the claims from the JWTs, right?

Yep.

Related ticket: #1024

HaoK · 2017-02-15T19:10:32Z

src/Microsoft.AspNetCore.Authentication/ClaimResolver.cs

+
+namespace Microsoft.AspNetCore.Authentication
+{
+    public abstract class ClaimResolver<T>


Is this going to be used outside of OAuth? Should we target this against OAuth/JObject for now?

Trying to see if it applies to Twitter, OIDC, etc.. We'll see.

HaoK · 2017-02-15T19:16:29Z

src/Microsoft.AspNetCore.Authentication.MicrosoftAccount/MicrosoftAccountOptions.cs

-            // Last one wins, if present
-            ClaimResolvers.Add(ClaimTypes.Email, "userPrincipalName");
-            ClaimResolvers.Add(ClaimTypes.Email, "mail");
+            ClaimResolvers.AddCustom(ClaimTypes.Email, user => user.Value<string>("mail") ?? user.Value<string>("userPrincipalName"));


Honestly I almost prefer if we didn't have the sugar and only exposed the lambda syntax to be clear, its super obvious what's going on that way and we can expose helpers as needed to make the nested stuff nicer

Really? I thought the new version was supper clean and easy. It sounds like you want some more explicit names though.

Well for code like this I'd prefer if it was self describing, i.e. if it was JsonPayloadClaimsMap.Add("ClaimsTypes.Email", "userPrincipalName) that has much more context

brentschmaltz · 2017-02-15T19:28:29Z

@Tratcher @HaoK @PinpointTownes I am not clear on what problem are we trying to solve. Pinpoint referred us to #1024 . Which combined with AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#550 would reduce the size of cookies. I am not a fan of mapping claims in the JWT as the claim types have strong definitions https://tools.ietf.org/html/rfc7519#section-4.1

Tratcher · 2017-02-15T19:35:55Z

@brentschmaltz see #969. We often get requests to map additional user data to claims during auth. Today if you want to add any you have to hook the events do everything yourself. This is too much for basic users and they ask to add it for them. We don't want to maintain an exhaustive list, especially since they keep changing. This PR is to make it easier for devs to add/change claim mappings themselves. It makes our own implementation cleaner too.

HaoK · 2017-02-15T19:58:38Z

My first suggestion for naming is: Resolver => Mapper which gets you the additional Apply => Map. I'd suggest Transform but that will collide with the real ClaimsTransformation feature... unless we call it PayloadClaimsTransform :)

HaoK · 2017-02-15T19:59:50Z

ClaimMappings or PayloadClaimMappings also would read well with the current collection/AddXyz methods.

brentschmaltz · 2017-02-15T20:09:04Z

@Tratcher perhaps I am misunderstanding, what will the ClaimReslover do? Add claims OR transform?

Tratcher · 2017-02-15T20:12:47Z

@brentschmaltz it adds claims from the User Info

Tratcher · 2017-02-15T20:40:04Z

Renamed. The funny thing is that I'd started with Map.

HaoK · 2017-02-15T20:44:58Z

samples/SocialSample/Startup.cs

                    }
                }
-            });
+            };
+            githubOptions.ClaimMaps.AddJsonKeyMap(ClaimTypes.NameIdentifier, "id");


Since the property is ClaimsMaps, can maybe simplify to AddJsonKey?

MapFromJsonKey? MapFromJsonSubKey, MapFromCustomJson...?

I don't think the From adds anything does it? Map[Custom]Json[Sub]Key works for me

HaoK · 2017-02-15T20:45:46Z

samples/SocialSample/Startup.cs

+            githubOptions.ClaimMaps.AddJsonKeyMap(ClaimTypes.NameIdentifier, "id");
+            githubOptions.ClaimMaps.AddJsonKeyMap(ClaimTypes.Name, "login");
+            githubOptions.ClaimMaps.AddJsonKeyMap("urn:github:name", "name");
+            githubOptions.ClaimMaps.AddJsonKeyMap(ClaimTypes.Email, "email", ClaimValueTypes.Email);


Can call this AddJsonSubKey if you want to preserve the different method for subsections if you want?

HaoK · 2017-02-15T20:46:14Z

src/Microsoft.AspNetCore.Authentication.Facebook/FacebookHandler.cs

-            {
-                identity.AddClaim(new Claim("urn:facebook:timezone", timeZone, ClaimValueTypes.String, Options.ClaimsIssuer));
-            }
+            context.ResolveClaims();


HaoK · 2017-02-15T20:47:04Z

src/Microsoft.AspNetCore.Authentication.Facebook/FacebookOptions.cs

@@ -30,6 +32,21 @@ public FacebookOptions()
            Fields.Add("email");
            Fields.Add("first_name");
            Fields.Add("last_name");
+
+            ClaimMaps.AddJsonKeyMap(ClaimTypes.NameIdentifier, "id");
+            ClaimMaps.AddNestedJsonKeyMap("urn:facebook:age_range_min", "age_range", "min");


AddNestedJsonKeyMap => AddJsonSubKey?

I think subkey is more common than nested.

Tratcher · 2017-02-16T00:20:39Z

Drat, to add this to Twitter and OIDC I have to clone all the classes. They all depend on Newtonsoft.Json but don't share any dependencies that require it. The alternative is to make Twitter and OIDC depend on OAuth, which makes sense from a protocol perspective but not really from a packaging perspective.

HaoK · 2017-02-16T00:38:20Z

I figured as much, oh well :(

HaoK · 2017-02-16T00:38:55Z

You could just put the shared classes in base Auth, we've done it before in the past I believe

Tratcher · 2017-02-16T00:50:07Z

Not without adding the JSON dependency to the base class.

HaoK · 2017-02-16T01:17:18Z

Yuck, can we shared source at least?

Tratcher · 2017-02-16T13:13:16Z

Not for public types...

Tratcher · 2017-02-16T20:15:36Z

Changed Twitter to depend on OAuth. Mostly harmless.

I could do the same for OIDC, but there's a larger question on if we want to use this pattern for OIDC. Right now OIDC maps all user data to claims by default. In hindsight this is overkill and you end up with a bunch of junk claims. OIDC also gets claims from the JWT and needs to de-duplicate (with preference given to the JWT claims). I'd need a MapUnique... variant.

OIDC defines some standard claims: http://openid.net/specs/openid-connect-core-1_0.html#StandardClaims
Of those, I think I'd pick out these defaults:

sub, name, given_name, family_name, profile, email

@PinpointTownes, this was your pet feature in OIDC, what do you think about scaling it back?

kevinchalet · 2017-02-17T19:37:53Z

This got my test cookie (an outlook.com account via AAD) down from 5,267 bytes broken into 2 chunks to 3,567 bytes with no chunking.

This sounds really large. How many claims do you have?

I suspect using the WS-Fed claim types here doesn't help.
FWIW, there's an ardent debate in the AzureAD repo about the IdentityModel claims mapping feature: AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#550.

Disabling this feature by default and using the JWT claim types returned by the OP as-is would likely help reduce the size of the authentication cookies, but of course, this decision has its own downsides, as every external middleware will now have its own claim types, depending on the standard they implement.

Tratcher · 2017-02-17T20:52:02Z

I had 21 claims before and 10 after. Yes, the long claim types contribute quite a bit to the size. Using the short oidc terms for ClaimType doesn't help (yet) because the claims coming from the JWT take precedence.

Tratcher · 2017-02-21T22:02:30Z

@HaoK I think this is ready for a formal review.

The only open question is if this also satisfies #1024 around removing some of the OIDC protocol claims that IM adds by default. @PinpointTownes, @brockallen, @leastprivilege, @brentschmaltz, is this a workable solution?
See: https://github.com/aspnet/Security/pull/1124/files#diff-5f153b21e61d008634c479494de719d6

HaoK · 2017-02-22T00:51:57Z

samples/SocialSample/Startup.cs

                    }
                }
-            });
+            };
+            githubOptions.ClaimMaps.MapJsonKey(ClaimTypes.NameIdentifier, "id");


Would we lose any meaning if we called the property Claims since all of the methods are called Map? Just looks a bit odd for it to be ClaimsMaps.Map everywhere?

HaoK · 2017-02-22T01:07:23Z

src/Microsoft.AspNetCore.Authentication.OAuth/DeleteClaimMapper.cs

+    /// <summary>
+    /// A JsonClaimMapper that deletes all claims from the given ClaimsIdentity with the given ClaimType.
+    /// </summary>
+    public class DeleteClaimMapper : JsonClaimMapper


A delete mapper makes me think we should choose a different word.

like ClaimAction/Operation? with Map/Delete becoming something paired?
Copy/Delete or Add/Remove

Claims.CopyJsonKey(ClaimTypes.NameIdentifier, "id"); Claims.Delete("ClaimTypeToDelete");

HaoK · 2017-02-22T01:17:26Z

src/Microsoft.AspNetCore.Authentication.OAuth/JsonClaimMapperCollectionExtensions.cs

+
+namespace Microsoft.AspNetCore.Authentication
+{
+    public static class JsonClaimMapperCollectionExtensions


I'm not sure I like this as extension methods on an IEnumerable.

What do you think about making the public API into a ClaimsManager instead, with similar API? The property is then just named ClaimsManager.

ClaimsManager.CopyJsonKey(ClaimsTypes.NameIdentifier, "id"); ClaimsManager.Delete(ClaimsTypes.ReallyLongStuffIDon'tWant);

You can still have these methods add some kind of strongly typed Action that maps to the Map classes, or directly add an Action with the signature of Map. I don't think the extensiblity changes much as they could still add extension methods to the Manager just like the collection.

What does the IEnumerable have to do with anything? The extension methods are for the JsonClaimMapperCollection and they all wrap the Add method.

Sorry what I meant was instead of a JsonClaimMapperCollection, maybe that should be called something like ClaimsManager which implies that its got more than just basic add/remove

It only has basic AddRemove. I renamed it to ClaimActionCollection and the property is called ClaimActions.
ClaimActions.Map...
ClaimActions.DeleteClaim...

HaoK · 2017-02-22T01:19:47Z

src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectOptions.cs

@@ -55,6 +56,24 @@ public OpenIdConnectOptions(string authenticationScheme)
            Events = new OpenIdConnectEvents();
            Scope.Add("openid");
            Scope.Add("profile");
+
+            ClaimMaps.DeleteClaim("nonce");


Maybe add sugar to pass in a params string[] of claims to delete?

We try to avoid params in public APIs. We'll see how often this gets used externally.

What's the rationale against params? We certainly have them in several places today already for exactly this situation, truncating a lot of duplicated lines, maybe you don't even really need DeleteClaim, just a single params DeleteClaims

When you have params on a method it makes it hard to ever add other inputs to that method.

That's what overloads are for :) I'd have the params method for this usage alone personally...

Tratcher · 2017-02-22T19:52:23Z

More renames :-). ClaimMappers are now ClaimActions, I also made a few changes to make sure that the actions run even when user-info retrieval is disabled (Twitter, OIDC), and that the rules are hardened against null user info.

kevinchalet · 2017-02-22T19:56:55Z

@PinpointTownes, @brockallen, @leastprivilege, @brentschmaltz, is this a workable solution?

I'd bet a lot of money @leastprivilege won't like the fact the JWT claims are magically converted to their WS-Fed equivalent, specially since he'll now have to disable that in two places: in IdentityModel and in ASP.NET Core 😅

kevinchalet · 2017-02-22T19:58:54Z

src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectOptions.cs

+            ClaimActions.DeleteClaim("iat");
+            ClaimActions.DeleteClaim("nbf");
+            ClaimActions.DeleteClaim("exp");
+            ClaimActions.DeleteClaim("c_hash");


You can also add at_hash, auth_time, acr, amr and azp.

Tratcher · 2017-02-22T20:03:09Z

@PinpointTownes what do you mean? I used the short names: https://github.com/aspnet/Security/pull/1124/files#diff-5f153b21e61d008634c479494de719d6R71.

kevinchalet · 2017-02-22T20:08:27Z

@Tratcher ah, my bad, I had not realized you had changed that in the latest commits 😅

Now, the question is: do we want to generalize that to the other middleware?

Also, do we want to have a global claims transformer that would convert all the claim types to a unique standard, so the application code wouldn't have to deal with multiple specs?

Tratcher · 2017-02-22T20:20:51Z

Sounds like the ClaimsTransformation component :-)

HaoK · 2017-02-23T18:25:11Z

src/Microsoft.AspNetCore.Authentication.OpenIdConnect/ClaimActionCollectionUniqueExtensions.cs

+        /// <param name="valueType">The value to use for Claim.ValueType when creating a Claim.</param>
+        public static void MapUniqueJsonKey(this ClaimActionCollection collection, string claimType, string jsonKey, string valueType)
+        {
+            collection.Add(new UniqueJsonKeyClaimAction(claimType, valueType, jsonKey));


So my earlier point about the Collection vs Manager wasn't so much about that name, but that it seems unnecessary to have public types that are only used in the extension method. Why can't we just bake the logic directly into Method instead of having the method and a type (that's just a class around a Action. Cleans up the public surface area, and I don't see much use for the action types outside of the methods. Which lead me to ask why we shouldn't just call it a Manager with the methods, instead of a List of action thingies...

Basically today it looks like there's almost two ways to use the feature (adding a custom Action object to the Collection, or using the sugar Map/Delete methods).

Given that there's already the MapCustom which basically serves the same purpose adding a custom action would, I think we should just eliminate all of the Action classes and only have the Collection API

Yes you could implement all of the rules with MapCustom, but that comes down to a style choice. The classes provide some minor code cleanliness benefits like inherited properties, better stack traces, avoids closures, etc..

Fair enough, but do the classes need to be public? Are you expecting them to be used other than in the explicit methods?

I'd expect some new types to be derived from the ones provided. We could dump them into an infrastructure namespace (e.g. Oath.Claims).

Cool moving them all into their own namespace works

HaoK · 2017-02-23T18:34:10Z

src/Microsoft.AspNetCore.Authentication.MicrosoftAccount/MicrosoftAccountHandler.cs

-            {
-                identity.AddClaim(new Claim(ClaimTypes.Email, email, ClaimValueTypes.String, Options.ClaimsIssuer));
-            }
+            context.MapClaims();


MapClaims => RunActions/InvokeActions/ExecuteActions/FireWhenReady?

InvokeClaimsActions

HaoK · 2017-02-23T18:41:12Z

src/Microsoft.AspNetCore.Authentication.MicrosoftAccount/MicrosoftAccountOptions.cs

@@ -23,6 +25,12 @@ public MicrosoftAccountOptions()
            TokenEndpoint = MicrosoftAccountDefaults.TokenEndpoint;
            UserInformationEndpoint = MicrosoftAccountDefaults.UserInformationEndpoint;
            Scope.Add("https://graph.microsoft.com/user.read");
+
+            ClaimActions.MapJsonKey(ClaimTypes.NameIdentifier, "id");


Now that mapper is gone from everywhere else, maybe Import/Copy is better, since map is kinda vague what we are mapping.

CopyJsonKey
ImportJsonKey

I think Map still works. Map JSON Key to Claim

HaoK · 2017-02-23T18:46:18Z

src/Microsoft.AspNetCore.Authentication.OAuth/ClaimAction.cs

+        /// <param name="userData">The source data to exhamine. This value may be null.</param>
+        /// <param name="identity">The identity to add Claims to.</param>
+        /// <param name="issuer">The value to use for Claim.Issuer when creating a Claim.</param>
+        public abstract void Run(JObject userData, ClaimsIdentity identity, string issuer);


Yeah I think rather than making this something classes implement, this could just be an Action<JObject, ClaimsIdentity, string>Action property, which the extension methods set. So the change could be to move the Run implementations into the extension methods as property setters on the single ClaimsAction class

HaoK · 2017-02-23T18:50:40Z

I like the usage, definitely way better than what we had before.

kevinchalet · 2017-02-28T19:52:12Z

Sounds like the ClaimsTransformation component :-)

Yeah, but it's totally useless OOTB if you don't provide your own handler.

I had more something like a "convert all these JWT/SWT/WS-Fed/custom claims to the ClaimsTypes names I'm used to" mapping table in mind... 😄

#1024 Remove OIDC protocol claims

natemcmaster · 2017-03-01T21:30:12Z

@Tratcher don't forget to update API usage in downstream repos, like MusicStore.

Tratcher added this to the 2.0.0 milestone Feb 15, 2017

Tratcher self-assigned this Feb 15, 2017

dnfclas added the cla-already-signed label Feb 15, 2017

HaoK reviewed Feb 15, 2017

View reviewed changes

Tratcher changed the title ~~[WIP] Make social auth claims mapping more configurable~~ Make claim mapping more configurable Feb 21, 2017

HaoK reviewed Feb 22, 2017

View reviewed changes

kevinchalet reviewed Feb 22, 2017

View reviewed changes

HaoK reviewed Feb 23, 2017

View reviewed changes

Tratcher force-pushed the tratcher/resolvers branch from 59a0018 to d857f49 Compare February 28, 2017 20:43

HaoK approved these changes Mar 1, 2017

View reviewed changes

#969 Make social auth claims mapping more configurable

ad42516

#1024 Remove OIDC protocol claims

Tratcher force-pushed the tratcher/resolvers branch from c2ae26b to ad42516 Compare March 1, 2017 18:56

Tratcher merged commit ad42516 into dev Mar 1, 2017

Tratcher deleted the tratcher/resolvers branch March 1, 2017 19:05

kevinchalet mentioned this pull request Jun 9, 2017

#812 Standardized claims to OpenIdConnectConstants OrchardCMS/OrchardCore#813

Closed

Tratcher mentioned this pull request Sep 26, 2017

OIDC, I cannot add extra claims from userinfo endpoint #1449

Closed

Tratcher mentioned this pull request Jan 9, 2018

Document the claims actions dotnet/AspNetCore.Docs#5106

Closed

Make claim mapping more configurable #1124

Make claim mapping more configurable #1124

Conversation

Tratcher commented Feb 15, 2017

dnfclas commented Feb 15, 2017

kevinchalet commented Feb 15, 2017

Tratcher commented Feb 15, 2017

HaoK commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Feb 15, 2017

kevinchalet commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentschmaltz commented Feb 15, 2017

Tratcher commented Feb 15, 2017

HaoK commented Feb 15, 2017

HaoK commented Feb 15, 2017

brentschmaltz commented Feb 15, 2017

Tratcher commented Feb 15, 2017

Tratcher commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Feb 16, 2017

HaoK commented Feb 16, 2017

HaoK commented Feb 16, 2017

Tratcher commented Feb 16, 2017

HaoK commented Feb 16, 2017

Tratcher commented Feb 16, 2017

Tratcher commented Feb 16, 2017

kevinchalet commented Feb 17, 2017 • edited Loading

Tratcher commented Feb 17, 2017

Tratcher commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Feb 22, 2017

kevinchalet commented Feb 22, 2017

Choose a reason for hiding this comment

Tratcher commented Feb 22, 2017

kevinchalet commented Feb 22, 2017

Tratcher commented Feb 22, 2017

Choose a reason for hiding this comment

HaoK Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Feb 23, 2017

kevinchalet commented Feb 28, 2017 • edited Loading

natemcmaster commented Mar 1, 2017

HaoK Feb 15, 2017 •

edited

Loading

kevinchalet commented Feb 17, 2017 •

edited

Loading

HaoK Feb 23, 2017 •

edited

Loading

kevinchalet commented Feb 28, 2017 •

edited

Loading