diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 23d7b0145..c4b658fdf 100644 --- a/app/controllers/application_controller.rb +++ b/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 diff --git a/app/controllers/ip_bans_controller.rb b/app/controllers/ip_bans_controller.rb index ec2eecc64..cfeef921b 100644 --- a/app/controllers/ip_bans_controller.rb +++ b/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 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index bdd99f2ac..064d7ad71 100644 --- a/app/controllers/users_controller.rb +++ b/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] diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index c018be562..89e890f7c 100644 --- a/app/helpers/application_helper.rb +++ b/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 diff --git a/app/models/ip_ban.rb b/app/models/ip_ban.rb index 9d1d027d4..24a75dd38 100644 --- a/app/models/ip_ban.rb +++ b/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) - where("ip_addr >>= ?", ip_addr).exists? + validate :validate_ip_addr + 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 diff --git a/app/models/mod_action.rb b/app/models/mod_action.rb index 60e5108f9..2a7c42f92 100644 --- a/app/models/mod_action.rb +++ b/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 diff --git a/app/policies/ip_ban_policy.rb b/app/policies/ip_ban_policy.rb index c9b290ee2..c10665b50 100644 --- a/app/policies/ip_ban_policy.rb +++ b/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 diff --git a/app/views/ip_addresses/_info.html.erb b/app/views/ip_addresses/_info.html.erb index adf31775f..968369499 100644 --- a/app/views/ip_addresses/_info.html.erb +++ b/app/views/ip_addresses/_info.html.erb @@ -26,7 +26,7 @@ IP Banned - <% 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 diff --git a/app/views/ip_bans/index.html.erb b/app/views/ip_bans/index.html.erb index 2c32c70d4..5a68e17f9 100644 --- a/app/views/ip_bans/index.html.erb +++ b/app/views/ip_bans/index.html.erb @@ -2,16 +2,35 @@

IP Bans

- <%= 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| %> - <%= link_to_user ip_ban.creator %> + <% t.column :reason, td: { class: "col-expand" } %> + <% 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 }) %> +
<%= time_ago_in_words_tagged(ip_ban.created_at) %>
<% 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 %> diff --git a/app/views/ip_bans/new.html.erb b/app/views/ip_bans/new.html.erb index 6aa520479..0cf53e332 100644 --- a/app/views/ip_bans/new.html.erb +++ b/app/views/ip_bans/new.html.erb @@ -1,12 +1,23 @@
-
+

New IP Ban

+

+ A normal IP ban restricts the IP from creating new accounts, logging in to + existing accounts, or editing the site in any way. +

+ +

+ A signup IP ban restricts new signups from editing anything until after + they've verified their email address. +

+ <%= 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 %>

diff --git a/app/views/ip_bans/destroy.js.erb b/app/views/ip_bans/update.js.erb similarity index 100% rename from app/views/ip_bans/destroy.js.erb rename to app/views/ip_bans/update.js.erb diff --git a/config/routes.rb b/config/routes.rb index 012c737f6..95392b1e1 100644 --- a/config/routes.rb +++ b/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 diff --git a/db/migrate/20200406054838_add_type_to_ip_bans.rb b/db/migrate/20200406054838_add_type_to_ip_bans.rb new file mode 100644 index 000000000..46dc785d7 --- /dev/null +++ b/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 diff --git a/db/structure.sql b/db/structure.sql index bbcbade6c..78b0a5d5d 100644 --- a/db/structure.sql +++ b/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'); diff --git a/test/functional/ip_bans_controller_test.rb b/test/functional/ip_bans_controller_test.rb index 44e98c6ef..bd6465afc 100644 --- a/test/functional/ip_bans_controller_test.rb +++ b/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 - should "destroy an ip ban" do - assert_difference("IpBan.count", -1) do - delete_auth ip_ban_path(@ip_ban), @admin, params: {:format => "js"} - assert_response :success - end + context "update action" do + should "mark an ip ban as deleted" do + put_auth ip_ban_path(@ip_ban), @admin, params: { ip_ban: { is_deleted: true }, format: "js" } + assert_response :success + assert_equal(true, @ip_ban.reload.is_deleted) + assert_equal("ip_ban_delete", ModAction.last.category) end end end diff --git a/test/functional/sessions_controller_test.rb b/test/functional/sessions_controller_test.rb index d78ea4b16..53e2c9206 100644 --- a/test/functional/sessions_controller_test.rb +++ b/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 diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb index 4fd277963..8217dd1de 100644 --- a/test/functional/users_controller_test.rb +++ b/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" diff --git a/test/unit/ip_ban_test.rb b/test/unit/ip_ban_test.rb index 6a0e3deb6..2fd8a0725 100644 --- a/test/unit/ip_ban_test.rb +++ b/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.is_banned?("1.2.3.255")) + assert(IpBan.ip_matches("1.2.3.0").exists?) + 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)