REFACTOR: Migrate GoogleOAuth2Authenticator to use ManagedAuthenticator (#7120)

https://meta.discourse.org/t/future-social-authentication-improvements/94691/3

app/models/google_user_info.rb

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

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

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)

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

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)
+  def primary_email_verified?(auth_token)
-    info = GoogleUserInfo.find_by(user_id: user.id)
+    # note, emails that come back from google via omniauth are always valid
-    info&.email || info&.name || ""
+    # this protects against future regressions
-  end
+    auth_token[:extra][:raw_info][:email_verified]
-
-  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))
   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

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)))
+
       UserAssociatedAccount.where(user: user, provider_name: auth_token[:provider]).destroy_all # Destroy existing associations for the new user
       association.user = user
     end
@@ -60,7 +70,7 @@ class Auth::ManagedAuthenticator < Auth::Authenticator
     result.email = info[:email]
     result.name = "#{info[:first_name]} #{info[:last_name]}"
     result.username = info[:nickname]
-    result.email_valid = true if result.email
+    result.email_valid = primary_email_verified?(auth_token) if result.email
     result.extra_data = {
       provider: auth_token[:provider],
       uid: auth_token[:uid]

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, Oauth2UserInfo,
+    [UserAssociatedAccount, GithubUserInfo, Oauth2UserInfo,
       SingleSignOnRecord, EmailChangeRequest
     ].each do |c|
       copy_model(c, skip_if_merged: true, is_a_user_model: true)
@@ -628,11 +628,6 @@ class BulkImport::DiscourseMerger < BulkImport::Base
     r
   end
 
-  def process_google_user_info(r)
-    return nil if GoogleUserInfo.where(google_user_id: r['google_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/google_oauth2_authenticator_spec.rb

@@ -10,6 +10,7 @@ describe Auth::GoogleOAuth2Authenticator do
     user = Fabricate(:user)
 
     hash = {
+      provider: "google_oauth2",
       uid: "123456789",
       info: {
           name: "John Doe",
@@ -35,6 +36,7 @@ describe Auth::GoogleOAuth2Authenticator do
       user = Fabricate(:user)
 
       hash = {
+        provider: "google_oauth2",
         uid: "123456789",
         info: {
             name: "John Doe",
@@ -59,9 +61,10 @@ describe Auth::GoogleOAuth2Authenticator do
       user1 = Fabricate(:user)
       user2 = Fabricate(:user)
 
-      GoogleUserInfo.create!(user_id: user1.id, google_user_id: 100)
+      UserAssociatedAccount.create!(provider_name: "google_oauth2", user_id: user1.id, provider_uid: 100)
 
       hash = {
+        provider: "google_oauth2",
         uid: "100",
         info: {
             name: "John Doe",
@@ -79,14 +82,17 @@ describe Auth::GoogleOAuth2Authenticator do
       result = authenticator.after_authenticate(hash, existing_account: user2)
 
       expect(result.user.id).to eq(user2.id)
-      expect(GoogleUserInfo.exists?(user_id: user1.id)).to eq(false)
+      expect(UserAssociatedAccount.exists?(user_id: user1.id)).to eq(false)
-      expect(GoogleUserInfo.exists?(user_id: user2.id)).to eq(true)
+      expect(UserAssociatedAccount.exists?(user_id: user2.id)).to eq(true)
     end
 
     it 'can create a proper result for non existing users' do
       hash = {
+        provider: "google_oauth2",
         uid: "123456789",
         info: {
+            first_name: "Jane",
+            last_name: "Doe",
             name: "Jane Doe",
             email: "jane.doe@the.google.com"
         },
@@ -103,7 +109,7 @@ describe Auth::GoogleOAuth2Authenticator do
       result = authenticator.after_authenticate(hash)
 
       expect(result.user).to eq(nil)
-      expect(result.extra_data[:name]).to eq("Jane Doe")
+      expect(result.name).to eq("Jane Doe")
     end
   end
 
@@ -116,7 +122,7 @@ describe Auth::GoogleOAuth2Authenticator do
     end
 
       it 'revokes correctly' do
-        GoogleUserInfo.create!(user_id: user.id, google_user_id: 12345, email: 'someuser@somedomain.tld')
+        UserAssociatedAccount.create!(provider_name: "google_oauth2", user_id: user.id, provider_uid: 12345)
         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

@@ -38,13 +38,13 @@ describe Jobs::InvalidateInactiveAdmins do
         before do
           GithubUserInfo.create!(user_id: not_seen_admin.id, screen_name: 'bob', github_user_id: 100)
           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')
+          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(GoogleUserInfo.where(user_id: not_seen_admin.id).exists?).to eq(false)
+          expect(UserAssociatedAccount.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
       end

spec/models/user_spec.rb

@@ -428,7 +428,7 @@ 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)
+      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)
 
       user.reload

spec/requests/omniauth_callbacks_controller_spec.rb

@@ -96,7 +96,9 @@ RSpec.describe Users::OmniauthCallbacksController do
           uid: '123545',
           info: OmniAuth::AuthHash::InfoHash.new(
             email: email,
-            name: 'Some name'
+            name: 'Some name',
+            first_name: "Some",
+            last_name: "name"
           ),
           extra: {
             raw_info: OmniAuth::AuthHash.new(
@@ -107,7 +109,7 @@ RSpec.describe Users::OmniauthCallbacksController do
               gender: 'male',
               name: "Some name Huh",
             )
-          },
+          }
         )
 
         Rails.application.env_config["omniauth.auth"] = OmniAuth.config.mock_auth[:google_oauth2]
@@ -262,7 +264,7 @@ RSpec.describe Users::OmniauthCallbacksController do
           @sso.return_sso_url = "http://somewhere.over.rainbow/sso"
           cookies[:sso_payload] = @sso.payload
 
-          GoogleUserInfo.create!(google_user_id: '12345', user: user)
+          UserAssociatedAccount.create!(provider_name: "google_oauth2", provider_uid: '12345', user: user)
 
           OmniAuth.config.mock_auth[:google_oauth2] = OmniAuth::AuthHash.new(
             provider: 'google_oauth2',
@@ -299,7 +301,7 @@ RSpec.describe Users::OmniauthCallbacksController do
 
       context 'when user has not verified his email' do
         before do
-          GoogleUserInfo.create!(google_user_id: '12345', user: user)
+          UserAssociatedAccount.create!(provider_name: "google_oauth2", provider_uid: '12345', user: user)
           user.update!(active: false)
 
           OmniAuth.config.mock_auth[:google_oauth2] = OmniAuth::AuthHash.new(
@@ -341,8 +343,8 @@ RSpec.describe Users::OmniauthCallbacksController do
     context 'when attempting reconnect' do
       let(:user2) { Fabricate(:user) }
       before do
-        GoogleUserInfo.create!(google_user_id: '12345', user: user)
+        UserAssociatedAccount.create!(provider_name: "google_oauth2", provider_uid: '12345', user: user)
-        GoogleUserInfo.create!(google_user_id: '123456', user: user2)
+        UserAssociatedAccount.create!(provider_name: "google_oauth2", provider_uid: '123456', user: user2)
 
         OmniAuth.config.mock_auth[:google_oauth2] = OmniAuth::AuthHash.new(
           provider: 'google_oauth2',
@@ -385,7 +387,7 @@ RSpec.describe Users::OmniauthCallbacksController do
         get "/auth/google_oauth2/callback.json"
         expect(response.status).to eq(200)
         expect(session[:current_user_id]).to eq(user2.id)
-        expect(GoogleUserInfo.count).to eq(2)
+        expect(UserAssociatedAccount.count).to eq(2)
       end
 
       it 'should reconnect if parameter supplied' do
@@ -402,7 +404,7 @@ RSpec.describe Users::OmniauthCallbacksController do
         expect(session[:auth_reconnect]).to eq(nil)
 
         # Disconnect
-        GoogleUserInfo.find_by(user_id: user.id).destroy
+        UserAssociatedAccount.find_by(user_id: user.id).destroy
 
         # Reconnect flow:
         get "/auth/google_oauth2?reconnect=true"
@@ -414,7 +416,7 @@ RSpec.describe Users::OmniauthCallbacksController do
         expect(response.status).to eq(200)
         expect(JSON.parse(response.body)["authenticated"]).to eq(true)
         expect(session[:current_user_id]).to eq(user.id)
-        expect(GoogleUserInfo.count).to eq(1)
+        expect(UserAssociatedAccount.count).to eq(1)
       end
 
     end

spec/services/user_anonymizer_spec.rb

@@ -190,7 +190,6 @@ describe UserAnonymizer do
     end
 
     it "removes external auth assocations" do
-      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")]
       user.single_sign_on_record = SingleSignOnRecord.create(user_id: user.id, external_id: "example", last_payload: "looks good")
@@ -198,7 +197,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.google_user_info).to eq(nil)
       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)

spec/services/user_merger_spec.rb

@@ -978,7 +978,6 @@ 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")
-    GoogleUserInfo.create(user_id: source_user.id, google_user_id: "google@gmail.com")
     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)
@@ -987,7 +986,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(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)