Correct Twitter OAuth signing code. Also, refactor copy/pasted code & make reusable TwitterHandler.ExecuteRequestAsync public #1720

speige · 2018-04-06T13:34:00Z

Tested in my app & working

dnfclas · 2018-04-06T13:34:12Z

All CLA requirements met.

Tratcher

Before I review this too closely, please add a sample or test showing how you expect the new public APIs to be called from an application. Getting access to an instance of the handler is non-trivial.

Tratcher · 2018-04-06T15:56:03Z

src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs

 namespace Microsoft.AspNetCore.Authentication.Twitter
 {
-    public class TwitterHandler : RemoteAuthenticationHandler<TwitterOptions>
+  public class TwitterHandler : RemoteAuthenticationHandler<TwitterOptions>


Spacing changed? Revert.

Eilon · 2018-04-06T22:24:17Z

src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs

+
+		public async Task<HttpResponseMessage> ExecuteRequestAsync(string url, HttpMethod httpMethod, RequestToken accessToken = null, Dictionary<string, string> extraOAuthPairs = null, Dictionary<string, string> queryParameters = null, Dictionary<string, string> formData = null)
+    {
+      StringBuilder canonicalizedRequestBuilder = new StringBuilder();


Also use var everywhere in the new code.

speige · 2018-04-06T22:29:44Z

Accidental whitespace changes removed

speige · 2018-04-07T18:40:10Z

updated based on request to use "var" instead of specific types

speige · 2018-04-07T18:53:10Z

@Tratcher
The original intent of the PR wasn't to make the reusable method public (although that's an added bonus). It was because the code didn't match Twitter's API Spec (based on the issue I submitted). It seemed to work most of the time, but I was nervous there would be random hard to debug errors.

To get an instance of TwitterHandler via the constructor is painful. However, with DI it's not too bad.

I'm use .net core with SimpleInjector instead of the base DI. You have to use CrossWire to make classes registered with the built-in DI available to your custom classes. I assume it's trivial with the built-in DI, so I'm leaving out the DI code itself.

After DI of IHttpContextAccessor & AuthenticationHandlerProvider.

	        TwitterHandler twitterHandler = (TwitterHandler)await _authenticationHandlerProvider.GetHandlerAsync(_httpContextAccessor.HttpContext, "Twitter");

You could also DI TwitterHandler, but the Scheme won't be set, so you have to call InitializeAsync (which also requires DI of HttpContext).

Tratcher · 2018-04-07T19:20:34Z

Yeah, that's about what I'd expect. Can you add something to https://github.com/aspnet/Security/blob/dev/test/Microsoft.AspNetCore.Authentication.Test/TwitterTests.cs and/or https://github.com/aspnet/Security/blob/dev/samples/SocialSample/Startup.cs? We don't want to have any public APIs that we don't call, we'll never know if they break.

speige · 2018-04-09T20:16:17Z

@Tratcher I'm confused. Pretty much every test in TwitterTests.cs already calls ExecuteRequestAsync. If you add a breakpoint to the method & debug unit tests, you'll see it.

I'm kindof braindead when it comes to unit testing, so I'm not confident I can write a test that will be satisfactory. Would you mind coding that portion if it's important for the pull request to be accepted? Sorry :(

Tratcher · 2018-04-10T16:12:34Z

Sorry, I meant that shows what it's like to call the API directly like an end user would.

speige · 2018-04-10T18:23:26Z

Makes sense. I'll throw something together.

kevinchalet · 2018-04-10T20:13:20Z

Having public APIs in the handlers classes reminds me of a cool story about Pandora and a Box...

IMHO, pointing folks to user-friendly Twitter clients like https://github.com/JoeMayo/LinqToTwitter or https://github.com/linvi/tweetinvi would be much better than exposing an obscure method in the handler itself (both will take care of generating the Authorization header with all the signing madness for you)

speige · 2018-04-10T21:27:19Z

I'm not opposed to leaving the method private if it raises concerns. The initial intent of the pull request was because the current signing method did not match the spec. However, in that case, why have aspnet/Security implement the signing code itself instead of also referencing a 3rd party library?

Eilon · 2018-04-12T22:15:34Z

Let's keep it private for now because it might otherwise be difficult code to maintain forever.

Tratcher

This has a few tradeoffs that need to be worked out. I like how you've reduced the duplication, but some of the refactors have also reduced the readability and efficiency.

Tratcher · 2018-04-13T17:11:08Z

src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs

+            Logger.ObtainRequestToken();

-            var response = await Backchannel.SendAsync(request, Context.RequestAborted);
+            var response = await ExecuteRequestAsync(RequestTokenEndpoint, HttpMethod.Post, null, new Dictionary<string, string>() { { "oauth_callback", callBackUri } });


You can drop the null param, it has a default, and use a named parameter for the last param. The same goes for the other calls to ExecuteRequestAsync.

Tratcher · 2018-04-13T17:12:50Z

src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs

        }

-        private async Task<RequestToken> ObtainRequestTokenAsync(string callBackUri, AuthenticationProperties properties)
+        public async Task<HttpResponseMessage> ExecuteRequestAsync(string url, HttpMethod httpMethod, RequestToken accessToken = null, Dictionary<string, string> extraOAuthPairs = null, Dictionary<string, string> queryParameters = null, Dictionary<string, string> formData = null)


Let's make this private for now.

Wrap long lines over ~160 chars

Tratcher · 2018-04-13T17:14:30Z

src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs

-            canonicalizedRequestBuilder.Append("&");
-            canonicalizedRequestBuilder.Append(UrlEncoder.Encode(RequestTokenEndpoint));
+            var signatureParts = new SortedDictionary<string, string>((queryParameters ?? new Dictionary<string, string>()).Union(authorizationParts).Union(formData ?? new Dictionary<string, string>()).ToDictionary(x => x.Key, x => x.Value));
+            string parameterString = signatureParts.Select(x => string.Format("{0}={1}", Uri.EscapeDataString(x.Key), Uri.EscapeDataString(x.Value))).Aggregate((x, y) => string.Format("{0}&{1}", x, y));


Please revert the complex linq expressions. It reduces readability and code efficiency compared to the StringBuilder pattern that was used before.

Tratcher · 2018-04-13T17:16:16Z

src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs

-            canonicalizedRequestBuilder.Append(HttpMethod.Post.Method);
-            canonicalizedRequestBuilder.Append("&");
-            canonicalizedRequestBuilder.Append(UrlEncoder.Encode(RequestTokenEndpoint));
+            var signatureParts = new SortedDictionary<string, string>((queryParameters ?? new Dictionary<string, string>()).Union(authorizationParts).Union(formData ?? new Dictionary<string, string>()).ToDictionary(x => x.Key, x => x.Value));


This is the one linq expression that it may make sense to keep, but it needs a more readable layout rather than stringing it out all on one line.

Tratcher · 2018-04-17T16:34:25Z

Thanks for the help

…#1720)

speige mentioned this pull request Apr 6, 2018

Twitter Handler OAuth Signing code is incorrect #1695

Closed

Tratcher self-assigned this Apr 6, 2018

Tratcher reviewed Apr 6, 2018

View reviewed changes

Eilon reviewed Apr 6, 2018

View reviewed changes

Tratcher suggested changes Apr 13, 2018

View reviewed changes

Correct Twitter OAuth signing code & refactor copy/pasted code. #1695

d25b80e

Tratcher approved these changes Apr 17, 2018

View reviewed changes

Tratcher merged commit 2e3a107 into aspnet:dev Apr 17, 2018

Tratcher added this to the 2.2.0-mq milestone Apr 17, 2018

HaoK pushed a commit that referenced this pull request Jul 11, 2018

Correct Twitter OAuth signing code & refactor copy/pasted code. #1695 (…

1bba144

…#1720)

Correct Twitter OAuth signing code. Also, refactor copy/pasted code & make reusable TwitterHandler.ExecuteRequestAsync public #1720

Correct Twitter OAuth signing code. Also, refactor copy/pasted code & make reusable TwitterHandler.ExecuteRequestAsync public #1720

Uh oh!

Conversation

speige commented Apr 6, 2018

Uh oh!

dnfclas commented Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tratcher left a comment

Choose a reason for hiding this comment

Uh oh!

Tratcher Apr 6, 2018

Choose a reason for hiding this comment

Uh oh!

Eilon Apr 6, 2018

Choose a reason for hiding this comment

Uh oh!

speige commented Apr 6, 2018

Uh oh!

speige commented Apr 7, 2018

Uh oh!

speige commented Apr 7, 2018

Uh oh!

Tratcher commented Apr 7, 2018

Uh oh!

speige commented Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tratcher commented Apr 10, 2018

Uh oh!

speige commented Apr 10, 2018

Uh oh!

kevinchalet commented Apr 10, 2018

Uh oh!

speige commented Apr 10, 2018

Uh oh!

Eilon commented Apr 12, 2018

Uh oh!

Tratcher left a comment

Choose a reason for hiding this comment

Uh oh!

Tratcher Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

Tratcher Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

Tratcher Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

Tratcher Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Apr 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dnfclas commented Apr 6, 2018 •

edited

Loading

speige commented Apr 9, 2018 •

edited

Loading