-
Notifications
You must be signed in to change notification settings - Fork 409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ability to create token without kid #2968
Conversation
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.
LGTM
SummarySummary
CoverageMicrosoft.IdentityModel.Abstractions - 70.2%
Microsoft.IdentityModel.JsonWebTokens - 79.9%
Microsoft.IdentityModel.Logging - 74.9%
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%
Microsoft.IdentityModel.Tokens.Tests - 4.6%
Microsoft.IdentityModel.Validators - 95.8%
Microsoft.IdentityModel.Xml - 80.6%
System.IdentityModel.Tokens.Jwt - 82.6%
System.IdentityModel.Tokens.Jwt.Tests - 20.1%
|
There are a lot of API's for a single logical control. Can we limit this to API's that take SecurityTokenDescriptor when creating a token? |
I'm fine with this. It will clean things up here. Theoretically everything should just go through the
This I'm not sure on. I would assume we don't want to touch SAML/2.
Can you expand on this? |
Agree, consider designing for what is needed today i.e. optionally opting out of adding kid for JsonWebTokenHandler alone.
Correct. This change is expected to be backward compatible. |
Perhaps we should just comment in the SecurityTokenDescriptor that this only applies to JWTs. |
src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.CreateToken.cs
Outdated
Show resolved
Hide resolved
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.
LGTM
Let's try to have minimal public API changes.
f7c4bd9
to
507cd66
Compare
SummarySummary
CoverageMicrosoft.IdentityModel.Abstractions - 70.2%
Microsoft.IdentityModel.JsonWebTokens - 80%
Microsoft.IdentityModel.Logging - 74.9%
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%
Microsoft.IdentityModel.Tokens.Tests - 4.6%
Microsoft.IdentityModel.Validators - 95.8%
Microsoft.IdentityModel.Xml - 80.6%
System.IdentityModel.Tokens.Jwt - 83.8%
System.IdentityModel.Tokens.Jwt.Tests - 20.1%
|
The source code LGTM. Consider adding tests to ensure the changes introduced work as expected. |
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.
Thanks @msbw2
507cd66
to
a52c6c9
Compare
a52c6c9
to
efb3205
Compare
SummarySummary
CoverageMicrosoft.IdentityModel.Abstractions - 70.2%
Microsoft.IdentityModel.JsonWebTokens - 80%
Microsoft.IdentityModel.Logging - 74.9%
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%
Microsoft.IdentityModel.Tokens.Tests - 4.6%
Microsoft.IdentityModel.Validators - 95.8%
Microsoft.IdentityModel.Xml - 80.6%
System.IdentityModel.Tokens.Jwt - 83.8%
System.IdentityModel.Tokens.Jwt.Tests - 20.1%
|
SummarySummary
CoverageMicrosoft.IdentityModel.Abstractions - 70.2%
Microsoft.IdentityModel.JsonWebTokens - 80%
Microsoft.IdentityModel.Logging - 74.9%
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%
Microsoft.IdentityModel.Tokens.Tests - 4.6%
Microsoft.IdentityModel.Validators - 95.8%
Microsoft.IdentityModel.Xml - 80.6%
System.IdentityModel.Tokens.Jwt - 83.8%
System.IdentityModel.Tokens.Jwt.Tests - 20.1%
|
#2960