REFACTOR: Migrate GoogleOAuth2Authenticator to use ManagedAuthenticator (#7120)

REFACTOR: Migrate GoogleOAuth2Authenticator to use ManagedAuthenticator (#7120)

Future Social Authentication Improvements - feature - Discourse Meta

diff --git a/app/models/google_user_info.rb b/app/models/google_user_info.rb
deleted file mode 100644
index 343fe99..0000000
--- a/app/models/google_user_info.rb
+++ /dev/null
@@ -1,27 +0,0 @@
-class GoogleUserInfo < ActiveRecord::Base
-  belongs_to :user
-end
-
-# == Schema Information
-#
-# Table name: google_user_infos
-#
-#  id             :integer          not null, primary key
-#  user_id        :integer          not null
-#  google_user_id :string           not null
-#  first_name     :string
-#  last_name      :string
-#  email          :string
-#  gender         :string
-#  name           :string
-#  link           :string
-#  profile_link   :string
-#  picture        :string
-#  created_at     :datetime         not null
-#  updated_at     :datetime         not null
-#
-# Indexes
-#
-#  index_google_user_infos_on_google_user_id  (google_user_id) UNIQUE
-#  index_google_user_infos_on_user_id         (user_id) UNIQUE
-#
diff --git a/app/models/user.rb b/app/models/user.rb
index 6c9e3be..97c8440 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -66,7 +66,6 @@ class User < ActiveRecord::Base
   has_one :user_avatar, dependent: :destroy
   has_many :user_associated_accounts, dependent: :destroy
   has_one :github_user_info, dependent: :destroy
-  has_one :google_user_info, dependent: :destroy
   has_many :oauth2_user_infos, dependent: :destroy
   has_one :instagram_user_info, dependent: :destroy
   has_many :user_second_factors, dependent: :destroy
diff --git a/app/services/user_anonymizer.rb b/app/services/user_anonymizer.rb
index ae48025..dfeb941 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.google_user_info.try(:destroy)
       @user.github_user_info.try(:destroy)
       @user.single_sign_on_record.try(:destroy)
       @user.oauth2_user_infos.try(:destroy_all)
diff --git a/db/migrate/20190306154335_migrate_google_user_info.rb b/db/migrate/20190306154335_migrate_google_user_info.rb
new file mode 100644
index 0000000..a3a9c0e
--- /dev/null
+++ b/db/migrate/20190306154335_migrate_google_user_info.rb
@@ -0,0 +1,27 @@
+class MigrateGoogleUserInfo < 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
+      'google_oauth2',
+      google_user_id,
+      user_id,
+      json_build_object('email', email, 'first_name', first_name, 'last_name', last_name, 'name', name),
+      updated_at,
+      created_at,
+      updated_at
+    FROM google_user_infos
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/lib/auth/google_oauth2_authenticator.rb b/lib/auth/google_oauth2_authenticator.rb
index 297fa50..118d081 100644
--- a/lib/auth/google_oauth2_authenticator.rb
+++ b/lib/auth/google_oauth2_authenticator.rb
@@ -1,5 +1,4 @@
-class Auth::GoogleOAuth2Authenticator < Auth::Authenticator
-
+class Auth::GoogleOAuth2Authenticator < Auth::ManagedAuthenticator
   def name
     "google_oauth2"
   end
@@ -8,77 +7,10 @@ class Auth::GoogleOAuth2Authenticator < Auth::Authenticator
     SiteSetting.enable_google_oauth2_logins
   end
 
-  def description_for_user(user)
-    info = GoogleUserInfo.find_by(user_id: user.id)
-    info&.email || info&.name || ""
-  end
-
-  def can_revoke?
-    true
-  end
-
-  def revoke(user, skip_remote: false)
-    info = GoogleUserInfo.find_by(user_id: user.id)
-    raise Discourse::NotFound if info.nil?
-
-    # We get a temporary token from google 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 google's end
-
-    info.destroy!
-    true
-  end
-
-  def can_connect_existing_user?
-    true
-  end
-
-  def after_authenticate(auth_hash, existing_account: nil)
-    session_info = parse_hash(auth_hash)
-    google_hash = session_info[:google]
-
-    result = ::Auth::Result.new
-    result.email = session_info[:email]
-    result.email_valid = session_info[:email_valid]
-    result.name = session_info[:name]
-
-    result.extra_data = google_hash
-
-    user_info = ::GoogleUserInfo.find_by(google_user_id: google_hash[:google_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 = GoogleUserInfo.create!({ user_id: result.user.id }.merge(google_hash))
-    else
-      result.user = user_info&.user
-    end
-
-    if !result.user && !result.email.blank? && result.email_valid
-      result.user = User.find_by_email(result.email)
-      if result.user
-        # we've matched an existing user to this login attempt...
-        if result.user.google_user_info && result.user.google_user_info.google_user_id != google_hash[:google_user_id]
-          # but the user has changed the google account used to log in...
-          if result.user.google_user_info.email != google_hash[:email]
-            # the user changed their email, go ahead and scrub the old record
-            result.user.google_user_info.destroy!
-          else
-            # same email address but different account? likely a takeover scenario
-            result.failed = true
-            result.failed_reason = I18n.t('errors.conflicting_google_user_id')
-            return result
-          end
-        end
-        ::GoogleUserInfo.create({ user_id: result.user.id }.merge(google_hash))
-      end
-    end
-
-    result
-  end
-
-  def after_create_account(user, auth)
-    data = auth[:extra_data]
-    GoogleUserInfo.create({ user_id: user.id }.merge(data))
+  def primary_email_verified?(auth_token)
+    # note, emails that come back from google via omniauth are always valid
+    # this protects against future regressions
+    auth_token[:extra][:raw_info][:email_verified]
   end
 
   def register_middleware(omniauth)
@@ -95,37 +27,8 @@ class Auth::GoogleOAuth2Authenticator < Auth::Authenticator
         if (google_oauth2_prompt = SiteSetting.google_oauth2_prompt).present?
           strategy.options[:prompt] = google_oauth2_prompt.gsub("|", " ")
         end
-      },
-      skip_jwt: true
+      }
     }
-    # jwt encoding is causing auth to fail in quite a few conditions
-    # skipping
     omniauth.provider :google_oauth2, options
   end
-
-  protected
-
-  def parse_hash(hash)
-    extra = hash[:extra][:raw_info]
-
-    h = {}
-
-    h[:email] = hash[:info][:email]
-    h[:name] = hash[:info][:name]
-    h[:email_valid] = extra[:email_verified]
-
-    h[:google] = {
-      google_user_id: hash[:uid] || extra[:sub],
-      email: extra[:email],
-      first_name: extra[:given_name],
-      last_name: extra[:family_name],
-      gender: extra[:gender],
-      name: extra[:name],
-      link: extra[:hd],
-      profile_link: extra[:profile],
-      picture: extra[:picture]
-    }
-
-    h
-  end
 end
diff --git a/lib/auth/managed_authenticator.rb b/lib/auth/managed_authenticator.rb
index a56989c..1d21453 100644
--- a/lib/auth/managed_authenticator.rb
+++ b/lib/auth/managed_authenticator.rb
@@ -10,6 +10,12 @@ class Auth::ManagedAuthenticator < Auth::Authenticator
     true
   end
 
+  def primary_email_verified?(auth_token)
+    # Omniauth providers should only provide verified emails in the :info hash.
+    # This method allows additional checks to be added
+    true
+  end
+
   def can_revoke?
     true
   end
@@ -35,7 +41,11 @@ class Auth::ManagedAuthenticator < Auth::Authenticator
     end
 
     # Matching an account by email
-    if match_by_email && association.user.nil? && (user = User.find_by_email(auth_token.dig(:info, :email)))
+    if primary_email_verified?(auth_token) &&
+        match_by_email &&
+        association.user.nil? &&
+        (user = User.find_by_email(auth_token.dig(:info, :email)))
+

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

GitHub sha: fc7938f7

1 Like

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

DEV: Drop unused google and instagram auth_info tables