ip bans: add hit counter, deleted flag, new ban type.

* Make IP bans soft deletable.
* Add a hit counter to track how many times an IP ban has blocked someone.
* Add a last hit timestamp to track when the IP ban last blocked someone.
* Add a new type of IP ban, the signup ban. Signup bans restrict new
  signups from editing anything until they've verified their email
  address.

app/controllers/application_controller.rb

@@ -163,7 +163,7 @@ class ApplicationController < ActionController::Base
   end
 
   def ip_ban_check
-    raise User::PrivilegeError if !request.get? && IpBan.is_banned?(CurrentUser.ip_addr)
+    raise User::PrivilegeError if !request.get? && IpBan.hit!(:normal, CurrentUser.ip_addr)
   end
 
   def pundit_user

app/controllers/ip_bans_controller.rb

@@ -19,9 +19,10 @@ class IpBansController < ApplicationController
     respond_with(@ip_bans)
   end
 
-  def destroy
+  def update
     @ip_ban = authorize IpBan.find(params[:id])
-    @ip_ban.destroy
+    @ip_ban.update(permitted_attributes(@ip_ban))
+
     respond_with(@ip_ban)
   end
 end

app/controllers/users_controller.rb

@@ -59,9 +59,11 @@ class UsersController < ApplicationController
   end
 
   def create
+    requires_verification = IpLookup.new(CurrentUser.ip_addr).is_proxy? || IpBan.hit!(:signup, CurrentUser.ip_addr)
+
     @user = authorize User.new(
       last_ip_addr: CurrentUser.ip_addr,
-      requires_verification: IpLookup.new(CurrentUser.ip_addr).is_proxy?,
+      requires_verification: requires_verification,
       name: params[:user][:name],
       password: params[:user][:password],
       password_confirmation: params[:user][:password_confirmation]

app/helpers/application_helper.rb

@@ -122,7 +122,9 @@ module ApplicationHelper
   end
 
   def time_ago_in_words_tagged(time, compact: false)
-    if time.past?
+    if time.nil?
+      tag.em(tag.time("unknown"))
+    elsif time.past?
       if compact
         text = time_ago_in_words(time)
         text = text.gsub(/almost|about|over/, "").strip

app/models/ip_ban.rb

@@ -1,13 +1,27 @@
 class IpBan < ApplicationRecord
   belongs_to :creator, class_name: "User"
-  validate :validate_ip_addr
-  validates_presence_of :reason
-  validates_uniqueness_of :ip_addr
-  after_create  { ModAction.log("#{creator.name} created ip ban for #{ip_addr}", :ip_ban_create) }
-  after_destroy { ModAction.log("#{creator.name} deleted ip ban for #{ip_addr}", :ip_ban_delete) }
 
-  def self.is_banned?(ip_addr)
+  validate :validate_ip_addr
-    where("ip_addr >>= ?", ip_addr).exists?
+  validates :reason, presence: true
+
+  before_save :create_mod_action
+
+  deletable
+  enum category: {
+    normal: 0,
+    signup: 100
+  }, _suffix: "ban"
+
+  def self.ip_matches(ip_addr)
+    where("ip_addr >>= ?", ip_addr)
+  end
+
+  def self.hit!(category, ip_addr)
+    ip_ban = active.where(category: category).ip_matches(ip_addr).first
+    return false unless ip_ban
+
+    IpBan.increment_counter(:hit_count, ip_ban.id, touch: [:last_hit_at])
+    true
   end
 
   def self.search(params)
@@ -21,15 +35,31 @@ class IpBan < ApplicationRecord
     q.apply_default_order(params)
   end
 
+  def create_mod_action
+    if new_record?
+      ModAction.log("#{creator.name} created ip ban for #{ip_addr}", :ip_ban_create)
+    elsif is_deleted? == true && is_deleted_was == false
+      ModAction.log("#{creator.name} deleted ip ban for #{ip_addr}", :ip_ban_delete)
+    elsif is_deleted? == false && is_deleted_was == true
+      ModAction.log("#{creator.name} undeleted ip ban for #{ip_addr}", :ip_ban_undelete)
+    end
+  end
+
   def validate_ip_addr
     if ip_addr.blank?
       errors[:ip_addr] << "is invalid"
-    elsif ip_addr.ipv4? && ip_addr.prefix < 24
-      errors[:ip_addr] << "may not have a subnet bigger than /24"
-    elsif ip_addr.ipv6? && ip_addr.prefix < 64
-      errors[:ip_addr] << "may not have a subnet bigger than /64"
     elsif ip_addr.private? || ip_addr.loopback? || ip_addr.link_local?
       errors[:ip_addr] << "must be a public address"
+    elsif normal_ban? && ip_addr.ipv4? && ip_addr.prefix < 24
+      errors[:ip_addr] << "may not have a subnet bigger than /24"
+    elsif signup_ban? && ip_addr.ipv4? && ip_addr.prefix < 8
+      errors[:ip_addr] << "may not have a subnet bigger than /8"
+    elsif normal_ban? && ip_addr.ipv6? && ip_addr.prefix < 64
+      errors[:ip_addr] << "may not have a subnet bigger than /64"
+    elsif signup_ban? && ip_addr.ipv6? && ip_addr.prefix < 20
+      errors[:ip_addr] << "may not have a subnet bigger than /20"
+    elsif new_record? && IpBan.active.ip_matches(subnetted_ip).exists?
+      errors[:ip_addr] << "is already banned"
     end
   end
 

app/models/mod_action.rb

@@ -48,6 +48,7 @@ class ModAction < ApplicationRecord
     tag_implication_update: 141,
     ip_ban_create: 160,
     ip_ban_delete: 162,
+    ip_ban_undelete: 163,
     mass_update: 1000,
     bulk_revert: 1001, # XXX unused
     other: 2000

app/policies/ip_ban_policy.rb

@@ -7,11 +7,11 @@ class IpBanPolicy < ApplicationPolicy
     user.is_moderator?
   end
 
-  def destroy?
+  def update?
     user.is_moderator?
   end
 
   def permitted_attributes
-    [:ip_addr, :reason]
+    [:ip_addr, :reason, :is_deleted, :category]
   end
 end

app/views/ip_addresses/_info.html.erb

@@ -26,7 +26,7 @@
     <tr>
       <th>IP Banned</th>
       <td>
-        <% if IpBan.is_banned?(ip_address.ip_addr.to_s) %>
+        <% if IpBan.ip_matches(ip_address.ip_addr.to_s).exists? %>
           yes (<%= link_to "info", ip_bans_path(search: { ip_addr: ip_address.to_s }) %>)
         <% else %>
           no

app/views/ip_bans/index.html.erb

@@ -2,16 +2,35 @@
   <div id="a-index">
     <h1>IP Bans</h1>
 
-    <%= table_for @ip_bans, width: "100%" do |t| %>
+    <%= table_for @ip_bans, class: "striped autofit", width: "100%" do |t| %>
       <% t.column "IP Address" do |ip_ban| %>
         <%= link_to_ip ip_ban.subnetted_ip %>
       <% end %>
-      <% t.column "Banner" do |ip_ban| %>
+      <% t.column :reason, td: { class: "col-expand" } %>
-        <%= link_to_user ip_ban.creator %>
+      <% t.column "Status" do |ip_ban| %>
+        <% if ip_ban.is_deleted? %>
+          Deleted
+        <% end %>
+      <% end %>
+      <% t.column "Type" do |ip_ban| %>
+        <%= ip_ban.category.delete_suffix("_ban").capitalize %>
+      <% end %>
+      <% t.column "Last Seen" do |ip_ban| %>
+        <%= time_ago_in_words_tagged ip_ban.last_hit_at %>
+      <% end %>
+      <% t.column :hit_count, name: "Hits" %>
+      <% t.column "Creator" do |ip_ban| %>
+        <%= link_to_user ip_ban.creator %>
+        <%= link_to "»", ip_bans_path(search: { creator_name: ip_ban.creator.name }) %>
+        <div><%= time_ago_in_words_tagged(ip_ban.created_at) %></div>
       <% end %>
-      <% t.column :reason %>
       <% t.column column: "control" do |ip_ban| %>
-        <%= link_to "Unban", ip_ban_path(ip_ban), :remote => true, :method => :delete, :data => {:confirm => "Do your really want to unban #{ip_ban.ip_addr}?"} %>
+        <%= link_to "Details", ip_address_path(ip_ban.ip_addr.to_s) %> |
+        <% if ip_ban.is_deleted? %>
+          <%= link_to "Undelete", ip_ban_path(ip_ban), remote: true, method: :put, "data-params": "ip_ban[is_deleted]=false", "data-confirm": "Are you sure you want to undelete this IP ban?" %>
+        <% else %>
+          <%= link_to "Delete", ip_ban_path(ip_ban), remote: true, method: :put, "data-params": "ip_ban[is_deleted]=true", "data-confirm": "Are you sure you want to remove this IP ban?" %>
+        <% end %>
       <% end %>
     <% end %>
 

app/views/ip_bans/new.html.erb

@@ -1,12 +1,23 @@
 <div id="c-ip-bans">
-  <div id="a-new">
+  <div id="a-new" class="fixed-width-container">
     <h1>New IP Ban</h1>
 
+    <p>
+      A normal IP ban restricts the IP from creating new accounts, logging in to
+      existing accounts, or editing the site in any way.
+    </p>
+
+    <p>
+      A signup IP ban restricts new signups from editing anything until after
+      they've verified their email address.
+    <p>
+
     <%= error_messages_for "ip_ban" %>
 
     <%= edit_form_for(@ip_ban) do |f| %>
       <%= f.input :ip_addr, label: "IP Address", as: :string, hint: "Add /24 to ban a subnet. Example: 1.2.3.4/24" %>
-      <%= f.input :reason %>
+      <%= f.input :reason, as: :string %>
+      <%= f.input :category, as: :select, include_blank: false, collection: [["Normal", "normal"], ["Signup", "signup"]] %>
       <%= f.button :submit, "Submit" %>
     <% end %>
   </div>

config/routes.rb

@@ -129,7 +129,7 @@ Rails.application.routes.draw do
     end
   end
   resources :forum_topic_visits, only: [:index]
-  resources :ip_bans
+  resources :ip_bans, only: [:index, :new, :create, :update]
   resources :ip_addresses, only: [:show, :index], id: /.+?(?=\.json|\.xml|\.html)|.+/
   resource :iqdb_queries, :only => [:show, :create] do
     collection do

db/migrate/20200406054838_add_type_to_ip_bans.rb

@@ -0,0 +1,11 @@
+class AddTypeToIpBans < ActiveRecord::Migration[6.0]
+  def change
+    add_column :ip_bans, :is_deleted, :boolean, default: false, null: false
+    add_column :ip_bans, :category, :integer, default: 0, null: false
+    add_column :ip_bans, :hit_count, :integer, default: 0, null: false
+    add_column :ip_bans, :last_hit_at, :datetime
+
+    add_index :ip_bans, :is_deleted
+    add_index :ip_bans, :category
+  end
+end

db/structure.sql

@@ -2316,12 +2316,16 @@ UNION ALL
 --
 
 CREATE TABLE public.ip_bans (
-    id integer NOT NULL,
     creator_id integer NOT NULL,
     ip_addr inet NOT NULL,
     reason text NOT NULL,
     created_at timestamp without time zone NOT NULL,
-    updated_at timestamp without time zone NOT NULL
+    updated_at timestamp without time zone NOT NULL,
+    id integer NOT NULL,
+    is_deleted boolean DEFAULT false NOT NULL,
+    category integer DEFAULT 0 NOT NULL,
+    hit_count integer DEFAULT 0 NOT NULL,
+    last_hit_at timestamp without time zone
 );
 
 
@@ -6428,6 +6432,13 @@ CREATE INDEX index_forum_topics_on_text_index ON public.forum_topics USING gin (
 CREATE INDEX index_forum_topics_on_updated_at ON public.forum_topics USING btree (updated_at);
 
 
+--
+-- Name: index_ip_bans_on_category; Type: INDEX; Schema: public; Owner: -
+--
+
+CREATE INDEX index_ip_bans_on_category ON public.ip_bans USING btree (category);
+
+
 --
 -- Name: index_ip_bans_on_ip_addr; Type: INDEX; Schema: public; Owner: -
 --
@@ -6435,6 +6446,13 @@ CREATE INDEX index_forum_topics_on_updated_at ON public.forum_topics USING btree
 CREATE INDEX index_ip_bans_on_ip_addr ON public.ip_bans USING btree (ip_addr);
 
 
+--
+-- Name: index_ip_bans_on_is_deleted; Type: INDEX; Schema: public; Owner: -
+--
+
+CREATE INDEX index_ip_bans_on_is_deleted ON public.ip_bans USING btree (is_deleted);
+
+
 --
 -- Name: index_mod_actions_on_created_at; Type: INDEX; Schema: public; Owner: -
 --
@@ -7365,6 +7383,7 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20200318224633'),
 ('20200325073456'),
 ('20200325074859'),
-('20200403210353');
+('20200403210353'),
+('20200406054838');
 
 

test/functional/ip_bans_controller_test.rb

@@ -21,6 +21,11 @@ class IpBansControllerTest < ActionDispatch::IntegrationTest
           assert_response :redirect
         end
       end
+
+      should "log a mod action" do
+        post_auth ip_bans_path, @admin, params: { ip_ban: { ip_addr: "1.2.3.4", reason: "xyz" }}
+        assert_equal("ip_ban_create", ModAction.last.category)
+      end
     end
 
     context "index action" do
@@ -37,12 +42,12 @@ class IpBansControllerTest < ActionDispatch::IntegrationTest
       end
     end
 
-    context "destroy action" do
+    context "update action" do
-      should "destroy an ip ban" do
+      should "mark an ip ban as deleted" do
-        assert_difference("IpBan.count", -1) do
+        put_auth ip_ban_path(@ip_ban), @admin, params: { ip_ban: { is_deleted: true }, format: "js" }
-          delete_auth ip_ban_path(@ip_ban), @admin, params: {:format => "js"}
+        assert_response :success
-          assert_response :success
+        assert_equal(true, @ip_ban.reload.is_deleted)
-        end
+        assert_equal("ip_ban_delete", ModAction.last.category)
       end
     end
   end

test/functional/sessions_controller_test.rb

@@ -35,11 +35,33 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest
       end
 
       should "not allow IP banned users to login" do
-        create(:ip_ban, ip_addr: "1.2.3.4")
+        @ip_ban = create(:ip_ban, category: :normal, ip_addr: "1.2.3.4")
         post session_path, params: { name: @user.name, password: "password" }, headers: { REMOTE_ADDR: "1.2.3.4" }
 
         assert_response 403
         assert_not_equal(@user.id, session[:user_id])
+        assert_equal(1, @ip_ban.reload.hit_count)
+        assert(@ip_ban.last_hit_at > 1.minute.ago)
+      end
+
+      should "allow signup-restricted IP banned users to login" do
+        @ip_ban = create(:ip_ban, category: :signup, ip_addr: "1.2.3.4")
+        post session_path, params: { name: @user.name, password: "password" }, headers: { REMOTE_ADDR: "1.2.3.4" }
+
+        assert_redirected_to posts_path
+        assert_equal(@user.id, session[:user_id])
+        assert_equal(0, @ip_ban.reload.hit_count)
+        assert_nil(@ip_ban.last_hit_at)
+      end
+
+      should "ignore deleted IP bans when logging in" do
+        @ip_ban = create(:ip_ban, is_deleted: true, category: :normal, ip_addr: "1.2.3.4")
+        post session_path, params: { name: @user.name, password: "password" }, headers: { REMOTE_ADDR: "1.2.3.4" }
+
+        assert_redirected_to posts_path
+        assert_equal(@user.id, session[:user_id])
+        assert_equal(0, @ip_ban.reload.hit_count)
+        assert_nil(@ip_ban.last_hit_at)
       end
     end
 

test/functional/users_controller_test.rb

@@ -168,6 +168,19 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
         assert_equal(true, User.last.requires_verification)
       end
 
+      should "mark users signing up from a signup banned IP as requiring verification" do
+        skip unless IpLookup.enabled?
+        self.remote_addr = "187.37.226.17"
+
+        @ip_ban = create(:ip_ban, ip_addr: self.remote_addr, category: :signup)
+        post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }}
+
+        assert_redirected_to User.last
+        assert_equal(true, User.last.requires_verification)
+        assert_equal(1, @ip_ban.reload.hit_count)
+        assert(@ip_ban.last_hit_at > 1.minute.ago)
+      end
+
       should "not mark users signing up from non-proxies as requiring verification" do
         skip unless IpLookup.enabled?
         self.remote_addr = "187.37.226.17"

test/unit/ip_ban_test.rb

@@ -5,18 +5,19 @@ class IpBanTest < ActiveSupport::TestCase
     ip_ban = create(:ip_ban, ip_addr: "1.2.3.4")
 
     assert_equal("1.2.3.4", ip_ban.subnetted_ip)
-    assert(IpBan.is_banned?("1.2.3.4"))
+    assert(IpBan.ip_matches("1.2.3.4").exists?)
   end
 
   should "be able to ban a subnet" do
     ip_ban = create(:ip_ban, ip_addr: "1.2.3.4/24")
 
     assert_equal("1.2.3.0/24", ip_ban.subnetted_ip)
-    assert(IpBan.is_banned?("1.2.3.0"))
+    assert(IpBan.ip_matches("1.2.3.0").exists?)
-    assert(IpBan.is_banned?("1.2.3.255"))
+    assert(IpBan.ip_matches("1.2.3.255").exists?)
   end
 
   context "validation" do
+    setup { create(:ip_ban: ip_addr: "5.6.7.8") }
     subject { build(:ip_ban) }
 
     should allow_value("1.2.3.4").for(:ip_addr)
@@ -26,6 +27,7 @@ class IpBanTest < ActiveSupport::TestCase
 
     should_not allow_value("").for(:ip_addr)
     should_not allow_value("foo").for(:ip_addr)
+    should_not allow_value("5.6.7.8").for(:ip_addr)
     should_not allow_value("10.0.0.1").for(:ip_addr)
     should_not allow_value("127.0.0.1").for(:ip_addr)
     should_not allow_value("1.2.3.4/16").for(:ip_addr)