Replace JObject with JsonDocument in Authentication #7105

Tratcher · 2019-01-29T20:05:45Z

#4260

"Drop in" replacement with some odd gymnastics sometimes.
It's not as breaking as I thought it might be for derived OAuth handlers. ClaimActions shield them from most of the changes. They primarily have to call Parse with the new type. @PinpointTownes
Users doing advanced claims mapping or inspection via events may be broken.
Also fixed launch settings so the samples would work again.

src/Security/Authentication/OAuth/src/Events/OAuthCreatingTicketContext.cs

src/Security/Authentication/OAuth/src/JsonKeyClaimAction.cs

Tratcher · 2019-01-29T21:08:25Z

@ahsonkhan

Tratcher · 2019-01-29T22:02:10Z

If we get the C# 8 infrastructure working soon then we can use using var to clean up all of these new using blocks.

src/Security/Authentication/Core/src/JsonDocumentAuthExtensions.cs

kevinchalet

@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).

src/Security/Authentication/Core/src/JsonDocumentAuthExtensions.cs

kevinchalet · 2019-01-29T23:42:02Z

src/Security/Authentication/Facebook/src/FacebookHandler.cs

-
-            return new AuthenticationTicket(context.Principal, context.Properties, Scheme.Name);
-
+            using (var payload = JsonDocument.Parse(await response.Content.ReadAsStringAsync()))


Out of curiosity, do you happen to know why JsonDocument implements IDisposable?

(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 😄)

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.

Why would we keep using ReadAsStringAsync ?

https://github.com/dotnet/corefx/blob/28e216680a990ce4a58798aa0d2026c9d3e1f148/src/System.Net.Http/src/System/Net/Http/HttpContent.cs#L189-L245

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.

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.

@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.

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.

OK lets agree to get those added as part of preview3. I'd like to take this opportunity to know where we stand.

Filed #7162

src/Security/Authentication/test/ClaimActionTests.cs

ahsonkhan

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

src/Security/Authentication/Google/src/GoogleHandler.cs

src/Security/Authentication/OAuth/src/JsonKeyClaimAction.cs

src/Security/Authentication/OAuth/src/JsonSubKeyClaimAction.cs

src/Security/Authentication/OAuth/src/MapAllClaimsAction.cs

src/Security/Authentication/Core/src/JsonDocumentAuthExtensions.cs

src/Security/Authentication/OAuth/src/OAuthTokenResponse.cs

src/Security/Authentication/Core/src/JsonDocumentAuthExtensions.cs

src/Security/Authentication/OAuth/src/OAuthHandler.cs

ahsonkhan · 2019-01-30T23:46:29Z

src/Security/Authentication/OpenIdConnect/samples/OpenIdConnectSample/Startup.cs

@@ -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"));


nit: cache RootElement in a local wherever its references 3+ times.

ahsonkhan

Other than ToString() usage and some redundant enumeration, LGTM!

Tratcher · 2019-02-01T18:00:20Z

/AzurePipelines run AspNetCore-ci

azure-pipelines · 2019-02-01T18:00:33Z

Successfully queued 1 pipeline(s).

Tratcher · 2019-02-01T20:18:03Z

@HaoK rebased and ready for a final review.

Tratcher · 2019-02-04T20:30:04Z

/AzurePipelines run AspNetCore-ci

azure-pipelines · 2019-02-04T20:30:22Z

Successfully queued 1 pipeline(s).

davidfowl · 2019-02-05T14:12:51Z

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs

            }
            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());


This is kinda gross. This isn't a hot path either right?

cc @ahsonkhan Some potential API feedback here.

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

Still, its is just a crappything to have to do.

HaoK · 2019-02-05T21:09:29Z

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs

-            user = userInformationReceivedContext.User;
-
-            Options.ProtocolValidator.ValidateUserInfoResponse(new OpenIdConnectProtocolValidationContext()
+            using (user)


This new using is unfortunate but I guess we can't avoid it :/

Most of them can get squashed with the new C# 8 using syntax.

Tratcher added the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label Jan 29, 2019

Tratcher added this to the 3.0.0-preview3 milestone Jan 29, 2019

Tratcher self-assigned this Jan 29, 2019

Tratcher requested review from HaoK and BrennanConroy January 29, 2019 20:05

JamesNK reviewed Jan 29, 2019

View reviewed changes

src/Security/Authentication/OAuth/src/Events/OAuthCreatingTicketContext.cs Outdated Show resolved Hide resolved

src/Security/Authentication/OAuth/src/JsonKeyClaimAction.cs Show resolved Hide resolved

Eilon added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jan 29, 2019

BrennanConroy reviewed Jan 29, 2019

View reviewed changes

src/Security/Authentication/Core/src/JsonDocumentAuthExtensions.cs Show resolved Hide resolved

kevinchalet reviewed Jan 29, 2019

View reviewed changes

ahsonkhan reviewed Jan 30, 2019

View reviewed changes

ahsonkhan mentioned this pull request Jan 30, 2019

Seed the porting guide for going from Json.NET to System.Text.Json. dotnet/corefx#34944

Merged

ahsonkhan reviewed Jan 30, 2019

View reviewed changes

src/Security/Authentication/Core/src/JsonDocumentAuthExtensions.cs Show resolved Hide resolved

ahsonkhan reviewed Jan 30, 2019

View reviewed changes

src/Security/Authentication/OAuth/src/OAuthHandler.cs Show resolved Hide resolved

ahsonkhan reviewed Jan 30, 2019

View reviewed changes

ahsonkhan approved these changes Jan 30, 2019

View reviewed changes

natemcmaster mentioned this pull request Jan 31, 2019

Remove Newtonsoft.Json from the shared framework #4260

Closed

7 tasks

ahsonkhan approved these changes Jan 31, 2019

View reviewed changes

Tratcher force-pushed the tratcher/nojson branch from 7b62f66 to 1d34d50 Compare February 1, 2019 20:17

Tratcher added 5 commits February 4, 2019 13:49

Replace JObject with JsonDocument in Authentication #4260

06ff568

Use the pipe

c613e54

Add null array value

7691abf

Empty doc

dc101b9

More dispose

e8b0210

Tratcher added 4 commits February 4, 2019 13:49

Prefer JsonElement

dcdc9a0

Remove JsonDocument.GetString

8e74456

Remove double lookup

50aa4e8

Fix musicstore

bee3e86

Tratcher force-pushed the tratcher/nojson branch from 1d34d50 to bee3e86 Compare February 4, 2019 21:50

davidfowl reviewed Feb 5, 2019

View reviewed changes

HaoK reviewed Feb 5, 2019

View reviewed changes

HaoK approved these changes Feb 5, 2019

View reviewed changes

Tratcher merged commit 67037a0 into master Feb 5, 2019

Tratcher deleted the tratcher/nojson branch February 5, 2019 21:37

This was referenced Feb 5, 2019

[Announcement] Newtonsoft.Json types replaced in Microsoft.AspNetCore.Authentication APIs #7289

Closed

Newtonsoft.Json types replaced in Microsoft.AspNetCore.Authentication APIs aspnet/Announcements#341

Open

Some samples do not work out of box #7288

Open

martincostello mentioned this pull request Mar 9, 2019

Support ASP.NET Core 3.0 aspnet-contrib/AspNet.Security.OAuth.Providers#280

Merged

HaoK mentioned this pull request Sep 30, 2019

Newtonsoft.Json types replaced in Microsoft.AspNetCore.Authentication APIs dotnet/docs#14836

Closed

Tratcher mentioned this pull request Feb 1, 2020

Make it easier to get json property values dotnet/runtime#28588

Closed

DixitArora-MSFT mentioned this pull request Feb 25, 2020

Breaking changes should mention Json.Net vs System.Text.Json MicrosoftDocs/azure-docs#48643

Closed

WillMarcouiller mentioned this pull request Aug 1, 2022

OAuthCreatingTicketContext still uses Newtonsoft JObject rather than JsonElement as replaced back in 2019? #43034

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace JObject with JsonDocument in Authentication #7105

Replace JObject with JsonDocument in Authentication #7105

Tratcher commented Jan 29, 2019

Tratcher commented Jan 29, 2019

Tratcher commented Jan 29, 2019

kevinchalet left a comment

kevinchalet Jan 29, 2019

kevinchalet Jan 29, 2019

Tratcher Jan 30, 2019

davidfowl Jan 30, 2019

davidfowl Jan 30, 2019

Tratcher Jan 30, 2019 •

edited

davidfowl Jan 30, 2019

Tratcher Jan 30, 2019

davidfowl Jan 31, 2019

Tratcher Jan 31, 2019

ahsonkhan left a comment

ahsonkhan Jan 30, 2019

ahsonkhan left a comment

Tratcher commented Feb 1, 2019

azure-pipelines bot commented Feb 1, 2019

Tratcher commented Feb 1, 2019

Tratcher commented Feb 4, 2019

azure-pipelines bot commented Feb 4, 2019

davidfowl Feb 5, 2019

Tratcher Feb 5, 2019

davidfowl Feb 5, 2019 •

edited

HaoK Feb 5, 2019

Tratcher Feb 5, 2019


		return new AuthenticationTicket(context.Principal, context.Properties, Scheme.Name);

		using (var payload = JsonDocument.Parse(await response.Content.ReadAsStringAsync()))

Replace JObject with JsonDocument in Authentication #7105

Replace JObject with JsonDocument in Authentication #7105

Conversation

Tratcher commented Jan 29, 2019

Tratcher commented Jan 29, 2019

Tratcher commented Jan 29, 2019

kevinchalet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher Jan 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan left a comment

Choose a reason for hiding this comment

Tratcher commented Feb 1, 2019

azure-pipelines bot commented Feb 1, 2019

Tratcher commented Feb 1, 2019

Tratcher commented Feb 4, 2019

azure-pipelines bot commented Feb 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Feb 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher Jan 30, 2019 •

edited

davidfowl Feb 5, 2019 •

edited