Update JwtBearer and WsFederation to use updated IdentityModel #48966
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |||||
| using System.Net.Http; | ||||||
| using Microsoft.IdentityModel.Protocols; | ||||||
| using Microsoft.IdentityModel.Protocols.OpenIdConnect; | ||||||
| using Microsoft.IdentityModel.JsonWebTokens; | ||||||
| using Microsoft.IdentityModel.Tokens; | ||||||
|
|
||||||
| namespace Microsoft.AspNetCore.Authentication.JwtBearer; | ||||||
|
|
@@ -15,13 +16,22 @@ namespace Microsoft.AspNetCore.Authentication.JwtBearer; | |||||
| public class JwtBearerOptions : AuthenticationSchemeOptions | ||||||
| { | ||||||
| private readonly JwtSecurityTokenHandler _defaultHandler = new JwtSecurityTokenHandler(); | ||||||
| private readonly JsonWebTokenHandler _defaultTokenHandler = new JsonWebTokenHandler | ||||||
| { | ||||||
| MapInboundClaims = JwtSecurityTokenHandler.DefaultMapInboundClaims | ||||||
| }; | ||||||
|
|
||||||
| private bool _mapInboundClaims = JwtSecurityTokenHandler.DefaultMapInboundClaims; | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Initializes a new instance of <see cref="JwtBearerOptions"/>. | ||||||
| /// </summary> | ||||||
| public JwtBearerOptions() | ||||||
| { | ||||||
| #pragma warning disable CS0618 // Type or member is obsolete | ||||||
| SecurityTokenValidators = new List<ISecurityTokenValidator> { _defaultHandler }; | ||||||
| #pragma warning restore CS0618 // Type or member is obsolete | ||||||
| TokenHandlers = new List<TokenHandler> { _defaultTokenHandler }; | ||||||
|
There was a problem hiding this comment. @kevinchalet i am thinking that creating the interface ITokenValidator that has a couple of methods instead of the abstract class TokenHandler, this would make it easier for users who currently have an implementation of ISecurityTokenValidator. Thoughts? There was a problem hiding this comment.
IM already has WIF only had There was a problem hiding this comment. @kevinchalet OK let's stick with TokenHandler then as this was meant to be the replacement for SecurityTokenHandler. There was a problem hiding this comment. @kevinchalet we could implement ISecurityTokenValidator on JsonWebTokenHandler, but just because we can cast it to TokenHandler doesn't mean that users will not have a runtime break as the SecurityToken will change from JwtSecurityToken to JsonWebToken. There was a problem hiding this comment. Sure, but that would mean an instance of There was a problem hiding this comment. I'm hoping to make it opt-out. |
||||||
| } | ||||||
|
|
||||||
| /// <summary> | ||||||
|
|
@@ -103,8 +113,14 @@ public JwtBearerOptions() | |||||
| /// <summary> | ||||||
| /// Gets the ordered list of <see cref="ISecurityTokenValidator"/> used to validate access tokens. | ||||||
| /// </summary> | ||||||
| [Obsolete("SecurityTokenValidators is no longer used by default. Use TokenHandlers instead. To continue using SecurityTokenValidators, set UseSecurityTokenValidators to true. See https://aka.ms/aspnetcore8/security-token-changes")] | ||||||
| public IList<ISecurityTokenValidator> SecurityTokenValidators { get; private set; } | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Gets the ordered list of <see cref="TokenHandler"/> used to validate access tokens. | ||||||
| /// </summary> | ||||||
| public IList<TokenHandler> TokenHandlers { get; private set; } | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Gets or sets the parameters used to validate identity tokens. | ||||||
| /// </summary> | ||||||
|
|
@@ -126,15 +142,20 @@ public JwtBearerOptions() | |||||
| public bool IncludeErrorDetails { get; set; } = true; | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Gets or sets the <see cref="MapInboundClaims"/> property on the default instance of <see cref="JwtSecurityTokenHandler"/> in SecurityTokenValidators, or <see cref="JsonWebTokenHandler"/> in TokenHandlers which is used when determining | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| /// whether or not to map claim types that are extracted when validating a <see cref="JwtSecurityToken"/> or a <see cref="JsonWebToken"/>. | ||||||
| /// <para>If this is set to true, the Claim Type is set to the JSON claim 'name' after translating using this mapping. Otherwise, no mapping occurs.</para> | ||||||
| /// <para>The default value is true.</para> | ||||||
| /// </summary> | ||||||
| public bool MapInboundClaims | ||||||
| { | ||||||
| get => _mapInboundClaims; | ||||||
| set | ||||||
| { | ||||||
| _mapInboundClaims = value; | ||||||
| _defaultHandler.MapInboundClaims = value; | ||||||
| _defaultTokenHandler.MapInboundClaims = value; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// <summary> | ||||||
|
|
@@ -152,4 +173,15 @@ public bool MapInboundClaims | |||||
| /// Defaults to <see cref="ConfigurationManager{OpenIdConnectConfiguration}.DefaultRefreshInterval" />. | ||||||
| /// </value> | ||||||
| public TimeSpan RefreshInterval { get; set; } = ConfigurationManager<OpenIdConnectConfiguration>.DefaultRefreshInterval; | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Gets or sets whether <see cref="TokenHandlers"/> or <see cref="SecurityTokenValidators"/> will be used to validate the inbound token. | ||||||
| /// </summary> | ||||||
| /// <remarks> | ||||||
| /// The advantage of using TokenHandlers are: | ||||||
| /// <para>There is an Async model.</para> | ||||||
| /// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is faster than a <see cref="JwtSecurityTokenHandler"/>.</para> | ||||||
| /// <para>There is an ability to make use of a Last-Known-Good model for metadata that protects applications when metadata is published with errors.</para> | ||||||
|
Comment on lines
+181
to
+184
There was a problem hiding this comment. I wonder if we should rethink these doc comments as they might not make full sense now that we've inverted the boolean to be Maybe we should add why someone would want to use SecurityTokenValidators as well. |
||||||
| /// </remarks> | ||||||
| public bool UseSecurityTokenValidators { get; set; } | ||||||
| } | ||||||
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,5 @@ | ||
| #nullable enable | ||
| Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerHandler.JwtBearerHandler(Microsoft.Extensions.Options.IOptionsMonitor<Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerOptions!>! options, Microsoft.Extensions.Logging.ILoggerFactory! logger, System.Text.Encodings.Web.UrlEncoder! encoder) -> void | ||
| Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerOptions.TokenHandlers.get -> System.Collections.Generic.IList<Microsoft.IdentityModel.Tokens.TokenHandler!>! | ||
| Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerOptions.UseSecurityTokenValidators.get -> bool | ||
| Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerOptions.UseSecurityTokenValidators.set -> void |
Oops, something went wrong.
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.
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.
Previously we would cache the
_configurationon the handler. However, we don't cache it anymore, and instead will always callGetConfigurationAsync. Is this OK? Will it cause any sort of performance issue?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.
GetConfigurationAsync should do its own caching, and handlers are re-created per request anyways. It looks like this is the only place config is read?
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.
Correct, ConfigurationManager does its own caching