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

OAuth - Creating event for ExchangeCode #9448

Closed
wants to merge 6 commits into from
Closed
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
15 changes: 14 additions & 1 deletion src/Security/Authentication/OAuth/src/Events/OAuthEvents.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ public class OAuthEvents : RemoteAuthenticationEvents
return Task.CompletedTask;
};

/// <summary>
/// Gets or sets the delegate that is invoked when the ExchangeCode method is invoked.
/// </summary>
public Func<OAuthExchangeCodeContext, Task> OnExchangeCode { get; set; } = context => Task.CompletedTask;

/// <summary>
/// Invoked after the provider successfully authenticates a user.
/// </summary>
Expand All @@ -37,5 +42,13 @@ public class OAuthEvents : RemoteAuthenticationEvents
/// </summary>
/// <param name="context">Contains redirect URI and <see cref="AuthenticationProperties"/> of the challenge.</param>
public virtual Task RedirectToAuthorizationEndpoint(RedirectContext<OAuthOptions> context) => OnRedirectToAuthorizationEndpoint(context);

/// <summary>
/// Invoked before the request to exchange the code for the access token.
/// </summary>
/// <param name="context">Contains the code returned, the redirect URI and the <see cref="AuthenticationProperties"/>.</param>
/// <returns></returns>
public virtual Task ExchangeCode(OAuthExchangeCodeContext context) => OnExchangeCode(context);
Copy link
Member

Choose a reason for hiding this comment

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

PrepareCodeExchange?

Copy link
Author

Choose a reason for hiding this comment

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

I think BeforeExchangeCode is better, what you think?


}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// 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.Collections.Generic;
using Microsoft.AspNetCore.Http;

namespace Microsoft.AspNetCore.Authentication.OAuth
{
/// <summary>
/// Contains information about the context of exchanging code for access token .
/// </summary>
public class OAuthExchangeCodeContext : PropertiesContext<OAuthOptions>
{
/// <summary>
/// Initializes a new <see cref="OAuthExchangeCodeContext"/>.
/// </summary>
/// <param name="properties">The <see cref="AuthenticationProperties"/>.</param>
/// <param name="context">The HTTP environment.</param>
/// <param name="scheme">The authentication scheme.</param>
/// <param name="options">The options used by the authentication middleware.</param>
/// <param name="tokenRequestParameters">The parameters that will be sent as query string for the token request</param>
public OAuthExchangeCodeContext(
AuthenticationProperties properties,
HttpContext context,
AuthenticationScheme scheme,
OAuthOptions options,
IDictionary<string, string> tokenRequestParameters)
: base(context, scheme, options, properties)
{
TokenRequestParameters = tokenRequestParameters;
}

/// <summary>
/// Gets the request parameters for the token request
/// </summary>
public IDictionary<string, string> TokenRequestParameters { get; }
}
}
7 changes: 5 additions & 2 deletions src/Security/Authentication/OAuth/src/OAuthHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync
return HandleRequestResult.Fail("Code was not found.", properties);
}

using (var tokens = await ExchangeCodeAsync(code, BuildRedirectUri(Options.CallbackPath)))
using (var tokens = await ExchangeCodeAsync(code, BuildRedirectUri(Options.CallbackPath), properties))
{
if (tokens.Error != null)
{
Expand Down Expand Up @@ -159,7 +159,7 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync
}
}

protected virtual async Task<OAuthTokenResponse> ExchangeCodeAsync(string code, string redirectUri)
protected virtual async Task<OAuthTokenResponse> ExchangeCodeAsync(string code, string redirectUri, AuthenticationProperties properties)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a non-breaking way to add this new parameter? E.g. with a new overload?

Copy link
Author

Choose a reason for hiding this comment

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

@Tratcher hi, I was trying to make this non-breaking but I really don't see how... An overload won't work because now it is called from HandleRemoteAuthenticateAsync with the properties parameter. So to make, it non breakable I would need the new method to call the old one. And that is not possible because I would need to pass the tokenRequestParameters setted in the new method to the old one... I couldn't think of anything else, do you see a way out of this? How bad is this breaking change? n_n'

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/aspnet-contrib/AspNet.Security.OAuth.Providers/search?q=ExchangeCodeAsync&unscoped_q=ExchangeCodeAsync
It would break at least 9 providers, which is enough to try to mitigate. Let's think about it while we deal with the other issue.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, the first one about simply augmenting the Dictionary, I totally agree with you and I already changed it.
Will be giving some thought about how to not break it... But I don't see now how to make it without some smelly artifact...

Copy link
Contributor

Choose a reason for hiding this comment

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

@Tratcher is it possible to add a new API ExchangeCodeAsnyc(ExchangeCodeContext context)?

This would future proof us a bit. We have used this in other places.

Copy link
Member

Choose a reason for hiding this comment

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

@brentschmaltz yes, that's plausible. Either way it's a breaking change for many providers. If we added PKCE support directly we might be able to avoid this break. #9448 (comment)

{
var tokenRequestParameters = new Dictionary<string, string>()
{
Expand All @@ -170,6 +170,9 @@ protected virtual async Task<OAuthTokenResponse> ExchangeCodeAsync(string code,
{ "grant_type", "authorization_code" },
};

var exchangeCodeContext = new OAuthExchangeCodeContext(properties, Context, Scheme, Options, tokenRequestParameters);
await Events.OnExchangeCode(exchangeCodeContext);

var requestContent = new FormUrlEncodedContent(tokenRequestParameters);

var requestMessage = new HttpRequestMessage(HttpMethod.Post, Options.TokenEndpoint);
Expand Down