Remove access token for profile picture URL

[Rubyist007]

Access token for profile picture URL is not necessary and could lead to problems with expired access tokens when URL stored in DB

This pull request removes the access token param in the image URL, which resolves the issue I posted: #387

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[Rubyist007]

I made a tiny update of the readme corresponding to the changes

[simi]

What about to provide 2 methods to cover both variants (app scoped id and user id)? Or any other way to keep both variants working? Maybe additional argument or so?

[Rubyist007]

Thanks for your response @simi!

What about to provide 2 methods to cover both variants (app scoped id and user id)? Or any other way to keep both variants working? Maybe additional argument or so?

I'm not sure what exactly you are mean, in this PR there are no changes related to app scoped id or user id

How I can see (UID) is passed as an argument to an image_url method and in this PR we don't affect this argument

omniauth-facebook/lib/omniauth/strategies/facebook.rb

Lines 163 to 178 in ebfca0e

	def image_url(uid, options)
	uri_class = options[:secure_image_url] ? URI::HTTPS : URI::HTTP
	site_uri = URI.parse(client.site)
	url = uri_class.build({host: site_uri.host, path: "#{site_uri.path}/#{uid}/picture"})
	query = { access_token: access_token.token }
	if options[:image_size].is_a?(String) || options[:image_size].is_a?(Symbol)
	query[:type] = options[:image_size]
	elsif options[:image_size].is_a?(Hash)
	query.merge!(options[:image_size])
	end
	url.query = Rack::Utils.build_query(query)
	url.to_s
	end

If you mean that we should support behaviour that received UID will be user-id and for that case, we will need access_token in our URL to get the image
In that case, I'm not sure if such a case could happen, how received UID can be user-id if the gem uses app access tokens?
That's why we need to provide FACEBOOK_APP_ID and FACEBOOK_APP_SECRET and use the Facebook app access token (Facebook doc)

omniauth-facebook/lib/omniauth/strategies/facebook.rb

Lines 15 to 19 in ebfca0e

	option :client_options, {
	site: "https://graph.facebook.com/#{DEFAULT_FACEBOOK_API_VERSION}",
	authorize_url: "https://www.facebook.com/#{DEFAULT_FACEBOOK_API_VERSION}/dialog/oauth",
	token_url: 'oauth/access_token'
	}

User uid for picture url we take from /me endpoint (Facebook doc) that works the same as user endpoint (Facebook doc)

omniauth-facebook/lib/omniauth/strategies/facebook.rb

Lines 58 to 68 in ebfca0e

	def raw_info
	@raw_info ||= access_token.get('me', info_options).parsed || {}
	end
	def info_options
	params = {appsecret_proof: appsecret_proof}
	params.merge!({fields: (options[:info_fields] || 'name,email')})
	params.merge!({locale: options[:locale]}) if options[:locale]
	{ params: params }
	end

omniauth-facebook/lib/omniauth/strategies/facebook.rb

Line 32 in ebfca0e

uid { raw_info['id'] }

omniauth-facebook/lib/omniauth/strategies/facebook.rb

Line 41 in ebfca0e

'image' => image_url(uid, options),

And the Facebook documentation tells us that returned ID is App-Scoped User ID

The app user's App-Scoped User ID. This ID is unique to the app and cannot be used by other apps.

So for me, it looks like there is no way that we could use another type of ID

If I missing some use cases when gem could use user id instead of app scoped id or misunderstood the way of gem behaviour please let me know

[Rubyist007]

Hi @simi
I see that you reopened this PR 3 times so I believe that this PR is interesting for you
Waiting for your comment or any action related to it

I just don't want this PR to become forgotten without some final decision about it, so just pinging you

[simi]

Yup, I need to disable this auto-closing somehow.

[Rubyist007]

I believe behaviour with auto-closing PR could be edited here: https://github.com/simi/omniauth-facebook/blob/master/.github/workflows/stale.yml

It looks like it closes PR with no activity in 5 days based on this line:

omniauth-facebook/.github/workflows/stale.yml

Line 21 in 15e9a4b

days-before-close: 5

[simi]

Thanks @Rubyist007, updated ca9b64f. I'll try to find some time to revisit this, since I think I'm mixing 2 issues together here probably and I don't fully follow for now.

[Rubyist007]

Welcome

Yep sure, no problem

[Rubyist007]

@simi Hi
Just a kind reminder about PR