REFACTOR: Migrate TwitterAuthenticator to use ManagedAuthenticator (#6739)

REFACTOR: Migrate TwitterAuthenticator to use ManagedAuthenticator (#6739)

No changes to functionality. TwitterAuthenticator goes from 136 lines to 24, and all twitter-specific logic elsewhere has been deleted :tada:

From 160d29b18a0ff68f0cdc152b9d5f461869190b7e Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Fri, 7 Dec 2018 15:39:06 +0000
Subject: [PATCH] REFACTOR: Migrate TwitterAuthenticator to use
 ManagedAuthenticator (#6739)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

No changes to functionality. TwitterAuthenticator goes from 136 lines to 24, and all twitter-specific logic elsewhere has been deleted 🎉

diff --git a/app/models/twitter_user_info.rb b/app/models/twitter_user_info.rb
deleted file mode 100644
index 07302bc..0000000
--- a/app/models/twitter_user_info.rb
+++ /dev/null
@@ -1,21 +0,0 @@
-class TwitterUserInfo < ActiveRecord::Base
-  belongs_to :user
-end
-
-# == Schema Information
-#
-# Table name: twitter_user_infos
-#
-#  id              :integer          not null, primary key
-#  user_id         :integer          not null
-#  screen_name     :string           not null
-#  twitter_user_id :bigint(8)        not null
-#  created_at      :datetime         not null
-#  updated_at      :datetime         not null
-#  email           :string(1000)
-#
-# Indexes
-#
-#  index_twitter_user_infos_on_twitter_user_id  (twitter_user_id) UNIQUE
-#  index_twitter_user_infos_on_user_id          (user_id) UNIQUE
-#
diff --git a/app/models/user.rb b/app/models/user.rb
index 464740d..dea7a5f 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -66,7 +66,6 @@ class User < ActiveRecord::Base
   has_one :user_option, dependent: :destroy
   has_one :user_avatar, 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
   has_many :oauth2_user_infos, dependent: :destroy
diff --git a/app/services/user_anonymizer.rb b/app/services/user_anonymizer.rb
index a1f227b..d87c7e6 100644
--- a/app/services/user_anonymizer.rb
+++ b/app/services/user_anonymizer.rb
@@ -53,7 +53,6 @@ class UserAnonymizer
       end
 
       @user.user_avatar.try(:destroy)
-      @user.twitter_user_info.try(:destroy)
       @user.google_user_info.try(:destroy)
       @user.github_user_info.try(:destroy)
       @user.single_sign_on_record.try(:destroy)
diff --git a/db/migrate/20181207141900_migrate_twitter_user_info.rb b/db/migrate/20181207141900_migrate_twitter_user_info.rb
new file mode 100644
index 0000000..f47798f
--- /dev/null
+++ b/db/migrate/20181207141900_migrate_twitter_user_info.rb
@@ -0,0 +1,27 @@
+class MigrateTwitterUserInfo < 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
+      'twitter',
+      twitter_user_id,
+      user_id,
+      json_build_object('email', email, 'nickname', screen_name),
+      updated_at,
+      created_at,
+      updated_at
+    FROM twitter_user_infos
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/lib/auth/twitter_authenticator.rb b/lib/auth/twitter_authenticator.rb
index be591a0..06b4ee1 100644
--- a/lib/auth/twitter_authenticator.rb
+++ b/lib/auth/twitter_authenticator.rb
@@ -1,5 +1,4 @@
-class Auth::TwitterAuthenticator < Auth::Authenticator
-
+class Auth::TwitterAuthenticator < Auth::ManagedAuthenticator
   def name
     "twitter"
   end
@@ -8,94 +7,10 @@ class Auth::TwitterAuthenticator < Auth::Authenticator
     SiteSetting.enable_twitter_logins
   end
 
-  def description_for_user(user)
-    info = TwitterUserInfo.find_by(user_id: user.id)
-    info&.email || info&.screen_name || ""
-  end
-
-  def can_revoke?
-    true
-  end
-
-  def revoke(user, skip_remote: false)
-    info = TwitterUserInfo.find_by(user_id: user.id)
-    raise Discourse::NotFound if info.nil?
-
-    # We get a token from twitter upon login but do not need it, and do not store it.
-    # Therefore we do not have any way to revoke the token automatically on twitter's end
-
-    info.destroy!
-    true
-  end
-
-  def can_connect_existing_user?
-    true
-  end
-
   def after_authenticate(auth_token, existing_account: nil)
-    result = Auth::Result.new
-
-    data = auth_token[:info]
-
-    result.email = data["email"]
-    result.email_valid = result.email.present?
-    result.username = data["nickname"]
-    result.name = data["name"]
-    twitter_user_id = auth_token["uid"]
-
-    result.extra_data = {
-      twitter_email: result.email,
-      twitter_user_id: twitter_user_id,
-      twitter_screen_name: result.username,
-      twitter_image: data["image"],
-      twitter_description: data["description"],
-      twitter_location: data["location"]
-    }
-
-    user_info = TwitterUserInfo.find_by(twitter_user_id: twitter_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 = TwitterUserInfo.create!(
-        user_id: result.user.id,
-        screen_name: result.username,
-        twitter_user_id: twitter_user_id,
-        email: result.email
-      )
-    else
-      result.user = user_info&.user
-    end
-
-    if (!result.user) && result.email_valid && (result.user = User.find_by_email(result.email))
-      TwitterUserInfo.create!(
-        user_id: result.user.id,
-        screen_name: result.username,
-        twitter_user_id: twitter_user_id,
-        email: result.email
-      )
-    end
-
-    retrieve_avatar(result.user, result.extra_data)
-    retrieve_profile(result.user, result.extra_data)
-
-    result
-  end
-
-  def after_create_account(user, auth)
-    extra_data = auth[:extra_data]
-
-    TwitterUserInfo.create(
-      user_id: user.id,
-      screen_name: extra_data[:twitter_screen_name],
-      twitter_user_id: extra_data[:twitter_user_id],
-      email: extra_data[:email]
-    )
-
-    retrieve_avatar(user, extra_data)
-    retrieve_profile(user, extra_data)
-
-    true
+    # Twitter sends a huge amount of data which we don't need, so ignore it
+    auth_token[:extra] = {}
+    super
   end
 
   def register_middleware(omniauth)
@@ -106,31 +21,4 @@ class Auth::TwitterAuthenticator < Auth::Authenticator
               strategy.options[:consumer_secret] = SiteSetting.twitter_consumer_secret
            }
   end
-
-  protected
-
-  def retrieve_avatar(user, data)
-    return unless user
-    return if user.user_avatar.try(:custom_upload_id).present?
-
-    if (avatar_url = data[:twitter_image]).present?
-      url = avatar_url.sub("_normal", "")
-      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[:twitter_description]
-    location = data[:twitter_location]
-
-    if bio || location
-      profile = user.user_profile
-      profile.bio_raw  = bio      unless profile.bio_raw.present?
-      profile.location = location unless profile.location.present?
-      profile.save
-    end
-  end
-
 end
diff --git a/script/bulk_import/discourse_merger.rb b/script/bulk_import/discourse_merger.rb
index bfaddc1..1f6d6c9 100644
--- a/script/bulk_import/discourse_merger.rb
+++ b/script/bulk_import/discourse_merger.rb
@@ -152,7 +152,7 @@ class BulkImport::DiscourseMerger < BulkImport::Base
     end
 
     [UserAssociatedAccount, GithubUserInfo, GoogleUserInfo, InstagramUserInfo, Oauth2UserInfo,
-      SingleSignOnRecord, TwitterUserInfo, EmailChangeRequest
+      SingleSignOnRecord, EmailChangeRequest
     ].each do |c|
       copy_model(c, skip_if_merged: true, is_a_user_model: true)
     end
@@ -653,11 +653,6 @@ class BulkImport::DiscourseMerger < BulkImport::Base
     r
   end
 
-  def process_twitter_user_info(r)
-    return nil if TwitterUserInfo.where(twitter_user_id: r['twitter_user_id']).exists?
-    r
-

GitHub

2 Likes

@davidtaylorhq Is the plan to merge Google, Instagram, Github and the other oAuth accounts into this generic model? I’m just interested for the purposes of keeping the Legal Tools plugin up to date.

Yes, the generic model is where it is at

1 Like

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

@angusmcleod, please see Adding a new authentication method to Discourse - dev - Discourse Meta for some more information on this change

1 Like

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