From 5d35c38db272225c3f3af6125a4836905fc31359 Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Tue, 11 Jan 2022 09:16:51 +0200 Subject: [PATCH] FEATURE: Search screened IP address in blocks (#15461) An admin could search for all screened ip addresses in a block by using wildcards. 192.168.* returned all IPs in range 192.168.0.0/16. This feature allows admins to search for a single IP address in all screened IP blocks. 192.168.0.1 returns all IP blocks that match it, for example 192.168.0.0/16. * FEATURE: Remove roll up button for screened IPs * FIX: Match more specific screened IP address first --- .../admin-logs-screened-ip-addresses.js | 30 ----------- .../admin/addon/models/screened-ip-address.js | 4 -- .../templates/logs/screened-ip-addresses.hbs | 5 -- .../admin/screened_ip_addresses_controller.rb | 9 ++-- app/models/screened_ip_address.rb | 6 +-- config/locales/client.en.yml | 3 -- config/routes.rb | 6 +-- spec/models/screened_ip_address_spec.rb | 12 +++++ .../screened_ip_addresses_controller_spec.rb | 53 ++++--------------- 9 files changed, 29 insertions(+), 99 deletions(-) diff --git a/app/assets/javascripts/admin/addon/controllers/admin-logs-screened-ip-addresses.js b/app/assets/javascripts/admin/addon/controllers/admin-logs-screened-ip-addresses.js index fb923131ed7..df734004ece 100644 --- a/app/assets/javascripts/admin/addon/controllers/admin-logs-screened-ip-addresses.js +++ b/app/assets/javascripts/admin/addon/controllers/admin-logs-screened-ip-addresses.js @@ -107,36 +107,6 @@ export default Controller.extend({ this.model.unshiftObject(arg); }, - rollUp() { - return bootbox.confirm( - I18n.t("admin.logs.screened_ips.roll_up_confirm"), - I18n.t("no_value"), - I18n.t("yes_value"), - (confirmed) => { - if (confirmed) { - this.set("loading", true); - return ScreenedIpAddress.rollUp().then((results) => { - if (results && results.subnets) { - if (results.subnets.length > 0) { - this.send("show"); - bootbox.alert( - I18n.t("admin.logs.screened_ips.rolled_up_some_subnets", { - subnets: results.subnets.join(", "), - }) - ); - } else { - this.set("loading", false); - bootbox.alert( - I18n.t("admin.logs.screened_ips.rolled_up_no_subnet") - ); - } - } - }); - } - } - ); - }, - exportScreenedIpList() { exportEntity("screened_ip").then(outputExportResult); }, diff --git a/app/assets/javascripts/admin/addon/models/screened-ip-address.js b/app/assets/javascripts/admin/addon/models/screened-ip-address.js index 7389509f898..0fe7578f89f 100644 --- a/app/assets/javascripts/admin/addon/models/screened-ip-address.js +++ b/app/assets/javascripts/admin/addon/models/screened-ip-address.js @@ -47,10 +47,6 @@ ScreenedIpAddress.reopenClass({ screened_ips.map((b) => ScreenedIpAddress.create(b)) ); }, - - rollUp() { - return ajax("/admin/logs/screened_ip_addresses/roll_up", { type: "POST" }); - }, }); export default ScreenedIpAddress; diff --git a/app/assets/javascripts/admin/addon/templates/logs/screened-ip-addresses.hbs b/app/assets/javascripts/admin/addon/templates/logs/screened-ip-addresses.hbs index 3da09f772d8..1531a541bef 100644 --- a/app/assets/javascripts/admin/addon/templates/logs/screened-ip-addresses.hbs +++ b/app/assets/javascripts/admin/addon/templates/logs/screened-ip-addresses.hbs @@ -8,11 +8,6 @@ placeholderKey="admin.logs.screened_ips.form.filter" autocorrect="off" autocapitalize="off"}} - {{d-button - class="btn-default" - action=(action "rollUp") - title="admin.logs.screened_ips.roll_up.title" - label="admin.logs.screened_ips.roll_up.text"}} {{d-button class="btn-default" action=(action "exportScreenedIpList") diff --git a/app/controllers/admin/screened_ip_addresses_controller.rb b/app/controllers/admin/screened_ip_addresses_controller.rb index 400620c4ccf..85897d503c6 100644 --- a/app/controllers/admin/screened_ip_addresses_controller.rb +++ b/app/controllers/admin/screened_ip_addresses_controller.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require_dependency 'ip_addr' + class Admin::ScreenedIpAddressesController < Admin::AdminController before_action :fetch_screened_ip_address, only: [:update, :destroy] @@ -9,7 +11,7 @@ class Admin::ScreenedIpAddressesController < Admin::AdminController filter = IPAddr.handle_wildcards(filter) screened_ip_addresses = ScreenedIpAddress - screened_ip_addresses = screened_ip_addresses.where("cidr :filter >>= ip_address", filter: filter) if filter.present? + screened_ip_addresses = screened_ip_addresses.where("cidr :filter >>= ip_address OR ip_address >>= cidr :filter", filter: filter) if filter.present? screened_ip_addresses = screened_ip_addresses.limit(200).order('match_count desc') begin @@ -44,11 +46,6 @@ class Admin::ScreenedIpAddressesController < Admin::AdminController render json: success_json end - def roll_up - subnets = ScreenedIpAddress.roll_up(current_user) - render json: success_json.merge!(subnets: subnets) - end - private def allowed_params diff --git a/app/models/screened_ip_address.rb b/app/models/screened_ip_address.rb index d371864eda3..9d25a3c9a1a 100644 --- a/app/models/screened_ip_address.rb +++ b/app/models/screened_ip_address.rb @@ -67,7 +67,7 @@ class ScreenedIpAddress < ActiveRecord::Base # # http://www.postgresql.org/docs/9.1/static/datatype-net-types.html # http://www.postgresql.org/docs/9.1/static/functions-net.html - find_by("? <<= ip_address", ip_address.to_s) + order('masklen(ip_address) DESC').find_by("? <<= ip_address", ip_address.to_s) end def self.should_block?(ip_address) @@ -80,8 +80,8 @@ class ScreenedIpAddress < ActiveRecord::Base def self.exists_for_ip_address_and_action?(ip_address, action_type, opts = {}) b = match_for_ip_address(ip_address) - found = (!!b && b.action_type == (action_type)) - b.record_match! if found && opts[:record_match] != (false) + found = !!b && b.action_type == action_type + b.record_match! if found && opts[:record_match] != false found end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 72f2e25ad65..12fb9cd1603 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -4908,9 +4908,6 @@ en: title: "Screened IPs" description: 'IP addresses that are being watched. Use "Allow" to allowlist IP addresses.' delete_confirm: "Are you sure you want to remove the rule for %{ip_address}?" - roll_up_confirm: "Are you sure you want to roll up commonly screened IP addresses into subnets?" - rolled_up_some_subnets: "Successfully rolled up IP ban entries to these subnets: %{subnets}." - rolled_up_no_subnet: "There was nothing to roll up." actions: block: "Block" do_nothing: "Allow" diff --git a/config/routes.rb b/config/routes.rb index 1f75aaa30ce..32e5a05c1db 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -178,11 +178,7 @@ Discourse::Application.routes.draw do resources :staff_action_logs, only: [:index] get 'staff_action_logs/:id/diff' => 'staff_action_logs#diff' resources :screened_emails, only: [:index, :destroy] - resources :screened_ip_addresses, only: [:index, :create, :update, :destroy] do - collection do - post "roll_up" - end - end + resources :screened_ip_addresses, only: [:index, :create, :update, :destroy] resources :screened_urls, only: [:index] resources :search_logs, only: [:index] get 'search_logs/term/' => 'search_logs#term' diff --git a/spec/models/screened_ip_address_spec.rb b/spec/models/screened_ip_address_spec.rb index 5c069135fb5..eda9e75d3f8 100644 --- a/spec/models/screened_ip_address_spec.rb +++ b/spec/models/screened_ip_address_spec.rb @@ -187,6 +187,18 @@ describe ScreenedIpAddress do expect(described_class.should_block?('222.12.12.12')).to eq(false) end + it 'returns false if a more specific recrord matches and action is :do_nothing' do + Fabricate(:screened_ip_address, ip_address: '111.234.23.0/24', action_type: described_class.actions[:block]) + Fabricate(:screened_ip_address, ip_address: '111.234.23.11', action_type: described_class.actions[:do_nothing]) + expect(described_class.should_block?('111.234.23.11')).to eq(false) + expect(described_class.should_block?('111.234.23.12')).to eq(true) + + Fabricate(:screened_ip_address, ip_address: '222.234.23.0/24', action_type: described_class.actions[:do_nothing]) + Fabricate(:screened_ip_address, ip_address: '222.234.23.11', action_type: described_class.actions[:block]) + expect(described_class.should_block?('222.234.23.11')).to eq(true) + expect(described_class.should_block?('222.234.23.12')).to eq(false) + end + context 'IPv4' do it 'returns false when when record matches and action is :do_nothing' do Fabricate(:screened_ip_address, ip_address: '111.234.23.11', action_type: described_class.actions[:do_nothing]) diff --git a/spec/requests/admin/screened_ip_addresses_controller_spec.rb b/spec/requests/admin/screened_ip_addresses_controller_spec.rb index c4ae9927ef1..62d1b06eba2 100644 --- a/spec/requests/admin/screened_ip_addresses_controller_spec.rb +++ b/spec/requests/admin/screened_ip_addresses_controller_spec.rb @@ -20,63 +20,30 @@ describe Admin::ScreenedIpAddressesController do Fabricate(:screened_ip_address, ip_address: "1.2.3.5") Fabricate(:screened_ip_address, ip_address: "1.2.3.6") Fabricate(:screened_ip_address, ip_address: "4.5.6.7") + Fabricate(:screened_ip_address, ip_address: "5.0.0.0/8") get "/admin/logs/screened_ip_addresses.json", params: { filter: "1.2.*" } expect(response.status).to eq(200) - result = response.parsed_body - expect(result.length).to eq(3) + expect(response.parsed_body.map { |record| record["ip_address"] }) + .to contain_exactly("1.2.3.4", "1.2.3.5", "1.2.3.6") get "/admin/logs/screened_ip_addresses.json", params: { filter: "4.5.6.7" } expect(response.status).to eq(200) - result = response.parsed_body - expect(result.length).to eq(1) - end - end + expect(response.parsed_body.map { |record| record["ip_address"] }) + .to contain_exactly("4.5.6.7") - describe '#roll_up' do - it "rolls up 1.2.3.* entries" do - Fabricate(:screened_ip_address, ip_address: "1.2.3.4", match_count: 1) - Fabricate(:screened_ip_address, ip_address: "1.2.3.5", match_count: 1) - Fabricate(:screened_ip_address, ip_address: "1.2.3.6", match_count: 1) - - Fabricate(:screened_ip_address, ip_address: "42.42.42.4", match_count: 1) - Fabricate(:screened_ip_address, ip_address: "42.42.42.5", match_count: 1) - - SiteSetting.min_ban_entries_for_roll_up = 3 - - expect do - post "/admin/logs/screened_ip_addresses/roll_up.json" - end.to change { UserHistory.where(action: UserHistory.actions[:roll_up]).count }.by(1) + get "/admin/logs/screened_ip_addresses.json", params: { filter: "5.0.0.1" } expect(response.status).to eq(200) + expect(response.parsed_body.map { |record| record["ip_address"] }) + .to contain_exactly("5.0.0.0/8") - subnet = ScreenedIpAddress.where(ip_address: "1.2.3.0/24").first - expect(subnet).to be_present - expect(subnet.match_count).to eq(3) - end - - it "rolls up 1.2.*.* entries" do - Fabricate(:screened_ip_address, ip_address: "1.2.3.4", match_count: 1) - Fabricate(:screened_ip_address, ip_address: "1.2.3.5", match_count: 1) - Fabricate(:screened_ip_address, ip_address: "1.2.4.6", match_count: 1) - Fabricate(:screened_ip_address, ip_address: "1.2.7.8", match_count: 1) - Fabricate(:screened_ip_address, ip_address: "1.2.9.1", match_count: 1) - - Fabricate(:screened_ip_address, ip_address: "1.2.42.0/24", match_count: 1) - - SiteSetting.min_ban_entries_for_roll_up = 5 - - expect do - post "/admin/logs/screened_ip_addresses/roll_up.json" - end.to change { UserHistory.where(action: UserHistory.actions[:roll_up]).count }.by(1) + get "/admin/logs/screened_ip_addresses.json", params: { filter: "6.0.0.1" } expect(response.status).to eq(200) - - subnet = ScreenedIpAddress.where(ip_address: "1.2.0.0/16").first - expect(subnet).to be_present - expect(subnet.match_count).to eq(6) + expect(response.parsed_body).to be_blank end end end