Skip to content

Option to use JsonWebTokenHandler in oidc handler #49333

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

Option to use JsonWebTokenHandler in oidc handler #49333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2260e57
Option to use JsonWebTokenHandler in OpenIdConnectHandler
Jun 23, 2023
61323b8
fix sample
Jun 29, 2023
7f45829
split event tests
Jun 29, 2023
b208837
Use 7.0.0-preview for identity model libraries
Jul 11, 2023
9a4cc56
use var for identitymodel versions
Jul 11, 2023
a4c116c
Move new apis to unshipped
Jul 11, 2023
b7c4701
Changes from API review
Jul 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions eng/Versions.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
<AspNetCorePatchVersion>0</AspNetCorePatchVersion>
<PreReleaseVersionIteration>6</PreReleaseVersionIteration>
<ValidateBaseline>true</ValidateBaseline>
<IdentityModelVersion>7.0.0-preview</IdentityModelVersion>
<!--
When StabilizePackageVersion is set to 'true', this branch will produce stable outputs for 'Shipping' packages
-->
Expand Down Expand Up @@ -248,15 +249,15 @@
<MicrosoftCodeAnalysisCSharpAnalyzerTestingXUnitVersion>1.1.2-beta1.22531.1</MicrosoftCodeAnalysisCSharpAnalyzerTestingXUnitVersion>
<MicrosoftCodeAnalysisCSharpCodeFixTestingXUnitVersion>1.1.2-beta1.22531.1</MicrosoftCodeAnalysisCSharpCodeFixTestingXUnitVersion>
<MicrosoftCssParserVersion>1.0.0-20230414.1</MicrosoftCssParserVersion>
<MicrosoftIdentityModelLoggingVersion>6.15.1</MicrosoftIdentityModelLoggingVersion>
<MicrosoftIdentityModelProtocolsOpenIdConnectVersion>6.15.1</MicrosoftIdentityModelProtocolsOpenIdConnectVersion>
<MicrosoftIdentityModelProtocolsWsFederationVersion>6.15.1</MicrosoftIdentityModelProtocolsWsFederationVersion>
<MicrosoftIdentityModelLoggingVersion>$(IdentityModelVersion)</MicrosoftIdentityModelLoggingVersion>
<MicrosoftIdentityModelProtocolsOpenIdConnectVersion>$(IdentityModelVersion)</MicrosoftIdentityModelProtocolsOpenIdConnectVersion>
<MicrosoftIdentityModelProtocolsWsFederationVersion>$(IdentityModelVersion)</MicrosoftIdentityModelProtocolsWsFederationVersion>
<MicrosoftInternalAspNetCoreH2SpecAllVersion>2.2.1</MicrosoftInternalAspNetCoreH2SpecAllVersion>
<MicrosoftNETCoreWindowsApiSetsVersion>1.0.1</MicrosoftNETCoreWindowsApiSetsVersion>
<MicrosoftOwinSecurityCookiesVersion>3.0.1</MicrosoftOwinSecurityCookiesVersion>
<MicrosoftOwinTestingVersion>3.0.1</MicrosoftOwinTestingVersion>
<MicrosoftWebAdministrationVersion>11.1.0</MicrosoftWebAdministrationVersion>
<SystemIdentityModelTokensJwtVersion>6.21.0</SystemIdentityModelTokensJwtVersion>
<SystemIdentityModelTokensJwtVersion>$(IdentityModelVersion)</SystemIdentityModelTokensJwtVersion>
<SystemComponentModelAnnotationsVersion>5.0.0</SystemComponentModelAnnotationsVersion>
<SystemNetExperimentalMsQuicVersion>5.0.0-alpha.20560.6</SystemNetExperimentalMsQuicVersion>
<SystemSecurityPrincipalWindowsVersion>5.0.0</SystemSecurityPrincipalWindowsVersion>
Expand Down
4 changes: 2 additions & 2 deletions src/Security/Authentication/OpenIdConnect/samples/OpenIdConnectSample/Startup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ await WriteHtmlAsync(response, async res =>
// Persist the new acess token
props.UpdateTokenValue("access_token", payload.RootElement.GetString("access_token"));
props.UpdateTokenValue("refresh_token", payload.RootElement.GetString("refresh_token"));
if (payload.RootElement.TryGetProperty("expires_in", out var property) && property.TryGetInt32(out var seconds))
if (payload.RootElement.TryGetProperty("expires_in", out var property) && int.TryParse(property.GetString(), out var seconds))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

property.TryGetInt32 was throwing a InvalidOperationException on ValueKind not being a number

Copy link
Member

@eerhardt eerhardt Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/dotnet/runtime/blob/0f56e166b16100c23dc81ae082f6155362b7c596/src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.cs#L481

is the check that ensures it is a Number.

This seems like a bug. There should be no reason for a TryGet method to throw an exception.

Looks like this was designed this way according to dotnet/runtime#28132.

        // InvalidOperationException if Type is not Number
        // false if value does not fit.
        public bool TryGetDecimal(out decimal value);
        public bool TryGetDouble(out double value);
        public bool TryGetInt32(out int value);
        public bool TryGetInt64(out long value);
        public bool TryGetSingle(out float value);
        [CLSCompliantAttribute(false)]
        public bool TryGetUInt32(out uint value);
        [CLSCompliantAttribute(false)]
        public bool TryGetUInt64(out ulong value);

{
var expiresAt = DateTimeOffset.UtcNow + TimeSpan.FromSeconds(seconds);
props.UpdateTokenValue("expires_at", expiresAt.ToString("o", CultureInfo.InvariantCulture));
Expand All @@ -283,7 +283,7 @@ await WriteHtmlAsync(response, async res =>
await WriteTableHeader(res, new string[] { "Token Type", "Value" }, props.GetTokens().Select(token => new string[] { token.Name, token.Value }));

await res.WriteAsync("<h2>Payload:</h2>");
await res.WriteAsync(HtmlEncoder.Default.Encode(payload.ToString()).Replace(",", ",<br>") + "<br>");
await res.WriteAsync(HtmlEncoder.Default.Encode(payload.RootElement.ToString()).Replace(",", ",<br>") + "<br>");
});
}

Expand Down
6 changes: 6 additions & 0 deletions src/Security/Authentication/OpenIdConnect/src/LoggingExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,4 +164,10 @@ internal static partial class LoggingExtensions
[LoggerMessage(55, LogLevel.Error, "The remote signout request was ignored because the 'iss' parameter didn't match " +
"the expected value, which may indicate an unsolicited logout.", EventName = "RemoteSignOutIssuerInvalid")]
public static partial void RemoteSignOutIssuerInvalid(this ILogger logger);

[LoggerMessage(56, LogLevel.Error, "Unable to validate the 'id_token', no suitable TokenHandler was found for: '{IdToken}'.", EventName = "UnableToValidateIdTokenFromHandler")]
public static partial void UnableToValidateIdTokenFromHandler(this ILogger logger, string idToken);

[LoggerMessage(57, LogLevel.Error, "The Validated Security Token must be of type JsonWebToken, but instead its type is: '{SecurityTokenType}'", EventName = "InvalidSecurityTokenTypeFromHandler")]
public static partial void InvalidSecurityTokenTypeFromHandler(this ILogger logger, string? securityTokenType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static partial void InvalidSecurityTokenTypeFromHandler(this ILogger logger, string? securityTokenType);
public static partial void InvalidSecurityTokenTypeFromHandler(this ILogger logger, string securityTokenType);

I don't think we'd want null to be passed here. If the object is null, we should probably say it is null somehow rather than have a message that says "but instead its type is: ".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same approach as taken in InvalidSecurityTokenType earlier in the file

}
87 changes: 85 additions & 2 deletions src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Microsoft.Extensions.Primitives;
using Microsoft.IdentityModel.JsonWebTokens;
using Microsoft.IdentityModel.Protocols.OpenIdConnect;
using Microsoft.IdentityModel.Tokens;
using JwtRegisteredClaimNames = Microsoft.IdentityModel.JsonWebTokens.JwtRegisteredClaimNames;

namespace Microsoft.AspNetCore.Authentication.OpenIdConnect;

Expand Down Expand Up @@ -649,7 +651,17 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync
if (!string.IsNullOrEmpty(authorizationResponse.IdToken))
{
Logger.ReceivedIdToken();
user = ValidateToken(authorizationResponse.IdToken, properties, validationParameters, out jwt);

if (!Options.UseSecurityTokenValidator)
{
var tokenValidationResult = await ValidateTokenUsingHandlerAsync(authorizationResponse.IdToken, properties, validationParameters);
user = new ClaimsPrincipal(tokenValidationResult.ClaimsIdentity);
jwt = JwtSecurityTokenConverter.Convert(tokenValidationResult.SecurityToken as JsonWebToken);
}
else
{
user = ValidateToken(authorizationResponse.IdToken, properties, validationParameters, out jwt);
}

nonce = jwt.Payload.Nonce;
if (!string.IsNullOrEmpty(nonce))
Expand Down Expand Up @@ -717,7 +729,19 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync

// At least a cursory validation is required on the new IdToken, even if we've already validated the one from the authorization response.
// And we'll want to validate the new JWT in ValidateTokenResponse.
var tokenEndpointUser = ValidateToken(tokenEndpointResponse.IdToken, properties, validationParameters, out var tokenEndpointJwt);
ClaimsPrincipal tokenEndpointUser;
JwtSecurityToken tokenEndpointJwt;

if (!Options.UseSecurityTokenValidator)
{
var tokenValidationResult = await ValidateTokenUsingHandlerAsync(tokenEndpointResponse.IdToken, properties, validationParameters);
tokenEndpointUser = new ClaimsPrincipal(tokenValidationResult.ClaimsIdentity);
tokenEndpointJwt = JwtSecurityTokenConverter.Convert(tokenValidationResult.SecurityToken as JsonWebToken);
}
else
{
tokenEndpointUser = ValidateToken(tokenEndpointResponse.IdToken, properties, validationParameters, out tokenEndpointJwt);
}

// Avoid reading & deleting the nonce cookie, running the event, etc, if it was already done as part of the authorization response validation.
if (user == null)
Expand Down Expand Up @@ -1244,11 +1268,13 @@ private async Task<AuthenticationFailedContext> RunAuthenticationFailedEventAsyn
// Note this modifies properties if Options.UseTokenLifetime
private ClaimsPrincipal ValidateToken(string idToken, AuthenticationProperties properties, TokenValidationParameters validationParameters, out JwtSecurityToken jwt)
{
#pragma warning disable CS0618 // Type or member is obsolete
if (!Options.SecurityTokenValidator.CanReadToken(idToken))
{
Logger.UnableToReadIdToken(idToken);
throw new SecurityTokenException(string.Format(CultureInfo.InvariantCulture, Resources.UnableToValidateToken, idToken));
}
#pragma warning restore CS0618 // Type or member is obsolete

if (_configuration != null)
{
Expand All @@ -1259,7 +1285,9 @@ private ClaimsPrincipal ValidateToken(string idToken, AuthenticationProperties p
?? _configuration.SigningKeys;
}

#pragma warning disable CS0618 // Type or member is obsolete
var principal = Options.SecurityTokenValidator.ValidateToken(idToken, validationParameters, out SecurityToken validatedToken);
#pragma warning restore CS0618 // Type or member is obsolete
if (validatedToken is JwtSecurityToken validatedJwt)
{
jwt = validatedJwt;
Expand Down Expand Up @@ -1294,6 +1322,61 @@ private ClaimsPrincipal ValidateToken(string idToken, AuthenticationProperties p
return principal;
}

// Note this modifies properties if Options.UseTokenLifetime
private async Task<TokenValidationResult> ValidateTokenUsingHandlerAsync(string idToken, AuthenticationProperties properties, TokenValidationParameters validationParameters)
{
if (Options.ConfigurationManager is BaseConfigurationManager baseConfigurationManager)
{
validationParameters.ConfigurationManager = baseConfigurationManager;
}
else if (_configuration != null)
{
var issuer = new[] { _configuration.Issuer };
validationParameters.ValidIssuers = validationParameters.ValidIssuers?.Concat(issuer) ?? issuer;

validationParameters.IssuerSigningKeys = validationParameters.IssuerSigningKeys?.Concat(_configuration.SigningKeys)
?? _configuration.SigningKeys;
}

var validationResult = await Options.TokenHandler.ValidateTokenAsync(idToken, validationParameters);

if (validationResult.Exception != null)
{
throw validationResult.Exception;
}

var validatedToken = validationResult.SecurityToken;

if (!validationResult.IsValid || validatedToken == null)
{
Logger.UnableToValidateIdTokenFromHandler(idToken);
throw new SecurityTokenException(string.Format(CultureInfo.InvariantCulture, Resources.UnableToValidateTokenFromHandler, idToken));
}

if (validatedToken is not JsonWebToken)
{
Logger.InvalidSecurityTokenTypeFromHandler(validatedToken?.GetType().ToString());
throw new SecurityTokenException(string.Format(CultureInfo.InvariantCulture, Resources.ValidatedSecurityTokenNotJsonWebToken, validatedToken?.GetType()));
}

if (Options.UseTokenLifetime)
{
var issued = validatedToken.ValidFrom;
if (issued != DateTime.MinValue)
{
properties.IssuedUtc = issued;
}

var expires = validatedToken.ValidTo;
if (expires != DateTime.MinValue)
{
properties.ExpiresUtc = expires;
}
}

return validationResult;
}

/// <summary>
/// Build a redirect path if the given path is a relative path.
/// </summary>
Expand Down
36 changes: 33 additions & 3 deletions src/Security/Authentication/OpenIdConnect/src/OpenIdConnectOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.IdentityModel.Tokens.Jwt;
using Microsoft.AspNetCore.Authentication.OAuth.Claims;
using Microsoft.AspNetCore.Http;
using Microsoft.IdentityModel.JsonWebTokens;
using Microsoft.IdentityModel.Protocols;
using Microsoft.IdentityModel.Protocols.OpenIdConnect;
using Microsoft.IdentityModel.Tokens;
Expand All @@ -17,6 +18,12 @@ public class OpenIdConnectOptions : RemoteAuthenticationOptions
{
private CookieBuilder _nonceCookieBuilder;
private readonly JwtSecurityTokenHandler _defaultHandler = new JwtSecurityTokenHandler();
private readonly JsonWebTokenHandler _defaultTokenHandler = new JsonWebTokenHandler
{
MapInboundClaims = JwtSecurityTokenHandler.DefaultMapInboundClaims
};

private bool _mapInboundClaims = JwtSecurityTokenHandler.DefaultMapInboundClaims;

/// <summary>
/// Initializes a new <see cref="OpenIdConnectOptions"/>
Expand All @@ -37,7 +44,10 @@ public OpenIdConnectOptions()
CallbackPath = new PathString("/signin-oidc");
SignedOutCallbackPath = new PathString("/signout-callback-oidc");
RemoteSignOutPath = new PathString("/signout-oidc");
#pragma warning disable CS0618 // Type or member is obsolete
SecurityTokenValidator = _defaultHandler;
#pragma warning restore CS0618 // Type or member is obsolete
TokenHandler = _defaultTokenHandler;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should TokenHandler be plural, as in the WsFed and JwtBearer PRs? Or all singular? @keegan-caruso

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we discussed off-line. resolved.


Events = new OpenIdConnectEvents();
Scope.Add("openid");
Expand Down Expand Up @@ -253,8 +263,17 @@ public override void Validate()
/// <summary>
/// Gets or sets the <see cref="ISecurityTokenValidator"/> used to validate identity tokens.
/// </summary>
[Obsolete("SecurityTokenValidator is no longer used by default. Use TokenHandler instead. To continue using SecurityTokenValidator, set UseSecurityTokenValidator to true. See https://aka.ms/aspnetcore8/security-token-changes")]
public ISecurityTokenValidator SecurityTokenValidator { get; set; }

/// <summary>
/// Gets or sets the <see cref="TokenHandler"/> used to validate identity tokens.
/// <para>
/// This will be used instead of <see cref="SecurityTokenValidator"/> if <see cref="UseSecurityTokenValidator"/> is <see langword="false"/>
/// </para>
/// </summary>
public TokenHandler TokenHandler { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other PR returns public IList<TokenHandler> TokenHandlers { get; private set; } should everything be aligned or does that matter?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved


/// <summary>
/// Gets or sets the parameters used to validate identity tokens.
/// </summary>
Expand Down Expand Up @@ -353,14 +372,25 @@ public override CookieOptions Build(HttpContext context, DateTimeOffset expiresF
public TimeSpan RefreshInterval { get; set; } = ConfigurationManager<OpenIdConnectConfiguration>.DefaultRefreshInterval;

/// <summary>
/// Gets or sets the <see cref="MapInboundClaims"/> property on the default instance of <see cref="JwtSecurityTokenHandler"/> in SecurityTokenValidator, which is used when determining
/// Gets or sets the <see cref="MapInboundClaims"/> property on the default instance of <see cref="JwtSecurityTokenHandler"/> in SecurityTokenValidator
/// and default instance of <see cref="JsonWebTokenHandler"/> in TokenHandler, which is used when determining
/// whether or not to map claim types that are extracted when validating a <see cref="JwtSecurityToken"/>.
/// <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 => _defaultHandler.MapInboundClaims;
set => _defaultHandler.MapInboundClaims = value;
get => _mapInboundClaims;
set
{
_mapInboundClaims = value;
_defaultHandler.MapInboundClaims = value;
_defaultTokenHandler.MapInboundClaims = value;
}
}

/// <summary>
/// Gets or sets whether to use the <see cref="TokenHandler"/> or the <see cref="SecurityTokenValidator"/> for validating identity tokens.
/// </summary>
public bool UseSecurityTokenValidator { get; set; }
}
4 changes: 4 additions & 0 deletions src/Security/Authentication/OpenIdConnect/src/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,6 @@
#nullable enable
Microsoft.AspNetCore.Authentication.OpenIdConnect.OpenIdConnectHandler.OpenIdConnectHandler(Microsoft.Extensions.Options.IOptionsMonitor<Microsoft.AspNetCore.Authentication.OpenIdConnect.OpenIdConnectOptions!>! options, Microsoft.Extensions.Logging.ILoggerFactory! logger, System.Text.Encodings.Web.HtmlEncoder! htmlEncoder, System.Text.Encodings.Web.UrlEncoder! encoder) -> void
Microsoft.AspNetCore.Authentication.OpenIdConnect.OpenIdConnectOptions.TokenHandler.get -> Microsoft.IdentityModel.Tokens.TokenHandler!
Microsoft.AspNetCore.Authentication.OpenIdConnect.OpenIdConnectOptions.TokenHandler.set -> void
Microsoft.AspNetCore.Authentication.OpenIdConnect.OpenIdConnectOptions.UseSecurityTokenValidator.get -> bool
Microsoft.AspNetCore.Authentication.OpenIdConnect.OpenIdConnectOptions.UseSecurityTokenValidator.set -> void
6 changes: 6 additions & 0 deletions src/Security/Authentication/OpenIdConnect/src/Resources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,4 +135,10 @@
<data name="IdTokenCodeMissing" xml:space="preserve">
<value>Cannot process the message. Both id_token and code are missing.</value>
</data>
<data name="ValidatedSecurityTokenNotJsonWebToken" xml:space="preserve">
<value>The Validated Security Token must be of type JsonWebToken, but instead its tye is '{0}'.</value>
</data>
<data name="UnableToValidateTokenFromHandler" xml:space="preserve">
<value>Unable to validate the 'id_token', no suitable TokenHandler was found for: '{0}'."</value>
</data>
</root>
4 changes: 4 additions & 0 deletions src/Security/Authentication/test/OpenIdConnect/OpenIdConnectEventTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Primitives;
using Microsoft.IdentityModel.JsonWebTokens;
using Microsoft.IdentityModel.Protocols.OpenIdConnect;
using Microsoft.IdentityModel.Tokens;
using Microsoft.Net.Http.Headers;
Expand Down Expand Up @@ -1285,7 +1286,10 @@ private TestServer CreateServer(OpenIdConnectEvents events, RequestDelegate appC
EndSessionEndpoint = "http://testhost/end"
};
o.StateDataFormat = new TestStateDataFormat();
#pragma warning disable CS0618 // Type or member is obsolete
o.SecurityTokenValidator = new TestTokenValidator();
#pragma warning restore CS0618 // Type or member is obsolete
o.UseSecurityTokenValidator = true;
o.ProtocolValidator = new TestProtocolValidator();
o.BackchannelHttpHandler = new TestBackchannel();
});
Expand Down
Loading