Skip to content
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

Merged
merged 9 commits into from
Feb 5, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ internal static Task OnCreatingTicket(OAuthCreatingTicketContext context)
Helpers.ThrowIfConditionFailed(() => context.Identity.FindFirst(ClaimTypes.NameIdentifier)?.Value == "Id", "");
Helpers.ThrowIfConditionFailed(() => context.Identity.FindFirst("urn:facebook:link")?.Value == "https://www.facebook.com/myLink", "");
Helpers.ThrowIfConditionFailed(() => context.Identity.FindFirst(ClaimTypes.Name)?.Value == "AspnetvnextTest AspnetvnextTest", "");
Helpers.ThrowIfConditionFailed(() => context.User.SelectToken("id").ToString() == context.Identity.FindFirst(ClaimTypes.NameIdentifier)?.Value, "");
Helpers.ThrowIfConditionFailed(() => context.User.GetString("id") == context.Identity.FindFirst(ClaimTypes.NameIdentifier)?.Value, "");
Helpers.ThrowIfConditionFailed(() => context.ExpiresIn.Value == TimeSpan.FromSeconds(100), "");
Helpers.ThrowIfConditionFailed(() => context.AccessToken == "ValidAccessToken", "");
context.Principal.Identities.First().AddClaim(new Claim("ManageStore", "false"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ internal static Task OnCreatingTicket(OAuthCreatingTicketContext context)
Helpers.ThrowIfConditionFailed(() => context.Identity.FindFirst(ClaimTypes.Surname)?.Value == "AspnetvnextTest", "FamilyName is not valid");
Helpers.ThrowIfConditionFailed(() => context.Identity.FindFirst(ClaimTypes.Name)?.Value == "AspnetvnextTest AspnetvnextTest", "Name is not valid");
Helpers.ThrowIfConditionFailed(() => context.ExpiresIn.Value == TimeSpan.FromSeconds(1200), "ExpiresIn is not valid");
Helpers.ThrowIfConditionFailed(() => context.User != null, "User object is not valid");
context.Principal.Identities.First().AddClaim(new Claim("ManageStore", "false"));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ internal static Task OnCreatingTicket(OAuthCreatingTicketContext context)
Helpers.ThrowIfConditionFailed(() => context.Identity.FindFirst(ClaimTypes.NameIdentifier)?.Value == "fccf9a24999f4f4f", "Id is not valid");
Helpers.ThrowIfConditionFailed(() => context.Identity.FindFirst(ClaimTypes.Name)?.Value == "AspnetvnextTest AspnetvnextTest", "Name is not valid");
Helpers.ThrowIfConditionFailed(() => context.ExpiresIn.Value == TimeSpan.FromSeconds(3600), "ExpiresIn is not valid");
Helpers.ThrowIfConditionFailed(() => context.User != null, "User object is not valid");
Helpers.ThrowIfConditionFailed(() => context.Identity.FindFirst(ClaimTypes.NameIdentifier)?.Value == context.User.SelectToken("id").ToString(), "User id is not valid");
Helpers.ThrowIfConditionFailed(() => context.Identity.FindFirst(ClaimTypes.NameIdentifier)?.Value == context.User.GetString("id"), "User id is not valid");
context.Principal.Identities.First().AddClaim(new Claim("ManageStore", "false"));
}

Expand Down
2 changes: 2 additions & 0 deletions src/Security/Authentication/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
#launchSettings.json files are required for the auth samples. The ports must not change.
!launchSettings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"iisSettings": {
"windowsAuthentication": false,
"anonymousAuthentication": true,
"iisExpress": {
"applicationUrl": "http://localhost:1780/",
"sslPort": 0
}
},
"profiles": {
"IIS Express": {
"commandName": "IISExpress",
"launchBrowser": true,
"environmentVariables": {
"ASPNETCORE_ENVIRONMENT": "Development"
}
},
"CookieSample": {
"commandName": "Project",
"launchBrowser": true,
"environmentVariables": {
"ASPNETCORE_ENVIRONMENT": "Development"
},
"applicationUrl": "http://localhost:1782/"
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"iisSettings": {
"windowsAuthentication": false,
"anonymousAuthentication": true,
"iisExpress": {
"applicationUrl": "http://localhost:1771/",
"sslPort": 0
}
},
"profiles": {
"IIS Express": {
"commandName": "IISExpress",
"launchBrowser": true,
"environmentVariables": {
"ASPNETCORE_ENVIRONMENT": "Development"
}
},
"CookieSessionSample": {
"commandName": "Project",
"launchBrowser": true,
"environmentVariables": {
"ASPNETCORE_ENVIRONMENT": "Development"
},
"applicationUrl": "http://localhost:1776/"
}
}
}
14 changes: 14 additions & 0 deletions src/Security/Authentication/Core/src/JsonDocumentAuthExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Text.Json;

namespace Microsoft.AspNetCore.Authentication
{
public static class JsonDocumentAuthExtensions
Tratcher marked this conversation as resolved.
Show resolved Hide resolved
{
public static string GetString(this JsonElement element, string key) =>
element.TryGetProperty(key, out var property)
? property.ToString() : null;
Tratcher marked this conversation as resolved.
Show resolved Hide resolved
}
}
18 changes: 8 additions & 10 deletions src/Security/Authentication/Facebook/src/FacebookHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@
using System.Security.Cryptography;
using System.Text;
using System.Text.Encodings.Web;
using System.Text.Json;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Authentication.OAuth;
using Microsoft.AspNetCore.WebUtilities;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Newtonsoft.Json.Linq;

namespace Microsoft.AspNetCore.Authentication.Facebook
{
Expand Down Expand Up @@ -41,15 +41,13 @@ protected override async Task<AuthenticationTicket> CreateTicketAsync(ClaimsIden
throw new HttpRequestException($"An error occurred when retrieving Facebook user information ({response.StatusCode}). Please check if the authentication information is correct and the corresponding Facebook Graph API is enabled.");
}

var payload = JObject.Parse(await response.Content.ReadAsStringAsync());

var context = new OAuthCreatingTicketContext(new ClaimsPrincipal(identity), properties, Context, Scheme, Options, Backchannel, tokens, payload);
context.RunClaimActions();

await Events.CreatingTicket(context);

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

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

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?

Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Member

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@Tratcher Tratcher Jan 30, 2019

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #7162

{
var context = new OAuthCreatingTicketContext(new ClaimsPrincipal(identity), properties, Context, Scheme, Options, Backchannel, tokens, payload.RootElement);
context.RunClaimActions();
await Events.CreatingTicket(context);
return new AuthenticationTicket(context.Principal, context.Properties, Scheme.Name);
}
}

private string GenerateAppSecretProof(string accessToken)
Expand Down
16 changes: 8 additions & 8 deletions src/Security/Authentication/Google/src/GoogleHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@
using System.Net.Http.Headers;
using System.Security.Claims;
using System.Text.Encodings.Web;
using System.Text.Json;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Authentication.OAuth;
using Microsoft.AspNetCore.WebUtilities;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Newtonsoft.Json.Linq;

namespace Microsoft.AspNetCore.Authentication.Google
{
Expand All @@ -37,13 +37,13 @@ public GoogleHandler(IOptionsMonitor<GoogleOptions> options, ILoggerFactory logg
throw new HttpRequestException($"An error occurred when retrieving Google user information ({response.StatusCode}). Please check if the authentication information is correct and the corresponding Google+ API is enabled.");
}

var payload = JObject.Parse(await response.Content.ReadAsStringAsync());

var context = new OAuthCreatingTicketContext(new ClaimsPrincipal(identity), properties, Context, Scheme, Options, Backchannel, tokens, payload);
context.RunClaimActions();

await Events.CreatingTicket(context);
return new AuthenticationTicket(context.Principal, context.Properties, Scheme.Name);
using (var payload = JsonDocument.Parse(await response.Content.ReadAsStringAsync()))
Tratcher marked this conversation as resolved.
Show resolved Hide resolved
{
var context = new OAuthCreatingTicketContext(new ClaimsPrincipal(identity), properties, Context, Scheme, Options, Backchannel, tokens, payload.RootElement);
context.RunClaimActions();
await Events.CreatingTicket(context);
return new AuthenticationTicket(context.Principal, context.Properties, Scheme.Name);
}
}

// TODO: Abstract this properties override pattern into the base class?
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"iisSettings": {
"windowsAuthentication": false,
"anonymousAuthentication": true,
"iisExpress": {
"applicationUrl": "https://localhost:44318/",
"sslPort": 44318
}
},
"profiles": {
"IIS Express": {
"commandName": "IISExpress",
"launchBrowser": true,
"environmentVariables": {
"ASPNETCORE_ENVIRONMENT": "Development"
}
},
"SocialSample": {
"commandName": "Project",
"launchBrowser": true,
"environmentVariables": {
"ASPNETCORE_ENVIRONMENT": "Development"
},
"applicationUrl": "https://localhost:44318/"
}
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
using System;
using System.Buffers;
using System.Collections.Generic;
using System.IO;
using System.IO.Pipelines;
using System.Runtime.ExceptionServices;
using System.Text.Json;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Authentication;
using Microsoft.AspNetCore.Authentication.JwtBearer;
using Microsoft.AspNetCore.Builder;
Expand All @@ -10,7 +14,6 @@
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Net.Http.Headers;
using Newtonsoft.Json.Linq;

namespace JwtBearerSample
{
Expand Down Expand Up @@ -93,19 +96,37 @@ public void Configure(IApplicationBuilder app)
{
var reader = new StreamReader(context.Request.Body);
var body = await reader.ReadToEndAsync();
var obj = JObject.Parse(body);
var todo = new Todo() { Description = obj["Description"].Value<string>(), Owner = context.User.Identity.Name };
Todos.Add(todo);
using (var json = JsonDocument.Parse(body))
{
var obj = json.RootElement;
var todo = new Todo() { Description = obj.GetProperty("Description").GetString(), Owner = context.User.Identity.Name };
Todos.Add(todo);
}
}
else
{
response.ContentType = "application/json";
response.Headers[HeaderNames.CacheControl] = "no-cache";
var json = JToken.FromObject(Todos);
await response.WriteAsync(json.ToString());
Serialize(Todos, response.BodyPipe);
await response.BodyPipe.FlushAsync();
}
});
});
}

private void Serialize(IList<Todo> todos, IBufferWriter<byte> output)
{
var writer = new Utf8JsonWriter(output);
writer.WriteStartArray();
foreach (var todo in todos)
{
writer.WriteStartObject();
writer.WriteString("Description", todo.Description);
writer.WriteString("Owner", todo.Owner);
writer.WriteEndObject();
}
writer.WriteEndArray();
writer.Flush();
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@
using System.Net.Http.Headers;
using System.Security.Claims;
using System.Text.Encodings.Web;
using System.Text.Json;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Authentication.OAuth;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Newtonsoft.Json.Linq;

namespace Microsoft.AspNetCore.Authentication.MicrosoftAccount
{
Expand All @@ -30,13 +30,13 @@ protected override async Task<AuthenticationTicket> CreateTicketAsync(ClaimsIden
throw new HttpRequestException($"An error occurred when retrieving Microsoft user information ({response.StatusCode}). Please check if the authentication information is correct and the corresponding Microsoft Account API is enabled.");
}

var payload = JObject.Parse(await response.Content.ReadAsStringAsync());

var context = new OAuthCreatingTicketContext(new ClaimsPrincipal(identity), properties, Context, Scheme, Options, Backchannel, tokens, payload);
context.RunClaimActions();

await Events.CreatingTicket(context);
return new AuthenticationTicket(context.Principal, context.Properties, Scheme.Name);
using (var payload = JsonDocument.Parse(await response.Content.ReadAsStringAsync()))
{
var context = new OAuthCreatingTicketContext(new ClaimsPrincipal(identity), properties, Context, Scheme, Options, Backchannel, tokens, payload.RootElement);
context.RunClaimActions();
await Events.CreatingTicket(context);
return new AuthenticationTicket(context.Principal, context.Properties, Scheme.Name);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,8 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Security.Claims;
using Microsoft.AspNetCore.Authentication;
using Microsoft.AspNetCore.Authentication.MicrosoftAccount;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Authentication.OAuth;
using Microsoft.AspNetCore.Http;

namespace Microsoft.AspNetCore.Authentication.MicrosoftAccount
{
Expand All @@ -29,7 +27,7 @@ public MicrosoftAccountOptions()
ClaimActions.MapJsonKey(ClaimTypes.Name, "displayName");
ClaimActions.MapJsonKey(ClaimTypes.GivenName, "givenName");
ClaimActions.MapJsonKey(ClaimTypes.Surname, "surname");
ClaimActions.MapCustomJson(ClaimTypes.Email, user => user.Value<string>("mail") ?? user.Value<string>("userPrincipalName"));
ClaimActions.MapCustomJson(ClaimTypes.Email, user => user.GetString("mail") ?? user.GetString("userPrincipalName"));
}
}
}
6 changes: 3 additions & 3 deletions src/Security/Authentication/OAuth/src/ClaimAction.cs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Security.Claims;
using Newtonsoft.Json.Linq;
using System.Text.Json;

namespace Microsoft.AspNetCore.Authentication.OAuth.Claims
{
Expand Down Expand Up @@ -37,6 +37,6 @@ public ClaimAction(string claimType, string valueType)
/// <param name="userData">The source data to examine. This value may be null.</param>
/// <param name="identity">The identity to add Claims to.</param>
/// <param name="issuer">The value to use for Claim.Issuer when creating a Claim.</param>
public abstract void Run(JObject userData, ClaimsIdentity identity, string issuer);
public abstract void Run(JsonElement userData, ClaimsIdentity identity, string issuer);
}
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Security.Claims;
using System.Text.Json;
using Microsoft.AspNetCore.Authentication.OAuth.Claims;
using Newtonsoft.Json.Linq;

namespace Microsoft.AspNetCore.Authentication
{
Expand Down Expand Up @@ -69,7 +69,7 @@ public static void MapJsonSubKey(this ClaimActionCollection collection, string c
/// <param name="collection"></param>
/// <param name="claimType">The value to use for Claim.Type when creating a Claim.</param>
/// <param name="resolver">The Func that will be called to select value from the given json user data.</param>
public static void MapCustomJson(this ClaimActionCollection collection, string claimType, Func<JObject, string> resolver)
public static void MapCustomJson(this ClaimActionCollection collection, string claimType, Func<JsonElement, string> resolver)
{
collection.MapCustomJson(claimType, ClaimValueTypes.String, resolver);
}
Expand All @@ -82,7 +82,7 @@ public static void MapCustomJson(this ClaimActionCollection collection, string c
/// <param name="claimType">The value to use for Claim.Type when creating a Claim.</param>
/// <param name="valueType">The value to use for Claim.ValueType when creating a Claim.</param>
/// <param name="resolver">The Func that will be called to select value from the given json user data.</param>
public static void MapCustomJson(this ClaimActionCollection collection, string claimType, string valueType, Func<JObject, string> resolver)
public static void MapCustomJson(this ClaimActionCollection collection, string claimType, string valueType, Func<JsonElement, string> resolver)
{
collection.Add(new CustomJsonClaimAction(claimType, valueType, resolver));
}
Expand Down
Loading