From 33a50884e5ea30bb74eafedfe78a2885f22eed1e Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 2 May 2024 22:56:21 +0200 Subject: [PATCH] Fix not being able to block a subdomain of an already-blocked domain through the API (#30119) --- .../api/v1/admin/domain_blocks_controller.rb | 9 ++- .../v1/admin/domain_blocks_controller_spec.rb | 71 ++++++++++++++----- 2 files changed, 60 insertions(+), 20 deletions(-) diff --git a/app/controllers/api/v1/admin/domain_blocks_controller.rb b/app/controllers/api/v1/admin/domain_blocks_controller.rb index 8b77e9717d..dacab4d191 100644 --- a/app/controllers/api/v1/admin/domain_blocks_controller.rb +++ b/app/controllers/api/v1/admin/domain_blocks_controller.rb @@ -19,10 +19,11 @@ class Api::V1::Admin::DomainBlocksController < Api::BaseController def create authorize :domain_block, :create? + @domain_block = DomainBlock.new(resource_params) existing_domain_block = resource_params[:domain].present? ? DomainBlock.rule_for(resource_params[:domain]) : nil - return render json: existing_domain_block, serializer: REST::Admin::ExistingDomainBlockErrorSerializer, status: 422 if existing_domain_block.present? + return render json: existing_domain_block, serializer: REST::Admin::ExistingDomainBlockErrorSerializer, status: 422 if conflicts_with_existing_block?(@domain_block, existing_domain_block) - @domain_block = DomainBlock.create!(resource_params) + @domain_block.save! DomainBlockWorker.perform_async(@domain_block.id) log_action :create, @domain_block render json: @domain_block, serializer: REST::Admin::DomainBlockSerializer @@ -55,6 +56,10 @@ class Api::V1::Admin::DomainBlocksController < Api::BaseController private + def conflicts_with_existing_block?(domain_block, existing_domain_block) + existing_domain_block.present? && (existing_domain_block.domain == TagManager.instance.normalize_domain(domain_block.domain) || !domain_block.stricter_than?(existing_domain_block)) + end + def set_domain_blocks @domain_blocks = filtered_domain_blocks.order(id: :desc).to_a_paginated_by_id(limit_param(LIMIT), params_slice(:max_id, :since_id, :min_id)) end diff --git a/spec/controllers/api/v1/admin/domain_blocks_controller_spec.rb b/spec/controllers/api/v1/admin/domain_blocks_controller_spec.rb index 606def602f..dc65aef5dc 100644 --- a/spec/controllers/api/v1/admin/domain_blocks_controller_spec.rb +++ b/spec/controllers/api/v1/admin/domain_blocks_controller_spec.rb @@ -16,6 +16,8 @@ RSpec.describe Api::V1::Admin::DomainBlocksController, type: :controller do let(:scopes) { wrong_scope } it 'returns http forbidden' do + subject + expect(response).to have_http_status(403) end end @@ -24,6 +26,8 @@ RSpec.describe Api::V1::Admin::DomainBlocksController, type: :controller do let(:role) { UserRole.find_by(name: wrong_role) } it 'returns http forbidden' do + subject + expect(response).to have_http_status(403) end end @@ -140,39 +144,70 @@ RSpec.describe Api::V1::Admin::DomainBlocksController, type: :controller do describe 'POST #create' do let(:existing_block_domain) { 'example.com' } + let(:params) { { domain: 'foo.bar.com', severity: :silence } } let!(:block) { Fabricate(:domain_block, domain: existing_block_domain, severity: :suspend) } - before do - post :create, params: { domain: 'foo.bar.com', severity: :silence } + subject do + post :create, params: params end it_behaves_like 'forbidden for wrong scope', 'write:statuses' it_behaves_like 'forbidden for wrong role', '' it_behaves_like 'forbidden for wrong role', 'Moderator' - it 'returns http success' do + it 'creates a domain block and returns expected domain name', :aggregate_failures do + subject + expect(response).to have_http_status(200) - end - - it 'returns expected domain name' do - json = body_as_json - expect(json[:domain]).to eq 'foo.bar.com' - end - - it 'creates a domain block' do + expect(body_as_json[:domain]).to eq 'foo.bar.com' expect(DomainBlock.find_by(domain: 'foo.bar.com')).to_not be_nil end - context 'when a stricter domain block already exists' do - let(:existing_block_domain) { 'bar.com' } + context 'when a looser domain block already exists on a higher level domain' do + let(:params) { { domain: 'foo.bar.com', severity: :suspend } } - it 'returns http unprocessable entity' do - expect(response).to have_http_status(422) + before do + Fabricate(:domain_block, domain: 'bar.com', severity: :silence) end - it 'renders existing domain block in error' do - json = body_as_json - expect(json[:existing_domain_block][:domain]).to eq existing_block_domain + it 'creates a domain block with the expected domain name and severity', :aggregate_failures do + subject + + body = body_as_json + + expect(response).to have_http_status(200) + expect(body).to match a_hash_including( + { + domain: 'foo.bar.com', + severity: 'suspend', + } + ) + + expect(DomainBlock.find_by(domain: 'foo.bar.com')).to be_present + end + end + + context 'when a domain block already exists on the same domain' do + before do + Fabricate(:domain_block, domain: 'foo.bar.com', severity: :silence) + end + + it 'returns existing domain block in error', :aggregate_failures do + subject + + expect(response).to have_http_status(422) + expect(body_as_json[:existing_domain_block][:domain]).to eq('foo.bar.com') + end + end + + context 'when a stricter domain block already exists on a higher level domain' do + let(:existing_block_domain) { 'bar.com' } + + it 'returns http unprocessable entity with existing domain block in error', :aggregate_reblogs do + subject + + expect(response).to have_http_status(422) + expect(body_as_json[:existing_domain_block][:domain]).to eq existing_block_domain end end end