diff --git a/app/models/github_user_info.rb b/app/models/github_user_info.rb deleted file mode 100644 index ca088c0cb14..00000000000 --- a/app/models/github_user_info.rb +++ /dev/null @@ -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 -# diff --git a/app/models/user.rb b/app/models/user.rb index 72933dfdc7d..f0275931780 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/services/user_anonymizer.rb b/app/services/user_anonymizer.rb index 2760e3e387e..472a1eb715c 100644 --- a/app/services/user_anonymizer.rb +++ b/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) diff --git a/db/migrate/20201109170951_migrate_github_user_infos.rb b/db/migrate/20201109170951_migrate_github_user_infos.rb new file mode 100644 index 00000000000..8803d06f950 --- /dev/null +++ b/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 diff --git a/lib/auth/github_authenticator.rb b/lib/auth/github_authenticator.rb index 1925086217d..c082bd021eb 100644 --- a/lib/auth/github_authenticator.rb +++ b/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 - - data = auth_token[:info] - result.username = screen_name = data[:nickname] - result.name = data[:name] - - github_user_id = auth_token[:uid] - - result.extra_data = { - 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 - ) + result = super + return result if result.user + # If email domain restrictions are configured, + # pick a secondary email which is allowed + all_github_emails(auth_token).each do |candidate| + next if !EmailValidator.allowed?(candidate[:email]) + result.email = candidate[:email] + result.email_valid = !!candidate[:verified] + break 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) - data = auth[:extra_data] - GithubUserInfo.create( - user_id: user.id, - screen_name: data[:github_screen_name], - github_user_id: data[:github_user_id] - ) + def find_user_by_email(auth_token) + # Use verified secondary emails to find a match + all_github_emails(auth_token).each do |candidate| + next if !candidate[:verified] + if user = User.find_by_email(candidate[:email]) + 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 diff --git a/lib/auth/managed_authenticator.rb b/lib/auth/managed_authenticator.rb index ac9f8ebef41..8cb182094a9 100644 --- a/lib/auth/managed_authenticator.rb +++ b/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) && - match_by_email && + if 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? diff --git a/script/bulk_import/discourse_merger.rb b/script/bulk_import/discourse_merger.rb index 35ff800822c..6bc189a6648 100644 --- a/script/bulk_import/discourse_merger.rb +++ b/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 diff --git a/spec/components/auth/github_authenticator_spec.rb b/spec/components/auth/github_authenticator_spec.rb index b9a9f4788df..2a3a83cf298 100644 --- a/spec/components/auth/github_authenticator_spec.rb +++ b/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 - { - extra: { - all_emails: [{ - email: user.email, - primary: true, - verified: true, - }] - }, - info: { - email: user.email, - nickname: user.username, - name: user.name, - }, - uid: "100" - } - end + let(:data) { auth_token_for(user) } 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(GithubUserInfo.exists?(user_id: user2.id)).to eq(true) + expect(UserAssociatedAccount.exists?(user_id: user1.id)).to eq(false) + 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("") diff --git a/spec/jobs/invalidate_inactive_admins_spec.rb b/spec/jobs/invalidate_inactive_admins_spec.rb index d3fd43aeb39..0ca2eeb26d0 100644 --- a/spec/jobs/invalidate_inactive_admins_spec.rb +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 7203680ac3d..3e69f08afa3 100644 --- a/spec/models/user_spec.rb +++ b/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') diff --git a/spec/services/user_anonymizer_spec.rb b/spec/services/user_anonymizer_spec.rb index 71d0d2f3360..84723680e8c 100644 --- a/spec/services/user_anonymizer_spec.rb +++ b/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 diff --git a/spec/services/user_authenticator_spec.rb b/spec/services/user_authenticator_spec.rb index c7f6b989f41..5db539d6618 100644 --- a/spec/services/user_authenticator_spec.rb +++ b/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", - github_screen_name: "joedoe546" + provider: "github", + uid: "100" }, skip_email_validation: false } diff --git a/spec/services/user_merger_spec.rb b/spec/services/user_merger_spec.rb index 88b704590c3..ef77a76a5c1 100644 --- a/spec/services/user_merger_spec.rb +++ b/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