Return givename and surname claims from Facebook provider by default.

[mikes-gh]

I know things are really busy round here so don't spend too much time looking at this. @Tratcher
Its a bit nitty.
If you don't think this is going anywhere please close.

I would like to (re)present my reasons for this.

• We are not really requesting any more information than name already provides but it is very useful to have givenname and surname separated from a programming perspective.
• First name and last name are actually pretty useful for the user site registration process and splitting the full name is not always straight forward e.g. Jesus De Souza
• Facebook and Google are by far the 2 most used social logins. It would be consistent for the providers to return the same set of http://schemas.xmlsoap.org/ws/2005/05/identity/ claims
• New developers that have never seen the source code may take a while to discover they need to add fields to Facebook options to get first_name and last_name (probably save a bit of forum traffic).
• The only impact this will have is a few more bytes in the request and JSON response for Facebook.
• No more permissions are requested to the Facebook end user (the request for info is still within the already requested scope)
• It requires minimal code change to the Facebook options and that is already tested
• It allows a better abstraction from the implementation of the provider to the end user developer.

Currently

Using the social sample and a minimal setup

Google

  app.UseGoogleAuthentication(new GoogleOptions
            {
                ClientId = Configuration["google:clientid"],
                ClientSecret = Configuration["google:clientsecret"]
            });

Returns

Hello Joe BLogs
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier: 9999999999999999999
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/givenname: Joe
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname: Blogs
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/name: Joe Blogs
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress: [email protected]
urn:google:profile: https://plus.google.com/99999999999999999999
Logout

Facebook

            app.UseFacebookAuthentication(new FacebookOptions
            {
                AppId = Configuration["facebook:appid"],
                AppSecret = Configuration["facebook:appsecret"],
            });

returns

Hello Joe Blogs
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier: 956866371056181
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress: [email protected]
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/name: Joe Blogs
Logout

Would it make sense to add?

Fields.Add("first_name");
Fields.Add("last_name");

To the Facebook options

[Tratcher]

We want to keep the defaults to minimum required functionality. Google is an interesting comparison, but what do the next 5 do? MSA, Twitter, Yahoo, Github, LinkedIn, etc.. https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/tree/dev/src

[mikes-gh]

I used Google as between it and Facebook they hold the 80% market share depending on what sources you look at.
If we are sticking to minimum functional request I wonder if facebook works without any fields in the request

[mikes-gh]

OK I accept your point its hard to be consistent with so many providers doing there own thing.. Out of interest what is the functional minimum for a provider

[mikes-gh]

Looking at the providers there seems a lot of inconsistency in the claims available.
Paypal,Foursquare and Yahoo return givenname and surname.
I guess the difference is with Facebook we have the choice of fields to request.

It seems reasonable and not anymore inconsistent than things already are to request these two fields to align with Google for common scenarios. However if you don't agree that's also a reasonable angle so please close.

[jarrettv]

@mikes-gh I support your issue and appreciate your well thought-out reasoning.

[kevinchalet]

@mikes-gh curious: why closing this thread? You had a valid point.

[mikes-gh]

Reopening as there seems to be some support for this.

[mikes-gh]

Even Microsoft thinks its a good idea to return given name and surname claims by default
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication.MicrosoftAccount/MicrosoftAccountHelper.cs
also see PR
#691

[Eilon]

We don't think it's a good idea in general for the middleware to alter what a service considers to be its own defaults. If the identity provider changes what it offers, it's a best practice to explicitly add the claims requests or scopes to the auth process.

[kevinchalet]

@Eilon I'm not sure to understand your point. A few months ago, folks at Facebook have decided to remove their old endpoints and to update their Graph API to include a fields parameter, probably to save some precious bandwidth. This parameter directly controls the properties returned by the userinfo endpoint in the JSON payload. If you don't specify it, you get a minimal response, with just the user identifier and the complete name.

This ticket is only about adding first_name and last_name to the list of defaults fields asked by the Facebook middleware to make it consistent with the other middleware (currently: email and name are automatically included: https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication.Facebook/FacebookOptions.cs#L29-L30)

[mikes-gh]

@Eilon if you use this argument then Facebook should return no fields as that is the default. Do we want that?
The difference with facebook api is that you have to select the fields. So why not give the user a set of defaults that more closely matches the other leading providers.

If you don't give a good set of defaults you put the burden back to the programmer to discover the Facebook graph Api fields fuctilnallity,

I thought one of the points of the oauth provider is to minimise the inconsistencies between providers.

[jarrettv]

I agree, the middleware should normalize oauth and aim for the 80/20 rule and reduce the amount of custom code needed.

[Eilon]

Do folks have an idea on where we would draw the line? Ultimately every provider will have its own set of data with whatever keys they want. For some it will just be "full name" (because the concept of a "first" and "last" name being separate fields makes little sense), so what then?

Plus, for some providers these fields might require additional authorization that might not be needed for a particular app. This could cause an auth failure.

I honestly don't mind too much that we add some fields, I just want to make sure it's for the right reasons.

As far as the Facebook provider adding some extra fields, well, I don't really like that either, for all the same reasons 😄

[Eilon]

Discussed with @Tratcher @hao @blowdart @vibronet @brentschmaltz and we concluded this would be a slippery slope: what about middle name? Age? Etc.

We feel that it's easy enough to add the fields in the FacebookOptions during configuration.

[kevinchalet]

Age? AFAIK, none of the built-in providers retrieves or stores it.

This ticket is all about consistency, not exhaustiveness.

Currently, a developer using User.FindFirst(ClaimTypes.GivenName) will get a value with the Google and MSFT providers, but not with the Facebook middleware (the claim won't be present). It may be a nasty surprise when trying to pre-fill a registration form.

[mikes-gh]

@PinpointTownes
Exactly the common scenario this issue is about.

@elion
I know its easy to request these fields. Its just it requires that developers 'discover' this functionality and the peculiarities of the Facebook API.
Please reconsider and save some forum traffic. and developer time.

[Eilon]

I guess the question is: what's so special about these fields? What you're saying might be true about GivenName right now, but that could change at any time due to a change in what the 3rd parties provide, no? I think that realistically an app has to specify exactly what it needs from each provider (and update their app every time a 3rd party provider changes their defaults) if it wants to have some reasonable guarantee about what data it gets back, no?

[mikes-gh]

I guess the question is: what's so special about these fields?

They are common registration fields returned by the other market leading providers.

What you're saying might be true about GivenName right now, but that could change at any time due to a change in what the 3rd parties provide, no?

Not really . Facebook would change the version of its api so the provider would need updating with new urls etc (as in the past)

I think that realistically an app has to specify exactly what it needs from each provider (and update their app every time a 3rd party provider changes their defaults) if it wants to have some reasonable guarantee about what data it gets back, no?

Every one except Facebook (AFAIK) works at the scope level. What fields are defined at he scope level is up to the provider..Facebook is the odd one out as it allows you to specify exactly which fields you want so in effect there are no defaults. So why not take advantage of this to make Facebook more like Google to give further abstraction of the implementation details to the developer.

[Eilon]

I deci

[Eilon]

Oh weird I don't know where that random text came from! I'm drinking my Starbucks Chai Tea Latte right now, no alcohol yet today 😄

[mikes-gh]

leaning towards including just these properties

Are you leaning enough for a PR to add these 2 claims before rc2

Code changes required are

Fields.Add("first_name");
Fields.Add("last_name");

to FacebookOptions constructor

so minimal risk testing etc

[Eilon]

@mikes-gh by all means send a PR and we'll have a look. I still have to convince my colleagues, though 😄

[mikes-gh]

I'd rather not pollute the system until a decision is made here.
Its only those 2 lines of code above so code review is trivial.

[mikes-gh]

Do you want me to do a PR? I did the PR for adding all the facebook field processing so I am farmiliar with this code

[Eilon]

If you have the time, sure 😄

[mikes-gh]

OK leave it with me I will do it this weekend.

[Eilon]

Thanks!

If there's a logical place to add a test, please try to do so as well (I'm not quite sure what type of tests we have for the Facebook provider already).

[mikes-gh]

Unfortunately I can no longer compile in VS 2015
I am on Windows 7.

Steps I am taking

• Delete all packages in .nuget folder
• dnvm upgrade -u (giving 1.0.0-rc2-16595 clr x86 win default)
• dotnet restore

Rebuild Solution in VS gives loads of

error NU1008: "netstandard1.3" is an unsupported framework.

Apologies probably a known thing.

[Tratcher]

I recommend using the command line for now.
git clean -xdf
build

[mikes-gh]

Still no joy

C:\Users\mikes\Testing\Security>git clean -xdf
Removing .build/
Removing .vs/
Removing artifacts/
Removing samples/CookieSample/CookieSample.xproj.user
Removing samples/CookieSample/project.lock.json
Removing samples/CookieSessionSample/CookieSessionSample.xproj.user
Removing samples/CookieSessionSample/project.lock.json
Removing samples/JwtBearerSample/JwtBearerSample.xproj.user
Removing samples/JwtBearerSample/project.lock.json
Removing samples/OpenIdConnect.AzureAdSample/OpenIdConnect.AzureAdSample.xproj.u
ser
Removing samples/OpenIdConnect.AzureAdSample/project.lock.json
Removing samples/OpenIdConnectSample/OpenIdConnectSample.xproj.user
Removing samples/OpenIdConnectSample/project.lock.json
Removing samples/SocialSample/SocialSample.xproj.user
Removing samples/SocialSample/project.lock.json
Removing src/Microsoft.AspNetCore.Authentication.Cookies/Microsoft.AspNetCore.Au
thentication.Cookies.xproj.user
Removing src/Microsoft.AspNetCore.Authentication.Cookies/project.lock.json
Removing src/Microsoft.AspNetCore.Authentication.Facebook/Microsoft.AspNetCore.A
uthentication.Facebook.xproj.user
Removing src/Microsoft.AspNetCore.Authentication.Facebook/bin/
Removing src/Microsoft.AspNetCore.Authentication.Facebook/obj/
Removing src/Microsoft.AspNetCore.Authentication.Facebook/project.lock.json
Removing src/Microsoft.AspNetCore.Authentication.Google/Microsoft.AspNetCore.Aut
hentication.Google.xproj.user
Removing src/Microsoft.AspNetCore.Authentication.Google/project.lock.json
Removing src/Microsoft.AspNetCore.Authentication.JwtBearer/Microsoft.AspNetCore.
Authentication.JwtBearer.xproj.user
Removing src/Microsoft.AspNetCore.Authentication.JwtBearer/project.lock.json
Removing src/Microsoft.AspNetCore.Authentication.MicrosoftAccount/Microsoft.AspN
etCore.Authentication.MicrosoftAccount.xproj.user
Removing src/Microsoft.AspNetCore.Authentication.MicrosoftAccount/project.lock.j
son
Removing src/Microsoft.AspNetCore.Authentication.OAuth/Microsoft.AspNetCore.Auth
entication.OAuth.xproj.user
Removing src/Microsoft.AspNetCore.Authentication.OAuth/project.lock.json
Removing src/Microsoft.AspNetCore.Authentication.OpenIdConnect/Microsoft.AspNetC
ore.Authentication.OpenIdConnect.xproj.user
Removing src/Microsoft.AspNetCore.Authentication.OpenIdConnect/project.lock.json

Removing src/Microsoft.AspNetCore.Authentication.Twitter/Microsoft.AspNetCore.Au
thentication.Twitter.xproj.user
Removing src/Microsoft.AspNetCore.Authentication.Twitter/project.lock.json
Removing src/Microsoft.AspNetCore.Authentication/Microsoft.AspNetCore.Authentica
tion.xproj.user
Removing src/Microsoft.AspNetCore.Authentication/project.lock.json
Removing src/Microsoft.AspNetCore.Authorization/Microsoft.AspNetCore.Authorizati
on.xproj.user
Removing src/Microsoft.AspNetCore.Authorization/project.lock.json
Removing src/Microsoft.AspNetCore.CookiePolicy/Microsoft.AspNetCore.CookiePolicy
.xproj.user
Removing src/Microsoft.AspNetCore.CookiePolicy/project.lock.json
Removing src/Microsoft.Owin.Security.Interop/Microsoft.Owin.Security.Interop.xpr
oj.user
Removing src/Microsoft.Owin.Security.Interop/project.lock.json
Removing test/Microsoft.AspNetCore.Authentication.Test/Microsoft.AspNetCore.Auth
entication.Test.xproj.user
Removing test/Microsoft.AspNetCore.Authentication.Test/Properties/
Removing test/Microsoft.AspNetCore.Authentication.Test/project.lock.json
Removing test/Microsoft.AspNetCore.Authorization.Test/Microsoft.AspNetCore.Autho
rization.Test.xproj.user
Removing test/Microsoft.AspNetCore.Authorization.Test/Properties/
Removing test/Microsoft.AspNetCore.Authorization.Test/project.lock.json
Removing test/Microsoft.AspNetCore.CookiePolicy.Test/Microsoft.AspNetCore.Cookie
Policy.Test.xproj.user
Removing test/Microsoft.AspNetCore.CookiePolicy.Test/Properties/
Removing test/Microsoft.AspNetCore.CookiePolicy.Test/project.lock.json
Removing test/Microsoft.Owin.Security.Interop.Test/Microsoft.Owin.Security.Inter
op.Test.xproj.user
Removing test/Microsoft.Owin.Security.Interop.Test/Properties/
Removing test/Microsoft.Owin.Security.Interop.Test/project.lock.json

C:\Users\mikes\Testing\Security>build
Downloading KoreBuild from https://github.com/aspnet/KoreBuild/archive/dev.zip
Building C:\Users\mikes\Testing\Security
dotnet_install: Preparing to install .NET Tools to C:\Users\mikes\AppData\Local\
Microsoft\dotnet
dotnet_install: 1.0.0.001540 is already installed.
Adding C:\Users\mikes\AppData\Local\Microsoft\dotnet\cli\bin to PATH
log  : Restoring packages for C:\Users\mikes\Testing\Security\.build\project.jso
n...
error: Object reference not set to an instance of an object.
Using makefile: .build\shade\makefile.shade
& : The module '.build' could not be loaded. For more information, run 'Import-
Module .build'.
At C:\Users\mikes\Testing\Security\.build\KoreBuild.ps1:64 char:2
+ &"$koreBuildFolder\Sake\0.2.2\tools\Sake.exe" -I $koreBuildFolder\shade -f $m
ake ...
+  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : ObjectNotFound: (.build\Sake\0.2.2\tools\Sake.ex
   e:String) [], CommandNotFoundException
    + FullyQualifiedErrorId : CouldNotAutoLoadModule


C:\Users\mikes\Testing\Security>

[Eilon]

I just did a clean clone and it's building just fine.

Perhaps clear out these folders too?

• C:\Users\mikes\AppData\Local\Microsoft\dotnet
• C:\Users\mikes\.nuget

?

[mikes-gh]

Must be something wrong with my environment.

• New fork
• Clean down as per recommendations (see below)

Still getting

error: Object reference not set to an instance of an object

Output enclosed

C:\Users\mikes\Testing>rm -Rf C:\Users\mikes\.nuget\packages

C:\Users\mikes\Testing>rm -Rf C:\USers\mikes\Testing\Security

C:\Users\mikes\Testing>rm -Rf  C:\Users\mikes\AppData\Local\Microsoft\dotnet

C:\Users\mikes\Testing>git clone https://github.com/mikes-gh/Security.git
Cloning into 'Security'...
remote: Counting objects: 9083, done.
remote: Compressing objects: 100% (46/46), done.
remote: Total 9083 (delta 20), reused 0 (delta 0), pack-reused 9037
Receiving objects: 100% (9083/9083), 2.53 MiB | 1.43 MiB/s, done.
Resolving deltas: 100% (7109/7109), done.
Checking connectivity... done.

C:\Users\mikes\Testing>cd Security

C:\Users\mikes\Testing\Security>build
Building C:\Users\mikes\Testing\Security
dotnet_install: Preparing to install .NET Tools to C:\Users\mikes\AppData\Local\
Microsoft\dotnet
dotnet_install: Downloading dotnet-win-x64.1.0.0.001540.zip from https://dotnetc
li.blob.core.windows.net/dotnet/beta/Binaries/1.0.0.001540
dotnet_install: Extracting zip
dotnet_install: The .NET Tools have been installed to C:\Users\mikes\AppData\Loc
al\Microsoft\dotnet\cli!
dotnet_install: Add 'C:\Users\mikes\AppData\Local\Microsoft\dotnet\cli\bin' to y
our PATH to use dotnet
Adding C:\Users\mikes\AppData\Local\Microsoft\dotnet\cli\bin to PATH
log  : Restoring packages for C:\Users\mikes\Testing\Security\.build\project.jso
n...
error: Object reference not set to an instance of an object.
Using makefile: .build\shade\makefile.shade
& : The module '.build' could not be loaded. For more information, run
'Import-Module .build'.
At C:\Users\mikes\Testing\Security\.build\KoreBuild.ps1:64 char:2
+ &"$koreBuildFolder\Sake\0.2.2\tools\Sake.exe" -I $koreBuildFolder\shade -f
$make ...
+  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : ObjectNotFound: (.build\Sake\0.2.2\tools\Sake.ex
   e:String) [], ParentContainsErrorRecordException
    + FullyQualifiedErrorId : CouldNotAutoLoadModule


C:\Users\mikes\Testing\Security>

[Eilon]

Hmm I dunno, that's pretty weird. I was able to do several successful clean builds (on brand new clones).

[Eilon]

A few other folders to try clearing out:

• %LOCALAPPDATA%\dnu\cache\
• %USERPROFILE%\.dnx\packages\
• %USERPROFILE%\.nuget\
• %LOCALAPPDATA%\NuGet\Cache\
• %LOCALAPPDATA%\NuGet\v3-cache\

[mikes-gh]

I wonder if its a powershell thing I see the build scripts changed to powershell recently.
I am on windows 7 sp1 with powershell 3. Wonder what the build servers or other users are running.

PS C:\Users\mikes> $PSVersionTable

Name                           Value
----                           -----
PSVersion                      3.0
WSManStackVersion              3.0
SerializationVersion           1.1.0.1
CLRVersion                     4.0.30319.42000
BuildVersion                   6.2.9200.16481
PSCompatibleVersions           {1.0, 2.0, 3.0}
PSRemotingProtocolVersion      2.2

[Eilon]

I ran the build from a CMD window. But apparently we do require PowerShell 4 (which Windows 7 doesn't have by default).

Here's what I have on Windows 10:

Name                           Value
----                           -----
PSVersion                      5.0.10586.63
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.10586.63
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1

[Eilon]

BTW logged a bug here to show some better error if the PowerShell version is too old: aspnet/KoreBuild#30

[mikes-gh]

Unfortunately powershell 5 gives same result.
I dont think this is anything todo with Powershell 3
I narrowed it down using powershell debug to this line (i substituted the paths variables)

Command

dotnet restore .build\project.json --packages .build -f https://www.myget.org/F/dnxtools/api/v3/index.json -v Debug

Result

trace: Running restore with 2 concurrent jobs.
trace: Reading project file C:\Users\mikes\Testing\Security\.build\project.json.
trace: Loaded project .build from C:\Users\mikes\Testing\Security\.build\project.json.
trace: Found project root directory: C:\Users\mikes\Testing\Security.
trace: Using packages directory: .build.
log  : Restoring packages for C:\Users\mikes\Testing\Security\.build\project.json...
trace: Restoring packages for DNX,Version=v4.5.1...
error: Object reference not set to an instance of an object.
trace: System.AggregateException: One or more errors occurred. (Object reference not set to an instance of an object.) ---> System.NullRefe
renceException: Object reference not set to an instance of an object.
trace:    at System.Net.Http.WinHttpAuthHelper.SetWinHttpCredential(SafeWinHttpHandle requestHandle, ICredentials credentials, Uri uri, UIn
t32 authScheme, UInt32 authTarget)
trace:    at System.Net.Http.WinHttpAuthHelper.PreAuthenticateRequest(WinHttpRequestState state, UInt32 proxyAuthScheme)
trace:    at System.Net.Http.WinHttpHandler.<StartRequest>d__101.MoveNext()

Doing a debug it seems dotnet restore does not supply any credentials to my proxy when in a powershell script.
If I run dotnet restore in a cmd window it seems works fine.

Thing used to work so it seems the new build scripts are not proxy friendly.
Now back to the real task.