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