REFACTOR: Migrate InstagramAuthenticator to use ManagedAuthenticator (#7081)

db/migrate/20190227150413_migrate_instagram_user_info.rb

@@ -0,0 +1,27 @@
+class MigrateInstagramUserInfo < 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
+      'instagram',
+      instagram_user_id,
+      user_id,
+      json_build_object('nickname', screen_name),
+      updated_at,
+      created_at,
+      updated_at
+    FROM instagram_user_infos
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

lib/auth/instagram_authenticator.rb

@@ -1,5 +1,4 @@
-class Auth::InstagramAuthenticator < Auth::Authenticator
+class Auth::InstagramAuthenticator < Auth::ManagedAuthenticator
-
   def name
     "instagram"
   end
@@ -8,67 +7,6 @@ class Auth::InstagramAuthenticator < Auth::Authenticator
     SiteSetting.enable_instagram_logins
   end
 
-  def description_for_user(user)
-    info = InstagramUserInfo.find_by(user_id: user.id)
-    info&.screen_name || ""
-  end
-
-  def can_revoke?
-    true
-  end
-
-  def revoke(user, skip_remote: false)
-    info = InstagramUserInfo.find_by(user_id: user.id)
-    raise Discourse::NotFound if info.nil?
-    # Instagram does not have any way for us to revoke tokens on their 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.username = screen_name = data["nickname"]
-    result.name = name = data["name"].slice!(0)
-    instagram_user_id = auth_token["uid"]
-
-    result.extra_data = {
-      instagram_user_id: instagram_user_id,
-      instagram_screen_name: screen_name
-    }
-
-    user_info = InstagramUserInfo.find_by(instagram_user_id: instagram_user_id)
-
-    if existing_account && (user_info.nil? || existing_account.id != user_info.user_id)
-      user_info.destroy! if user_info
-      user_info = InstagramUserInfo.create!(
-        user_id: existing_account.id,
-        screen_name: screen_name,
-        instagram_user_id: instagram_user_id
-      )
-    end
-
-    result.user = user_info&.user
-
-    result
-  end
-
-  def after_create_account(user, auth)
-    data = auth[:extra_data]
-    InstagramUserInfo.create(
-      user_id: user.id,
-      screen_name: data[:instagram_screen_name],
-      instagram_user_id: data[:instagram_user_id]
-    )
-  end
-
   def register_middleware(omniauth)
     omniauth.provider :instagram,
            setup: lambda { |env|
@@ -77,5 +15,4 @@ class Auth::InstagramAuthenticator < Auth::Authenticator
               strategy.options[:client_secret] = SiteSetting.instagram_consumer_secret
            }
   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
 
-    [UserAssociatedAccount, GithubUserInfo, GoogleUserInfo, InstagramUserInfo, Oauth2UserInfo,
+    [UserAssociatedAccount, GithubUserInfo, GoogleUserInfo, Oauth2UserInfo,
       SingleSignOnRecord, EmailChangeRequest
     ].each do |c|
       copy_model(c, skip_if_merged: true, is_a_user_model: true)
@@ -633,11 +633,6 @@ class BulkImport::DiscourseMerger < BulkImport::Base
     r
   end
 
-  def process_instagram_user_info(r)
-    return nil if InstagramUserInfo.where(instagram_user_id: r['instagram_user_id']).exists?
-    r
-  end
-
   def process_oauth2_user_info(r)
     return nil if Oauth2UserInfo.where(uid: r['uid'], provider: r['provider']).exists?
     r

spec/components/auth/instagram_authenticator_spec.rb

@@ -0,0 +1,58 @@
+require 'rails_helper'
+
+describe Auth::InstagramAuthenticator do
+
+  it "takes over account if email is supplied" do
+    auth = Auth::InstagramAuthenticator.new
+
+    user = Fabricate(:user)
+
+    auth_token = {
+      info: { email: user.email },
+      uid: "123",
+      provider: "instagram"
+    }
+
+    result = auth.after_authenticate(auth_token)
+
+    expect(result.user.id).to eq(user.id)
+
+    info = UserAssociatedAccount.find_by(provider_name: "instagram", user_id: user.id)
+    expect(info.info["email"]).to eq(user.email)
+  end
+
+  it 'can connect to a different existing user account' do
+    authenticator = Auth::InstagramAuthenticator.new
+    user1 = Fabricate(:user)
+    user2 = Fabricate(:user)
+
+    hash = {
+      info: { email: user1.email },
+      uid: "100",
+      provider: "instagram"
+    }
+
+    result = authenticator.after_authenticate(hash, existing_account: user2)
+
+    expect(result.user.id).to eq(user2.id)
+    expect(UserAssociatedAccount.exists?(provider_name: "instagram", user_id: user1.id)).to eq(false)
+    expect(UserAssociatedAccount.exists?(provider_name: "instagram", user_id: user2.id)).to eq(true)
+  end
+
+  context 'revoke' do
+    let(:user) { Fabricate(:user) }
+    let(:authenticator) { Auth::InstagramAuthenticator.new }
+
+    it 'raises exception if no entry for user' do
+      expect { authenticator.revoke(user) }.to raise_error(Discourse::NotFound)
+    end
+
+    it 'revokes correctly' do
+      UserAssociatedAccount.create!(provider_name: "instagram", 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("")
+    end
+  end
+
+end

spec/jobs/invalidate_inactive_admins_spec.rb

@@ -37,7 +37,6 @@ describe Jobs::InvalidateInactiveAdmins do
       context 'with social logins' do
         before do
           GithubUserInfo.create!(user_id: not_seen_admin.id, screen_name: 'bob', github_user_id: 100)
-          InstagramUserInfo.create!(user_id: not_seen_admin.id, screen_name: 'bob', instagram_user_id: 'examplel123123')
           UserOpenId.create!(url: 'https://me.yahoo.com/id/123' , user_id: not_seen_admin.id, email: 'bob@example.com', active: true)
           GoogleUserInfo.create!(user_id: not_seen_admin.id, google_user_id: 100, email: 'bob@example.com')
         end
@@ -45,7 +44,6 @@ describe Jobs::InvalidateInactiveAdmins do
         it 'removes the social logins' do
           subject
           expect(GithubUserInfo.where(user_id: not_seen_admin.id).exists?).to eq(false)
-          expect(InstagramUserInfo.where(user_id: not_seen_admin.id).exists?).to eq(false)
           expect(GoogleUserInfo.where(user_id: not_seen_admin.id).exists?).to eq(false)
           expect(UserOpenId.where(user_id: not_seen_admin.id).exists?).to eq(false)
         end

spec/models/user_spec.rb

@@ -427,9 +427,9 @@ describe User do
 
       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" })
+      UserAssociatedAccount.create(user_id: user.id, provider_name: "instagram", provider_uid: "examplel123123", info: { nickname: "sam" })
       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")
 
       user.reload
       expect(user.associated_accounts.map { |a| a[:name] }).to contain_exactly('twitter', 'facebook', 'google_oauth2', 'github', 'instagram')

spec/services/user_anonymizer_spec.rb

@@ -195,7 +195,6 @@ describe UserAnonymizer do
       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")
       UserOpenId.create(user_id: user.id, email: user.email, url: "http://example.com/openid", active: true)
       make_anonymous
       user.reload

spec/services/user_merger_spec.rb

@@ -979,7 +979,6 @@ describe UserMerger do
     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")
     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")
     UserOpenId.create(user_id: source_user.id, email: source_user.email, url: "http://example.com/openid", active: true)
@@ -989,7 +988,6 @@ describe UserMerger do
     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)
     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(UserOpenId.where(user_id: source_user.id).count).to eq(0)