Skip to content

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

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

Open
jamesgurung opened this issue Mar 6, 2023 · 18 comments
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer bug This issue describes a behavior which is not expected - a bug.

Comments

@jamesgurung
Copy link

jamesgurung commented Mar 6, 2023

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 : #47054 (comment)

@captainsafia captainsafia added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Mar 6, 2023
@Tratcher
Copy link
Member

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.

@mkArtakMSFT mkArtakMSFT added this to the 8.0-preview5 milestone Mar 30, 2023
@mkArtakMSFT
Copy link
Contributor

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

@Tratcher
Copy link
Member

Tratcher commented Apr 3, 2023

@mkArtakMSFT yes, this needs to be investigated.

@mkArtakMSFT mkArtakMSFT modified the milestones: 8.0-preview6, 8.0-rc1 Jun 26, 2023
@mkArtakMSFT mkArtakMSFT modified the milestones: 8.0-rc1, 8.0-rc2 Aug 9, 2023
@mkArtakMSFT mkArtakMSFT modified the milestones: 8.0-rc2, 8.0 Sep 6, 2023
@wtgodbe wtgodbe modified the milestones: 8.0, 8.0.0 Oct 3, 2023
@mkArtakMSFT mkArtakMSFT modified the milestones: 8.0.0, .NET 9 Planning Oct 11, 2023
@ghost
Copy link

ghost commented Oct 11, 2023

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.

@Tratcher Tratcher added the bug This issue describes a behavior which is not expected - a bug. label Oct 11, 2023
@PowerSaka
Copy link

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

@Tratcher Tratcher added the help wanted Up for grabs. We would accept a PR to help resolve this issue label Oct 24, 2023
@mkArtakMSFT mkArtakMSFT added help candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team and removed help wanted Up for grabs. We would accept a PR to help resolve this issue labels Oct 28, 2023
@Tratcher
Copy link
Member

Tratcher commented Oct 30, 2023

Found the regression: afb6ec4#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

return QueryHelpers.AddQueryString(Options.AuthorizationEndpoint, parameters!);

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:

return QueryHelpers.AddQueryString(Options.AuthorizationEndpoint, queryStrings);

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

Workaround:

  • Add the parameter in the RedirectToAuthorizationEndpoint event.
  .AddGoogle(o =>
  {
    o.ClientId = builder.Configuration["Google:ClientId"];
    o.ClientSecret = builder.Configuration["Google:ClientSecret"];
-    o.AuthorizationEndpoint += "?prompt=select_account"; // <-- Broken in .NET 8
+    o.Events = new OAuthEvents()
+    {
+        OnRedirectToAuthorizationEndpoint = c =>
+        {
+          c.RedirectUri += "&prompt=consent";
+          c.Response.Redirect(c.RedirectUri);
+          return Task.CompletedTask;
+        }
+    };
  });

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.

return QueryHelpers.AddQueryString(Options.AuthorizationEndpoint, queryStrings);

Note this is more complex than what other handlers support because they only support append, not per-challenge replacement of base parameters like scope.

SetQueryParam(queryStrings, properties, GoogleChallengeProperties.ScopeKey, FormatScope, Options.Scope);

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

@Tratcher Tratcher removed the help candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team label Oct 30, 2023
@Tratcher Tratcher added the help wanted Up for grabs. We would accept a PR to help resolve this issue label Oct 30, 2023
@Tratcher
Copy link
Member

@martincostello
Copy link
Member

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

@kevinchalet
Copy link
Contributor

@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
Copy link
Member

Tratcher commented Nov 1, 2023

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

@martincostello
Copy link
Member

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
Copy link
Member

Tratcher commented Nov 7, 2023

@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
Copy link
Member

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

mkArtakMSFT added a commit that referenced this issue Nov 9, 2023
Update the process to include summary comment link in the description of the issue.

This is important as I have noticed some issues have a long comment list and it's hard to discover the summary comment. Here is an example where I've noticed this: #47054
mkArtakMSFT added a commit that referenced this issue Nov 9, 2023
* Update HelpWantedProcess.md

Update the process to include summary comment link in the description of the issue.

This is important as I have noticed some issues have a long comment list and it's hard to discover the summary comment. Here is an example where I've noticed this: #47054

* Update docs/HelpWantedProcess.md

Co-authored-by: Mackinnon Buck <[email protected]>

* Update docs/HelpWantedProcess.md

Co-authored-by: Mackinnon Buck <[email protected]>

* Update docs/HelpWantedProcess.md

Co-authored-by: Stephen Halter <[email protected]>

* Update docs/HelpWantedProcess.md

---------

Co-authored-by: Mackinnon Buck <[email protected]>
Co-authored-by: Stephen Halter <[email protected]>
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@knopa
Copy link

knopa commented Apr 2, 2024

Any update on this?

@MattyLeslie
Copy link
Contributor

Should this be closed after the merged fixes ?

@imparikshith
Copy link

Is anybody contributing to this story, if not I would like to contribute.

@halter73
Copy link
Member

halter73 commented May 7, 2025

I'd hold off @imparikshith. We might stop shipping the Microsoft.AspnetCore.Authentication.Google package altogether. See #61817 for more details.

Instead, I'd recommend using the official official Google.Apis.Auth.AspNetCore3 NuGet package from Google which has a lot more functionality. You can find documentation for it at https://developers.google.com/api-client-library/dotnet/guide/aaa_oauth#web-applications-asp.net-core-3. It mentions "ASP.NET Core 3", but it works with .NET 6.0+ as noted on their GitHub README at https://github.com/googleapis/google-api-dotnet-client.

@halter73 halter73 removed the help wanted Up for grabs. We would accept a PR to help resolve this issue label May 7, 2025
@halter73 halter73 removed their assignment May 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer bug This issue describes a behavior which is not expected - a bug.
Projects
None yet
Development

No branches or pull requests

13 participants