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

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);
     },

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;

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")

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

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))
+    found = !!b && b.action_type == action_type
-    b.record_match! if found && opts[:record_match] != (false)
+    b.record_match! if found && opts[:record_match] != false
     found
   end
 

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"

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
+        resources :screened_ip_addresses, only: [:index, :create, :update, :destroy]
-          collection do
-            post "roll_up"
-          end
-        end
         resources :screened_urls,         only: [:index]
         resources :search_logs,           only: [:index]
         get 'search_logs/term/' => 'search_logs#term'

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])

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(response.parsed_body.map { |record| record["ip_address"] })
-      expect(result.length).to eq(3)
+        .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(response.parsed_body.map { |record| record["ip_address"] })
-      expect(result.length).to eq(1)
+        .to contain_exactly("4.5.6.7")
-    end
-  end
 
-  describe '#roll_up' do
+      get "/admin/logs/screened_ip_addresses.json", params: { filter: "5.0.0.1" }
-    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)
 
       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
+      get "/admin/logs/screened_ip_addresses.json", params: { filter: "6.0.0.1" }
-      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)
 
       expect(response.status).to eq(200)
-
+      expect(response.parsed_body).to be_blank
-      subnet = ScreenedIpAddress.where(ip_address: "1.2.0.0/16").first
-      expect(subnet).to be_present
-      expect(subnet.match_count).to eq(6)
     end
   end
 end