-
Notifications
You must be signed in to change notification settings - Fork 588
Add ClaimActions for bulk add and remove #1636
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,6 +87,27 @@ public static void MapCustomJson(this ClaimActionCollection collection, string c | |
collection.Add(new CustomJsonClaimAction(claimType, valueType, resolver)); | ||
} | ||
|
||
/// <summary> | ||
/// Clears any current ClaimsActions and maps all values from the json user data as claims, excluding duplicates. | ||
/// </summary> | ||
/// <param name="collection"></param> | ||
public static void MapAll(this ClaimActionCollection collection) | ||
{ | ||
collection.Clear(); | ||
collection.Add(new MapAllClaimsAction()); | ||
} | ||
|
||
/// <summary> | ||
/// Clears any current ClaimsActions and maps all values from the json user data as claims, excluding the specified types. | ||
/// </summary> | ||
/// <param name="collection"></param> | ||
/// <param name="exclusions"></param> | ||
public static void MapAllExcept(this ClaimActionCollection collection, params string[] exclusions) | ||
{ | ||
collection.MapAll(); | ||
collection.DeleteClaims(exclusions); | ||
} | ||
|
||
/// <summary> | ||
/// Delete all claims from the given ClaimsIdentity with the given ClaimType. | ||
/// </summary> | ||
|
@@ -96,5 +117,23 @@ public static void DeleteClaim(this ClaimActionCollection collection, string cla | |
{ | ||
collection.Add(new DeleteClaimAction(claimType)); | ||
} | ||
|
||
/// <summary> | ||
/// Delete all claims from the ClaimsIdentity with the given claimTypes. | ||
/// </summary> | ||
/// <param name="collection"></param> | ||
/// <param name="claimTypes"></param> | ||
public static void DeleteClaims(this ClaimActionCollection collection, params string[] claimTypes) | ||
{ | ||
if (claimTypes == null) | ||
{ | ||
throw new ArgumentNullException(nameof(claimTypes)); | ||
} | ||
|
||
foreach (var claimType in claimTypes) | ||
{ | ||
collection.Add(new DeleteClaimAction(claimType)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider enhancing DeleteClaimAction type to take a list of claims so you don't have to add one action per type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Security.Claims; | ||
using Newtonsoft.Json.Linq; | ||
|
||
namespace Microsoft.AspNetCore.Authentication.OAuth.Claims | ||
{ | ||
/// <summary> | ||
/// A ClaimAction that selects all top level values from the json user data and adds them as Claims. | ||
/// This excludes duplicate sets of names and values. | ||
/// </summary> | ||
public class MapAllClaimsAction : ClaimAction | ||
{ | ||
public MapAllClaimsAction() : base("All", ClaimValueTypes.String) | ||
{ | ||
} | ||
|
||
public override void Run(JObject userData, ClaimsIdentity identity, string issuer) | ||
{ | ||
if (userData == null) | ||
{ | ||
return; | ||
} | ||
foreach (var pair in userData) | ||
{ | ||
var claimValue = userData.TryGetValue(pair.Key, out var value) ? value.ToString() : null; | ||
|
||
// 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 = identity.FindFirst(c => string.Equals(c.Type, pair.Key, StringComparison.OrdinalIgnoreCase) | ||
&& string.Equals(c.Value, claimValue, StringComparison.Ordinal)) != null; | ||
|
||
if (!duplicate) | ||
{ | ||
identity.AddClaim(new Claim(pair.Key, claimValue, ClaimValueTypes.String, issuer)); | ||
} | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People have been very confused by Remove vs Delete. We need to avoid aggravating that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could also just change DeleteClaim to take a params string[] right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, but it would be binary breaking.