sorcery
sorcery copied to clipboard
NoamB
Default initializer config for LinkedIn doesn't retrieve UID
Not sure if something's recently changed with LinkedIn's API, but I had a bug in my app that traces back to config.linkedin.user_info_fields = ['first-name', 'last-name'] in the initializer created by sorcery. During authentication, retrieving a uid fails since LinkedIn's response doesn't automatically include the field unless requested to.
My solution was simply config.linkedin.user_info_fields << "id", but I think the generator should at least be updated to reflect working usage. Alternatively, perhaps the LinkedIn adapter should automatically include the id field in it's request, since several internal methods depend on its presence?
From what I remember, we ran into a similar issue a couple months back where we didn't realize the :uid wasn't being included, and therefore, it created an Authentication instance with :uid => ""... so any user's attempt to login to LinkedIn would authenticate as THAT user. Not good, haha.
Our working config looks like this now:
config.linkedin.key = ENV['LINKEDIN_KEY']
config.linkedin.secret = ENV['LINKEDIN_SECRET']
config.linkedin.callback_url = "https://#{Rails.application.config.x.url}/oauth/callback?provider=linkedin"
config.linkedin.user_info_fields = ['id']
config.linkedin.access_permissions = ['r_basicprofile']
In my previous post I already mentioned what worked for me, which was adding "id" to the array of user_info_fields.
I think Sorcery should automatically request the ID field, since the integration doesn't work without it.
Sorry, I was reading it on my phone. I'll look at the generators. Honestly, we're probably going to take a serious look at the generators for 1.0.
Cool, but I think this is beyond generators -- If the provider code assumes there's a UID available (which it needs in order to work at all) then perhaps it should always be included in the requested fields regardless of config values?
That makes sense. We could make this default behavior for linkedin, but it would probably be good to generalize this sort of thing.
Yeah. I suspect (most of) the other providers automatically receive a uid from their respective API servers, it might just be LinkedIn that needs the explicit request. Haven't verified against Sorcery's code though.
I'm looking into this problem right now, but found something rather confusing. It looks like the ID param already is being required, so why does it need to be included twice?
Not really sure how I can manually call the method and run byebug on it, so I can't confirm this yet.
# lib/sorcery/providers/linkedin.rb
def get_user_hash(access_token)
fields = self.user_info_fields.join(',')
response = access_token.get("#{@user_info_path}:(id,#{fields})", 'x-li-format' => 'json')
auth_hash(access_token).tap do |h|
h[:user_info] = JSON.parse(response.body)
h[:uid] = h[:user_info]['id'].to_s
end
end
Just discovered a serious problem, linkedin's get_user_hash isn't being tested, period. I tried commenting out twitter's get_user_hash, 12 failures as expected. linkedin's get_user_hash, 0 failures. I'll add some tests for it and report back. Explains why my calls to byebug weren't being hit. :P
@athix yeah that seems weird. I just tested on an app of mine to confirm, it definitely only works if the ID is explicitly added to the config fields, otherwise the returned value is blank. Sorcery 0.9.1
Currently linkedin is using oauth 1.0, but they have a 2.0 version available. Thinking about just ditching the 1.0 code and making a new 2.0 module, but I don't know yet if that would cause others to need new keys/secrets. The other option is to refactor the 1.0 specs to use all 1.0 providers instead of just twitter. (or do both and have two modules for linkedin) @Ch4s3, @arnvald, opinions?
I think it might be best to drop Oauth support in Sorcery 1.0 for Linkedin. But, I'm not 100% confident in that decision. Thoughts?
Sorry, I meant OAuth 1.0 in favor of 2.0. I have to stop replying to these on my phone.
I would be in favor of dropping 1.0 support wherever a 2.0 solution is available. (With a deprecation warning on 0.9.X)
I'll see if I can make any progress on oauth 2.0 for linkedin this weekend, now that the Travis builds should be fixed.