dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.5k stars 10.04k forks source link

Updating to .NET 8 Preview 1 breaks AuthorizationEndpoint query on AddGoogle #47054

Open jamesgurung opened 1 year ago

jamesgurung commented 1 year ago

The following web application correctly shows a Google sign in page when it is built with net7.0 and version 7.0.3 of Microsoft.AspNetCore.Authentication.Google. Note that a prompt query parameter is added to the authorization endpoint to force an account selection dialog to appear, rather than automatically signing in with the current Google account.

In net8.0 (Preview 1), the same app shows an error page on accounts.google.com:

Access blocked: authorisation error OAuth 2 parameters can only have a single value: prompt Error 400: invalid_request

This is a breaking change, but I couldn't find it on the Breaking Changes in .NET 8 page. It might be enough of an edge case that it doesn't matter, but I thought I'd raise an issue just in case.

WebApplication1.csproj

<Project Sdk="Microsoft.NET.Sdk.Web">
  <PropertyGroup>
    <TargetFramework>net8.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.Authentication.Google" Version="8.0.0-preview.1.23112.2" />
  </ItemGroup>
</Project>

Program.cs

using Microsoft.AspNetCore.Authentication.Cookies;
using Microsoft.AspNetCore.Authentication.Google;

var builder = WebApplication.CreateBuilder(args);
builder.Services
  .AddAuthentication(o => {
    o.DefaultScheme = CookieAuthenticationDefaults.AuthenticationScheme;
    o.DefaultChallengeScheme = GoogleDefaults.AuthenticationScheme;
  })
  .AddCookie()
  .AddGoogle(o =>
  {
    o.ClientId = builder.Configuration["Google:ClientId"];
    o.ClientSecret = builder.Configuration["Google:ClientSecret"];
    o.AuthorizationEndpoint += "?prompt=select_account"; // <-- Broken in .NET 8
  });
var app = builder.Build();
app.UseAuthentication();
app.MapGet("/", () => Results.Challenge());
app.Run();

dotnet --version

8.0.100-preview.1.23115.2

Summary Comment : https://github.com/dotnet/aspnetcore/issues/47054#issuecomment-1786192809

Tratcher commented 1 year ago

What did the resulting url look like? It sounds like the options callback is being invoked twice for the same instance which would be bad. There was a bug in lazy options caching like that recently.

jamesgurung commented 1 year ago

@Tratcher The URL is:

https://accounts.google.com/o/oauth2/v2/auth?prompt=select_account&prompt=select_account&client_id=REDACTED.apps.googleusercontent.com&scope=openid%20profile%20email&response_type=code&redirect_uri=https%3A%2F%2Flocalhost%3A5001%2Fsignin-google&code_challenge=REDACTED&code_challenge_method=S256&state=REDACTED

mkArtakMSFT commented 1 year ago

@Tratcher do you think this is a bug we've introduced that we need to address?

Tratcher commented 1 year ago

@mkArtakMSFT yes, this needs to be investigated.

ghost commented 1 year ago

To learn more about what this message means, what to expect next, and how this issue will be handled you can read our Triage Process document. We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. Because it's not immediately obvious what is causing this behavior, we would like to keep this around to collect more feedback, which can later help us determine how to handle this. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact work.

PowerSaka commented 1 year ago

The last line of BuildChallengeUrl() function causes duplicate parameters: ?prompt=select_account&prompt=select_account

Tratcher commented 1 year ago

Found the regression: https://github.com/dotnet/aspnetcore/commit/afb6ec434fe319676d290b778e85468261c2bc7f#diff-dbfbf20eedf9c001d9d7848400c42e852216354b81f651f5bc82dbf42632475cR63 (thanks @PowerSaka)

To enable PKCE in 8.0 the Google auth handler was changed to call base.BuildChallengeUrl, var queryStrings = QueryHelpers.ParseQuery(new Uri(base.BuildChallengeUrl(properties, redirectUri)).Query);

which calls https://github.com/dotnet/aspnetcore/blob/c15938bb5f23fd3ad2374bde554a6b492a36ecde/src/Security/Authentication/OAuth/src/OAuthHandler.cs#L331

This results in a query param list that contains the original parameters from AuthorizationEndpoint. Once google adds it's extra properties to the list it calls this again: https://github.com/dotnet/aspnetcore/blob/c15938bb5f23fd3ad2374bde554a6b492a36ecde/src/Security/Authentication/Google/src/GoogleHandler.cs#L84

pulling the original parameters from the AuthorizationEndpoint again, causing duplication.

Workaround:

Possible fix: Since we know that base.BuildChallengeUrl already includes the original query params from AuthorizationEndpoint, drop them from the AuthorizationEndpoint before adding the final set of query params here. https://github.com/dotnet/aspnetcore/blob/c15938bb5f23fd3ad2374bde554a6b492a36ecde/src/Security/Authentication/Google/src/GoogleHandler.cs#L84

Note this is more complex than what other handlers support because they only support append, not per-challenge replacement of base parameters like scope. https://github.com/dotnet/aspnetcore/blob/c15938bb5f23fd3ad2374bde554a6b492a36ecde/src/Security/Authentication/Google/src/GoogleHandler.cs#L74

Also, adding GoogleOptions.PromptParamter like we have for other parameters would make direct manipulation of the AuthorizationEndpoint less necessary. Also consider ApprovalPrompt.

Tratcher commented 1 year ago

@kevinchalet @martincostello I see a similar issue here: https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/blob/163a006c3b3ec38a82da4cb0949c209ae4e04fd3/src/AspNet.Security.OAuth.VisualStudio/VisualStudioAuthenticationHandler.cs#L90-L100

martincostello commented 1 year ago

Thanks - I'll take a look at this some time in the next week to resolve that for our v8 release.

kevinchalet commented 1 year ago

@kevinchalet @martincostello I see a similar issue here: https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/blob/163a006c3b3ec38a82da4cb0949c209ae4e04fd3/src/AspNet.Security.OAuth.VisualStudio/VisualStudioAuthenticationHandler.cs#L90-L100

Thanks for letting us know. Did you see any other provider that may be affected?

If it's only VSO, then it's probably not a huge deal as it's almost a legacy thing: pretty much all APIs that use this provider can now be used with Azure AD directly (and Azure AD offers a much more OIDC-compliant implementation).

Tratcher commented 1 year ago

I didn't see any other providers with issues, they all append rather than try to replace parameters.

martincostello commented 1 year ago

Looking at the docs, I'm not sure if we really need to do anything in our VisualStudioOnline provider.

It seems that the only value recognised for response_type is Assertion, so if you were overriding it in AuthorizationEndpoint you would be either setting to to the same value we set for you already, so it would be redundant/duplicated, or you'd be setting it to something that isn't supported.

I can't see a use case for someone genuinely needing to set it themselves and getting the duplication issue, unless I'm misunderstanding something about how the original issue applies in our case.

Tratcher commented 1 year ago

@martincostello if someone added any query parameter to AuthorizationEndpoint it would get duplicated by the current code because of the pattern used to allow replacing an existing parameter. If nobody ever adds to AuthorizationEndpoint then you might never notice.

martincostello commented 1 year ago

Ah right, I see. Thanks - I'll take another look tomorrow.

knopa commented 7 months ago

Any update on this?

MattyLeslie commented 5 months ago

Should this be closed after the merged fixes ?