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 🎉

app/models/twitter_user_info.rb

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

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

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)

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

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
+    # Twitter sends a huge amount of data which we don't need, so ignore it
-
+    auth_token[:extra] = {}
-    data = auth_token[:info]
+    super
-
-    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
   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

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
-  end
-
   def process_user_badge(user_badge)
     user_badge['granted_by_id'] = user_id_from_imported_id(user_badge['granted_by_id']) if user_badge['granted_by_id']
     user_badge['notification_id'] = notification_id_from_imported_id(user_badge['notification_id']) if user_badge['notification_id']

spec/components/auth/twitter_authenticator_spec.rb

@@ -9,20 +9,21 @@ describe Auth::TwitterAuthenticator do
 
     auth_token = {
       info: {
-        "email" => user.email,
+        email: user.email,
-        "username" => "test",
+        username: "test",
-        "name" => "test",
+        name: "test",
-        "nickname" => "minion",
+        nickname: "minion",
       },
-      "uid" => "123"
+      uid: "123",
+      provider: "twitter"
     }
 
     result = auth.after_authenticate(auth_token)
 
     expect(result.user.id).to eq(user.id)
 
-    info = TwitterUserInfo.find_by(user_id: user.id)
+    info = UserAssociatedAccount.find_by(provider_name: "twitter", user_id: user.id)
-    expect(info.email).to eq(user.email)
+    expect(info.info["email"]).to eq(user.email)
   end
 
   it 'can connect to a different existing user account' do
@@ -30,23 +31,24 @@ describe Auth::TwitterAuthenticator do
     user1 = Fabricate(:user)
     user2 = Fabricate(:user)
 
-    TwitterUserInfo.create!(user_id: user1.id, twitter_user_id: 100, screen_name: "boris")
+    UserAssociatedAccount.create!(provider_name: "twitter", user_id: user1.id, provider_uid: 100)
 
     hash = {
       info: {
-        "email" => user1.email,
+        email: user1.email,
-        "username" => "test",
+        username: "test",
-        "name" => "test",
+        name: "test",
-        "nickname" => "minion",
+        nickname: "minion",
       },
-      "uid" => "100"
+      uid: "100",
+      provider: "twitter"
     }
 
     result = authenticator.after_authenticate(hash, existing_account: user2)
 
     expect(result.user.id).to eq(user2.id)
-    expect(TwitterUserInfo.exists?(user_id: user1.id)).to eq(false)
+    expect(UserAssociatedAccount.exists?(provider_name: "twitter", user_id: user1.id)).to eq(false)
-    expect(TwitterUserInfo.exists?(user_id: user2.id)).to eq(true)
+    expect(UserAssociatedAccount.exists?(provider_name: "twitter", user_id: user2.id)).to eq(true)
   end
 
   context 'revoke' do
@@ -58,7 +60,7 @@ describe Auth::TwitterAuthenticator do
     end
 
       it 'revokes correctly' do
-        TwitterUserInfo.create!(user_id: user.id, twitter_user_id: 100, screen_name: "boris")
+        UserAssociatedAccount.create!(provider_name: "twitter", user_id: user.id, provider_uid: 100)
         expect(authenticator.can_revoke?).to eq(true)
         expect(authenticator.revoke(user)).to eq(true)
         expect(authenticator.description_for_user(user)).to eq("")

spec/models/twitter_user_info_spec.rb

@@ -1,10 +0,0 @@
-require 'rails_helper'
-
-describe TwitterUserInfo do
-  it "does not overflow" do
-    id = 22019458041
-    info = TwitterUserInfo.create!(user_id: -1, screen_name: 'sam', twitter_user_id: id)
-    info.reload
-    expect(info.twitter_user_id).to eq(id)
-  end
-end

spec/models/user_spec.rb

@@ -418,7 +418,7 @@ describe User do
       user = Fabricate(:user)
       expect(user.associated_accounts).to eq([])
 
-      TwitterUserInfo.create(user_id: user.id, screen_name: "sam", twitter_user_id: 1)
+      UserAssociatedAccount.create(user_id: user.id, provider_name: "twitter", provider_uid: "1", info: { nickname: "sam" })
       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)

spec/requests/users_controller_spec.rb

@@ -835,7 +835,7 @@ describe UsersController do
           }
 
           expect(response.status).to eq(200)
-          expect(TwitterUserInfo.count).to eq(1)
+          expect(UserAssociatedAccount.where(provider_name: "twitter").count).to eq(1)
         end
 
         it "returns an error when email has been changed from the validated email address" do

spec/services/user_anonymizer_spec.rb

@@ -176,7 +176,6 @@ describe UserAnonymizer do
     end
 
     it "removes external auth assocations" 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.user_associated_accounts = [UserAssociatedAccount.create(user_id: user.id, provider_uid: "example", provider_name: "facebook")]
@@ -186,7 +185,6 @@ describe UserAnonymizer do
       UserOpenId.create(user_id: user.id, email: user.email, url: "http://example.com/openid", active: true)
       make_anonymous
       user.reload
-      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.user_associated_accounts).to be_empty

spec/services/user_merger_spec.rb

@@ -970,7 +970,6 @@ describe UserMerger do
     InstagramUserInfo.create(user_id: source_user.id, screen_name: "example", instagram_user_id: "examplel123123")
     Oauth2UserInfo.create(user_id: source_user.id, uid: "example", provider: "example")
     SingleSignOnRecord.create(user_id: source_user.id, external_id: "example", last_payload: "looks good")
-    TwitterUserInfo.create(user_id: source_user.id, screen_name: "example", twitter_user_id: "examplel123123")
     UserOpenId.create(user_id: source_user.id, email: source_user.email, url: "http://example.com/openid", active: true)
 
     merge_users!
@@ -981,7 +980,6 @@ describe UserMerger do
     expect(InstagramUserInfo.where(user_id: source_user.id).count).to eq(0)
     expect(Oauth2UserInfo.where(user_id: source_user.id).count).to eq(0)
     expect(SingleSignOnRecord.where(user_id: source_user.id).count).to eq(0)
-    expect(TwitterUserInfo.where(user_id: source_user.id).count).to eq(0)
     expect(UserOpenId.where(user_id: source_user.id).count).to eq(0)
   end