Add ClaimActions for bulk add and remove #1636

Tratcher · 2018-02-02T19:01:11Z

#1609 @leastprivilege @brockallen how does this look? It covers your asks in three parts:

Clear the JWT claims type map.
Map all claims
bulk delete protocol claims

HaoK · 2018-02-02T19:48:42Z

src/Microsoft.AspNetCore.Authentication.OAuth/Claims/ClaimActionCollectionMapExtensions.cs

+        /// Maps all values from the json user data as claims, excluding duplicates.
+        /// </summary>
+        /// <param name="collection"></param>
+        public static void MapAllClaims(this ClaimActionCollection collection)


Shorten to MapAll to be consistent with all the other methods that don't have Claims suffix?

HaoK · 2018-02-02T19:49:07Z

src/Microsoft.AspNetCore.Authentication.OAuth/Claims/ClaimActionCollectionMapExtensions.cs

+        /// </summary>
+        /// <param name="collection"></param>
+        /// <param name="claimTypes"></param>
+        public static void DeleteClaims(this ClaimActionCollection collection, params string[] claimTypes)


People have been very confused by Remove vs Delete. We need to avoid aggravating that.

you could also just change DeleteClaim to take a params string[] right?

In theory, but it would be binary breaking.

HaoK · 2018-02-02T19:57:14Z

src/Microsoft.AspNetCore.Authentication.OAuth/Claims/MapAllClaimsAction.cs

+                // Avoid adding a claim if there's a duplicate name and value. This often happens in OIDC when claims are
+                // retrieved both from the id_token and from the user-info endpoint.
+                var duplicate = false;
+                foreach (var existingClaim in identity.FindAll(pair.Key))


Nit: can you something like this?

var duplicate = identity.FindFirst(c => c.Key == pair.Key && string.Equals(c.Value, claimValue, StringComparison.Ordinal) != null

HaoK · 2018-02-02T19:58:50Z

src/Microsoft.AspNetCore.Authentication.OAuth/Claims/ClaimActionCollectionMapExtensions.cs

+
+            foreach (var claimType in claimTypes)
+            {
+                collection.Add(new DeleteClaimAction(claimType));


Consider enhancing DeleteClaimAction type to take a list of claims so you don't have to add one action per type?

Performance wise it's the same, you still need to loop through all of the names.

Adding them individually preserves the functionality of methods like ClaimActionsCollection.Remove(claimType) that removes the action by name.

Pretty sure that's exactly the behavior that's confusing today.

options.ClaimActions.Remove("something") you would think is the same as options.ClaimActions.DeleteClaim("something").

I think this api should be mostly focusing on making it easy to do the common things.

MapJsonKey
MapAll
Delete

The list of delegate/actions is fine as an implementation detail, but I really don't think generally developers should have to understand/deal with that... when all they want to do is include/ignore some set of claims. Only someone who is trying to plug in a complex mapping should have to understand the whole claim action stuff

leastprivilege · 2018-03-08T06:58:03Z

Clear the JWT claims type map.

Maybe I am missing something - but this is only done in the sample. Not in the handler...I want this to be turned off by default. Your default mapping can do the conversion from standard to propriety if that is a requirement.

Map all claims
bulk delete protocol claims

Can't we make that one operation like

MapAll(params string[] filterList)

Tratcher · 2018-03-08T20:20:25Z

samples/OpenIdConnectSample/Startup.cs

+                o.ClaimActions.Clear();
+                o.ClaimActions.MapAllClaims();
+                // Note some of these protocol claims also show up in the user info
+                o.ClaimActions.DeleteClaims("aud", "iss", "iat", "nbf", "exp", "aio", "c_hash", "uti", "nonce");


RemoveProtocolClaims
MapAllUserInfo

leastprivilege · 2018-03-09T04:57:45Z

just to be really clear

this

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

is not really an improvement over just turning of the inbound claims mapping globally via their static DefaultInboundClaimTypeMap.

I would like to see no claims mapping at all.

Tratcher · 2018-03-16T22:32:19Z

@leastprivilege @brockallen Updated. MapAllExcept combines both operations like you asked.

brockallen · 2018-03-17T01:20:18Z

Thanks. Will investigate.

leastprivilege · 2018-03-17T09:17:48Z

..and what about the claim type mappings - will this be turned off at the handler level by default?

Tratcher · 2018-03-17T14:25:13Z

That's a breaking change, I can't do that right now.

Tratcher · 2018-03-20T19:49:22Z

@leastprivilege @brockallen please review this week or we'll need to close this PR.

brockallen · 2018-03-20T20:15:42Z

I have a VM with preview-1. I assume I can just pull the source for this and test that.

leastprivilege · 2018-03-20T20:36:25Z

That's a breaking change, I can't do that right now.

Two things you can do:

Do the mapping of standard to proprietary claims in the default claim actions
Allow setting/clearing the inbound claim type map directly on the OIDC handler options

Tratcher · 2018-03-20T20:42:10Z

@brockallen the code should even work against 2.0 RTM.

Tratcher · 2018-03-20T21:39:18Z

@leastprivilege

Do the mapping of standard to proprietary claims in the default claim actions

I don't follow. Can you give an example?

Allow setting/clearing the inbound claim type map directly on the OIDC handler options

So the code you want to simplify is:

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

Down to something like o.InboundClaimTypeMap.Clear();? That might be doable but it would rely on downcasting ISecurityTokenValidator SecurityTokenValidator to it's default type JwtSecurityTokenHandler.

brockallen · 2018-03-21T01:16:33Z

@brockallen the code should even work against 2.0 RTM.

Even easier. I'll do it first thing tomorrow.

leastprivilege · 2018-03-21T06:29:53Z

I don't follow. Can you give an example?

Well - you could have a MakeYourClaimsWorse (naming is up to you ;)) option. And when it is set - whenever you run a claim action, you use the dictionary provided by the JwtSecurityTokenHandler to do a mapping if present. If not you use the raw claim types.

This way the mapping behavior would be an OIDC handler thing...

brockallen · 2018-03-21T17:26:38Z

I was unable to get this branch working on either 2.0 or 2.1-preview-1. Mainly build issues on the Security repo.

Tratcher · 2018-03-21T17:29:44Z

@brockallen no need to build this repo, you should be able to copy the MapAllClaimsAction class directly into your app. You could also copy the new extension methods if you like.

brockallen · 2018-03-21T17:55:52Z

src/Microsoft.AspNetCore.Authentication.OAuth/Claims/MapAllClaimsAction.cs

+
+        public override void Run(JObject userData, ClaimsIdentity identity, string issuer)
+        {
+            foreach (var pair in userData)


When I use the new MapAll I get a null reference exception on userData. Implicit flow only in my client, BTW: ResponseType="id_token".

brockallen · 2018-03-21T18:59:45Z

I think all these new helpers should all call Clear on the ClaimsActions. For example:

options.ClaimActions.MapAllExcept("aud", "iss", "iat", "nbf", "exp", "aio", "c_hash", "uti", "nonce")

Still doesn't allow amr through. I had to call:

options.ClaimActions.Clear();
options.ClaimActions.MapAllExcept("aud", "iss", "iat", "nbf", "exp", "aio", "c_hash", "uti", "nonce");

to get amr, and so then it makes the MapAllExcept name not make much sense.

Same goes for MapAll -- all by itself it doesn't really do much.

brockallen · 2018-03-21T19:00:36Z

And just to repeat since I don't know if you saw it:

When I use the new MapAll I get a null reference exception on userData. This fails when I have an implicit flow only client, meaning ResponseType="id_token".

Actually, any of these new helpers fail with the same null ref exception for "id_token" only client.

Tratcher · 2018-03-21T21:44:18Z

Updated

brockallen · 2018-03-21T22:22:17Z

Cool, thanks. I think with those changes, this now becomes a nice one-liner to allow controlling which claims are kept/removed. There's of course the claim type map issue, but that's another topic/thread I assume?

Tratcher · 2018-03-21T22:25:53Z

Agreed, the type map is orthogonal.

Tratcher · 2018-03-21T22:32:42Z

@HaoK please sanity check

HaoK · 2018-03-21T22:35:58Z

src/Microsoft.AspNetCore.Authentication.OAuth/Claims/ClaimActionCollectionMapExtensions.cs

+        public static void MapAllExcept(this ClaimActionCollection collection, params string[] exclusions)
+        {
+            collection.Clear();
+            collection.Add(new MapAllClaimsAction());


This should call MapAll

Tratcher · 2018-03-21T23:18:02Z

@leastprivilege second guessing the mappings is problematic, you're better off clearing JWT's mappings. I'm going to merge this PR as your suggestions along that line are independent. I'll file a new issue to simplify clearing the JWT type map.

Tratcher self-assigned this Feb 2, 2018

HaoK reviewed Feb 2, 2018

View reviewed changes

Tratcher commented Mar 8, 2018

View reviewed changes

Tratcher added this to the 2.1.0-preview2 milestone Mar 13, 2018

Tratcher changed the base branch from dev to release/2.1 March 16, 2018 19:16

Tratcher force-pushed the tratcher/allclaims branch from b6a8979 to b6e905d Compare March 16, 2018 22:30

Tratcher changed the title ~~[WIP] Add ClaimActions for bulk add and remove~~ Add ClaimActions for bulk add and remove Mar 16, 2018

brockallen reviewed Mar 21, 2018

View reviewed changes

Tratcher force-pushed the tratcher/allclaims branch from b6e905d to c02d783 Compare March 21, 2018 21:43

HaoK reviewed Mar 21, 2018

View reviewed changes

HaoK approved these changes Mar 21, 2018

View reviewed changes

Add ClaimActions for bulk add and remove. #1609

90064ce

Tratcher force-pushed the tratcher/allclaims branch from c02d783 to 90064ce Compare March 21, 2018 23:36

Tratcher merged commit 90064ce into release/2.1 Mar 22, 2018

Tratcher deleted the tratcher/allclaims branch March 22, 2018 16:37

Tratcher mentioned this pull request Dec 13, 2018

Simplify clearing the JWT claims type mappings dotnet/aspnetcore#4660

Closed

Add ClaimActions for bulk add and remove #1636

Add ClaimActions for bulk add and remove #1636

Uh oh!

Conversation

Tratcher commented Feb 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leastprivilege commented Mar 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leastprivilege commented Mar 9, 2018

Uh oh!

Tratcher commented Mar 16, 2018

Uh oh!

brockallen commented Mar 17, 2018

Uh oh!

leastprivilege commented Mar 17, 2018

Uh oh!

Tratcher commented Mar 17, 2018

Uh oh!

Tratcher commented Mar 20, 2018

Uh oh!

brockallen commented Mar 20, 2018

Uh oh!

leastprivilege commented Mar 20, 2018

Uh oh!

Tratcher commented Mar 20, 2018

Uh oh!

Tratcher commented Mar 20, 2018

Uh oh!

brockallen commented Mar 21, 2018

Uh oh!

leastprivilege commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brockallen commented Mar 21, 2018

Uh oh!

Tratcher commented Mar 21, 2018

Uh oh!

brockallen Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brockallen commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brockallen commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tratcher commented Mar 21, 2018

Uh oh!

brockallen commented Mar 21, 2018

Uh oh!

Tratcher commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tratcher commented Mar 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leastprivilege commented Mar 21, 2018 •

edited

Loading

brockallen Mar 21, 2018 •

edited

Loading

brockallen commented Mar 21, 2018 •

edited

Loading

brockallen commented Mar 21, 2018 •

edited

Loading

Tratcher commented Mar 21, 2018 •

edited

Loading