REFACTOR: Migrate FacebookAuthenticator to use ManagedAuthenticator

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.

From 208005f9c9662773b436c4ffa14272ac0888bb04 Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
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.

diff --git a/app/models/facebook_user_info.rb b/app/models/facebook_user_info.rb
deleted file mode 100644
index fd1ee51..0000000
--- 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 9741526..96908e4 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 ae1851c..745188b 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 daaa976..a1f227b 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 0000000..9209991
--- /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 01d86c7..a8c2fff 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.opti

GitHub

1 Like

This commit has been mentioned on Discourse Meta. There might be relevant details there: