Add ability to exclude default header claims when creating token #2979
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Is there an issue tracking this? |
Not at the moment. This was just something discussed since we wanted to remove |
SummarySummary
CoverageMicrosoft.IdentityModel.Abstractions - 70.2%
Microsoft.IdentityModel.JsonWebTokens - 80%
Microsoft.IdentityModel.Logging - 74.5%
Microsoft.IdentityModel.Protocols - 88.2%
Microsoft.IdentityModel.Protocols.OpenIdConnect - 81.4%
Microsoft.IdentityModel.Protocols.SignedHttpRequest - 93.4%
Microsoft.IdentityModel.Protocols.Tests - 0%
Microsoft.IdentityModel.Protocols.WsFederation - 80.9%
Microsoft.IdentityModel.TestUtils - 87%
Microsoft.IdentityModel.Tokens - 75.9%
Microsoft.IdentityModel.Tokens.Saml - 71.1%
Microsoft.IdentityModel.Tokens.Tests - 4.6%
Microsoft.IdentityModel.Validators - 95.8%
Microsoft.IdentityModel.Xml - 79.3%
System.IdentityModel.Tokens.Jwt - 84%
System.IdentityModel.Tokens.Jwt.Tests - 20.1%
|
src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.CreateToken.cs
Show resolved
Hide resolved
| /// The default header claims are 'alg', 'kid', 'x5t', 'enc', and 'zip'. | ||
| /// </remarks> | ||
| #pragma warning disable CA2227 // Collection properties should be read only | ||
| public ISet<string> ExcludedDefaultHeaderClaims { get; set; } |
There was a problem hiding this comment.
In general, Exclude APIs are not common in Identity Model. What are the alternatives that we considered?
There was a problem hiding this comment.
I don't think any others were considered. I proposed this since only excluding kid and x5t was very targeted and iffy, and the only other option that comes to mind is allocating a set with all the default header claims in every SecurityTokenDescriptor which can be modified as desired, but then you end up with those allocations.
94f421a to
a460730
Compare
a460730 to
887b216
Compare
jmprieur
reviewed
Dec 5, 2024
jmprieur
approved these changes
Dec 5, 2024
887b216 to
6a61dbe
Compare
There was a problem hiding this comment.
Copilot reviewed 5 out of 8 changed files in this pull request and generated no comments.
Files not reviewed (3)
- src/Microsoft.IdentityModel.JsonWebTokens/InternalAPI.Unshipped.txt: Language not supported
- src/Microsoft.IdentityModel.Tokens/PublicAPI.Unshipped.txt: Language not supported
- src/System.IdentityModel.Tokens.Jwt/InternalAPI.Unshipped.txt: Language not supported
Comments suppressed due to low confidence (3)
src/Microsoft.IdentityModel.Tokens/SecurityTokenDescriptor.cs:124
- The comment should end with a period for consistency.
/// Indicates if <c>kid</c> and <c>x5t</c> should be included in the header of a JSON web token (JWT)
src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.CreateToken.cs:881
- [nitpick] The method name 'IncludeDefaultHeaderClaim' is ambiguous. It should be renamed to 'ShouldIncludeDefaultHeaderClaim'.
internal static bool IncludeDefaultHeaderClaim(string claim, ISet<string> excludedDefaultHeaderClaims) =>
src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.CreateToken.cs:1045
- The comment on this line should use 'compression algorithm' instead of 'compressionAlgorithm' for better readability.
if (!string.IsNullOrEmpty(compressionAlgorithm) && IncludeDefaultHeaderClaim(JwtHeaderParameterNames.Zip, excludedDefaultHeaderClaims))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add ability to exclude default header claims when creating token.