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

@@ -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
-#

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

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,

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)

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

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

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

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)
-
+      result = authenticator.after_authenticate(hash.deep_merge(info: { email: user.email }))
-      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)
-
       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)
+      UserAssociatedAccount.create!(provider_name: "facebook", user_id: user1.id, provider_uid: 100)
-
-      hash = {
-        "extra" => {
-            "raw_info" => {
-            "username" => "bob"
-          }
-        },
-        "info" => {
-          "location" => "America",
-          "description" => "bio",
-          "urls" => {
-            "Website" => "https://awesome.com"
-          }
-        },
-        "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(UserAssociatedAccount.exists?(provider_name: "facebook", 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: 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
 

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")

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)

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)