Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 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
49 changes: 48 additions & 1 deletion src/Security/Authentication/OAuth/src/Events/OAuthEvents.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Net.Http;
using System.Net.Http.Headers;
using System.Text.Json;
using System.Threading.Tasks;

namespace Microsoft.AspNetCore.Authentication.OAuth
Expand All @@ -25,6 +29,41 @@ 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<OAuthTokenResponse>> OnExchangeCode { get; set; } = async context =>
Copy link
Member

Choose a reason for hiding this comment

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

This is more code than is appropriate for events. Deriving from OAuthHandler is more appropriate once you need to replace this much.

Is there a more specific event that would still be useful? Like adding an event on line 45 that passed the parameters dictionary so it could be augmented? The default implementation would no-op. Would that be enough for your PKCE example?

{
var tokenRequestParameters = new Dictionary<string, string>()
{
{ "client_id", context.Options.ClientId },
{ "redirect_uri", context.RedirectUri },
{ "client_secret", context.Options.ClientSecret },
{ "code", context.Code },
{ "grant_type", "authorization_code" },
};

var requestContent = new FormUrlEncodedContent(tokenRequestParameters);

var requestMessage = new HttpRequestMessage(HttpMethod.Post, context.Options.TokenEndpoint);
requestMessage.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
requestMessage.Content = requestContent;

var response = await context.Backchannel.SendAsync(requestMessage, context.HttpContext.RequestAborted);
if (response.IsSuccessStatusCode)
{
var payload = JsonDocument.Parse(await response.Content.ReadAsStringAsync());
return OAuthTokenResponse.Success(payload);
}
else
{
var error = "OAuth token endpoint failure: "
+ $"Status: {response.StatusCode}; Headers: {response.Headers.ToString()}; Body: {await response.Content.ReadAsStringAsync()};";
return OAuthTokenResponse.Failed(new Exception(error));
}
};


/// <summary>
/// Invoked after the provider successfully authenticates a user.
/// </summary>
Expand All @@ -37,5 +76,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 after user authenticates on the provider to exchange the code gained 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<OAuthTokenResponse> ExchangeCode(OAuthExchangeCodeContext context) => OnExchangeCode(context);

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

namespace Microsoft.AspNetCore.Authentication.OAuth
{
/// <summary>
/// Contains information about the relevant to exchanging the code for the 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="backchannel">The HTTP client used by the authentication middleware</param>
/// <param name="redirectUri">The redirect URI</param>
/// <param name="code">The authorization code gained after user authentication</param>
public OAuthExchangeCodeContext(
AuthenticationProperties properties,
HttpContext context,
AuthenticationScheme scheme,
OAuthOptions options,
HttpClient backchannel,
string redirectUri,
string code)
: base(context, scheme, options, properties)
{
Backchannel = backchannel ?? throw new ArgumentNullException(nameof(backchannel));
RedirectUri = redirectUri;
Code = code;
}

/// <summary>
/// Gets the backchannel used to communicate with the provider.
/// </summary>
public HttpClient Backchannel { get; }

/// <summary>
/// Gets the URI used for the redirect operation.
/// </summary>
public string RedirectUri { get; }

/// <summary>
/// Gets the code returned by the authentication provider after user authenticates
Copy link
Member

Choose a reason for hiding this comment

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

Wrong comment?

/// </summary>
public string Code { get; }

}
}
40 changes: 4 additions & 36 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,42 +159,10 @@ 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>()
{
{ "client_id", Options.ClientId },
{ "redirect_uri", redirectUri },
{ "client_secret", Options.ClientSecret },
{ "code", code },
{ "grant_type", "authorization_code" },
};

var requestContent = new FormUrlEncodedContent(tokenRequestParameters);

var requestMessage = new HttpRequestMessage(HttpMethod.Post, Options.TokenEndpoint);
requestMessage.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
requestMessage.Content = requestContent;
var response = await Backchannel.SendAsync(requestMessage, Context.RequestAborted);
if (response.IsSuccessStatusCode)
{
var payload = JsonDocument.Parse(await response.Content.ReadAsStringAsync());
return OAuthTokenResponse.Success(payload);
}
else
{
var error = "OAuth token endpoint failure: " + await Display(response);
return OAuthTokenResponse.Failed(new Exception(error));
}
}

private static async Task<string> Display(HttpResponseMessage response)
{
var output = new StringBuilder();
output.Append("Status: " + response.StatusCode + ";");
output.Append("Headers: " + response.Headers.ToString() + ";");
output.Append("Body: " + await response.Content.ReadAsStringAsync() + ";");
return output.ToString();
var exchangeCodeContext = new OAuthExchangeCodeContext(properties, Context, Scheme, Options, Backchannel, redirectUri, code);
return await Events.OnExchangeCode(exchangeCodeContext);
}

protected virtual async Task<AuthenticationTicket> CreateTicketAsync(ClaimsIdentity identity, AuthenticationProperties properties, OAuthTokenResponse tokens)
Expand Down