OAuth - Creating event for ExchangeCode #9448

Menighin · 2019-04-17T01:19:28Z

Summary of the changes

Created new event OnExchangeCode with default implementation it had before
Created OAuthExchangeCodeContext to hold properties necessary for developer to customize their own ExchangeCode instead of overriding the whole OAuthHandler.

Addresses #9092 (Use case described on the issue)

Sync to upstream

dnfclas · 2019-04-17T01:19:40Z

All CLA requirements met.

Tratcher · 2019-04-17T15:26:34Z

src/Security/Authentication/OAuth/src/Events/OAuthEvents.cs

+        /// <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 =>


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?

Tratcher · 2019-04-17T15:29:43Z

src/Security/Authentication/OAuth/src/OAuthHandler.cs

@@ -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)


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

@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'

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.

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...

@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.

@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)

Tratcher

Better, I added some cleanup comments. I'll follow up on the API break.

Tratcher · 2019-04-17T22:50:20Z

src/Security/Authentication/OAuth/src/OAuthHandler.cs

+                { "client_id", Options.ClientId },	
+                { "redirect_uri", redirectUri },	
+                { "client_secret", Options.ClientSecret },	
+                { "code", code },	


Tratcher · 2019-04-17T22:50:59Z

src/Security/Authentication/OAuth/src/Events/OAuthExchangeCodeContext.cs

+            HttpContext context,
+            AuthenticationScheme scheme,
+            OAuthOptions options,
+            Dictionary<string, string> tokenRequestParameters)


Suggested change

Dictionary<string, string> tokenRequestParameters)

IDictionary<string, string> tokenRequestParameters)

Tratcher · 2019-04-17T22:51:12Z

src/Security/Authentication/OAuth/src/Events/OAuthExchangeCodeContext.cs

+        /// <summary>
+        /// Gets the code returned by the authentication provider after user authenticates
+        /// </summary>
+        public Dictionary<string, string> TokenRequestParameters { get; }


Suggested change

public Dictionary<string, string> TokenRequestParameters { get; }

public IDictionary<string, string> TokenRequestParameters { get; }

Tratcher · 2019-04-17T22:51:31Z

src/Security/Authentication/OAuth/src/Events/OAuthExchangeCodeContext.cs

+        }
+
+        /// <summary>
+        /// Gets the code returned by the authentication provider after user authenticates


Wrong comment?

Tratcher · 2019-04-17T22:52:09Z

src/Security/Authentication/OAuth/src/Events/OAuthExchangeCodeContext.cs

+namespace Microsoft.AspNetCore.Authentication.OAuth
+{
+    /// <summary>
+    /// Contains information about the exchanging code for the access token context.


Suggested change

/// Contains information about the exchanging code for the access token context.

/// Contains information about exchanging code for the access token context.

Tratcher · 2019-04-17T22:54:55Z

src/Security/Authentication/OAuth/src/Events/OAuthEvents.cs

+        /// </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);


PrepareCodeExchange?

I think BeforeExchangeCode is better, what you think?

Tratcher · 2019-04-17T23:28:56Z

@PinpointTownes

So I looked at the 9 known providers that override ExchangeCode to see why.
https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/search?q=ExchangeCodeAsync&unscoped_q=ExchangeCodeAsync

Summary:
reddit: Move the clientid & secret to the auth header, optional user agent
fitbit: same as reddit
yandex: same as reddit
yahoo: same as reddit
visual studio: weird request parameter names, values
weixin: get+query
stackexchange: x-www-form-urlencoded response
yammer: weird response format
qq: get+query & form response

I'd be more willing to take the break API if I thought they could take advantage of the change and the new event. The Visual Studio provider looks like the only one that could remove it's overload and use the event instead to remap parameters. That tells me the event might not be general enough. We also might need to add first class support for some of these other scenarios.

A full half of the providers made the same customization (reddit-like), moving the client id and secret to the auth header. If we made that a first class option then they could remove their overrides.

The other providers are unique enough and mess with both the request and response so neither first classing their behavior nor the new event seem helpful.

kevinchalet · 2019-04-26T17:26:29Z

@Tratcher supporting basic authentication in both the OAuth2 and OIDC handlers would be an excellent idea, since it's a common source of pain and could help cut some code in the aspnet-contrib providers.

I wonder if this event should also allow handling the code redeeming dance manually, pretty much like the OIDC handler with its AuthorizationCodeReceived event. Is there a particular reason not to align the two handlers?

kevinchalet · 2019-04-26T17:28:26Z

/cc @poke @martincostello

Tratcher · 2019-05-29T16:00:48Z

FYI we're considering first class PKCE support that would make this unnecessary. #7734

Tratcher · 2019-06-05T22:24:59Z

I don't know how neccissary this will be after #10928.

Tratcher · 2019-06-07T18:32:08Z

Please rebase on #10928 and we can see what's left.

Tratcher · 2019-06-26T15:38:50Z

Closing this for now due to the unresolved merge conflict and design questions. We can revisit in the future.

Menighin added 2 commits April 15, 2019 11:27

Merge pull request #1 from aspnet/master

280f4a4

Sync to upstream

Creating event for ExchangeCode

d3e3b81

Menighin requested review from analogrelay and Tratcher as code owners April 17, 2019 01:19

analogrelay added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Apr 17, 2019

Tratcher requested changes Apr 17, 2019

View reviewed changes

Menighin added 2 commits April 17, 2019 19:07

Simplifying the exchange event

1e60e82

Removing unused imports

fca8b60

Tratcher reviewed Apr 17, 2019

View reviewed changes

Menighin added 2 commits April 17, 2019 21:12

Revert default tokenRequestParameters

0b57464

Update OAuthExchangeCodeContext.cs

43e3c22

Eilon assigned Tratcher Apr 18, 2019

This was referenced May 29, 2019

Consider adding Code + PKCE support in the OpenID Connect handler #7734

Closed

Supporting basic authentication Token exchange in both the OAuth2 and OIDC handlers #10615

Open

Tratcher closed this Jun 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OAuth - Creating event for ExchangeCode #9448

OAuth - Creating event for ExchangeCode #9448

Menighin commented Apr 17, 2019 •

edited

Loading

dnfclas commented Apr 17, 2019 •

edited

Loading

Tratcher Apr 17, 2019

Tratcher Apr 17, 2019

Menighin Apr 17, 2019

Tratcher Apr 17, 2019

Menighin Apr 17, 2019

brentschmaltz May 30, 2019

Tratcher May 30, 2019

Tratcher left a comment

Tratcher Apr 17, 2019

Tratcher Apr 17, 2019

Tratcher Apr 17, 2019

Tratcher Apr 17, 2019

Tratcher Apr 17, 2019

Tratcher Apr 17, 2019

Menighin Apr 17, 2019

Tratcher commented Apr 17, 2019

kevinchalet commented Apr 26, 2019

kevinchalet commented Apr 26, 2019

Tratcher commented May 29, 2019

Tratcher commented Jun 5, 2019

Tratcher commented Jun 7, 2019

Tratcher commented Jun 26, 2019

	Dictionary<string, string> tokenRequestParameters)
	IDictionary<string, string> tokenRequestParameters)

	public Dictionary<string, string> TokenRequestParameters { get; }
	public IDictionary<string, string> TokenRequestParameters { get; }

	/// Contains information about the exchanging code for the access token context.
	/// Contains information about exchanging code for the access token context.

OAuth - Creating event for ExchangeCode #9448

OAuth - Creating event for ExchangeCode #9448

Conversation

Menighin commented Apr 17, 2019 • edited Loading

dnfclas commented Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Apr 17, 2019

kevinchalet commented Apr 26, 2019

kevinchalet commented Apr 26, 2019

Tratcher commented May 29, 2019

Tratcher commented Jun 5, 2019

Tratcher commented Jun 7, 2019

Tratcher commented Jun 26, 2019

Menighin commented Apr 17, 2019 •

edited

Loading

dnfclas commented Apr 17, 2019 •

edited

Loading