From 208005f9c9662773b436c4ffa14272ac0888bb04 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 28 Nov 2018 15:49:24 +0000 Subject: [PATCH] REFACTOR: Migrate FacebookAuthenticator to use ManagedAuthenticator Changes to functionality - Removed syncing of user metadata including gender, location etc. These are no longer available to standard Facebook applications. - Removed the remote 'revoke' functionality. No other providers have it, and it does not appear to be standard practice in other apps. - The 'facebook_no_email' event is no longer logged. The system can cope fine with a missing email address. Data is migrated to the new user_associated_accounts table. facebook_user_infos can be dropped once we are confident the data has been migrated successfully. --- app/models/facebook_user_info.rb | 30 ---- app/models/user.rb | 2 +- app/models/user_history.rb | 2 +- app/services/user_anonymizer.rb | 2 +- ...181128140547_migrate_facebook_user_info.rb | 27 ++++ lib/auth/facebook_authenticator.rb | 149 +----------------- script/bulk_import/discourse_merger.rb | 12 +- .../auth/facebook_authenticator_spec.rb | 107 +++---------- spec/models/user_spec.rb | 2 +- spec/services/user_anonymizer_spec.rb | 4 +- spec/services/user_merger_spec.rb | 4 +- 11 files changed, 70 insertions(+), 271 deletions(-) delete mode 100644 app/models/facebook_user_info.rb create mode 100644 db/migrate/20181128140547_migrate_facebook_user_info.rb diff --git a/app/models/facebook_user_info.rb b/app/models/facebook_user_info.rb deleted file mode 100644 index fd1ee517518..00000000000 --- a/app/models/facebook_user_info.rb +++ /dev/null @@ -1,30 +0,0 @@ -class FacebookUserInfo < ActiveRecord::Base - belongs_to :user -end - -# == Schema Information -# -# Table name: facebook_user_infos -# -# id :integer not null, primary key -# user_id :integer not null -# facebook_user_id :bigint(8) not null -# username :string -# first_name :string -# last_name :string -# email :string -# gender :string -# name :string -# link :string -# created_at :datetime not null -# updated_at :datetime not null -# avatar_url :string -# about_me :text -# location :string -# website :text -# -# Indexes -# -# index_facebook_user_infos_on_facebook_user_id (facebook_user_id) UNIQUE -# index_facebook_user_infos_on_user_id (user_id) UNIQUE -# diff --git a/app/models/user.rb b/app/models/user.rb index 97415261ca6..96908e4755a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -65,7 +65,7 @@ class User < ActiveRecord::Base has_one :user_option, dependent: :destroy has_one :user_avatar, dependent: :destroy - has_one :facebook_user_info, dependent: :destroy + has_many :user_associated_accounts, dependent: :destroy has_one :twitter_user_info, dependent: :destroy has_one :github_user_info, dependent: :destroy has_one :google_user_info, dependent: :destroy diff --git a/app/models/user_history.rb b/app/models/user_history.rb index ae1851c0eb3..745188bc7ba 100644 --- a/app/models/user_history.rb +++ b/app/models/user_history.rb @@ -28,7 +28,7 @@ class UserHistory < ActiveRecord::Base notified_about_dominating_topic: 9, suspend_user: 10, unsuspend_user: 11, - facebook_no_email: 12, + facebook_no_email: 12, # not used anymore grant_badge: 13, revoke_badge: 14, auto_trust_level_change: 15, diff --git a/app/services/user_anonymizer.rb b/app/services/user_anonymizer.rb index daaa9763010..a1f227b0f2e 100644 --- a/app/services/user_anonymizer.rb +++ b/app/services/user_anonymizer.rb @@ -56,9 +56,9 @@ class UserAnonymizer @user.twitter_user_info.try(:destroy) @user.google_user_info.try(:destroy) @user.github_user_info.try(:destroy) - @user.facebook_user_info.try(:destroy) @user.single_sign_on_record.try(:destroy) @user.oauth2_user_infos.try(:destroy_all) + @user.user_associated_accounts.try(:destroy_all) @user.instagram_user_info.try(:destroy) @user.user_open_ids.find_each { |x| x.destroy } @user.api_key.try(:destroy) diff --git a/db/migrate/20181128140547_migrate_facebook_user_info.rb b/db/migrate/20181128140547_migrate_facebook_user_info.rb new file mode 100644 index 00000000000..9209991d13d --- /dev/null +++ b/db/migrate/20181128140547_migrate_facebook_user_info.rb @@ -0,0 +1,27 @@ +class MigrateFacebookUserInfo < ActiveRecord::Migration[5.2] + def up + execute <<~SQL + INSERT INTO user_associated_accounts ( + provider_name, + provider_uid, + user_id, + info, + last_used, + created_at, + updated_at + ) SELECT + 'facebook', + facebook_user_id, + user_id, + json_build_object('email', email, 'first_name', first_name, 'last_name', last_name, 'name', name), + updated_at, + created_at, + updated_at + FROM facebook_user_infos + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/auth/facebook_authenticator.rb b/lib/auth/facebook_authenticator.rb index 01d86c76842..a8c2ffff911 100644 --- a/lib/auth/facebook_authenticator.rb +++ b/lib/auth/facebook_authenticator.rb @@ -1,4 +1,4 @@ -class Auth::FacebookAuthenticator < Auth::Authenticator +class Auth::FacebookAuthenticator < Auth::ManagedAuthenticator AVATAR_SIZE ||= 480 @@ -10,158 +10,17 @@ class Auth::FacebookAuthenticator < Auth::Authenticator SiteSetting.enable_facebook_logins end - def description_for_user(user) - info = FacebookUserInfo.find_by(user_id: user.id) - info&.email || info&.username || "" - end - - def can_revoke? - true - end - - def revoke(user, skip_remote: false) - info = FacebookUserInfo.find_by(user_id: user.id) - raise Discourse::NotFound if info.nil? - - if skip_remote - info.destroy! - return true - end - - response = Excon.delete(revoke_url(info.facebook_user_id)) - - if response.status == 200 - info.destroy! - return true - end - - false - end - - def revoke_url(fb_user_id) - "https://graph.facebook.com/#{fb_user_id}/permissions?access_token=#{SiteSetting.facebook_app_id}|#{SiteSetting.facebook_app_secret}" - end - - def can_connect_existing_user? - true - end - - def after_authenticate(auth_token, existing_account: nil) - result = Auth::Result.new - - session_info = parse_auth_token(auth_token) - facebook_hash = session_info[:facebook] - - result.email = email = session_info[:email] - result.email_valid = !email.blank? - result.name = facebook_hash[:name] - - result.extra_data = facebook_hash - - user_info = FacebookUserInfo.find_by(facebook_user_id: facebook_hash[:facebook_user_id]) - - if existing_account && (user_info.nil? || existing_account.id != user_info.user_id) - user_info.destroy! if user_info - result.user = existing_account - user_info = FacebookUserInfo.create!({ user_id: result.user.id }.merge(facebook_hash)) - else - result.user = user_info&.user - end - - if !result.user && !email.blank? && result.user = User.find_by_email(email) - FacebookUserInfo.create!({ user_id: result.user.id }.merge(facebook_hash)) - end - - user_info.update_columns(facebook_hash) if user_info - - retrieve_avatar(result.user, result.extra_data) - retrieve_profile(result.user, result.extra_data) - - if email.blank? - UserHistory.create( - action: UserHistory.actions[:facebook_no_email], - details: "name: #{facebook_hash[:name]}, facebook_user_id: #{facebook_hash[:facebook_user_id]}" - ) - end - - result - end - - def after_create_account(user, auth) - extra_data = auth[:extra_data] - FacebookUserInfo.create!({ user_id: user.id }.merge(extra_data)) - - retrieve_avatar(user, extra_data) - retrieve_profile(user, extra_data) - - true - end - def register_middleware(omniauth) omniauth.provider :facebook, setup: lambda { |env| strategy = env["omniauth.strategy"] strategy.options[:client_id] = SiteSetting.facebook_app_id strategy.options[:client_secret] = SiteSetting.facebook_app_secret - strategy.options[:info_fields] = 'gender,email,name,about,first_name,link,last_name,website,location' + strategy.options[:info_fields] = 'name,first_name,last_name,email' + strategy.options[:image_size] = { width: AVATAR_SIZE, height: AVATAR_SIZE } + strategy.options[:secure_image_url] = true }, scope: "email" end - protected - - def parse_auth_token(auth_token) - raw_info = auth_token["extra"]["raw_info"] - info = auth_token["info"] - - email = auth_token["info"][:email] - - website = (info["urls"] && info["urls"]["Website"]) || nil - - { - facebook: { - facebook_user_id: auth_token["uid"], - link: raw_info["link"], - username: raw_info["username"], - first_name: raw_info["first_name"], - last_name: raw_info["last_name"], - email: email, - gender: raw_info["gender"], - name: raw_info["name"], - avatar_url: info["image"], - location: info["location"], - website: website, - about_me: info["description"] - }, - email: email, - email_valid: true - } - end - - def retrieve_avatar(user, data) - return unless user - return if user.user_avatar.try(:custom_upload_id).present? - - if (avatar_url = data[:avatar_url]).present? - url = "#{avatar_url}?height=#{AVATAR_SIZE}&width=#{AVATAR_SIZE}" - Jobs.enqueue(:download_avatar_from_url, url: url, user_id: user.id, override_gravatar: false) - end - end - - def retrieve_profile(user, data) - return unless user - - bio = data[:about_me] || data[:about] - location = data[:location] - website = data[:website] - - if bio || location || website - profile = user.user_profile - profile.bio_raw = bio unless profile.bio_raw.present? - profile.location = location unless profile.location.present? - profile.website = website unless profile.website.present? - profile.save - end - end - end diff --git a/script/bulk_import/discourse_merger.rb b/script/bulk_import/discourse_merger.rb index 2a44a343fa5..bfaddc14f4a 100644 --- a/script/bulk_import/discourse_merger.rb +++ b/script/bulk_import/discourse_merger.rb @@ -151,7 +151,7 @@ class BulkImport::DiscourseMerger < BulkImport::Base copy_model(c, skip_if_merged: true, is_a_user_model: true, skip_processing: true) end - [FacebookUserInfo, GithubUserInfo, GoogleUserInfo, InstagramUserInfo, Oauth2UserInfo, + [UserAssociatedAccount, GithubUserInfo, GoogleUserInfo, InstagramUserInfo, Oauth2UserInfo, SingleSignOnRecord, TwitterUserInfo, EmailChangeRequest ].each do |c| copy_model(c, skip_if_merged: true, is_a_user_model: true) @@ -623,11 +623,6 @@ class BulkImport::DiscourseMerger < BulkImport::Base notification end - def process_facebook_user_info(r) - return nil if FacebookUserInfo.where(facebook_user_id: r['facebook_user_id']).exists? - r - end - def process_github_user_info(r) return nil if GithubUserInfo.where(github_user_id: r['github_user_id']).exists? r @@ -648,6 +643,11 @@ class BulkImport::DiscourseMerger < BulkImport::Base r end + def process_user_associated_account(r) + return nil if UserAssociatedAccount.where(provider_uid: r['uid'], provider_name: r['provider']).exists? + r + end + def process_single_sign_on_record(r) return nil if SingleSignOnRecord.where(external_id: r['external_id']).exists? r diff --git a/spec/components/auth/facebook_authenticator_spec.rb b/spec/components/auth/facebook_authenticator_spec.rb index 2c6c2a3312e..3cf67fcaac4 100644 --- a/spec/components/auth/facebook_authenticator_spec.rb +++ b/spec/components/auth/facebook_authenticator_spec.rb @@ -1,101 +1,60 @@ require 'rails_helper' describe Auth::FacebookAuthenticator do + let(:hash) { + { + provider: "facebook", + extra: { + raw_info: { + } + }, + info: { + email: "bob@bob.com", + first_name: "Bob", + last_name: "Smith" + }, + uid: "100" + } + } + + let(:authenticator) { Auth::FacebookAuthenticator.new } context 'after_authenticate' do it 'can authenticate and create a user record for already existing users' do - authenticator = Auth::FacebookAuthenticator.new user = Fabricate(:user) - - hash = { - "extra" => { - "raw_info" => { - "username" => "bob" - } - }, - "info" => { - :email => user.email, - "location" => "America", - "description" => "bio", - "urls" => { - "Website" => "https://awesome.com" - } - }, - "uid" => "100" - } - - result = authenticator.after_authenticate(hash) - + result = authenticator.after_authenticate(hash.deep_merge(info: { email: user.email })) expect(result.user.id).to eq(user.id) - expect(result.user.user_profile.website).to eq("https://awesome.com") - expect(result.user.user_profile.bio_raw).to eq("bio") - expect(result.user.user_profile.location).to eq("America") end it 'can connect to a different existing user account' do - authenticator = Auth::FacebookAuthenticator.new user1 = Fabricate(:user) user2 = Fabricate(:user) - FacebookUserInfo.create!(user_id: user1.id, facebook_user_id: 100) - - hash = { - "extra" => { - "raw_info" => { - "username" => "bob" - } - }, - "info" => { - "location" => "America", - "description" => "bio", - "urls" => { - "Website" => "https://awesome.com" - } - }, - "uid" => "100" - } + UserAssociatedAccount.create!(provider_name: "facebook", user_id: user1.id, provider_uid: 100) result = authenticator.after_authenticate(hash, existing_account: user2) expect(result.user.id).to eq(user2.id) - expect(FacebookUserInfo.exists?(user_id: user1.id)).to eq(false) - expect(FacebookUserInfo.exists?(user_id: user2.id)).to eq(true) + expect(UserAssociatedAccount.exists?(provider_name: "facebook", user_id: user1.id)).to eq(false) + expect(UserAssociatedAccount.exists?(provider_name: "facebook", user_id: user2.id)).to eq(true) end it 'can create a proper result for non existing users' do - - hash = { - "extra" => { - "raw_info" => { - "username" => "bob", - "name" => "bob bob" - } - }, - "info" => { - email: "bob@bob.com" - }, - "uid" => "100" - } - - authenticator = Auth::FacebookAuthenticator.new - result = authenticator.after_authenticate(hash) - expect(result.user).to eq(nil) - expect(result.extra_data[:name]).to eq("bob bob") + expect(result.name).to eq("Bob Smith") end end context 'description_for_user' do let(:user) { Fabricate(:user) } - let(:authenticator) { Auth::FacebookAuthenticator.new } it 'returns empty string if no entry for user' do expect(authenticator.description_for_user(user)).to eq("") end it 'returns correct information' do - FacebookUserInfo.create!(user_id: user.id, facebook_user_id: 12345, email: 'someuser@somedomain.tld') + UserAssociatedAccount.create!(provider_name: "facebook", user_id: user.id, provider_uid: 100, info: { email: "someuser@somedomain.tld" }) expect(authenticator.description_for_user(user)).to eq('someuser@somedomain.tld') end end @@ -112,31 +71,15 @@ describe Auth::FacebookAuthenticator do before do SiteSetting.facebook_app_id = '123' SiteSetting.facebook_app_secret = 'abcde' - FacebookUserInfo.create!(user_id: user.id, facebook_user_id: 12345, email: 'someuser@somedomain.tld') + UserAssociatedAccount.create!(provider_name: "facebook", user_id: user.id, provider_uid: 100, info: { email: "someuser@somedomain.tld" }) end it 'revokes correctly' do - stub = stub_request(:delete, authenticator.revoke_url(12345)).to_return(body: "true") - + expect(authenticator.description_for_user(user)).to eq("someuser@somedomain.tld") expect(authenticator.can_revoke?).to eq(true) expect(authenticator.revoke(user)).to eq(true) - - expect(stub).to have_been_requested.once expect(authenticator.description_for_user(user)).to eq("") end - - it 'handles errors correctly' do - stub = stub_request(:delete, authenticator.revoke_url(12345)).to_return(status: 404) - - expect(authenticator.revoke(user)).to eq(false) - expect(stub).to have_been_requested.once - expect(authenticator.description_for_user(user)).to eq('someuser@somedomain.tld') - - expect(authenticator.revoke(user, skip_remote: true)).to eq(true) - expect(stub).to have_been_requested.once - expect(authenticator.description_for_user(user)).to eq("") - - end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 6849785b2d2..c4739dfeae0 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -419,7 +419,7 @@ describe User do expect(user.associated_accounts).to eq([]) TwitterUserInfo.create(user_id: user.id, screen_name: "sam", twitter_user_id: 1) - FacebookUserInfo.create(user_id: user.id, username: "sam", facebook_user_id: 1) + UserAssociatedAccount.create(user_id: user.id, provider_name: "facebook", provider_uid: "1234", info: { email: "test@example.com" }) GoogleUserInfo.create(user_id: user.id, email: "sam@sam.com", google_user_id: 1) GithubUserInfo.create(user_id: user.id, screen_name: "sam", github_user_id: 1) InstagramUserInfo.create(user_id: user.id, screen_name: "sam", instagram_user_id: "examplel123123") diff --git a/spec/services/user_anonymizer_spec.rb b/spec/services/user_anonymizer_spec.rb index 2dd931a1f63..5b068393840 100644 --- a/spec/services/user_anonymizer_spec.rb +++ b/spec/services/user_anonymizer_spec.rb @@ -179,7 +179,7 @@ describe UserAnonymizer do user.twitter_user_info = TwitterUserInfo.create(user_id: user.id, screen_name: "example", twitter_user_id: "examplel123123") user.google_user_info = GoogleUserInfo.create(user_id: user.id, google_user_id: "google@gmail.com") user.github_user_info = GithubUserInfo.create(user_id: user.id, screen_name: "example", github_user_id: "examplel123123") - user.facebook_user_info = FacebookUserInfo.create(user_id: user.id, facebook_user_id: "example") + user.user_associated_accounts = [UserAssociatedAccount.create(user_id: user.id, provider_uid: "example", provider_name: "facebook")] user.single_sign_on_record = SingleSignOnRecord.create(user_id: user.id, external_id: "example", last_payload: "looks good") user.oauth2_user_infos = [Oauth2UserInfo.create(user_id: user.id, uid: "example", provider: "example")] user.instagram_user_info = InstagramUserInfo.create(user_id: user.id, screen_name: "example", instagram_user_id: "examplel123123") @@ -189,7 +189,7 @@ describe UserAnonymizer do expect(user.twitter_user_info).to eq(nil) expect(user.google_user_info).to eq(nil) expect(user.github_user_info).to eq(nil) - expect(user.facebook_user_info).to eq(nil) + expect(user.user_associated_accounts).to be_empty expect(user.single_sign_on_record).to eq(nil) expect(user.oauth2_user_infos).to be_empty expect(user.instagram_user_info).to eq(nil) diff --git a/spec/services/user_merger_spec.rb b/spec/services/user_merger_spec.rb index 65799eb179d..19a5c9742e0 100644 --- a/spec/services/user_merger_spec.rb +++ b/spec/services/user_merger_spec.rb @@ -964,7 +964,7 @@ describe UserMerger do end it "deletes external auth infos of source user" do - FacebookUserInfo.create(user_id: source_user.id, facebook_user_id: "example") + UserAssociatedAccount.create(user_id: source_user.id, provider_name: "facebook", provider_uid: "1234") GithubUserInfo.create(user_id: source_user.id, screen_name: "example", github_user_id: "examplel123123") GoogleUserInfo.create(user_id: source_user.id, google_user_id: "google@gmail.com") InstagramUserInfo.create(user_id: source_user.id, screen_name: "example", instagram_user_id: "examplel123123") @@ -975,7 +975,7 @@ describe UserMerger do merge_users! - expect(FacebookUserInfo.where(user_id: source_user.id).count).to eq(0) + expect(UserAssociatedAccount.where(user_id: source_user.id).count).to eq(0) expect(GithubUserInfo.where(user_id: source_user.id).count).to eq(0) expect(GoogleUserInfo.where(user_id: source_user.id).count).to eq(0) expect(InstagramUserInfo.where(user_id: source_user.id).count).to eq(0)