diff --git a/app/jobs/scheduled/clean_up_associated_accounts.rb b/app/jobs/scheduled/clean_up_associated_accounts.rb new file mode 100644 index 00000000000..2d589cbcb38 --- /dev/null +++ b/app/jobs/scheduled/clean_up_associated_accounts.rb @@ -0,0 +1,12 @@ +module Jobs + + class CleanUpAssociatedAccounts < Jobs::Scheduled + every 1.day + + def execute(args) + UserAssociatedAccount.cleanup! + end + + end + +end diff --git a/app/models/user_associated_account.rb b/app/models/user_associated_account.rb index f23c4e069ae..ef85326ef1e 100644 --- a/app/models/user_associated_account.rb +++ b/app/models/user_associated_account.rb @@ -1,5 +1,11 @@ class UserAssociatedAccount < ActiveRecord::Base belongs_to :user + + def self.cleanup! + # This happens when a user starts the registration flow, but doesn't complete it + # Keeping the rows doesn't cause any technical issue, but we shouldn't store PII unless it's attached to a user + self.where("user_id IS NULL AND updated_at < ?", 1.day.ago).destroy_all + end end # == Schema Information @@ -9,7 +15,7 @@ end # id :bigint(8) not null, primary key # provider_name :string not null # provider_uid :string not null -# user_id :integer not null +# user_id :integer # last_used :datetime not null # info :jsonb not null # credentials :jsonb not null diff --git a/db/migrate/20181210122522_remove_not_null_user_associated_account_user.rb b/db/migrate/20181210122522_remove_not_null_user_associated_account_user.rb new file mode 100644 index 00000000000..b9613412bf3 --- /dev/null +++ b/db/migrate/20181210122522_remove_not_null_user_associated_account_user.rb @@ -0,0 +1,10 @@ +class RemoveNotNullUserAssociatedAccountUser < ActiveRecord::Migration[5.2] + def change + begin + Migration::SafeMigrate.disable! + change_column_null :user_associated_accounts, :user_id, true + ensure + Migration::SafeMigrate.enable! + end + end +end diff --git a/lib/auth/managed_authenticator.rb b/lib/auth/managed_authenticator.rb index 666c5a1cd8b..b7218b9084a 100644 --- a/lib/auth/managed_authenticator.rb +++ b/lib/auth/managed_authenticator.rb @@ -26,77 +26,56 @@ class Auth::ManagedAuthenticator < Auth::Authenticator end def after_authenticate(auth_token, existing_account: nil) - result = Auth::Result.new - - # Store all the metadata for later, in case the `after_create_account` hook is used - result.extra_data = { - provider: auth_token[:provider], - uid: auth_token[:uid], - info: auth_token[:info], - extra: auth_token[:extra], - credentials: auth_token[:credentials] - } - - # Build the Auth::Result object - info = auth_token[:info] - result.email = email = info[:email] - result.name = name = "#{info[:first_name]} #{info[:last_name]}" - result.username = info[:nickname] - # Try and find an association for this account - association = UserAssociatedAccount.find_by(provider_name: auth_token[:provider], provider_uid: auth_token[:uid]) - result.user = association&.user + association = UserAssociatedAccount.find_or_initialize_by(provider_name: auth_token[:provider], provider_uid: auth_token[:uid]) # Reconnecting to existing account - if can_connect_existing_user? && existing_account && (association.nil? || existing_account.id != association.user_id) - association.destroy! if association - association = nil - result.user = existing_account + if can_connect_existing_user? && existing_account && (association.user.nil? || existing_account.id != association.user_id) + association.user = existing_account end # Matching an account by email - if match_by_email && association.nil? && result.user.nil? && (user = User.find_by_email(email)) + if 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 - result.user = user - end - - # Add the association to the database if it doesn't already exist - if association.nil? && result.user - association = create_association!(result.extra_data.merge(user: result.user)) + association.user = user end # Update all the metadata in the association: - if association - association.update!( - info: auth_token[:info] || {}, - credentials: auth_token[:credentials] || {}, - extra: auth_token[:extra] || {} - ) - retrieve_avatar(result.user, auth_token.dig(:info, :image)) - retrieve_profile(result.user, auth_token[:info]) - end + association.info = auth_token[:info] || {} + association.credentials = auth_token[:credentials] || {} + association.extra = auth_token[:extra] || {} + # Save to the DB. Do this even if we don't have a user - it might be linked up later in after_create_account + association.save! + + # Update avatar/profile + retrieve_avatar(association.user, association.info["image"]) + retrieve_profile(association.user, association.info) + + # Build the Auth::Result object + result = Auth::Result.new + info = auth_token[:info] + result.email = info[:email] + result.name = "#{info[:first_name]} #{info[:last_name]}" + result.username = info[:nickname] result.email_valid = true if result.email + result.extra_data = { + provider: auth_token[:provider], + uid: auth_token[:uid] + } + result.user = association.user result end - def create_association!(hash) - association = UserAssociatedAccount.create!( - user: hash[:user], - provider_name: hash[:provider], - provider_uid: hash[:uid], - info: hash[:info] || {}, - credentials: hash[:credentials] || {}, - extra: hash[:extra] || {} - ) - end - def after_create_account(user, auth) - data = auth[:extra_data] - create_association!(data.merge(user: user)) - retrieve_avatar(user, data.dig(:info, :image)) - retrieve_profile(user, data[:info]) + auth_token = auth[:extra_data] + association = UserAssociatedAccount.find_or_initialize_by(provider_name: auth_token[:provider], provider_uid: auth_token[:uid]) + association.user = user + association.save! + + retrieve_avatar(user, association.info["image"]) + retrieve_profile(user, association.info) end def retrieve_avatar(user, url) @@ -108,8 +87,8 @@ class Auth::ManagedAuthenticator < Auth::Authenticator def retrieve_profile(user, info) return unless user - bio = info[:description] - location = info[:location] + bio = info["description"] + location = info["location"] if bio || location profile = user.user_profile diff --git a/spec/components/auth/managed_authenticator_spec.rb b/spec/components/auth/managed_authenticator_spec.rb index e02a45d3b20..870536be183 100644 --- a/spec/components/auth/managed_authenticator_spec.rb +++ b/spec/components/auth/managed_authenticator_spec.rb @@ -29,6 +29,13 @@ describe Auth::ManagedAuthenticator do } } + let(:create_hash) { + { + provider: "myauth", + uid: "1234" + } + } + describe 'after_authenticate' do it 'can match account from an existing association' do user = Fabricate(:user) @@ -110,10 +117,15 @@ describe Auth::ManagedAuthenticator do context 'when no matching user' do it 'returns the correct information' do - result = authenticator.after_authenticate(hash) + result = nil + expect { + result = authenticator.after_authenticate(hash) + }.to change { UserAssociatedAccount.count }.by(1) expect(result.user).to eq(nil) expect(result.username).to eq("IAmGroot") expect(result.email).to eq("awesome@example.com") + expect(UserAssociatedAccount.last.user).to eq(nil) + expect(UserAssociatedAccount.last.info["nickname"]).to eq("IAmGroot") end it 'works if there is already an association with the target account' do @@ -170,25 +182,34 @@ describe Auth::ManagedAuthenticator do describe "avatar on create" do let(:user) { Fabricate(:user) } + let!(:association) { UserAssociatedAccount.create!(provider_name: 'myauth', provider_uid: "1234") } + it "doesn't schedule with no image" do - expect { result = authenticator.after_create_account(user, extra_data: hash) } + expect { result = authenticator.after_create_account(user, extra_data: create_hash) } .to change { Jobs::DownloadAvatarFromUrl.jobs.count }.by(0) end it "schedules with image" do - expect { result = authenticator.after_create_account(user, extra_data: hash.deep_merge(info: { image: "https://some.domain/image.jpg" })) } + association.info["image"] = "https://some.domain/image.jpg" + association.save! + expect { result = authenticator.after_create_account(user, extra_data: create_hash) } .to change { Jobs::DownloadAvatarFromUrl.jobs.count }.by(1) end end describe "profile on create" do let(:user) { Fabricate(:user) } + let!(:association) { UserAssociatedAccount.create!(provider_name: 'myauth', provider_uid: "1234") } + it "doesn't explode without profile" do - authenticator.after_create_account(user, extra_data: hash) + authenticator.after_create_account(user, extra_data: create_hash) end it "works with profile" do - authenticator.after_create_account(user, extra_data: hash.deep_merge(info: { location: "DiscourseVille", description: "Online forum expert" })) + association.info["location"] = "DiscourseVille" + association.info["description"] = "Online forum expert" + association.save! + authenticator.after_create_account(user, extra_data: create_hash) expect(user.user_profile.bio_raw).to eq("Online forum expert") expect(user.user_profile.location).to eq("DiscourseVille") end diff --git a/spec/jobs/clean_up_associated_accounts_spec.rb b/spec/jobs/clean_up_associated_accounts_spec.rb new file mode 100644 index 00000000000..4874ad53320 --- /dev/null +++ b/spec/jobs/clean_up_associated_accounts_spec.rb @@ -0,0 +1,17 @@ +require 'rails_helper' + +describe Jobs::CleanUpAssociatedAccounts do + subject { Jobs::CleanUpAssociatedAccounts.new.execute({}) } + + it "deletes the correct records" do + freeze_time + + last_week = UserAssociatedAccount.create(provider_name: "twitter", provider_uid: "1", updated_at: 7.days.ago) + today = UserAssociatedAccount.create(provider_name: "twitter", provider_uid: "12", updated_at: 12.hours.ago) + connected = UserAssociatedAccount.create(provider_name: "twitter", provider_uid: "123", user: Fabricate(:user), updated_at: 12.hours.ago) + + expect { subject }.to change { UserAssociatedAccount.count }.by(-1) + expect(UserAssociatedAccount.all).to contain_exactly(today, connected) + end + +end