From 719a0311a2cbcfb29802c4e6bfdc19b823565152 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Tue, 12 Aug 2025 17:35:11 -0400 Subject: [PATCH 1/2] Add `domain_variants` helper to `DomainNormalizable` concern --- .../concerns/account/attribution_domains.rb | 5 +-- app/models/concerns/domain_normalizable.rb | 5 +++ app/models/domain_allow.rb | 2 +- app/models/domain_block.rb | 7 +--- app/models/email_domain_block.rb | 8 +--- app/models/preview_card_provider.rb | 3 +- spec/models/domain_allow_spec.rb | 23 ++++++++++ spec/models/domain_block_spec.rb | 11 +++++ spec/models/email_domain_block_spec.rb | 42 +++++++++++++++++-- spec/models/preview_card_provider_spec.rb | 32 ++++++++++++++ 10 files changed, 116 insertions(+), 22 deletions(-) diff --git a/app/models/concerns/account/attribution_domains.rb b/app/models/concerns/account/attribution_domains.rb index 22bf27cc5d..ee8e6d136a 100644 --- a/app/models/concerns/account/attribution_domains.rb +++ b/app/models/concerns/account/attribution_domains.rb @@ -12,8 +12,7 @@ module Account::AttributionDomains end def can_be_attributed_from?(domain) - segments = domain.split('.') - variants = segments.map.with_index { |_, i| segments[i..].join('.') }.to_set - self[:attribution_domains].to_set.intersect?(variants) + variants = self.class.domain_variants(domain) + self[:attribution_domains].to_set.intersect?(variants.to_set) end end diff --git a/app/models/concerns/domain_normalizable.rb b/app/models/concerns/domain_normalizable.rb index 76f91c5b64..6bdfd71e1e 100644 --- a/app/models/concerns/domain_normalizable.rb +++ b/app/models/concerns/domain_normalizable.rb @@ -17,6 +17,11 @@ module DomainNormalizable SQL ) end + + def domain_variants(domain) + segments = domain.to_s.split('.') + segments.map.with_index { |_, i| segments[i..].join('.') } + end end private diff --git a/app/models/domain_allow.rb b/app/models/domain_allow.rb index 47ada7ac23..85ec549190 100644 --- a/app/models/domain_allow.rb +++ b/app/models/domain_allow.rb @@ -33,7 +33,7 @@ class DomainAllow < ApplicationRecord def rule_for(domain) return if domain.blank? - uri = Addressable::URI.new.tap { |u| u.host = domain.delete('/') } + uri = Addressable::URI.new.tap { |u| u.host = domain.strip.delete('/') } find_by(domain: uri.normalized_host) end diff --git a/app/models/domain_block.rb b/app/models/domain_block.rb index 8e7d7b6afc..73b2256fd0 100644 --- a/app/models/domain_block.rb +++ b/app/models/domain_block.rb @@ -68,11 +68,8 @@ class DomainBlock < ApplicationRecord def rule_for(domain) return if domain.blank? - uri = Addressable::URI.new.tap { |u| u.host = domain.strip.delete('/') } - segments = uri.normalized_host.split('.') - variants = segments.map.with_index { |_, i| segments[i..].join('.') } - - where(domain: variants).by_domain_length.first + uri = Addressable::URI.new.tap { |u| u.host = domain.strip.delete('/') } + where(domain: domain_variants(uri.normalized_host)).by_domain_length.first rescue Addressable::URI::InvalidURIError, IDN::Idna::IdnaError nil end diff --git a/app/models/email_domain_block.rb b/app/models/email_domain_block.rb index 583d2e6c1b..1af0d16938 100644 --- a/app/models/email_domain_block.rb +++ b/app/models/email_domain_block.rb @@ -64,13 +64,7 @@ class EmailDomainBlock < ApplicationRecord end def domains_with_variants - @uris.flat_map do |uri| - next if uri.nil? - - segments = uri.normalized_host.split('.') - - segments.map.with_index { |_, i| segments[i..].join('.') } - end + @uris.compact.flat_map { |uri| EmailDomainBlock.domain_variants(uri.normalized_host) }.uniq end def extract_uris(domain_or_domains) diff --git a/app/models/preview_card_provider.rb b/app/models/preview_card_provider.rb index 889176036c..2eb9d3e472 100644 --- a/app/models/preview_card_provider.rb +++ b/app/models/preview_card_provider.rb @@ -36,7 +36,6 @@ class PreviewCardProvider < ApplicationRecord scope :not_trendable, -> { where(trendable: false) } def self.matching_domain(domain) - segments = domain.split('.') - where(domain: segments.map.with_index { |_, i| segments[i..].join('.') }).by_domain_length.first + where(domain: domain_variants(domain)).by_domain_length.first end end diff --git a/spec/models/domain_allow_spec.rb b/spec/models/domain_allow_spec.rb index fbb324657e..2990781a4d 100644 --- a/spec/models/domain_allow_spec.rb +++ b/spec/models/domain_allow_spec.rb @@ -12,4 +12,27 @@ RSpec.describe DomainAllow do it { is_expected.to_not allow_value('xn--r9j5b5b').for(:domain) } end end + + describe '.rule_for' do + subject { described_class.rule_for(domain) } + + let(:domain) { 'host.example' } + + context 'with no records' do + it { is_expected.to be_nil } + end + + context 'with matching record' do + let!(:domain_allow) { Fabricate :domain_allow, domain: } + + it { is_expected.to eq(domain_allow) } + end + + context 'when called with non normalized string' do + let!(:domain_allow) { Fabricate :domain_allow, domain: } + let(:domain) { ' HOST.example/' } + + it { is_expected.to eq(domain_allow) } + end + end end diff --git a/spec/models/domain_block_spec.rb b/spec/models/domain_block_spec.rb index 14f904ea7f..e7f463c8f5 100644 --- a/spec/models/domain_block_spec.rb +++ b/spec/models/domain_block_spec.rb @@ -35,6 +35,17 @@ RSpec.describe DomainBlock do expect(described_class.rule_for('example.com')).to eq block end + it 'returns most specific rule matching a blocked domain' do + _block = Fabricate(:domain_block, domain: 'example.com') + blog_block = Fabricate(:domain_block, domain: 'blog.example.com') + expect(described_class.rule_for('host.blog.example.com')).to eq blog_block + end + + it 'returns rule matching a blocked domain when string needs normalization' do + block = Fabricate(:domain_block, domain: 'example.com') + expect(described_class.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(described_class.rule_for('sub.example.com')).to eq block diff --git a/spec/models/email_domain_block_spec.rb b/spec/models/email_domain_block_spec.rb index 5874c5e53c..c3662b2d6c 100644 --- a/spec/models/email_domain_block_spec.rb +++ b/spec/models/email_domain_block_spec.rb @@ -4,6 +4,8 @@ require 'rails_helper' RSpec.describe EmailDomainBlock do describe 'block?' do + subject { described_class.block?(input) } + let(:input) { nil } context 'when given an e-mail address' do @@ -14,12 +16,12 @@ RSpec.describe EmailDomainBlock do it 'returns true if the domain is blocked' do Fabricate(:email_domain_block, domain: 'example.com') - expect(described_class.block?(input)).to be true + expect(subject).to be true end it 'returns false if the domain is not blocked' do Fabricate(:email_domain_block, domain: 'other-example.com') - expect(described_class.block?(input)).to be false + expect(subject).to be false end end @@ -28,7 +30,7 @@ RSpec.describe EmailDomainBlock do it 'returns true if it is a subdomain of a blocked domain' do Fabricate(:email_domain_block, domain: 'example.com') - expect(described_class.block?(input)).to be true + expect(subject).to be true end end end @@ -38,8 +40,40 @@ RSpec.describe EmailDomainBlock do it 'returns true if the domain is blocked' do Fabricate(:email_domain_block, domain: 'mail.foo.com') - expect(described_class.block?(input)).to be true + expect(subject).to be true end end + + context 'when given nil' do + it { is_expected.to be false } + end + + context 'when given empty string' do + let(:input) { '' } + + it { is_expected.to be true } + end + end + + describe '.requires_approval?' do + subject { described_class.requires_approval?(input) } + + let(:input) { nil } + + context 'with a matching block requiring approval' do + before { Fabricate :email_domain_block, domain: input, allow_with_approval: true } + + let(:input) { 'host.example' } + + it { is_expected.to be true } + end + + context 'with a matching block not requiring approval' do + before { Fabricate :email_domain_block, domain: input, allow_with_approval: false } + + let(:input) { 'host.example' } + + it { is_expected.to be false } + end end end diff --git a/spec/models/preview_card_provider_spec.rb b/spec/models/preview_card_provider_spec.rb index 561c93d0b2..cd3283faa3 100644 --- a/spec/models/preview_card_provider_spec.rb +++ b/spec/models/preview_card_provider_spec.rb @@ -25,4 +25,36 @@ RSpec.describe PreviewCardProvider do end end end + + describe '.matching_domain' do + subject { described_class.matching_domain(domain) } + + let(:domain) { 'host.example' } + + context 'without matching domains' do + it { is_expected.to be_nil } + end + + context 'with exact matching domain' do + let!(:preview_card_provider) { Fabricate :preview_card_provider, domain: 'host.example' } + + it { is_expected.to eq(preview_card_provider) } + end + + context 'with matching domain segment' do + let!(:preview_card_provider) { Fabricate :preview_card_provider, domain: 'host.example' } + let(:domain) { 'www.blog.host.example' } + + it { is_expected.to eq(preview_card_provider) } + end + + context 'with multiple matching records' do + let!(:preview_card_provider_more) { Fabricate :preview_card_provider, domain: 'blog.host.example' } + let(:domain) { 'www.blog.host.example' } + + before { Fabricate :preview_card_provider, domain: 'host.example' } + + it { is_expected.to eq(preview_card_provider_more) } + end + end end From ef0e88fde6b1b492ad0b13a82aaec39844b3d956 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Wed, 13 Aug 2025 11:16:32 -0400 Subject: [PATCH 2/2] Avoid unused variable from with_index --- app/models/concerns/domain_normalizable.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/concerns/domain_normalizable.rb b/app/models/concerns/domain_normalizable.rb index 6bdfd71e1e..a4700f3a6e 100644 --- a/app/models/concerns/domain_normalizable.rb +++ b/app/models/concerns/domain_normalizable.rb @@ -20,7 +20,7 @@ module DomainNormalizable def domain_variants(domain) segments = domain.to_s.split('.') - segments.map.with_index { |_, i| segments[i..].join('.') } + Array.new(segments.size) { |i| segments[i..].join('.') } end end