Serialize and deserialize actor token in claims identity #3219
Conversation
… update also involves serializing logic for Subject Actors
…king aot functionality or back compatibility
… the method we created
…tcase. Testcases need more modifications
…Added new test case to test serialization precedence
… working after fixing a bug in how claims dictionary was processed
… testcase ot now expect he new exception expected.
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
saurabhsathe-ms
changed the title
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements logic to serialize nested actor tokens within the claims identity and introduces a static property to control the maximum allowed nesting depth.
- Added a static property MaxActorChainLength to control recursion depth with appropriate validations.
- Updated token creation logic to handle nested actor tokens from either the Claims dictionary or the Subject.
- Extended API documentation and logging with a new log message for exceeding the maximum actor chain depth.
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Microsoft.IdentityModel.JsonWebTokens/PublicAPI.Unshipped.txt | Added API entries for the new MaxActorChainLength property. |
| src/Microsoft.IdentityModel.JsonWebTokens/LogMessages.cs | Added log message IDX14313 for exceeding maximum actor chain depth. |
| src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.CreateToken.cs | Added logic to serialize nested actor tokens, including new parameters and recursion checks. |
Files not reviewed (1)
- src/Microsoft.IdentityModel.JsonWebTokens/Microsoft.IdentityModel.JsonWebTokens.csproj: Language not supported
src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.CreateToken.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to serialize actor tokens within the claims identity while limiting recursion through a maximum actor chain depth. Key changes include:
- Introducing the MaxActorChainLength static property to control nested actor token depth.
- Updating CreateToken, WriteJwsPayload, and AddSubjectClaims methods to handle actor token serialization.
- Adding a new log message (IDX14313) to indicate when the actor token chain exceeds the allowed maximum depth.
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Microsoft.IdentityModel.JsonWebTokens/PublicAPI.Unshipped.txt | Added getters/setters for MaxActorChainLength in the public API |
| src/Microsoft.IdentityModel.JsonWebTokens/LogMessages.cs | Added new log message for serialization depth errors |
| src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.CreateToken.cs | Updated token creation logic to include actor token serialization |
Files not reviewed (1)
- src/Microsoft.IdentityModel.JsonWebTokens/Microsoft.IdentityModel.JsonWebTokens.csproj: Language not supported
src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.CreateToken.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.CreateToken.cs
Outdated
Show resolved
Hide resolved
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
…t I will need for deserialization as well
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
… testcase to ensure that proper logic is getting triggered for right parameter
|
Hello @kevinchalet ,
Deserialization:
@kevinchalet and @sruke , I wanted to have your opinion if the logic fits right. This approach looks good to me as we are avoiding those extra flags while giving the customers an option to use custom claim names too. Please let me know if anything else is expected or if we can continue with this approach. I will merge that branch into this one if everything looks good and we can proceed. I will rename the flag or delegate as we proceed. Thank you again and looking forward to your responses! |
Moving away from app context switches and checking if ActorClaimType is set on TVPs or SecurityTokenDescriptor LGTM. |
|
@saurabhsathe-ms thanks! Looks good: AFAICT, the only missing piece is the ability to map Note: I finished implementation OAuth 2.0 Token Exchange support in OpenIddict (the first preview will ship today). That would be awesome if this PR was included in the next IM minor version so we can offer the best delegation story possible to users 😃 |
…serializeClaimsIdentity
Hello @kevinchalet , thank you so much for your reply. ClaimsIdentity.Actor will be mapped to the claim whose name matches whats provided in TVP.ActorClaimType or "actort". TVP.ActorClaimType cannot take value as "actort". Its default value is "act". During serialization you can specify the actor claim type as "act" and it will be serialized as JsonObject. Right now, if you provide the TVP.ActorClaimType as "act" it will be deserialized assuming that actor claim named "act" was serialized as JSONObject and if the claim name is "actort" its deserialized assuming that it was serialized as JWT. So, can you please elaborate on what we are missing more? |
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
Interesting. Looking at the code, it seems the only way the if (isActorFound || tokenDescriptor.Subject?.Actor != null)
WriteActorToken(writer, tokenDescriptor, setDefaultTimesOnTokenCreation, tokenLifetimeInMinutes);Did I miss something? |
I took a look at JWTSecurityHandler which is the legacy handler that was before JsobWebtTokenHandler. What it used to do was take actor claim serialize it into a JWT and then serialize the serialized actor JWT while encoding the token as whole. Hence the generated JsonWebToken.Actor is a string that returned the encoded JWT that we needed to deserialize again. Also, it never had a logic for actor that was provided as Subject.Actor. What I am doing now is if you provide act as a claim in claims dictionary or a Subject.Actor it doesnt get serialized as a JWT rightaway but rather nested claims are processed and then serialized as a JWT as a part of token encoding process. |
It did - and still does, actually - support serializing But of course, it only supports serializing it as a JWT, not as a JSON node. Leaving |
|
Thank you so much for your response @kevinchalet . Sorry for overseeing the Subject.Actor details. I am little confused as why its not a JSON. For example in this testcase we can correctly correct get the JSON back: |
Yeah, that part works, but I was explicitly referring to the serialization of [Fact]
public void ActorAttachedToSubjectIdentityShouldBeProperlySerialized()
{
var context = new CompareContext($"{this}.ActorAttachedToSubjectIdentityShouldBeProperlySerialized");
try
{
// Create a ClaimsIdentity for the actor
var actorIdentity = new CaseSensitiveClaimsIdentity("ActorAuth");
actorIdentity.AddClaim(new Claim("sub", "actor-subject-id"));
actorIdentity.AddClaim(new Claim("name", "Actor Name"));
actorIdentity.AddClaim(new Claim("role", "admin"));
// Create the main identity
var mainIdentity = new CaseSensitiveClaimsIdentity("Bearer");
mainIdentity.AddClaim(new Claim("sub", "main-subject-id"));
mainIdentity.AddClaim(new Claim("name", "Main User"));
// Attach the actor to the main identity
mainIdentity.Actor = actorIdentity;
// Create a token using the standard "act" claim instead of "actort"
var tokenHandler = new JsonWebTokenHandler();
var tokenDescriptor = new SecurityTokenDescriptor
{
Subject = mainIdentity,
Issuer = "https://example.com",
Audience = "https://api.example.com",
Expires = DateTime.UtcNow.AddHours(1),
SigningCredentials = Default.AsymmetricSigningCredentials,
ActorClaimType = "act",
};
var token = tokenHandler.CreateToken(tokenDescriptor);
JsonWebToken decodedToken = tokenHandler.ReadJsonWebToken(token);
// Verify actor claim exists in the token
Assert.True(decodedToken.Payload.HasClaim(tokenDescriptor.ActorClaimType), "JWT token should contain 'act' claim");
// Verify the actor object directly
var actorObject = decodedToken.Payload.GetValue<JsonElement>(tokenDescriptor.ActorClaimType);
Assert.Equal(JsonValueKind.Object, actorObject.ValueKind);
// Verify actor claims directly from the JSON object
Assert.Equal("actor-subject-id", actorObject.GetProperty("sub").GetString());
Assert.Equal("Actor Name", actorObject.GetProperty("name").GetString());
Assert.Equal("admin", actorObject.GetProperty("role").GetString());
TestUtilities.AssertFailIfErrors(context);
}
catch (Exception ex)
{
context.Diffs.Add($"Exception: {ex}");
}
} |
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
| public class ActClaimSerializationTests | ||
| { | ||
| [Fact] | ||
| public void ActorTokenInClaimsDictionaryShouldBeProperlySerialized() |
There was a problem hiding this comment.
I suggest adding underscores to the test names like in other tests. Something like:
ActorTokenInClaimsDictionary_CorrectlySerializedNestedActorToken_InClaimsDictionary_IsCorrectlySerialized
See guidelines here: https://learn.microsoft.com/en-us/dotnet/core/testing/unit-testing-best-practices#follow-test-naming-standards
| /// <para>This limit applies to both token creation and validation processes.</para> | ||
| /// </remarks> | ||
| /// <exception cref="ArgumentOutOfRangeException">Thrown if the value is less than 0 or greater than 4.</exception> | ||
| public int MaxActorChainLength |
There was a problem hiding this comment.
From the previous discussions, was there a request to make MaxActorChainLength and ActorChainDepth be setteable by the user?
There was a problem hiding this comment.
From the offline conversation - I'd remove the MaxActorChainLength and ActorChainDepth public properties.
| /// and use "act" as the claim type.</para> | ||
| /// <para>To use the legacy string-based actor token format, leave the switch off and use "actort".</para> | ||
| /// </remarks> | ||
| public string ActorClaimType |
There was a problem hiding this comment.
According to specs, can a valid JWT have both, "act" and "actort" claims? What about multiple actor claims?
There was a problem hiding this comment.
Is there a need for this property? When reading a token, why not just parse the first "act" claim that we come upon?
There was a problem hiding this comment.
An option here is to allow setting this to only "act" or "actort" as valid actor claim type name values and default to "actort" since it's what is used now.
| /// Thrown if the value is null or empty. | ||
| /// </exception> | ||
| /// <remarks> | ||
| /// <para>To use the newer JSON object-based actor format, set <c>AppContext.SetSwitch(AppContextSwitches.EnableActClaimSupportSwitch, true)</c> |
There was a problem hiding this comment.
Does this switch exist? I don't see it in files.
| /// <remarks> | ||
| /// <para>To use the newer JSON object-based actor format, set <c>AppContext.SetSwitch(AppContextSwitches.EnableActClaimSupportSwitch, true)</c> | ||
| /// and use "act" as the claim type.</para> | ||
| /// <para>To use the legacy string-based actor token format, leave the switch off and use "actort".</para> |
There was a problem hiding this comment.
Why default to legacy and not the newest format?
There was a problem hiding this comment.
"actort" is what is currently used, so we should default to it.
| /// <para>This limit applies to both token creation and validation processes.</para> | ||
| /// </remarks> | ||
| /// <exception cref="ArgumentOutOfRangeException">Thrown if the value is less than 0 or greater than 4.</exception> | ||
| public int MaxActorChainLength |
There was a problem hiding this comment.
From the offline conversation - I'd remove the MaxActorChainLength and ActorChainDepth public properties.
| [Fact] | ||
| public void BasicJsonElementShouldCreateClaimsIdentityCorrectly() | ||
| { | ||
| var context = new CompareContext($"{this}.BasicJsonElementShouldCreateClaimsIdentityCorrectly"); |
There was a problem hiding this comment.
These tests are simple. I would remove the CompareContext and try/catch - any unexpected exceptions will just break the test anyway.
| context.Diffs.Add("Expected exception was not thrown."); | ||
| TestUtilities.AssertFailIfErrors(context); | ||
| } | ||
| catch (SecurityTokenException ex) |
There was a problem hiding this comment.
Assert.Throws should be used instead of try/catch.
| /// <remarks> | ||
| /// <para>To use the newer JSON object-based actor format, set <c>AppContext.SetSwitch(AppContextSwitches.EnableActClaimSupportSwitch, true)</c> | ||
| /// and use "act" as the claim type.</para> | ||
| /// <para>To use the legacy string-based actor token format, leave the switch off and use "actort".</para> |
There was a problem hiding this comment.
"actort" is what is currently used, so we should default to it.
| public const string IDX11023 = "IDX11023: Expecting json reader to be positioned on '{0}', reader was positioned at: '{1}', Reading: '{2}', Position: '{3}', CurrentDepth: '{4}', BytesConsumed: '{5}'."; | ||
| public const string IDX11025 = "IDX11025: Cannot serialize object of type: '{0}' into property: '{1}'."; | ||
| public const string IDX11026 = "IDX11026: Unable to get claim value as a string from claim type:'{0}', value type was:'{1}'. Acceptable types are String, IList<String>, and System.Text.Json.JsonElement."; | ||
| public const string IDX11027 = "IDX11027: Invalid JsonWebToken handler configuration parameter value provided for {0}"; |
There was a problem hiding this comment.
For this you can use the formatter for that extra message so the callers don't have to do string concatenation.
| public const string IDX11027 = "IDX11027: Invalid JsonWebToken handler configuration parameter value provided for {0}"; | |
| public const string IDX11027 = "IDX11027: Invalid JsonWebToken handler configuration parameter value provided for {0}. {1}"; |
| claimType = jwtClaim.Type; | ||
|
|
||
| if (claimType == ClaimTypes.Actor) | ||
| if (claimType.Equals(validationParameters.ActorClaimType) || claimType.Equals("actort")) |
There was a problem hiding this comment.
Use JwtRegisteredClaimNames.Actort instead of literal.
| /// <param name="tokenValidationParameters">These parameters have details like nested actor chain length and max permissible actor length</param> | ||
| /// <param name="issuer">The issuer for the claims.</param> | ||
| /// <returns>A ClaimsIdentity containing claims from the JsonElement.</returns> | ||
| public static ClaimsIdentity CreateActorClaimsIdentityFromJsonElement( |
| } | ||
|
|
||
| if (jsonElement.ValueKind != JsonValueKind.Object) | ||
| throw LogHelper.LogExceptionMessage(new ArgumentException("Actor token must be a JSON object")); |
| // Use CaseSensitiveClaimsIdentity for consistent behavior with the rest of the library | ||
| var identity = new CaseSensitiveClaimsIdentity(); | ||
|
|
||
| issuer = issuer ?? ClaimsIdentity.DefaultIssuer; |
There was a problem hiding this comment.
| issuer = issuer ?? ClaimsIdentity.DefaultIssuer; | |
| issuer ??= ClaimsIdentity.DefaultIssuer; |
| LogHelper.FormatInvariant( | ||
| LogMessages.IDX14315, | ||
| LogHelper.MarkAsNonPII(tokenDescriptor.ActorClaimType), | ||
| LogHelper.MarkAsNonPII(actorValue?.GetType().ToString() ?? "null")))); |
There was a problem hiding this comment.
| LogHelper.MarkAsNonPII(actorValue?.GetType().ToString() ?? "null")))); | |
| LogHelper.MarkAsNonPII(actorValue?.GetType().FullName ?? "null")))); |
|
A consideration is to split this PR into two, if it's easier to implement: (1) enable serialization/deserialization for "actort" claim type only and (2) support "act" claim type. |
| JsonPrimitives.WriteObject(ref writer, kvp.Key, kvp.Value); | ||
| } | ||
| } | ||
| if (isActorFound || tokenDescriptor.Subject?.Actor != null) |
There was a problem hiding this comment.
I think this should be: !isActorFound
| { | ||
| foreach (KeyValuePair<string, object> kvp in tokenDescriptor.Claims) | ||
| { | ||
| if (kvp.Key.Equals(tokenDescriptor.ActorClaimType, StringComparison.Ordinal)) |
There was a problem hiding this comment.
Following the logic for duplicate claims, we should write out the 'actor' claim, not sure why we continue.
// Duplicates are resolved according to the following priority:
// SecurityTokenDescriptor.{Audience/Audiences, Issuer, Expires, IssuedAt, NotBefore}, SecurityTokenDescriptor.Claims, SecurityTokenDescriptor.Subject.Claims
// SecurityTokenDescriptor.Claims are KeyValuePairs<string,object>, whereas SecurityTokenDescriptor.Subject.Claims are System.Security.Claims.Claim and are processed differently.| bool setDefaultTimesOnTokenCreation, | ||
| int tokenLifetimeInMinutes) | ||
| { | ||
| if (tokenDescriptor == null) |
There was a problem hiding this comment.
tokenDescriptor should never be null here, you can check for null, but don't throw, just return.
There was a problem hiding this comment.
Okay, will do this!
| return; | ||
|
|
||
| writer.WritePropertyName(tokenDescriptor.ActorClaimType); | ||
| WriteJwsPayload(ref writer, actorTokenDescriptor, setDefaultTimesOnTokenCreation, tokenLifetimeInMinutes); |
There was a problem hiding this comment.
We can simply this method by just passing the ClaimsIdentity, and the writing the claims into JSON.
Assume that only the claims in the ClaimsIdentity need to be added to the JSON.
The parameters to this method could be.
internal static void WriteActorToken(
ref Utf8JsonWriter writer,
string claimName,
ClaimsIdentity claimsIdentity)| } | ||
| } | ||
| internal static void WriteActorToken( | ||
| Utf8JsonWriter writer, |
There was a problem hiding this comment.
It is important to pass the writer using the 'ref' keyword.
There was a problem hiding this comment.
I will do this thank you for your review @brentschmaltz
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
Serialize and deserialize actor token in claims identity
Description
In this PR:
File changes:
Here’s a summary of all the changes made in this PR across the listed files:
1. src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.CreateToken.cs
act) in JWT payloads, controlled by a new AppContext switch.WriteActorTokenthat serializes nested actor tokens, with recursion depth checks viaMaxActorChainLength.ValidateActorChainDepth.2. src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.ValidateToken.cs
act) exists, validation is delegated to a newActorTokenValidationDelegate.3. src/Microsoft.IdentityModel.JsonWebTokens/LogMessages.cs
IDX14115for missingActorTokenValidationDelegate.IDX14313for exceeding maximum actor token nesting.4. src/Microsoft.IdentityModel.JsonWebTokens/Microsoft.IdentityModel.JsonWebTokens.csproj
5. src/Microsoft.IdentityModel.Tokens/AppContextSwitches.cs
SerializeDeserializeActorClaimSwitchand its property to enable new actor claim (act) behavior.6. src/Microsoft.IdentityModel.Tokens/Delegates.cs
ActorTokenValidationDelegate, which allows custom validation of the actor claim (act).7. src/Microsoft.IdentityModel.Tokens/InternalAPI.Unshipped.txt
8. src/Microsoft.IdentityModel.Tokens/LogMessages.cs
IDX11027for invalid handler configuration parameter values.9. src/Microsoft.IdentityModel.Tokens/PublicAPI.Unshipped.txt
TokenValidationParametersandSecurityTokenDescriptor.10. src/Microsoft.IdentityModel.Tokens/SecurityTokenDescriptor.cs
MaxActorChainLength,ActorClaimName, andActorChainDepth, with validation and documentation.11. src/Microsoft.IdentityModel.Tokens/TokenValidationParameters.cs
MaxActorChainLength,ActorClaimName,ActorChainDepth,ActorTokenValidationDelegate, andActorTokenValidationParameters.Clone()method to copy the new delegate property.12. test/Microsoft.IdentityModel.JsonWebTokens.Tests/ActorClaimsTests.cs
13. test/Microsoft.IdentityModel.TestUtils/ValidationDelegates.cs
ActorTokenValidationDelegatefor use in tests.14. test/Microsoft.IdentityModel.Tokens.Tests/TokenValidationParametersTests.cs
GetSets()tests.CreateTokenValidationParameters()to set the new delegate for testing.In summary:
This PR introduces configurable support for nested actor claims ("act") in JWTs, with depth limitation, customizable claim name, and a pluggable validation delegate, plus associated logging, AppContext switches, and extensive test and API surface updates for these new features.
Fixes #1840