Skip to content

[API Proposal]: Add AdditionalAuthorizationParameters to OAuthOptions/OpenIdConnectOptions #51250

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

Closed
joegoldman2 opened this issue Oct 10, 2023 · 7 comments · Fixed by #54119
Closed
Labels
api-approved API was approved in API review, it can be implemented area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Milestone

Comments

@joegoldman2
Copy link
Contributor

Background and Motivation

Today, there's no easy way to add additional parameters to the OAuth/OIDC authorization request.

With OAuth, we have to override OAuthHandler<TOptions>.BuildChallengeUrl as often done in AspNet.Security.OAuth.Providers:

With OIDC, we can use OpenIdConnectEvents.OnRedirectToIdentityProvider like:

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddAuthentication().AddOpenIdConnect(options =>
{
   options.Events.OnRedirectToIdentityProvider = context =>
   {
       context.ProtocolMessage.SetParameter("audience", "api.atlassian.com");
       return Task.CompletedTask;
   };
});

More context: #39243 and #39503

Proposed API

We can introduce a simpler and more generic solution for adding additional parameters:

namespace Microsoft.AspNetCore.Authentication.OAuth;

public class OAuthOptions : RemoteAuthenticationOptions
{
+    public IDictionary<string, string> AdditionalAuthorizationParameters { get; } = new Dictionary<string, string>();
}
namespace Microsoft.AspNetCore.Authentication.OpenIdConnect;

public class OpenIdConnectOptions : RemoteAuthenticationOptions
{
+    public IDictionary<string, string> AdditionalAuthorizationParameters { get; } = new Dictionary<string, string>();
}

Usage Examples

services.AddAuthentication().AddOAuth("Jira", options =>
{
    options.AdditionalAuthorizationParameters.Add("audience", "api.atlassian.com");
});

Risks

Nothing I can think about now.

@joegoldman2 joegoldman2 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 10, 2023
@Tratcher Tratcher added area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed area-security api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Oct 10, 2023
@ghost
Copy link

ghost commented Oct 10, 2023

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@Tratcher Tratcher added this to the .NET 9 Planning milestone Oct 10, 2023
@ghost
Copy link

ghost commented Oct 10, 2023

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. 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 issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@halter73
Copy link
Member

halter73 commented Jan 18, 2024

Note: API below is for historical reference, but final API shape is in a subsequent comment.

API Review Notes:

  • Could we move AdditionalAuthorizationParameters down to the RemoteAuthenticationOptions base type for consistency?
    • This might imply support for handlers that don't really support it.
    • Not much code abstracts over RemoteAuthenticationOptions. Most people deal with the derived types directly.
  • What about the AdditionalAuthorizationParameters name?
    • What about AdditionalChallengeQueryParameters?
    • What about AdditionalAuthorizationQueryParameters? Let's do this
  • What do we think about exposing an IDictionary in options?

API Approved!

namespace Microsoft.AspNetCore.Authentication.OAuth;

public class OAuthOptions : RemoteAuthenticationOptions
{
+    public IDictionary<string, string> AdditionalAuthorizationQueryParameters { get; }
}

namespace Microsoft.AspNetCore.Authentication.OpenIdConnect;

public class OpenIdConnectOptions : RemoteAuthenticationOptions
{
+    public IDictionary<string, string> AdditionalAuthorizationQueryParameters { get; }
}

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews api-approved API was approved in API review, it can be implemented labels Jan 18, 2024
@halter73
Copy link
Member

halter73 commented Jan 19, 2024

@amcasey It turns out that even though these parameters are normally query string parameters, they can be form posts too if you set Options.AuthenticationMethod == OpenIdConnectRedirectBehavior.FormPost. Given that I now support the API as originally proposed without "Query" in the name, but I'll just remove the api-approved label for now, so we can wait until the normal Monday API review meeting to see if everyone else is onboard.

I still think the doc comments should point out that these parameters are typically part of the redirect query string.

You could even encode request parameters as JWTs, but I don't think our stack supports that.

@amcasey
Copy link
Member

amcasey commented Jan 22, 2024

In light of the info above, dropped "query" from the name.

API re-approved.

namespace Microsoft.AspNetCore.Authentication.OAuth;

public class OAuthOptions : RemoteAuthenticationOptions
{
+    public IDictionary<string, string> AdditionalAuthorizationParameters { get; }
}

namespace Microsoft.AspNetCore.Authentication.OpenIdConnect;

public class OpenIdConnectOptions : RemoteAuthenticationOptions
{
+    public IDictionary<string, string> AdditionalAuthorizationParameters { get; }
}

@amcasey amcasey added the api-approved API was approved in API review, it can be implemented label Jan 22, 2024
@Kahbazi
Copy link
Member

Kahbazi commented Jan 23, 2024

@amcasey I'm interested in taking this one. Would you accept a PR for this issue?

@amcasey
Copy link
Member

amcasey commented Jan 23, 2024

@Kahbazi Thanks! I believe we would accept a PR, but the relevant reviewers are absent right now, so it might take a little while to get feedback.

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants