Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix default info_fields to 'name,email' #209

Merged
merged 2 commits into from
Jul 19, 2015
Merged

Conversation

blueplanet
Copy link
Contributor

Problem

Version 2.4, default not include the email field.

https://developers.facebook.com/blog/post/2015/07/08/graph-api-v2.4/

Fewer default fields for faster performance: To help improve performance on mobile network connections, we've reduced the number of fields that the API returns by default. You should now use the ?fields=field1,field2 syntax to declare all the fields you want the API to return.

The default values for 'omni_auth.auth' only name and id field.

"omniauth.auth":{
  "provider":"facebook",
  "uid":"xxxxxx",
  "info":{
    "name":"xxx",
    "image":"http://graph.facebook.com/....."
  },
  "credentials":{
    "token":"...",
    "expires_at":1441880921,
    "expires":true
  },
  "extra":{
    "raw_info": {
      "name":"xxx",
      "id":"xxxxxxxxx"
    }
  }
}

After

If set the fields to only 'email', the value of field name be set to the email value. So set fields to 'name,email'.

"omniauth.auth":{
  "provider":"facebook",
  "uid":"xxxxxxxxxx",
  "info":{
    "email":"xxxxx@gmail.com",
    "name":"xxx",
    "image":"http://graph.facebook.com/xxxxxxx"
  },
  "credentials":{
    "token":"...",
    "expires_at":1441880921,
    "expires":true
  },
  "extra":{
    "raw_info":{
      "name":"xxx",
      "email":"xxxxxx@gmail.com",
      "id":"xxxxxxxxxx"
    }
  }
}

@simi
Copy link
Owner

simi commented Jul 17, 2015

I'm ok with this. @mkdynamic can you confirm?

@mkdynamic
Copy link
Collaborator

@simi +1. Looks good to merge with me.

@simi
Copy link
Owner

simi commented Jul 17, 2015

@blueplanet can you please add change to README.md with defaults and add changelog record?

@blueplanet
Copy link
Contributor Author

@simi @mkdynamic Thank you for review! 👍
I changed README.md and added record to CHANGELOG. 🙇

simi added a commit that referenced this pull request Jul 19, 2015
Fix default info_fields to 'name,email'
@simi simi merged commit f20e906 into simi:master Jul 19, 2015
@simi
Copy link
Owner

simi commented Jul 19, 2015

❤️

@krzysiek1507
Copy link

New release today?

@dgilperez
Copy link

New release, please?

@senych-vitalii
Copy link

new release is really needed... I cannot use gem without it - I cannot get Email without it...

@vemv
Copy link

vemv commented Oct 24, 2015

You can reference the needed commit in your Gemfile.

@peterept
Copy link

peterept commented Nov 6, 2015

It's not returning info.verified or info.verified_email though, so you can't be sure that is really there email.. how do you get the verified flags?

@vemv
Copy link

vemv commented Nov 6, 2015

@peterept are you sure a user can "login with facebook" at all without having verified his FB email first?

I remember doing that check a few days ago - an unconfirmed FB account cannot connect with any API-consuming app.

@peterept
Copy link

peterept commented Nov 6, 2015

Hi @vemv is that documented somewhere?

What worked for me was to explicitly request the verified field:

config.omniauth :facebook, FBID,  FBSECRET, { scope: 'public_profile,email', info_fields: 'email,name,verified' }

@peterept
Copy link

peterept commented Nov 9, 2015

@vemv here's an update after some experimenting (Facebook v2.5):

  • The GOOD: If the user has signed up via email BUT not verified it, then you are correct - they can not complete the OAUTH flow - they will get a screen saying they need to verify their account.
  • The BAD: if the user has signed up via Phone Number and Verified it and has no email set, then they can complete the OAUTH flow - the email field will not be in the JSON. verified field is true.

So we can not rely on getting an email field.

@vemv
Copy link

vemv commented Nov 9, 2015

@peterept nice one, much appreciated! So verified_email cannot be ignored.

Do you think omniauth-facebook needs any change? Personally I don't. Maybe these details could be explained in a FAQ so one doesn't have to figure out stuff.

@peterept
Copy link

peterept commented Nov 9, 2015

Actually facebook don't support verified_email field either; they only support the verified flag (which is true if you verified by an email, but also true if you only verified with a phone number and no email).

https://developers.facebook.com/docs/graph-api/reference/user

In my account verified by phone, I added an email and did not verify it. The oauth data is the same - no email.

So technically, for facebook, if the email field is present then the email has been verified, otherwise the verified flag tells you if the account is verified by several ways (which may or may not be email).

I'm not sure about ominiauth-facebook, maybe it should add the verified_email if there is an email address. Or should omniauth itself handle it?

@herenow
Copy link

herenow commented Dec 9, 2015

I had a similar problem on a project where they implemented oauth following this article:

http://sourcey.com/rails-4-omniauth-using-devise-with-twitter-facebook-and-linkedin/

The problem with the above implementation is that they expected facebook to return a verified property, based on this value, they would determine if the email was "verified". After reading @peterept I know this is not what the verified property actually means to facebook.

Since facebook doesn't return the verified property anymore, all emails would be considered unverified, and a temporary email would be generated instead.

I know this problem is not actually related to omniauth-facebook, the article should be updated, but I hope this comment helps anyone else with a similar problem to find this thread.

@sfandrew
Copy link

@blueplanet

Still having problems getting user email
current setup

config.omniauth :facebook, ENV['FACEBOOK_APP_ID'], ENV['FACEBOOK_SECRET'], scope: 'email', info_fields: 'email'

omniauth_callbacks_controller.rb

def facebook
    # You need to implement the method below in your model (e.g. app/models/user.rb)
    @user = User.from_omniauth(request.env["omniauth.auth"])

    if @user.persisted?
      sign_in_and_redirect @user, :event => :authentication #this will throw if @user is not activated
      set_flash_message(:notice, :success, :kind => "Facebook") if is_navigational_format?
    else

      session["devise.facebook_data"] = request.env["omniauth.auth"]
      redirect_to new_user_registration_url
    end
end

def failure
    redirect_to root_path
end

user model

def self.from_omniauth(auth) where(provider: auth.provider, uid: auth.uid).first_or_create do |user| user.email = auth.info.email user.password = Devise.friendly_token[0,20] user.name = auth.info.name # assuming the user model has a name user.uid = auth.uid user.provider = auth.provider end end

def self.new_with_session(params, session) super.tap do |user| if data = session["devise.facebook_data"] && session["devise.facebook_data"]["extra"]["raw_info"] user.email = data["email"] if user.email.blank? end end end

@blueplanet
Copy link
Contributor Author

@sfandrew

If set the fields to only 'email', the value of field name be set to the email value. So set fields to 'name,email'.

So, you can try info_fields: 'name,email' ?

@sfandrew
Copy link

@blueplanet sorry for the rookie mistake.
Bundle update from v 1.40 to 3.0.0

@printercu
Copy link

@simi @blueplanet This breaks the code. I don't receive default fields anymore:

# (#<OmniAuth::Strategies::Facebook:0x007fbe6bd19d00>):1[21]
 > raw_info.keys
=> ["name", "email", "id"]

# def raw_info
#   @raw_info ||= access_token.get('me', info_options).parsed || {}
# end

> access_token.get('me', info_options.tap { |x| x[:params].delete(:fields) }).parsed.keys
["id",
 "email",
 "first_name",
 "gender",
 "last_name",
 "link",
 "locale",
 "name",
 "timezone",
 "updated_time",
 "verified"]

Does everybody like current default behaviour?

UPD For everybody who still wants whole public_profile: configure with info_fields: ''.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.