DEV: Migrate Github authentication to ManagedAuthenticator (#11170)

This commit adds an additional find_user_by_email hook to ManagedAuthenticator so that GitHub login can continue to support secondary email addresses

The github_user_infos table will be dropped in a follow-up commit.

This is the last core authenticator to be migrated to ManagedAuthenticator 🎉

app/models/github_user_info.rb

@@ -1,22 +0,0 @@
-# frozen_string_literal: true
-
-class GithubUserInfo < ActiveRecord::Base
-  belongs_to :user
-end
-
-# == Schema Information
-#
-# Table name: github_user_infos
-#
-#  id             :integer          not null, primary key
-#  user_id        :integer          not null
-#  screen_name    :string           not null
-#  github_user_id :integer          not null
-#  created_at     :datetime         not null
-#  updated_at     :datetime         not null
-#
-# Indexes
-#
-#  index_github_user_infos_on_github_user_id  (github_user_id) UNIQUE
-#  index_github_user_infos_on_user_id         (user_id) UNIQUE
-#

app/models/user.rb

@@ -40,7 +40,6 @@ class User < ActiveRecord::Base
 
   has_one :user_option, dependent: :destroy
   has_one :user_avatar, dependent: :destroy
-  has_one :github_user_info, dependent: :destroy
   has_one :primary_email, -> { where(primary: true)  }, class_name: 'UserEmail', dependent: :destroy
   has_one :user_stat, dependent: :destroy
   has_one :user_profile, dependent: :destroy, inverse_of: :user

app/services/user_anonymizer.rb

@@ -60,7 +60,6 @@ class UserAnonymizer
       end
 
       @user.user_avatar.try(:destroy)
-      @user.github_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)

db/migrate/20201109170951_migrate_github_user_infos.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+class MigrateGithubUserInfos < ActiveRecord::Migration[6.0]
+  def up
+    execute <<~SQL
+      INSERT INTO user_associated_accounts (
+        provider_name,
+        provider_uid,
+        user_id,
+        info,
+        last_used,
+        created_at,
+        updated_at
+      ) SELECT
+        'github',
+        github_user_id,
+        user_id,
+        json_build_object('nickname', screen_name),
+        updated_at,
+        created_at,
+        updated_at
+      FROM github_user_infos
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

lib/auth/github_authenticator.rb

@@ -2,7 +2,7 @@
 
 require_dependency 'has_errors'
 
-class Auth::GithubAuthenticator < Auth::Authenticator
+class Auth::GithubAuthenticator < Auth::ManagedAuthenticator
 
   def name
     "github"
@@ -12,146 +12,40 @@ class Auth::GithubAuthenticator < Auth::Authenticator
     SiteSetting.enable_github_logins
   end
 
-  def description_for_user(user)
-    info = GithubUserInfo.find_by(user_id: user.id)
-    info&.screen_name || ""
-  end
-
-  def can_revoke?
-    true
-  end
-
-  def revoke(user, skip_remote: false)
-    info = GithubUserInfo.find_by(user_id: user.id)
-    raise Discourse::NotFound if info.nil?
-    info.destroy!
-    true
-  end
-
-  class GithubEmailChecker
-    include ::HasErrors
-
-    def initialize(validator, email)
-      @validator = validator
-      @email = Email.downcase(email)
-    end
-
-    def valid?()
-      @validator.validate_each(self, :email, @email)
-      errors.blank?
-    end
-
-  end
-
-  def can_connect_existing_user?
-    true
-  end
-
   def after_authenticate(auth_token, existing_account: nil)
-    result = Auth::Result.new
+    result = super
-
+    return result if result.user
-    data = auth_token[:info]
+    # If email domain restrictions are configured,
-    result.username = screen_name = data[:nickname]
+    # pick a secondary email which is allowed
-    result.name = data[:name]
+    all_github_emails(auth_token).each do |candidate|
-
+      next if !EmailValidator.allowed?(candidate[:email])
-    github_user_id = auth_token[:uid]
+      result.email = candidate[:email]
-
+      result.email_valid = !!candidate[:verified]
-    result.extra_data = {
+      break
-      github_user_id: github_user_id,
-      github_screen_name: screen_name,
-    }
-
-    user_info = GithubUserInfo.find_by(github_user_id: github_user_id)
-
-    if existing_account && (user_info.nil? || existing_account.id != user_info.user_id)
-      user_info.destroy! if user_info
-      user_info = GithubUserInfo.create(
-        user_id: existing_account.id,
-        screen_name: screen_name,
-        github_user_id: github_user_id
-        )
     end
 
-    if user_info
-      # If there's existing user info with the given GitHub ID, that's all we
-      # need to know.
-      user = user_info.user
-      result.email = data[:email]
-      result.email_valid = data[:email].present?
-
-      # update GitHub screen_name
-      if user_info.screen_name != screen_name
-        user_info.screen_name = screen_name
-        user_info.save!
-      end
-    else
-      # Potentially use *any* of the emails from GitHub to find a match or
-      # register a new user, with preference given to the primary email.
-      all_emails = Array.new(auth_token[:extra][:all_emails])
-      primary = all_emails.detect { |email| email[:primary] && email[:verified] }
-      all_emails.unshift(primary) if primary.present?
-
-      # Only consider verified emails to match an existing user.  We don't want
-      # someone to be able to create a GitHub account with an unverified email
-      # in order to access someone else's Discourse account!
-      all_emails.each do |candidate|
-        if !!candidate[:verified] && (user = User.find_by_email(candidate[:email]))
-          result.email = candidate[:email]
-          result.email_valid = !!candidate[:verified]
-
-          GithubUserInfo
-            .where('user_id = ? OR github_user_id = ?', user.id, github_user_id)
-            .destroy_all
-
-          GithubUserInfo.create!(
-              user_id: user.id,
-              screen_name: screen_name,
-              github_user_id: github_user_id
-          )
-          break
-        end
-      end
-
-      # If we *still* don't have a user, check to see if there's an email that
-      # passes validation (this includes allowlist/blocklist filtering if any is
-      # configured).  When no allowlist/blocklist is in play, this will simply
-      # choose the primary email since it's at the front of the list.
-      if !user
-        validator = EmailValidator.new(attributes: :email)
-        found_email = false
-        all_emails.each do |candidate|
-          checker = GithubEmailChecker.new(validator, candidate[:email])
-          if checker.valid?
-            result.email = candidate[:email]
-            result.email_valid = !!candidate[:verified]
-            found_email = true
-            break
-          end
-        end
-
-        if !found_email
-          result.failed = true
-          escaped = Rack::Utils.escape_html(screen_name)
-          result.failed_reason = I18n.t("login.authenticator_error_no_valid_email", account: escaped)
-        end
-      end
-    end
-
-    retrieve_avatar(user, data)
-
-    result.user = user
     result
   end
 
-  def after_create_account(user, auth)
+  def find_user_by_email(auth_token)
-    data = auth[:extra_data]
+    # Use verified secondary emails to find a match
-    GithubUserInfo.create(
+    all_github_emails(auth_token).each do |candidate|
-      user_id: user.id,
+      next if !candidate[:verified]
-      screen_name: data[:github_screen_name],
+      if user = User.find_by_email(candidate[:email])
-      github_user_id: data[:github_user_id]
+        return user
-    )
+      end
+    end
+    nil
+  end
 
-    retrieve_avatar(user, data)
+  def all_github_emails(auth_token)
+    emails = Array.new(auth_token[:extra][:all_emails])
+    primary_email = emails.find { |email| email[:primary] }
+    if primary_email
+      emails.delete(primary_email)
+      emails.unshift(primary_email)
+    end
+    emails
   end
 
   def register_middleware(omniauth)
@@ -163,12 +57,4 @@ class Auth::GithubAuthenticator < Auth::Authenticator
            },
            scope: "user:email"
   end
-
-  private
-
-  def retrieve_avatar(user, data)
-    return unless data[:image].present? && user && user.user_avatar&.custom_upload_id.blank?
-
-    Jobs.enqueue(:download_avatar_from_url, url: data[:image], user_id: user.id, override_gravatar: false)
-  end
 end

lib/auth/managed_authenticator.rb

@@ -53,10 +53,9 @@ class Auth::ManagedAuthenticator < Auth::Authenticator
     end
 
     # Matching an account by email
-    if primary_email_verified?(auth_token) &&
+    if match_by_email &&
-        match_by_email &&
         association.user.nil? &&
-        (user = User.find_by_email(auth_token.dig(:info, :email)))
+        (user = find_user_by_email(auth_token))
 
       UserAssociatedAccount.where(user: user, provider_name: auth_token[:provider]).destroy_all # Destroy existing associations for the new user
       association.user = user
@@ -118,6 +117,13 @@ class Auth::ManagedAuthenticator < Auth::Authenticator
     retrieve_profile(user, association.info)
   end
 
+  def find_user_by_email(auth_token)
+    email = auth_token.dig(:info, :email)
+    if email && primary_email_verified?(auth_token)
+      User.find_by_email(email)
+    end
+  end
+
   def retrieve_avatar(user, url)
     return unless user && url
     return if user.user_avatar.try(:custom_upload_id).present?

script/bulk_import/discourse_merger.rb

@@ -153,7 +153,7 @@ class BulkImport::DiscourseMerger < BulkImport::Base
       copy_model(c, skip_if_merged: true, is_a_user_model: true, skip_processing: true)
     end
 
-    [UserAssociatedAccount, GithubUserInfo, Oauth2UserInfo,
+    [UserAssociatedAccount, Oauth2UserInfo,
       SingleSignOnRecord, EmailChangeRequest
     ].each do |c|
       copy_model(c, skip_if_merged: true, is_a_user_model: true)
@@ -625,11 +625,6 @@ class BulkImport::DiscourseMerger < BulkImport::Base
     notification
   end
 
-  def process_github_user_info(r)
-    return nil if GithubUserInfo.where(github_user_id: r['github_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/github_authenticator_spec.rb

@@ -4,6 +4,7 @@ require 'rails_helper'
 
 def auth_token_for(user)
   {
+    provider: "github",
     extra: {
       all_emails: [{
         email: user.email,
@@ -26,23 +27,7 @@ describe Auth::GithubAuthenticator do
   fab!(:user) { Fabricate(:user) }
 
   context 'after_authenticate' do
-    let(:data) do
+    let(:data) { auth_token_for(user) }
-      {
-        extra: {
-          all_emails: [{
-            email: user.email,
-            primary: true,
-            verified: true,
-          }]
-        },
-        info: {
-          email: user.email,
-          nickname: user.username,
-          name: user.name,
-        },
-        uid: "100"
-      }
-    end
 
     it 'can authenticate and create a user record for already existing users' do
       result = authenticator.after_authenticate(data)
@@ -61,18 +46,19 @@ describe Auth::GithubAuthenticator do
     end
 
     it 'can authenticate and update GitHub screen_name for existing user' do
-      GithubUserInfo.create!(user_id: user.id, github_user_id: 100, screen_name: "boris")
+      UserAssociatedAccount.create!(user_id: user.id, provider_name: "github", provider_uid: 100, info: { nickname: "boris" })
 
       result = authenticator.after_authenticate(data)
 
       expect(result.user.id).to eq(user.id)
       expect(result.email).to eq(user.email)
       expect(result.email_valid).to eq(true)
-      expect(GithubUserInfo.where(user_id: user.id).pluck(:screen_name)).to eq([user.username])
+      expect(UserAssociatedAccount.find_by(provider_name: "github", user_id: user.id).info["nickname"]).to eq(user.username)
     end
 
     it 'should use primary email for new user creation over other available emails' do
       hash = {
+        provider: "github",
         extra: {
           all_emails: [{
             email: "bob@example.com",
@@ -102,9 +88,10 @@ describe Auth::GithubAuthenticator do
       # There is a rare case where an end user had
       # 2 different github accounts and moved emails between the 2
 
-      GithubUserInfo.create!(user_id: user.id, screen_name: 'bob', github_user_id: 100)
+      UserAssociatedAccount.create!(user_id: user.id, info: { nickname: 'bob' }, provider_uid: 100, provider_name: "github")
 
       hash = {
+        provider: "github",
         extra: {
           all_emails: [{
             email: user.email,
@@ -123,11 +110,12 @@ describe Auth::GithubAuthenticator do
       result = authenticator.after_authenticate(hash)
 
       expect(result.user.id).to eq(user.id)
-      expect(GithubUserInfo.where(user_id: user.id).pluck(:github_user_id)).to eq([1001])
+      expect(UserAssociatedAccount.where(user_id: user.id).pluck(:provider_uid)).to eq(["1001"])
     end
 
     it 'will not authenticate for already existing users with an unverified email' do
       hash = {
+        provider: "github",
         extra: {
           all_emails: [{
             email: user.email,
@@ -154,6 +142,7 @@ describe Auth::GithubAuthenticator do
 
     it 'can create a proper result for non existing users' do
       hash = {
+        provider: "github",
         extra: {
           all_emails: [{
             email: "person@example.com",
@@ -180,6 +169,7 @@ describe Auth::GithubAuthenticator do
 
     it 'will skip blocklisted domains for non existing users' do
       hash = {
+        provider: "github",
         extra: {
           all_emails: [{
             email: "not_allowed@blocklist.com",
@@ -211,6 +201,7 @@ describe Auth::GithubAuthenticator do
 
     it 'will find allowlisted domains for non existing users' do
       hash = {
+        provider: "github",
         extra: {
           all_emails: [{
             email: "person@example.com",
@@ -250,13 +241,13 @@ describe Auth::GithubAuthenticator do
 
       expect(authenticator.can_connect_existing_user?).to eq(true)
 
-      GithubUserInfo.create!(user_id: user1.id, github_user_id: 100, screen_name: "boris")
+      UserAssociatedAccount.create!(provider_name: "github", user_id: user1.id, provider_uid: 100, info: { nickname: "boris" })
 
       result = authenticator.after_authenticate(data, existing_account: user2)
 
       expect(result.user.id).to eq(user2.id)
-      expect(GithubUserInfo.exists?(user_id: user1.id)).to eq(false)
+      expect(UserAssociatedAccount.exists?(user_id: user1.id)).to eq(false)
-      expect(GithubUserInfo.exists?(user_id: user2.id)).to eq(true)
+      expect(UserAssociatedAccount.exists?(user_id: user2.id)).to eq(true)
     end
 
   end
@@ -270,7 +261,7 @@ describe Auth::GithubAuthenticator do
     end
 
       it 'revokes correctly' do
-        GithubUserInfo.create!(user_id: user.id, github_user_id: 100, screen_name: "boris")
+        UserAssociatedAccount.create!(provider_name: "github", user_id: user.id, provider_uid: 100, info: { nickname: "boris" })
         expect(authenticator.can_revoke?).to eq(true)
         expect(authenticator.revoke(user)).to eq(true)
         expect(authenticator.description_for_user(user)).to eq("")

spec/jobs/invalidate_inactive_admins_spec.rb

@@ -45,13 +45,11 @@ 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)
           UserAssociatedAccount.create!(provider_name: "google_oauth2", user_id: not_seen_admin.id, provider_uid: 100, info: { email: "bob@google.account.com" })
         end
 
         it 'removes the social logins' do
           subject
-          expect(GithubUserInfo.where(user_id: not_seen_admin.id).exists?).to eq(false)
           expect(UserAssociatedAccount.where(user_id: not_seen_admin.id).exists?).to eq(false)
         end
       end

spec/models/user_spec.rb

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

spec/services/user_anonymizer_spec.rb

@@ -199,13 +199,11 @@ describe UserAnonymizer do
     end
 
     it "removes external auth assocations" do
-      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")]
       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")]
       make_anonymous
       user.reload
-      expect(user.github_user_info).to eq(nil)
       expect(user.user_associated_accounts).to be_empty
       expect(user.single_sign_on_record).to eq(nil)
       expect(user.oauth2_user_infos).to be_empty

spec/services/user_authenticator_spec.rb

@@ -11,8 +11,8 @@ def github_auth(email_valid)
     name: "Joe Doe 546",
     authenticator_name: "github",
     extra_data: {
-      github_user_id: "100",
+      provider: "github",
-      github_screen_name: "joedoe546"
+      uid: "100"
     },
     skip_email_validation: false
   }

spec/services/user_merger_spec.rb

@@ -1001,14 +1001,12 @@ describe UserMerger do
 
   it "deletes external auth infos of source user" 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")
     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")
 
     merge_users!
 
     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(Oauth2UserInfo.where(user_id: source_user.id).count).to eq(0)
     expect(SingleSignOnRecord.where(user_id: source_user.id).count).to eq(0)
   end