Merge pull request #111 from basecamp/ip-ban

Add IP-based user banning
Closes: #95

app/assets/images/cancel.svg

@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M12 2C17.5 2 22 6.5 22 12S17.5 22 12 22 2 17.5 2 12 6.5 2 12 2M12 4C10.1 4 8.4 4.6 7.1 5.7L18.3 16.9C19.3 15.5 20 13.8 20 12C20 7.6 16.4 4 12 4M16.9 18.3L5.7 7.1C4.6 8.4 4 10.1 4 12C4 16.4 7.6 20 12 20C13.9 20 15.6 19.4 16.9 18.3Z" /></svg>

app/assets/stylesheets/avatars.css

@@ -7,6 +7,7 @@
   inline-size: var(--avatar-size, 5ch);
   margin: 0;
   place-items: center;
+  position: relative;
 
   img {
     aspect-ratio: 1;
@@ -16,6 +17,25 @@
     inline-size: var(--avatar-size, 5ch);
     max-inline-size: 100%;
     object-fit: cover;
+
+    .banned & {
+      opacity: 0.5;
+    }
+  }
+
+  .banned &:after {
+    background: url(cancel.svg) no-repeat center center;
+    block-size: auto;
+    content: "";
+    filter: invert(0%);
+    inline-size: var(--avatar-size, 5ch);
+    inset: 0;
+    max-inline-size: 100%;
+    position: absolute;
+
+    @media (prefers-color-scheme: dark) {
+      filter: invert(100%);
+    }
   }
 }
 

app/controllers/accounts_controller.rb

@@ -3,7 +3,7 @@ class AccountsController < ApplicationController
   before_action :set_account
 
   def edit
-    set_page_and_extract_portion_from User.active.ordered, per_page: 500
+    set_page_and_extract_portion_from account_users.ordered, per_page: 500
   end
 
   def update
@@ -19,4 +19,12 @@ class AccountsController < ApplicationController
     def account_params
       params.require(:account).permit(:name, :logo)
     end
+
+    def account_users
+      if Current.user.can_administer?
+        User.where(status: [ :active, :banned ])
+      else
+        User.active
+      end
+    end
 end

app/controllers/application_controller.rb

@@ -1,4 +1,4 @@
 class ApplicationController < ActionController::Base
-  include AllowBrowser, Authentication, Authorization, SetCurrentRequest, SetPlatform, TrackedRoomVisit, VersionHeaders
+  include AllowBrowser, Authentication, Authorization, BlockBannedRequests, SetCurrentRequest, SetPlatform, TrackedRoomVisit, VersionHeaders
   include Turbo::Streams::Broadcasts, Turbo::Streams::StreamName
 end

app/controllers/concerns/block_banned_requests.rb

@@ -0,0 +1,16 @@
+module BlockBannedRequests
+  extend ActiveSupport::Concern
+
+  included do
+    before_action :reject_banned_ip, unless: :safe_request?
+  end
+
+  private
+    def reject_banned_ip
+      head :too_many_requests if Ban.banned?(request.remote_ip)
+    end
+
+    def safe_request?
+      request.get? || request.head?
+    end
+end

app/controllers/messages_controller.rb

@@ -42,7 +42,7 @@ class MessagesController < ApplicationController
 
   def destroy
     @message.destroy
-    @message.broadcast_remove_to @room, :messages
+    @message.broadcast_remove
   end
 
   private

app/controllers/users/bans_controller.rb

@@ -0,0 +1,19 @@
+class Users::BansController < ApplicationController
+  before_action :ensure_can_administer
+  before_action :set_user
+
+  def create
+    @user.ban
+    redirect_to @user
+  end
+
+  def destroy
+    @user.unban
+    redirect_to @user
+  end
+
+  private
+    def set_user
+      @user = User.find(params[:user_id])
+    end
+end

app/jobs/remove_banned_content_job.rb

@@ -0,0 +1,5 @@
+class RemoveBannedContentJob < ApplicationJob
+  def perform(user)
+    user.remove_banned_content
+  end
+end

app/models/ban.rb

@@ -0,0 +1,20 @@
+class Ban < ApplicationRecord
+  belongs_to :user
+
+  validate :ip_address_is_public
+
+  def self.banned?(ip_address)
+    exists?(ip_address: ip_address)
+  end
+
+  private
+    def ip_address_is_public
+      ip = IPAddr.new(ip_address)
+
+      if ip.loopback? || ip.private? || ip.link_local?
+        errors.add(:ip_address, "cannot be a private or internal IP address")
+      end
+    rescue IPAddr::InvalidAddressError
+      errors.add(:ip_address, "is not a valid IP address")
+    end
+end

app/models/message/broadcasts.rb

@@ -3,4 +3,8 @@ module Message::Broadcasts
     broadcast_append_to room, :messages, target: [ room, :messages ]
     ActionCable.server.broadcast("unread_rooms", { roomId: room.id })
   end
+
+  def broadcast_remove
+    broadcast_remove_to room, :messages
+  end
 end

app/models/user.rb

@@ -1,5 +1,5 @@
 class User < ApplicationRecord
-  include Avatar, Bot, Mentionable, Role, Transferable
+  include Avatar, Bannable, Bot, Mentionable, Role, Transferable
 
   has_many :memberships, dependent: :delete_all
   has_many :rooms, through: :memberships
@@ -13,8 +13,9 @@ class User < ApplicationRecord
   has_many :searches, dependent: :delete_all
 
   has_many :sessions, dependent: :destroy
+  has_many :bans, dependent: :destroy
 
-  scope :active, -> { where(active: true) }
+  enum :status, %i[ active deactivated banned ], default: :active
 
   has_secure_password validations: false
 
@@ -40,14 +41,10 @@ class User < ApplicationRecord
       searches.delete_all
       sessions.delete_all
 
-      update! active: false, email_address: deactived_email_address
+      update! status: :deactivated, email_address: deactived_email_address
     end
   end
 
-  def deactivated?
-    !active?
-  end
-
   def reset_remote_connections
     close_remote_connections reconnect: true
   end

app/models/user/bannable.rb

@@ -0,0 +1,42 @@
+module User::Bannable
+  extend ActiveSupport::Concern
+
+  def ban
+    transaction do
+      create_bans_from_sessions
+      apply_ban
+      banned!
+    end
+  end
+
+  def unban
+    transaction do
+      bans.delete_all
+      active!
+    end
+  end
+
+  def remove_banned_content_later
+    RemoveBannedContentJob.perform_later(self)
+  end
+
+  def remove_banned_content
+    messages.each do |message|
+      message.destroy
+      message.broadcast_remove
+    end
+  end
+
+  private
+    def create_bans_from_sessions
+      sessions.pluck(:ip_address).compact_blank.uniq.each do |ip|
+        bans.create!(ip_address: ip)
+      end
+    end
+
+    def apply_ban
+      close_remote_connections
+      sessions.delete_all
+      remove_banned_content_later
+    end
+end

app/views/accounts/users/_user.html.erb

@@ -1,4 +1,4 @@
-<li class="flex align-center gap margin-none">
+<li class="flex align-center gap margin-none <%= "banned" if user.banned? %>">
   <figure class="avatar flex-item-no-shrink" style="--avatar-size: 3.75ch;">
     <%= avatar_tag user, loading: :lazy %>
   </figure>
@@ -9,7 +9,7 @@
 
   <hr class="separator" aria-hidden="true">
 
-  <% if Current.user.can_administer? && user != Current.user %>
+  <% if Current.user.can_administer? && user != Current.user && user.active? %>
     <% unless user.bot? %>
       <%= form_with model: user, url: account_user_path(user), data: { controller: "form" }, method: :patch do | form | %>
         <label class="btn txt-small flex-item-no-shrink" for="<%= dom_id(user, :role) %>">

app/views/users/_ban_button.html.erb

@@ -0,0 +1,15 @@
+<% if user.active? %>
+  <%= button_to user_ban_path(user), method: :post,
+        class: "btn full-width",
+        data: { turbo_confirm: "Are you sure you want to ban this user? This will log them out, delete their messages, and block their IP addresses." } do %>
+    <%= image_tag "cancel.svg", aria: { hidden: "true", label: "Ban #{@user.name}" } %>
+    <span>Ban <%= user.name %></span>
+  <% end %>
+<% else %>
+  <%= button_to user_ban_path(user), method: :delete,
+        class: "btn btn--negative full-width",
+        data: { turbo_confirm: "Are you sure you want to remove the ban on this user?" } do %>
+    <%= image_tag "cancel.svg", aria: { hidden: "true", label: "Ban #{@user.name}" } %>
+    <span>Remove ban</span>
+  <% end %>
+<% end %>

app/views/users/show.html.erb

@@ -16,7 +16,7 @@
 <% end %>
 
 <section class="panel txt-align-center">
-  <div class="flex flex-column gap">
+  <div class="flex flex-column gap <%= "banned" if @user.banned? %>">
     <div class="avatar txt-xx-large center" style="background: white">
       <%= image_tag fresh_user_avatar_path(@user), alt: "Profile avatar", class: "avatar" %>
     </div>
@@ -30,7 +30,7 @@
         <% end %>
       </div>
     <% else %>
-      <% if @user.active? %>
+      <% unless @user.deactivated? %>
         <div class="flex flex-column gap" style="--row-gap: calc(var(--block-space) / 3)">
           <h1 class="txt-x-large txt-tight-lines margin-none"><%= @user.name %></h1>
           <% if Current.user.can_administer? %>
@@ -39,16 +39,24 @@
           <div><%= @user.bio %></div>
         </div>
 
-        <div class="pad-inline-double margin-inline margin-block-start">
-          <%= button_to rooms_directs_path(user_ids: [ @user.id ]), class: "btn btn--reversed full-width txt-large" do %>
-            <%= image_tag "messages.svg", aria: { hidden: "true", label: "Ping #{@user.name}" } %>
+        <% if @user.active? %>
+          <div class="pad-inline-double margin-inline margin-block-start">
+            <%= button_to rooms_directs_path(user_ids: [ @user.id ]), class: "btn btn--reversed full-width txt-large" do %>
+              <%= image_tag "messages.svg", aria: { hidden: "true", label: "Ping #{@user.name}" } %>
+            <% end %>
+          </div>
+
+          <% if Current.user.can_administer? %>
+            <hr class="margin-block-start borderless">
+
+            <%= render "users/profiles/transfer", user: @user %>
           <% end %>
-        </div>
+        <% end %>
 
-        <% if Current.user.can_administer? %>
-          <hr class="margin-block-start borderless">
-
-          <%= render "users/profiles/transfer", user: @user %>
+        <% if Current.user.can_administer? && Current.user != @user %>
+          <div class="margin-block-start">
+            <%= render "users/ban_button", user: @user %>
+          </div>
         <% end %>
       <% else %>
         <div>

config/routes.rb

@@ -37,6 +37,7 @@ Rails.application.routes.draw do
   resources :users, only: :show do
     scope module: "users" do
       resource :avatar, only: %i[ show destroy ]
+      resource :ban, only: %i[ create destroy ]
 
       scope defaults: { user_id: "me" } do
         resource :sidebar, only: :show

db/migrate/20251126092013_create_bans.rb

@@ -0,0 +1,11 @@
+class CreateBans < ActiveRecord::Migration[8.1]
+  def change
+    create_table :bans do |t|
+      t.references :user, null: false, foreign_key: true
+      t.string :ip_address, null: false
+      t.timestamps
+    end
+
+    add_index :bans, :ip_address
+  end
+end

db/migrate/20251126115722_change_active_to_status_on_users.rb

@@ -0,0 +1,13 @@
+class ChangeActiveToStatusOnUsers < ActiveRecord::Migration[8.1]
+  def change
+    add_column :users, :status, :integer, default: 0, null: false
+
+    reversible do |dir|
+      dir.up do
+        execute "UPDATE users SET status = 1 WHERE active = 0"
+      end
+    end
+
+    remove_column :users, :active, :boolean
+  end
+end

db/structure.sql

@@ -64,12 +64,20 @@ FOREIGN KEY ("user_id")
   REFERENCES "users" ("id")
 );
 CREATE INDEX "index_webhooks_on_user_id" ON "webhooks" ("user_id");
-CREATE TABLE IF NOT EXISTS "users" ("id" integer PRIMARY KEY AUTOINCREMENT NOT NULL, "name" varchar NOT NULL, "created_at" datetime(6) NOT NULL, "updated_at" datetime(6) NOT NULL, "role" integer DEFAULT 0 NOT NULL, "email_address" varchar DEFAULT NULL, "password_digest" varchar DEFAULT NULL, "active" boolean DEFAULT 1, "bio" text DEFAULT NULL, "bot_token" varchar DEFAULT NULL);
-CREATE UNIQUE INDEX "index_users_on_email_address" ON "users" ("email_address");
-CREATE UNIQUE INDEX "index_users_on_bot_token" ON "users" ("bot_token");
 CREATE TABLE IF NOT EXISTS "active_storage_blobs" ("id" integer PRIMARY KEY AUTOINCREMENT NOT NULL, "key" varchar NOT NULL, "filename" varchar NOT NULL, "content_type" varchar, "metadata" text, "service_name" varchar NOT NULL, "byte_size" bigint NOT NULL, "checksum" varchar, "created_at" datetime(6) NOT NULL);
 CREATE UNIQUE INDEX "index_active_storage_blobs_on_key" ON "active_storage_blobs" ("key") /*application='Campfire'*/;
+CREATE TABLE IF NOT EXISTS "bans" ("id" integer PRIMARY KEY AUTOINCREMENT NOT NULL, "user_id" integer NOT NULL, "ip_address" varchar NOT NULL, "created_at" datetime(6) NOT NULL, "updated_at" datetime(6) NOT NULL, CONSTRAINT "fk_rails_070022cd76"
+FOREIGN KEY ("user_id")
+  REFERENCES "users" ("id")
+);
+CREATE INDEX "index_bans_on_user_id" ON "bans" ("user_id") /*application='Campfire'*/;
+CREATE INDEX "index_bans_on_ip_address" ON "bans" ("ip_address") /*application='Campfire'*/;
+CREATE TABLE IF NOT EXISTS "users" ("id" integer PRIMARY KEY AUTOINCREMENT NOT NULL, "name" varchar NOT NULL, "created_at" datetime(6) NOT NULL, "updated_at" datetime(6) NOT NULL, "role" integer DEFAULT 0 NOT NULL, "email_address" varchar, "password_digest" varchar, "bio" text, "bot_token" varchar, "status" integer DEFAULT 0 NOT NULL);
+CREATE UNIQUE INDEX "index_users_on_email_address" ON "users" ("email_address") /*application='Campfire'*/;
+CREATE UNIQUE INDEX "index_users_on_bot_token" ON "users" ("bot_token") /*application='Campfire'*/;
 INSERT INTO "schema_migrations" (version) VALUES
+('20251126115722'),
+('20251126092013'),
 ('20250825100959'),
 ('20250825100958'),
 ('20250825100957'),

test/controllers/concerns/block_banned_requests_test.rb

@@ -0,0 +1,32 @@
+require "test_helper"
+
+class BlockBannedRequestsTest < ActionDispatch::IntegrationTest
+  setup do
+    sign_in :david
+    @room = rooms(:watercooler)
+
+    Ban.create!(user: users(:kevin), ip_address: "203.0.113.1")
+  end
+
+  test "POST requests from banned IPs are blocked with 429" do
+    post room_messages_url(@room),
+      params: { message: { body: "Test", client_message_id: "test-123" } },
+      headers: { "REMOTE_ADDR" => "203.0.113.1" }
+
+    assert_response :too_many_requests
+  end
+
+  test "POST requests from non-banned IPs are allowed" do
+    post room_messages_url(@room, format: :turbo_stream),
+      params: { message: { body: "Test", client_message_id: "test-123" } },
+      headers: { "REMOTE_ADDR" => "203.0.113.99" }
+
+    assert_response :success
+  end
+
+  test "GET requests from banned IPs are allowed" do
+    get room_messages_url(@room), headers: { "REMOTE_ADDR" => "203.0.113.1" }
+
+    assert_response :success
+  end
+end

test/controllers/users/bans_controller_test.rb

@@ -0,0 +1,85 @@
+require "test_helper"
+
+class Users::BansControllerTest < ActionDispatch::IntegrationTest
+  setup do
+    sign_in :david
+  end
+
+  test "create bans user and creates ban records from sessions" do
+    user = users(:kevin)
+    user.sessions.create!(ip_address: "203.0.113.1", user_agent: "Test")
+    user.sessions.create!(ip_address: "203.0.113.2", user_agent: "Test")
+
+    assert_difference -> { Ban.count }, 2 do
+      post user_ban_url(user)
+    end
+
+    assert_redirected_to user_url(user)
+    assert Ban.exists?(ip_address: "203.0.113.1", user: user)
+    assert Ban.exists?(ip_address: "203.0.113.2", user: user)
+  end
+
+  test "create destroys user sessions" do
+    user = users(:kevin)
+    user.sessions.create!(ip_address: "203.0.113.1", user_agent: "Test")
+
+    assert_difference -> { user.sessions.count }, -1 do
+      post user_ban_url(user)
+    end
+  end
+
+  test "create enqueues RemoveBannedContentJob" do
+    user = users(:kevin)
+
+    assert_enqueued_with(job: RemoveBannedContentJob, args: [ user ]) do
+      post user_ban_url(user)
+    end
+  end
+
+  test "RemoveBannedContentJob deletes messages" do
+    user = users(:kevin)
+    user.sessions.create!(ip_address: "203.0.113.1", user_agent: "Test")
+    user.messages.create!(room: rooms(:hq), body: "Test message", client_message_id: "test-123")
+
+    perform_enqueued_jobs do
+      post user_ban_url(user)
+    end
+
+    assert_empty user.reload.messages
+  end
+
+  test "non-admins cannot ban users" do
+    sign_in :kevin
+
+    post user_ban_url(users(:jz))
+
+    assert_response :forbidden
+  end
+
+  test "destroy removes ban records and sets user to active" do
+    user = users(:kevin)
+    user.sessions.create!(ip_address: "203.0.113.1", user_agent: "Test")
+    user.ban
+
+    assert user.reload.banned?
+    assert_equal 1, user.bans.count
+
+    assert_difference -> { Ban.count }, -1 do
+      delete user_ban_url(user)
+    end
+
+    assert_redirected_to user_url(user)
+    assert user.reload.active?
+  end
+
+  test "non-admins cannot unban users" do
+    sign_in :kevin
+
+    user = users(:jz)
+    user.banned!
+
+    delete user_ban_url(user)
+
+    assert_response :forbidden
+  end
+end