-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Replace JObject with JsonDocument in Authentication #7105
Conversation
src/Security/Authentication/OAuth/src/Events/OAuthCreatingTicketContext.cs
Outdated
Show resolved
Hide resolved
If we get the C# 8 infrastructure working soon then we can use |
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.
@Tratcher thanks for tagging, it's much appreciated 👍
The new JSON stack looks promising and it's good to know we now have a JObject
-like API, which is much more convenient than directly using a low-level JSON reader.
When you have a moment, you may want to take a look at the aspnet-contrib providers to ensure we have everything we need. Some of our providers use some nasty hacks to work around non-standard implementations, like re-creating JObject
from query string parameters (e.g StackExchange) or extracting the user profile from the token response (e.g Myob).
|
||
return new AuthenticationTicket(context.Principal, context.Properties, Scheme.Name); | ||
|
||
using (var payload = JsonDocument.Parse(await response.Content.ReadAsStringAsync())) |
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.
Out of curiosity, do you happen to know why JsonDocument
implements IDisposable
?
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.
(at some point, we may want to move away from ReadAsStringAsync()
in both the "official" and aspnet-contrib providers. I'm even surprised @davidfowl didn't cringe about the fact it's allocatey as hell 😄)
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.
It's disposable because it uses pooled buffers.
Sticking with ReadAsStringAsync might be a good idea. I know it's a bit less efficient but it does handle a bunch of decoding issues for us. The new json reader is hardcoded to UTF8.
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.
Why would we keep using ReadAsStringAsync ?
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.
How about this, if the encoding is UTF8, use the fast path (don't go byte[] -> string -> JsonDocument), if it's not UTF8, in those 2% cases, then use ReadAsStringAsync.
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.
ReadAsStringAsync deals with charsets and BOMs for us. ReadBufferAsString is exactly the method we don't want to re-implement, it would be useful if it were public.
This code path is not sufficiently perf sensitive to warrant a fast and slow path. Logins are a 0.1% scenario. Correctness is far more important and easier to maintain with a single code path.
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.
@Tratcher We need performance tests for auth, we have none today and we know it performs poorly. I buy that it isn't on the hot path but lets not get lazy with our performance work. It's a tiny set of code to make it do a better thing in the 90% case. We can always fallback in the other scenarios.
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.
Yes we need perf tests for auth that runs on most requests like Cookie & Bearer. OAuth and OIDC are much lower priority when the results are cached for weeks.
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.
OK lets agree to get those added as part of preview3. I'd like to take this opportunity to know where we stand.
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.
Filed #7162
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.
Given JsonDocument
is disposable, we should consider who ends up disposing it, and consider passing JsonElement
around that points to the sub-payload rather than the whole JsonDocument
. Granted there is a usability burden here, but its a side effect of having a high-performance implementation which emphasizes on low allocations and performance.
src/Security/Authentication/JwtBearer/samples/JwtBearerSample/Startup.cs
Outdated
Show resolved
Hide resolved
src/Security/Authentication/JwtBearer/samples/JwtBearerSample/Startup.cs
Outdated
Show resolved
Hide resolved
src/Security/Authentication/Core/src/JsonDocumentAuthExtensions.cs
Outdated
Show resolved
Hide resolved
@@ -211,8 +211,8 @@ await context.SignOutAsync(OpenIdConnectDefaults.AuthenticationScheme, new Authe | |||
using (var payload = JsonDocument.Parse(await tokenResponse.Content.ReadAsStringAsync())) | |||
{ | |||
// Persist the new acess token | |||
props.UpdateTokenValue("access_token", payload.GetString("access_token")); | |||
props.UpdateTokenValue("refresh_token", payload.GetString("refresh_token")); | |||
props.UpdateTokenValue("access_token", payload.RootElement.GetString("access_token")); |
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.
nit: cache RootElement
in a local wherever its references 3+ times.
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.
Other than ToString()
usage and some redundant enumeration, LGTM!
/AzurePipelines run AspNetCore-ci |
Successfully queued 1 pipeline(s). |
7b62f66
to
1d34d50
Compare
@HaoK rebased and ready for a final review. |
/AzurePipelines run AspNetCore-ci |
Successfully queued 1 pipeline(s). |
1d34d50
to
bee3e86
Compare
} | ||
else if (contentType.MediaType.Equals("application/jwt", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
var userInfoEndpointJwt = new JwtSecurityToken(userInfoResponse); | ||
user = JObject.FromObject(userInfoEndpointJwt.Payload); | ||
user = JsonDocument.Parse(userInfoEndpointJwt.Payload.SerializeToJson()); |
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.
This is kinda gross. This isn't a hot path either right?
cc @ahsonkhan Some potential API feedback here.
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.
Not a hot path. It's a corner case that I haven't seen anyone use. We can talk to IdentityModel about giving us the raw json. @brentschmaltz
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.
Still, its is just a crappything to have to do.
user = userInformationReceivedContext.User; | ||
|
||
Options.ProtocolValidator.ValidateUserInfoResponse(new OpenIdConnectProtocolValidationContext() | ||
using (user) |
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.
This new using is unfortunate but I guess we can't avoid it :/
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.
Most of them can get squashed with the new C# 8 using syntax.
#4260