From f6d35ed57d156f4225338a89372c8e83721e46c9 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 29 Apr 2022 23:27:03 +0200 Subject: [PATCH] Remove IP matching from e-mail domain blocks (#18190) Clear out e-mail domain blocks created from automatically resolved DNS records --- app/models/email_domain_block.rb | 24 ++++---- app/validators/email_mx_validator.rb | 6 +- .../email_domain_block_refresh_scheduler.rb | 31 ---------- config/sidekiq.yml | 4 -- ...0180615122121_add_autofollow_to_invites.rb | 2 +- ...025_remove_ips_from_email_domain_blocks.rb | 12 ++++ ...0220429101850_clear_email_domain_blocks.rb | 14 +++++ db/schema.rb | 4 +- spec/validators/email_mx_validator_spec.rb | 60 ------------------- 9 files changed, 43 insertions(+), 114 deletions(-) delete mode 100644 app/workers/scheduler/email_domain_block_refresh_scheduler.rb create mode 100644 db/post_migrate/20220429101025_remove_ips_from_email_domain_blocks.rb create mode 100644 db/post_migrate/20220429101850_clear_email_domain_blocks.rb diff --git a/app/models/email_domain_block.rb b/app/models/email_domain_block.rb index 36e7e62ab8..0e1e663c10 100644 --- a/app/models/email_domain_block.rb +++ b/app/models/email_domain_block.rb @@ -3,16 +3,19 @@ # # Table name: email_domain_blocks # -# id :bigint(8) not null, primary key -# domain :string default(""), not null -# created_at :datetime not null -# updated_at :datetime not null -# parent_id :bigint(8) -# ips :inet is an Array -# last_refresh_at :datetime +# id :bigint(8) not null, primary key +# domain :string default(""), not null +# created_at :datetime not null +# updated_at :datetime not null +# parent_id :bigint(8) # class EmailDomainBlock < ApplicationRecord + self.ignored_columns = %w( + ips + last_refresh_at + ) + include DomainNormalizable belongs_to :parent, class_name: 'EmailDomainBlock', optional: true @@ -27,7 +30,7 @@ class EmailDomainBlock < ApplicationRecord @history ||= Trends::History.new('email_domain_blocks', id) end - def self.block?(domain_or_domains, ips: [], attempt_ip: nil) + def self.block?(domain_or_domains, attempt_ip: nil) domains = Array(domain_or_domains).map do |str| domain = begin if str.include?('@') @@ -48,10 +51,7 @@ class EmailDomainBlock < ApplicationRecord blocked = domains.any?(&:nil?) - scope = where(domain: domains) - scope = scope.or(where('ips && ARRAY[?]::inet[]', ips)) if ips.any? - - scope.find_each do |block| + where(domain: domains).find_each do |block| blocked = true block.history.add(attempt_ip) if attempt_ip.present? end diff --git a/app/validators/email_mx_validator.rb b/app/validators/email_mx_validator.rb index 237ca4c7bc..20f2fd37c6 100644 --- a/app/validators/email_mx_validator.rb +++ b/app/validators/email_mx_validator.rb @@ -15,7 +15,7 @@ class EmailMxValidator < ActiveModel::Validator if resolved_ips.empty? user.errors.add(:email, :unreachable) - elsif on_blacklist?(resolved_domains, resolved_ips, user.sign_up_ip) + elsif on_blacklist?(resolved_domains, user.sign_up_ip) user.errors.add(:email, :blocked) end end @@ -57,7 +57,7 @@ class EmailMxValidator < ActiveModel::Validator [ips, records] end - def on_blacklist?(domains, resolved_ips, attempt_ip) - EmailDomainBlock.block?(domains, ips: resolved_ips, attempt_ip: attempt_ip) + def on_blacklist?(domains, attempt_ip) + EmailDomainBlock.block?(domains, attempt_ip: attempt_ip) end end diff --git a/app/workers/scheduler/email_domain_block_refresh_scheduler.rb b/app/workers/scheduler/email_domain_block_refresh_scheduler.rb deleted file mode 100644 index e0ad89866b..0000000000 --- a/app/workers/scheduler/email_domain_block_refresh_scheduler.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -class Scheduler::EmailDomainBlockRefreshScheduler - include Sidekiq::Worker - include Redisable - - sidekiq_options retry: 0 - - def perform - Resolv::DNS.open do |dns| - dns.timeouts = 5 - - EmailDomainBlock.find_each do |email_domain_block| - ips = begin - if ip?(email_domain_block.domain) - [email_domain_block.domain] - else - resources = dns.getresources(email_domain_block.domain, Resolv::DNS::Resource::IN::A).to_a + dns.getresources(email_domain_block.domain, Resolv::DNS::Resource::IN::AAAA).to_a - resources.map { |resource| resource.address.to_s } - end - end - - email_domain_block.update(ips: ips, last_refresh_at: Time.now.utc) - end - end - end - - def ip?(str) - str =~ Regexp.union([Resolv::IPv4::Regex, Resolv::IPv6::Regex]) - end -end diff --git a/config/sidekiq.yml b/config/sidekiq.yml index f2ae9279bc..26be263265 100644 --- a/config/sidekiq.yml +++ b/config/sidekiq.yml @@ -17,10 +17,6 @@ every: '5m' class: Scheduler::Trends::RefreshScheduler queue: scheduler - email_domain_block_refresh_scheduler: - every: '1h' - class: Scheduler::EmailDomainBlockRefreshScheduler - queue: scheduler trends_review_notifications_scheduler: every: '6h' class: Scheduler::Trends::ReviewNotificationsScheduler diff --git a/db/migrate/20180615122121_add_autofollow_to_invites.rb b/db/migrate/20180615122121_add_autofollow_to_invites.rb index 850b1d693e..8c5fb74105 100644 --- a/db/migrate/20180615122121_add_autofollow_to_invites.rb +++ b/db/migrate/20180615122121_add_autofollow_to_invites.rb @@ -5,7 +5,7 @@ class AddAutofollowToInvites < ActiveRecord::Migration[5.2] disable_ddl_transaction! - def change + def up safety_assured do add_column_with_default :invites, :autofollow, :bool, default: false, allow_null: false end diff --git a/db/post_migrate/20220429101025_remove_ips_from_email_domain_blocks.rb b/db/post_migrate/20220429101025_remove_ips_from_email_domain_blocks.rb new file mode 100644 index 0000000000..fbb74d99eb --- /dev/null +++ b/db/post_migrate/20220429101025_remove_ips_from_email_domain_blocks.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class RemoveIpsFromEmailDomainBlocks < ActiveRecord::Migration[5.2] + disable_ddl_transaction! + + def change + safety_assured do + remove_column :email_domain_blocks, :ips, :inet, array: true + remove_column :email_domain_blocks, :last_refresh_at, :datetime + end + end +end diff --git a/db/post_migrate/20220429101850_clear_email_domain_blocks.rb b/db/post_migrate/20220429101850_clear_email_domain_blocks.rb new file mode 100644 index 0000000000..ff525b6506 --- /dev/null +++ b/db/post_migrate/20220429101850_clear_email_domain_blocks.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class ClearEmailDomainBlocks < ActiveRecord::Migration[5.2] + disable_ddl_transaction! + + class EmailDomainBlock < ApplicationRecord + end + + def up + EmailDomainBlock.where.not(parent_id: nil).in_batches.delete_all + end + + def down; end +end diff --git a/db/schema.rb b/db/schema.rb index a58f424003..726989beff 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2022_04_28_114902) do +ActiveRecord::Schema.define(version: 2022_04_29_101850) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -389,8 +389,6 @@ ActiveRecord::Schema.define(version: 2022_04_28_114902) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.bigint "parent_id" - t.inet "ips", array: true - t.datetime "last_refresh_at" t.index ["domain"], name: "index_email_domain_blocks_on_domain", unique: true end diff --git a/spec/validators/email_mx_validator_spec.rb b/spec/validators/email_mx_validator_spec.rb index af0eb98f5c..4feedd0c7f 100644 --- a/spec/validators/email_mx_validator_spec.rb +++ b/spec/validators/email_mx_validator_spec.rb @@ -56,66 +56,6 @@ describe EmailMxValidator do expect(user.errors).to have_received(:add) end - it 'adds an error if the A record is blacklisted' do - EmailDomainBlock.create!(domain: 'alternate-example.com', ips: ['1.2.3.4']) - resolver = double - - allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::MX).and_return([]) - allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::A).and_return([double(address: '1.2.3.4')]) - allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::AAAA).and_return([]) - allow(resolver).to receive(:timeouts=).and_return(nil) - allow(Resolv::DNS).to receive(:open).and_yield(resolver) - - subject.validate(user) - expect(user.errors).to have_received(:add) - end - - it 'adds an error if the AAAA record is blacklisted' do - EmailDomainBlock.create!(domain: 'alternate-example.com', ips: ['fd00::1']) - resolver = double - - allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::MX).and_return([]) - allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::A).and_return([]) - allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::AAAA).and_return([double(address: 'fd00::1')]) - allow(resolver).to receive(:timeouts=).and_return(nil) - allow(Resolv::DNS).to receive(:open).and_yield(resolver) - - subject.validate(user) - expect(user.errors).to have_received(:add) - end - - it 'adds an error if the A record of the MX record is blacklisted' do - EmailDomainBlock.create!(domain: 'mail.other-domain.com', ips: ['2.3.4.5']) - resolver = double - - allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::MX).and_return([double(exchange: 'mail.example.com')]) - allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::A).and_return([]) - allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::AAAA).and_return([]) - allow(resolver).to receive(:getresources).with('mail.example.com', Resolv::DNS::Resource::IN::A).and_return([double(address: '2.3.4.5')]) - allow(resolver).to receive(:getresources).with('mail.example.com', Resolv::DNS::Resource::IN::AAAA).and_return([]) - allow(resolver).to receive(:timeouts=).and_return(nil) - allow(Resolv::DNS).to receive(:open).and_yield(resolver) - - subject.validate(user) - expect(user.errors).to have_received(:add) - end - - it 'adds an error if the AAAA record of the MX record is blacklisted' do - EmailDomainBlock.create!(domain: 'mail.other-domain.com', ips: ['fd00::2']) - resolver = double - - allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::MX).and_return([double(exchange: 'mail.example.com')]) - allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::A).and_return([]) - allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::AAAA).and_return([]) - allow(resolver).to receive(:getresources).with('mail.example.com', Resolv::DNS::Resource::IN::A).and_return([]) - allow(resolver).to receive(:getresources).with('mail.example.com', Resolv::DNS::Resource::IN::AAAA).and_return([double(address: 'fd00::2')]) - allow(resolver).to receive(:timeouts=).and_return(nil) - allow(Resolv::DNS).to receive(:open).and_yield(resolver) - - subject.validate(user) - expect(user.errors).to have_received(:add) - end - it 'adds an error if the MX record is blacklisted' do EmailDomainBlock.create!(domain: 'mail.example.com') resolver = double