From 707ddf7808f90e3ab042d7642d368c2ce8e95e6f Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sat, 22 Jun 2019 00:13:10 +0200 Subject: [PATCH] Change domain blocks to automatically support subdomains (#11138) * Change domain blocks to automatically support subdomains If a more authoritative domain is blocked (example.com), then the same block will be applied to a subdomain (foo.example.com) * Match subdomains of existing accounts when blocking/unblocking domains * Improve code style --- .../admin/domain_blocks_controller.rb | 2 +- app/controllers/admin/instances_controller.rb | 2 +- app/controllers/media_proxy_controller.rb | 2 +- app/lib/activitypub/activity/create.rb | 2 +- app/lib/activitypub/activity/flag.rb | 2 +- app/lib/ostatus/activity/creation.rb | 4 +-- app/models/account.rb | 1 + app/models/custom_emoji.rb | 1 + app/models/domain_block.rb | 33 +++++++++++++++++-- app/models/instance.rb | 2 +- .../activitypub/process_account_service.rb | 2 +- app/services/block_domain_service.rb | 4 +-- app/services/resolve_account_service.rb | 2 +- app/services/unblock_domain_service.rb | 3 +- app/services/update_remote_profile_service.rb | 4 +-- spec/models/account_spec.rb | 17 ++++++++++ spec/models/domain_block_spec.rb | 31 +++++++++++++---- 17 files changed, 89 insertions(+), 25 deletions(-) diff --git a/app/controllers/admin/domain_blocks_controller.rb b/app/controllers/admin/domain_blocks_controller.rb index 71597763b..377cac8ad 100644 --- a/app/controllers/admin/domain_blocks_controller.rb +++ b/app/controllers/admin/domain_blocks_controller.rb @@ -13,7 +13,7 @@ module Admin authorize :domain_block, :create? @domain_block = DomainBlock.new(resource_params) - existing_domain_block = resource_params[:domain].present? ? DomainBlock.find_by(domain: resource_params[:domain]) : nil + existing_domain_block = resource_params[:domain].present? ? DomainBlock.rule_for(resource_params[:domain]) : nil if existing_domain_block.present? && !@domain_block.stricter_than?(existing_domain_block) @domain_block.save diff --git a/app/controllers/admin/instances_controller.rb b/app/controllers/admin/instances_controller.rb index 6dd659a30..7888e844f 100644 --- a/app/controllers/admin/instances_controller.rb +++ b/app/controllers/admin/instances_controller.rb @@ -18,7 +18,7 @@ module Admin @blocks_count = Block.where(target_account: Account.where(domain: params[:id])).count @available = DeliveryFailureTracker.available?(Account.select(:shared_inbox_url).where(domain: params[:id]).first&.shared_inbox_url) @media_storage = MediaAttachment.where(account: Account.where(domain: params[:id])).sum(:file_file_size) - @domain_block = DomainBlock.find_by(domain: params[:id]) + @domain_block = DomainBlock.rule_for(params[:id]) end private diff --git a/app/controllers/media_proxy_controller.rb b/app/controllers/media_proxy_controller.rb index 950cf6d09..8fc18dd06 100644 --- a/app/controllers/media_proxy_controller.rb +++ b/app/controllers/media_proxy_controller.rb @@ -39,6 +39,6 @@ class MediaProxyController < ApplicationController end def reject_media? - DomainBlock.find_by(domain: @media_attachment.account.domain)&.reject_media? + DomainBlock.reject_media?(@media_attachment.account.domain) end end diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index f55dd35b2..487e8e91e 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -380,7 +380,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity def skip_download? return @skip_download if defined?(@skip_download) - @skip_download ||= DomainBlock.find_by(domain: @account.domain)&.reject_media? + @skip_download ||= DomainBlock.reject_media?(@account.domain) end def reply_to_local? diff --git a/app/lib/activitypub/activity/flag.rb b/app/lib/activitypub/activity/flag.rb index f73b93058..1659bc61f 100644 --- a/app/lib/activitypub/activity/flag.rb +++ b/app/lib/activitypub/activity/flag.rb @@ -23,7 +23,7 @@ class ActivityPub::Activity::Flag < ActivityPub::Activity private def skip_reports? - DomainBlock.find_by(domain: @account.domain)&.reject_reports? + DomainBlock.reject_reports?(@account.domain) end def object_uris diff --git a/app/lib/ostatus/activity/creation.rb b/app/lib/ostatus/activity/creation.rb index 3840c8fbf..60de712db 100644 --- a/app/lib/ostatus/activity/creation.rb +++ b/app/lib/ostatus/activity/creation.rb @@ -148,7 +148,7 @@ class OStatus::Activity::Creation < OStatus::Activity::Base end def save_media - do_not_download = DomainBlock.find_by(domain: @account.domain)&.reject_media? + do_not_download = DomainBlock.reject_media?(@account.domain) media_attachments = [] @xml.xpath('./xmlns:link[@rel="enclosure"]', xmlns: OStatus::TagManager::XMLNS).each do |link| @@ -176,7 +176,7 @@ class OStatus::Activity::Creation < OStatus::Activity::Base end def save_emojis(parent) - do_not_download = DomainBlock.find_by(domain: parent.account.domain)&.reject_media? + do_not_download = DomainBlock.reject_media?(parent.account.domain) return if do_not_download diff --git a/app/models/account.rb b/app/models/account.rb index 9276aa927..c588451fc 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -98,6 +98,7 @@ class Account < ApplicationRecord scope :tagged_with, ->(tag) { joins(:accounts_tags).where(accounts_tags: { tag_id: tag }) } scope :by_recent_status, -> { order(Arel.sql('(case when account_stats.last_status_at is null then 1 else 0 end) asc, account_stats.last_status_at desc')) } scope :popular, -> { order('account_stats.followers_count desc') } + scope :by_domain_and_subdomains, ->(domain) { where(domain: domain).or(where(arel_table[:domain].matches('%.' + domain))) } delegate :email, :unconfirmed_email, diff --git a/app/models/custom_emoji.rb b/app/models/custom_emoji.rb index d3cc70504..e73cd9bd2 100644 --- a/app/models/custom_emoji.rb +++ b/app/models/custom_emoji.rb @@ -39,6 +39,7 @@ class CustomEmoji < ApplicationRecord scope :local, -> { where(domain: nil) } scope :remote, -> { where.not(domain: nil) } scope :alphabetic, -> { order(domain: :asc, shortcode: :asc) } + scope :by_domain_and_subdomains, ->(domain) { where(domain: domain).or(where(arel_table[:domain].matches('%.' + domain))) } remotable_attachment :image, LIMIT diff --git a/app/models/domain_block.rb b/app/models/domain_block.rb index 84c08c158..25d3b87ef 100644 --- a/app/models/domain_block.rb +++ b/app/models/domain_block.rb @@ -24,14 +24,41 @@ class DomainBlock < ApplicationRecord scope :matches_domain, ->(value) { where(arel_table[:domain].matches("%#{value}%")) } - def self.blocked?(domain) - where(domain: domain, severity: :suspend).exists? + class << self + def suspend?(domain) + !!rule_for(domain)&.suspend? + end + + def silence?(domain) + !!rule_for(domain)&.silence? + end + + def reject_media?(domain) + !!rule_for(domain)&.reject_media? + end + + def reject_reports?(domain) + !!rule_for(domain)&.reject_reports? + end + + alias blocked? suspend? + + def rule_for(domain) + return if domain.blank? + + uri = Addressable::URI.new.tap { |u| u.host = domain.gsub(/[\/]/, '') } + segments = uri.normalized_host.split('.') + variants = segments.map.with_index { |_, i| segments[i..-1].join('.') } + + where(domain: variants[0..-2]).order(Arel.sql('char_length(domain) desc')).first + end end def stricter_than?(other_block) - return true if suspend? + return true if suspend? return false if other_block.suspend? && (silence? || noop?) return false if other_block.silence? && noop? + (reject_media || !other_block.reject_media) && (reject_reports || !other_block.reject_reports) end diff --git a/app/models/instance.rb b/app/models/instance.rb index 7bf000d40..a01db1212 100644 --- a/app/models/instance.rb +++ b/app/models/instance.rb @@ -8,7 +8,7 @@ class Instance def initialize(resource) @domain = resource.domain @accounts_count = resource.is_a?(DomainBlock) ? nil : resource.accounts_count - @domain_block = resource.is_a?(DomainBlock) ? resource : DomainBlock.find_by(domain: domain) + @domain_block = resource.is_a?(DomainBlock) ? resource : DomainBlock.rule_for(domain) end def cached_sample_accounts diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index ad22d37fe..05c017bdf 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/app/services/activitypub/process_account_service.rb @@ -205,7 +205,7 @@ class ActivityPub::ProcessAccountService < BaseService def domain_block return @domain_block if defined?(@domain_block) - @domain_block = DomainBlock.find_by(domain: @domain) + @domain_block = DomainBlock.rule_for(@domain) end def key_changed? diff --git a/app/services/block_domain_service.rb b/app/services/block_domain_service.rb index 497f0394b..c6eef04d4 100644 --- a/app/services/block_domain_service.rb +++ b/app/services/block_domain_service.rb @@ -76,7 +76,7 @@ class BlockDomainService < BaseService end def blocked_domain_accounts - Account.where(domain: blocked_domain) + Account.by_domain_and_subdomains(blocked_domain) end def media_from_blocked_domain @@ -84,6 +84,6 @@ class BlockDomainService < BaseService end def emojis_from_blocked_domains - CustomEmoji.where(domain: blocked_domain) + CustomEmoji.by_domain_and_subdomains(blocked_domain) end end diff --git a/app/services/resolve_account_service.rb b/app/services/resolve_account_service.rb index 11e33a83a..57c9ccfe1 100644 --- a/app/services/resolve_account_service.rb +++ b/app/services/resolve_account_service.rb @@ -146,7 +146,7 @@ class ResolveAccountService < BaseService def domain_block return @domain_block if defined?(@domain_block) - @domain_block = DomainBlock.find_by(domain: @domain) + @domain_block = DomainBlock.rule_for(@domain) end def atom_url diff --git a/app/services/unblock_domain_service.rb b/app/services/unblock_domain_service.rb index 9b8526fbe..fc262a50a 100644 --- a/app/services/unblock_domain_service.rb +++ b/app/services/unblock_domain_service.rb @@ -14,7 +14,8 @@ class UnblockDomainService < BaseService end def blocked_accounts - scope = Account.where(domain: domain_block.domain) + scope = Account.by_domain_and_subdomains(domain_block.domain) + if domain_block.silence? scope.where(silenced_at: @domain_block.created_at) else diff --git a/app/services/update_remote_profile_service.rb b/app/services/update_remote_profile_service.rb index 68d36addf..403395a0d 100644 --- a/app/services/update_remote_profile_service.rb +++ b/app/services/update_remote_profile_service.rb @@ -26,7 +26,7 @@ class UpdateRemoteProfileService < BaseService account.note = remote_profile.note || '' account.locked = remote_profile.locked? - if !account.suspended? && !DomainBlock.find_by(domain: account.domain)&.reject_media? + if !account.suspended? && !DomainBlock.reject_media?(account.domain) if remote_profile.avatar.present? account.avatar_remote_url = remote_profile.avatar else @@ -46,7 +46,7 @@ class UpdateRemoteProfileService < BaseService end def save_emojis - do_not_download = DomainBlock.find_by(domain: account.domain)&.reject_media? + do_not_download = DomainBlock.reject_media?(account.domain) return if do_not_download diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index 379872316..ce9ea250d 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -687,6 +687,23 @@ RSpec.describe Account, type: :model do end end + describe 'by_domain_and_subdomains' do + it 'returns exact domain matches' do + account = Fabricate(:account, domain: 'example.com') + expect(Account.by_domain_and_subdomains('example.com')).to eq [account] + end + + it 'returns subdomains' do + account = Fabricate(:account, domain: 'foo.example.com') + expect(Account.by_domain_and_subdomains('example.com')).to eq [account] + end + + it 'does not return partially matching domains' do + account = Fabricate(:account, domain: 'grexample.com') + expect(Account.by_domain_and_subdomains('example.com')).to_not eq [account] + end + end + describe 'expiring' do it 'returns remote accounts with followers whose subscription expiration date is past or not given' do local = Fabricate(:account, domain: nil) diff --git a/spec/models/domain_block_spec.rb b/spec/models/domain_block_spec.rb index 0035fd0ff..d98c5e118 100644 --- a/spec/models/domain_block_spec.rb +++ b/spec/models/domain_block_spec.rb @@ -21,23 +21,40 @@ RSpec.describe DomainBlock, type: :model do end end - describe 'blocked?' do + describe '.blocked?' do it 'returns true if the domain is suspended' do - Fabricate(:domain_block, domain: 'domain', severity: :suspend) - expect(DomainBlock.blocked?('domain')).to eq true + Fabricate(:domain_block, domain: 'example.com', severity: :suspend) + expect(DomainBlock.blocked?('example.com')).to eq true end it 'returns false even if the domain is silenced' do - Fabricate(:domain_block, domain: 'domain', severity: :silence) - expect(DomainBlock.blocked?('domain')).to eq false + Fabricate(:domain_block, domain: 'example.com', severity: :silence) + expect(DomainBlock.blocked?('example.com')).to eq false end it 'returns false if the domain is not suspended nor silenced' do - expect(DomainBlock.blocked?('domain')).to eq false + expect(DomainBlock.blocked?('example.com')).to eq false end end - describe 'stricter_than?' do + describe '.rule_for' do + it 'returns rule matching a blocked domain' do + block = Fabricate(:domain_block, domain: 'example.com') + expect(DomainBlock.rule_for('example.com')).to eq block + end + + it 'returns a rule matching a subdomain of a blocked domain' do + block = Fabricate(:domain_block, domain: 'example.com') + expect(DomainBlock.rule_for('sub.example.com')).to eq block + end + + it 'returns a rule matching a blocked subdomain' do + block = Fabricate(:domain_block, domain: 'sub.example.com') + expect(DomainBlock.rule_for('sub.example.com')).to eq block + end + end + + describe '#stricter_than?' do it 'returns true if the new block has suspend severity while the old has lower severity' do suspend = DomainBlock.new(domain: 'domain', severity: :suspend) silence = DomainBlock.new(domain: 'domain', severity: :silence)