Option to use JsonWebTokenHandler in oidc handler #49333
Conversation
ghost
added
the
community-contribution
dotnet-issue-labeler
bot
added
the
area-auth
|
Thanks for your PR, @keegan-caruso. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
eng/Versions.props
Outdated
| <MicrosoftIdentityModelLoggingVersion>6.15.1</MicrosoftIdentityModelLoggingVersion> | ||
| <MicrosoftIdentityModelProtocolsOpenIdConnectVersion>6.15.1</MicrosoftIdentityModelProtocolsOpenIdConnectVersion> | ||
| <MicrosoftIdentityModelProtocolsWsFederationVersion>6.15.1</MicrosoftIdentityModelProtocolsWsFederationVersion> | ||
| <MicrosoftIdentityModelLoggingVersion>7.0.0-preview</MicrosoftIdentityModelLoggingVersion> |
There was a problem hiding this comment.
Will 7.0.0 stable be released before .NET 8 ships?
There was a problem hiding this comment.
Yes, definitely. We can ship out of preview by end of August, at the latest. Depends on perf/stress testing and customer feedback.
| 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); |
There was a problem hiding this comment.
| 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: ".
There was a problem hiding this comment.
This is the same approach as taken in InvalidSecurityTokenType earlier in the file
| @@ -38,6 +40,10 @@ public OpenIdConnectOptions() | |||
| SignedOutCallbackPath = new PathString("/signout-callback-oidc"); | |||
| RemoteSignOutPath = new PathString("/signout-oidc"); | |||
| SecurityTokenValidator = _defaultHandler; | |||
| TokenHandler = _defaultTokenHandler; | |||
There was a problem hiding this comment.
Should TokenHandler be plural, as in the WsFed and JwtBearer PRs? Or all singular? @keegan-caruso
There was a problem hiding this comment.
we discussed off-line. resolved.
| /// This will be used instead of <see cref="SecurityTokenValidator"/> if <see cref="UseTokenHandler"/> is <see langword="true"/> | ||
| /// </para> | ||
| /// </summary> | ||
| public TokenHandler TokenHandler { get; set; } |
There was a problem hiding this comment.
The other PR returns public IList<TokenHandler> TokenHandlers { get; private set; } should everything be aligned or does that matter?
| @@ -266,7 +266,7 @@ await context.SignOutAsync(OpenIdConnectDefaults.AuthenticationScheme, new Authe | |||
| // 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)) | |||
There was a problem hiding this comment.
property.TryGetInt32 was throwing a InvalidOperationException on ValueKind not being a number
There was a problem hiding this comment.
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);
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/backport to release/8.0-preview7 |
|
Started backporting to release/8.0-preview7: https://github.com/dotnet/aspnetcore/actions/runs/5606681456 |
|
@BrennanConroy backporting to release/8.0-preview7 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Option to use JsonWebTokenHandler in OpenIdConnectHandler
Applying: fix sample
Applying: split event tests
Applying: Use 7.0.0-preview for identity model libraries
Applying: use var for identitymodel versions
error: sha1 information is lacking or useless (eng/Versions.props).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 use var for identitymodel versions
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@BrennanConroy an error occurred while backporting to release/8.0-preview7, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
Option to use JsonWebTokenHandler in oidc handler
Description
Gives the option to use JsonWebTokenHandler instead of JwtSecurityTokenHandler in OIDC Handler. This gives an async model, last known good support for metadata, and better performance.