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

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

Closed
1 task done
WillMarcouiller opened this issue Aug 1, 2022 · 6 comments
Labels
✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. Status: Resolved

Comments

@WillMarcouiller
Copy link

WillMarcouiller commented Aug 1, 2022

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

Currently writing an OAuthHandler. The OAuthCreatingTicketContext still requires a JObject as per below screenshot.

image

Yet, when I look at the source code, it is clear that JObject has been replaced by JsonElement.

https://github.com/dotnet/aspnetcore/blob/0c1a5de1abb14d9b5766374b64c320abb4ac5010/src/Security/Authentication/OAuth/src/Events/OAuthCreatingTicketContext.cs#L15

Also when I look at the file's history, I can see that the change took place on the 5th of Feb. 2019 as per change #7105.

https://github.com/dotnet/aspnetcore/commit/67037a003904efb67d1a9534c0c56936a37d5547#diff-50d7e69d5fb6bc63ce8a4cdd1c89c7de05253ccb7e90b9571946169132a83861

And an announcement has been made for it as well.

https://github.com/dotnet/aspnetcore/issues/7289

Expected Behavior

I then expected to be able to pass on a JsonElement as per examples for Google Authentication and the others.

https://github.com/dotnet/aspnetcore/blob/0c1a5de1abb14d9b5766374b64c320abb4ac5010/src/Security/Authentication/Google/src/GoogleHandler.cs

` ///
protected override async Task CreateTicketAsync(
ClaimsIdentity identity,
AuthenticationProperties properties,
OAuthTokenResponse tokens) {
// Get the Google user
var request = new HttpRequestMessage(HttpMethod.Get, Options.UserInformationEndpoint);
request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", tokens.AccessToken);
var response = await Backchannel.SendAsync(request, Context.RequestAborted);
if (!response.IsSuccessStatusCode)
{
throw new HttpRequestException($"An error occurred when retrieving Google user information ({response.StatusCode}). Please check if the authentication information is correct.");
}

        using (var payload = JsonDocument.Parse(await response.Content.ReadAsStringAsync(Context.RequestAborted)))
        {
            // Here, the payload.RootElement is a JsonElement.
            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);
       }
    }`

Steps To Reproduce

  1. Create new .Net 6 Project
  2. Install package Microsoft.AspNetCore.Authentication.OAuth

image

  1. Replicate code from Google Authentication

https://github.com/dotnet/aspnetcore/blob/0c1a5de1abb14d9b5766374b64c320abb4ac5010/src/Security/Authentication/Google/src/GoogleHandler.cs

  1. Then you should see a red underline for payload.RootElement

image

Exceptions (if any)

image

image

.NET Version

7.0.100-preview.6.22352.1

Anything else?

.Net 6 Project
VS 2022 v17.2.6
Nuget Package Manager v6.2.1

I noticed that the package Microsoft.AspNetCore.Authentication.OAuth was built from an archived source at

https://github.com/aspnet/Security/tree/93926543f8469614c2feb23de8a8c0561b8b2463

Rather than current source like the other authentication providers.

@WillMarcouiller
Copy link
Author

Looks like the package hasn't been updated since 2018.

image

What am I supposed to do when the official release ain't updated with latest changes?

@BrennanConroy
Copy link
Member

The package is part of the ASP.NET Core shared framework, so it is automatically included in ASP.NET Core apps.
https://docs.microsoft.com/aspnet/core/migration/22-to-30?view=aspnetcore-6.0&tabs=visual-studio#remove-obsolete-package-references

Remove the package reference to Microsoft.AspNetCore.Authentication.OAuth that you added and you should see System.Text.Json being used instead.

@adityamandaleeka adityamandaleeka added the ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. label Aug 1, 2022
@ghost ghost added the Status: Resolved label Aug 1, 2022
@WillMarcouiller
Copy link
Author

The package is part of the ASP.NET Core shared framework, so it is automatically included in ASP.NET Core apps. https://docs.microsoft.com/aspnet/core/migration/22-to-30?view=aspnetcore-6.0&tabs=visual-studio#remove-obsolete-package-references

Remove the package reference to Microsoft.AspNetCore.Authentication.OAuth that you added and you should see System.Text.Json being used instead.

When I remove the added package, all of my code is being highlighted in red like below screenshot.
image
How should I resolve this?

@BrennanConroy
Copy link
Member

You're probably writing a class library, so you need to add a framework reference.
https://docs.microsoft.com/aspnet/core/fundamentals/target-aspnetcore?view=aspnetcore-6.0&tabs=visual-studio

@WillMarcouiller
Copy link
Author

You're probably writing a class library, so you need to add a framework reference. https://docs.microsoft.com/aspnet/core/fundamentals/target-aspnetcore?view=aspnetcore-6.0&tabs=visual-studio

This seems to have solved my issue and yes, now the OAuthCreatingTicketContext now takes a JsonElement.

Now, this is the first time I ever encounter such an issue whilst writing a class library. Shouldn't such elements (FrameworkReference) be already included into the class library itself to ease it all? Because if you didn't have the kindness to answer my questions, I would still be stuck as I asked a question on StackOverflow and no answer came in. No one seems to know about it, except people who work in the framework itself.

@ghost
Copy link

ghost commented Aug 2, 2022

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.

@ghost ghost closed this as completed Aug 2, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 2, 2022
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. Status: Resolved
Projects
None yet
Development

No branches or pull requests

3 participants