Makes FacebookAuthenticationHandler respect Options.UserInformationEndpoint #366

bchavez · 2015-07-20T10:44:42Z

Fixes #365

dnfclas · 2015-07-20T10:44:46Z

Hi @bchavez, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

HaoK · 2015-07-20T21:06:46Z

Do you think you could add a test verifying your fix to this class?

https://github.com/aspnet/Security/blob/dev/test/Microsoft.AspNet.Authentication.Test/Facebook/FacebookMiddlewareTests.cs

bchavez · 2015-07-20T21:09:31Z

Sure, no problem, be back in a few. 👍

kevinchalet · 2015-07-20T21:51:44Z

src/Microsoft.AspNet.Authentication.Facebook/FacebookAuthenticationHandler.cs

+            var endpoint = Options.UserInformationEndpoint;
+            var accessToken = "access_token=" + UrlEncoder.UrlEncode(tokens.AccessToken);            
+
+            endpoint += endpoint.Contains("?") ? "&" : "?";


Note: you can also use QueryHelpers.AddQueryString 👪

@PinpointTownes Yeah, I was thinking about using a helper to build the query string, but wasn't sure about @HaoK 's thoughts on the subject since the other Auth providers don't do anything like that. I was exercising some caution not to move so much code. @HaoK , would you prefer that I use QueryHelpers?

Yeah please use QueryHelpers, the manual query string manipulation stood out when I saw the change as well.

bchavez · 2015-07-21T09:10:11Z

@HaoK @PinpointTownes Should be ok now. My new Unit Test is passing.

kevinchalet · 2015-07-24T11:05:56Z

src/Microsoft.AspNet.Authentication.Facebook/FacebookAuthenticationHandler.cs

@@ -51,10 +51,10 @@ public FacebookAuthenticationHandler(HttpClient httpClient)

        protected override async Task<AuthenticationTicket> CreateTicketAsync(ClaimsIdentity identity, AuthenticationProperties properties, OAuthTokenResponse tokens)
        {
-            var endpoint = Options.UserInformationEndpoint + "?access_token=" + UrlEncoder.UrlEncode(tokens.AccessToken);
+            var endpoint = QueryHelpers.AddQueryString(Options.UserInformationEndpoint, "access_token", UrlEncoder.UrlEncode(tokens.AccessToken));


When using QueryHelpers.AddQueryString, key and value are automatically url-encoded for you.
Pre-encoding them will result in a terrible double encoding. It's probably not a big deal since access tokens issued by Facebook only use alphanumerical chars, but it's not a guarantee and it may change in the future.

https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNet.WebUtilities/QueryHelpers.cs#L59-L62

@PinpointTownes Ah, thank you, I totally missed that. I've updated my branch accordingly. Thanks so much.

bchavez · 2015-07-24T16:50:04Z

1f8368e now voids double URL encoding with QueryHelpers. Thanks @PinpointTownes .

kevinchalet · 2015-07-24T17:04:54Z

You're welcome, @bchavez! 😄

On a side note, it seems that Facebook now supports the Authorization header with the Bearer scheme, just like the other OAuth2 providers. I just tested it and it works with the latest graph API.

Sadly, I can't find any reference 😢
That's really unfortunate, since flowing the access token in the headers is safer (intermediate proxies won't cache it)... but without an official reference, it's hard to know if it will be supported in the long term.

bchavez · 2015-07-24T17:25:04Z

@PinpointTownes Sounds great, hoping they publish a reference soon rather than passing access_token around the URL string.

Slightly off topic, but I tried to run the unit test again just to make sure everything was okay. But ran into a problem. Everything was working prior to opening the project today. After opening this project in VS2015 RTM, waited for everything to load, then saw the "package manager" console go crazy like it was doing some updates to dnx automatically. I'd prefer VS not do this without permission because I suspect the recent auto-update actions by VS broke my unit testing environment with this project.

------ Discover test started ------
------ Test started: Project: Microsoft.AspNet.Authentication.Test ------
Starting  Microsoft.Framework.TestHost [C:\Users\Profile\.dnx\runtimes\dnx-clr-win-x86.1.0.0-beta5\bin\dnx.exe --appbase "C:\Code\Projects\Public\Security\test\Microsoft.AspNet.Authentication.Test" Microsoft.Framework.ApplicationHost --port 47363 Microsoft.Framework.TestHost --port 47424]
System.MissingMethodException: Method not found: 'Boolean Microsoft.Framework.Runtime.Project.TryGetProject(System.String, Microsoft.Framework.Runtime.Project ByRef, System.Collections.Generic.ICollection`1<Microsoft.Framework.Runtime.DiagnosticMessage>)'.
   at Microsoft.Framework.TestHost.Program.<>c__DisplayClass2_0.<<Main>b__0>d.MoveNext()
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.Start[TStateMachine](TStateMachine& stateMachine)
   at Microsoft.Framework.TestHost.Program.<>c__DisplayClass2_0.<Main>b__0()
   at Microsoft.Framework.Runtime.Common.CommandLine.CommandLineApplication.<>c__DisplayClass68_0.<OnExecute>b__0()
   at Microsoft.Framework.Runtime.Common.CommandLine.CommandLineApplication.Execute(String[] args)
   at Microsoft.Framework.TestHost.Program.Main(String[] args)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at Microsoft.Framework.Runtime.Common.EntryPointExecutor.Execute(Assembly assembly, String[] args, IServiceProvider serviceProvider)
   at Microsoft.Framework.ApplicationHost.Program.ExecuteMain(DefaultHost host, String applicationName, String[] args)
   at Microsoft.Framework.ApplicationHost.Program.Main(String[] args)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at Microsoft.Framework.Runtime.Common.EntryPointExecutor.Execute(Assembly assembly, String[] args, IServiceProvider serviceProvider)
   at dnx.host.Bootstrapper.RunAsync(List`1 args, IRuntimeEnvironment env)
   at dnx.host.RuntimeBootstrapper.ExecuteAsync(String[] args)
   at dnx.host.RuntimeBootstrapper.Execute(String[] args)

Unable to start Microsoft.Framework.TestHost
========== Discover test finished: 0 found (0:00:03.4269352) ==========

:(

kevinchalet · 2015-07-24T17:51:12Z

I guess that's due to the fact you're using a beta5 DNX runtime while contributing on beta7 packages 😄
You can upgrade to a recent DNX version but sadly, there's currently a bug that prevents the latest one from working correctly. You can either wait for the next nightly build or use the volatile packages:

SET DNX_FEED=https://www.myget.org/F/aspnetvolatile/api/v2
dnvm upgrade -r clr

bchavez · 2015-07-24T17:58:20Z

I see. Thanks @PinpointTownes. My pull-request for Facebook here will eventually propagate to ASP.NET 4 too right? Ultimately, v4 is what I'm looking to fix at the moment. Thanks for all your help. 👍

kevinchalet · 2015-07-24T18:08:03Z

If by "ASP.NET 4" you mean Katana (aka Microsoft.Owin), nope, changes introduced in ASP.NET 5 are never backported. Actually, Katana is not under development anymore 😄

bchavez · 2015-07-24T18:10:01Z

Oh noes, so there is no way to fix this Nuget package?

https://www.nuget.org/packages/Microsoft.Owin.Security.Facebook/

:( :( :(

kevinchalet · 2015-07-24T18:18:50Z

Absolutely no way, I'm afraid. I guess the only reason the ASP.NET team would update a Katana package would be to fix a major security flaw.

Tratcher · 2015-07-27T22:38:46Z

Tratcher · 2015-07-28T17:20:29Z

Merged.

kevinchalet · 2015-07-28T18:05:32Z

Looks like the Authorization header support is not new (https://jira.spring.io/browse/SOCIAL-96), but the official page announcing it has since disappeared: http://developers.facebook.com/docs/authentication/oauth2_updates

👊

dnfclas added the cla-already-signed label Jul 20, 2015

kevinchalet reviewed Jul 20, 2015
View reviewed changes

kevinchalet reviewed Jul 24, 2015
View reviewed changes

Using QueryHelpers helps avoid issue #365.

1f8368e

bchavez changed the title * Makes FacebookAuthenticationHandler respect Options.UserInformationEndpoint Makes FacebookAuthenticationHandler respect Options.UserInformationEndpoint Jul 25, 2015

Tratcher closed this Jul 28, 2015

Tratcher mentioned this pull request Sep 4, 2015

Can't get email claim from Facebook #435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makes FacebookAuthenticationHandler respect Options.UserInformationEndpoint #366

Makes FacebookAuthenticationHandler respect Options.UserInformationEndpoint #366

bchavez commented Jul 20, 2015

dnfclas commented Jul 20, 2015

HaoK commented Jul 20, 2015

bchavez commented Jul 20, 2015

kevinchalet Jul 20, 2015

bchavez Jul 20, 2015

HaoK Jul 20, 2015

bchavez commented Jul 21, 2015

kevinchalet Jul 24, 2015

bchavez Jul 24, 2015

bchavez commented Jul 24, 2015

kevinchalet commented Jul 24, 2015

bchavez commented Jul 24, 2015

kevinchalet commented Jul 24, 2015

bchavez commented Jul 24, 2015

kevinchalet commented Jul 24, 2015

bchavez commented Jul 24, 2015

kevinchalet commented Jul 24, 2015

Tratcher commented Jul 27, 2015

Tratcher commented Jul 28, 2015

kevinchalet commented Jul 28, 2015

Makes FacebookAuthenticationHandler respect Options.UserInformationEndpoint #366

Makes FacebookAuthenticationHandler respect Options.UserInformationEndpoint #366

Conversation

bchavez commented Jul 20, 2015

dnfclas commented Jul 20, 2015

HaoK commented Jul 20, 2015

bchavez commented Jul 20, 2015

kevinchalet Jul 20, 2015

Choose a reason for hiding this comment

bchavez Jul 20, 2015

Choose a reason for hiding this comment

HaoK Jul 20, 2015

Choose a reason for hiding this comment

bchavez commented Jul 21, 2015

kevinchalet Jul 24, 2015

Choose a reason for hiding this comment

bchavez Jul 24, 2015

Choose a reason for hiding this comment

bchavez commented Jul 24, 2015

kevinchalet commented Jul 24, 2015

bchavez commented Jul 24, 2015

kevinchalet commented Jul 24, 2015

bchavez commented Jul 24, 2015

kevinchalet commented Jul 24, 2015

bchavez commented Jul 24, 2015

kevinchalet commented Jul 24, 2015

Tratcher commented Jul 27, 2015

Tratcher commented Jul 28, 2015

kevinchalet commented Jul 28, 2015