REFACTOR: Migrate InstagramAuthenticator to use ManagedAuthenticator (#7081)

REFACTOR: Migrate InstagramAuthenticator to use ManagedAuthenticator (#7081)

diff --git a/db/migrate/20190227150413_migrate_instagram_user_info.rb b/db/migrate/20190227150413_migrate_instagram_user_info.rb
new file mode 100644
index 0000000..37797fd
--- /dev/null
+++ b/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
diff --git a/lib/auth/instagram_authenticator.rb b/lib/auth/instagram_authenticator.rb
index 515616f..5ecb3a1 100644
--- a/lib/auth/instagram_authenticator.rb
+++ b/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
diff --git a/script/bulk_import/discourse_merger.rb b/script/bulk_import/discourse_merger.rb
index 1f6d6c9..2ad2499 100644
--- a/script/bulk_import/discourse_merger.rb
+++ b/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
diff --git a/spec/components/auth/instagram_authenticator_spec.rb b/spec/components/auth/instagram_authenticator_spec.rb
new file mode 100644
index 0000000..d8a3075
--- /dev/null
+++ b/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
diff --git a/spec/jobs/invalidate_inactive_admins_spec.rb b/spec/jobs/invalidate_inactive_admins_spec.rb
index 62cacc3..6fd0f30 100644
--- a/spec/jobs/invalidate_inactive_admins_spec.rb
+++ b/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
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index d4e8c35..0bbea09 100644
--- a/spec/models/user_spec.rb
+++ b/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" })

[... diff too long, it was truncated ...]

GitHub sha: 703c724c

2 Likes

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

DEV: Drop unused google and instagram auth_info tables

DEV: Remove unused instagram_user_info model