pundit: convert bans to pundit.

app/controllers/bans_controller.rb

@@ -1,59 +1,44 @@
 class BansController < ApplicationController
-  before_action :moderator_only, :except => [:show, :index]
   respond_to :html, :xml, :json
   helper_method :search_params
 
   def new
-    @ban = Ban.new(ban_params(:create))
+    @ban = authorize Ban.new(permitted_attributes(Ban))
+    respond_with(@ban)
   end
 
   def edit
-    @ban = Ban.find(params[:id])
+    @ban = authorize Ban.find(params[:id])
+    respond_with(@ban)
   end
 
   def index
-    @bans = Ban.paginated_search(params, count_pages: true)
+    @bans = authorize Ban.paginated_search(params, count_pages: true)
     @bans = @bans.includes(:user, :banner) if request.format.html?
 
     respond_with(@bans)
   end
 
   def show
-    @ban = Ban.find(params[:id])
+    @ban = authorize Ban.find(params[:id])
     respond_with(@ban)
   end
 
   def create
-    @ban = Ban.create(banner: CurrentUser.user, **ban_params(:create))
+    @ban = authorize Ban.new(banner: CurrentUser.user, **permitted_attributes(Ban))
-
+    @ban.save
-    if @ban.errors.any?
+    respond_with(@ban)
-      render :action => "new"
-    else
-      redirect_to ban_path(@ban), :notice => "Ban created"
-    end
   end
 
   def update
-    @ban = Ban.find(params[:id])
+    @ban = authorize Ban.find(params[:id])
-    if @ban.update(ban_params(:update))
+    @ban.update(permitted_attributes(@ban))
-      redirect_to ban_path(@ban), :notice => "Ban updated"
+    respond_with(@ban)
-    else
-      render :action => "edit"
-    end
   end
 
   def destroy
-    @ban = Ban.find(params[:id])
+    @ban = authorize Ban.find(params[:id])
     @ban.destroy
-    redirect_to bans_path, :notice => "Ban destroyed"
+    respond_with(@ban)
-  end
-
-  private
-
-  def ban_params(context)
-    permitted_params = %i[reason duration expires_at]
-    permitted_params += %i[user_id user_name] if context == :create
-
-    params.fetch(:ban, {}).permit(permitted_params)
   end
 end

app/models/ban.rb

@@ -6,7 +6,6 @@ class Ban < ApplicationRecord
   after_destroy :create_unban_mod_action
   belongs_to :user
   belongs_to :banner, :class_name => "User"
-  validate :user_is_inferior
   validates_presence_of :reason, :duration
 
   scope :unexpired, -> { where("bans.expires_at > ?", Time.now) }
@@ -49,25 +48,6 @@ class Ban < ApplicationRecord
     end
   end
 
-  def user_is_inferior
-    if user
-      if user.is_admin?
-        errors[:base] << "You can never ban an admin."
-        false
-      elsif user.is_moderator? && banner.is_admin?
-        true
-      elsif user.is_moderator?
-        errors[:base] << "Only admins can ban moderators."
-        false
-      elsif banner.is_admin? || banner.is_moderator?
-        true
-      else
-        errors[:base] << "No one else can ban."
-        false
-      end
-    end
-  end
-
   def update_user_on_create
     user.update!(is_banned: true)
   end
@@ -96,7 +76,7 @@ class Ban < ApplicationRecord
   end
 
   def expired?
-    expires_at < Time.now
+    persisted? && expires_at < Time.now
   end
 
   def create_feedback

app/policies/ban_policy.rb

@@ -0,0 +1,18 @@
+class BanPolicy < ApplicationPolicy
+  def bannable?
+    user.is_moderator? && (record.user.blank? || (record.user.level < user.level))
+  end
+
+  alias_method :edit?, :bannable?
+  alias_method :create?, :bannable?
+  alias_method :update?, :bannable?
+  alias_method :destroy?, :bannable?
+
+  def permitted_attributes_for_create
+    [:reason, :duration, :expires_at, :user_id, :user_name]
+  end
+
+  def permitted_attributes_for_update
+    [:reason, :duration, :expires_at]
+  end
+end

app/views/bans/index.html.erb

@@ -23,7 +23,7 @@
         <div><%= time_ago_in_words_tagged(ban.created_at) %></div>
       <% end %>
       <% t.column column: "control" do |ban| %>
-        <% if CurrentUser.is_moderator? %>
+        <% if policy(ban).update? %>
           <%= link_to "Edit", edit_ban_path(ban) %>
           | <%= link_to "Delete", ban_path(ban), :method => :delete, :remote => true %>
         <% end %>

test/functional/bans_controller_test.rb

@@ -10,51 +10,96 @@ class BansControllerTest < ActionDispatch::IntegrationTest
       end
     end
 
-    should "get the new page" do
+    context "new action" do
-      get_auth new_ban_path, @mod
+      should "render" do
-      assert_response :success
+        get_auth new_ban_path, @mod
-    end
+        assert_response :success
-
-    should "get the edit page" do
-      get_auth edit_ban_path(@ban.id), @mod
-      assert_response :success
-    end
-
-    should "get the show page" do
-      get_auth ban_path(@ban.id), @mod
-      assert_response :success
-    end
-
-    should "get the index page" do
-      get_auth bans_path, @mod
-      assert_response :success
-    end
-
-    should "search" do
-      get_auth bans_path(search: {user_name: @user.name}), @mod
-      assert_response :success
-    end
-
-    should "create a ban" do
-      assert_difference("Ban.count", 1) do
-        post_auth bans_path, @mod, params: {ban: {duration: 60, reason: "xxx", user_id: @user.id}}
       end
-      ban = Ban.last
-      assert_redirected_to(ban_path(ban))
     end
 
-    should "update a ban" do
+    context "edit action" do
-      put_auth ban_path(@ban.id), @mod, params: {ban: {reason: "xxx", duration: 60}}
+      should "render" do
-      @ban.reload
+        get_auth edit_ban_path(@ban.id), @mod
-      assert_equal("xxx", @ban.reason)
+        assert_response :success
-      assert_redirected_to(ban_path(@ban))
+      end
-    end
+    end
-
+
-    should "destroy a ban" do
+    context "show action" do
-      assert_difference("Ban.count", -1) do
+      should "render" do
-        delete_auth ban_path(@ban.id), @mod
+        get_auth ban_path(@ban.id), @mod
+        assert_response :success
+      end
+    end
+
+    context "index action" do
+      should "render" do
+        get_auth bans_path, @mod
+        assert_response :success
+      end
+    end
+
+    context "search action" do
+      should "render" do
+        get_auth bans_path(search: {user_name: @user.name}), @mod
+        assert_response :success
+      end
+    end
+
+    context "create action" do
+      should "allow mods to ban members" do
+        assert_difference("Ban.count", 1) do
+          post_auth bans_path, @mod, params: { ban: { duration: 60, reason: "xxx", user_id: @user.id }}
+
+          assert_response :redirect
+          assert_equal(true, @user.reload.is_banned?)
+        end
+      end
+
+      should "not allow mods to ban admins" do
+        assert_difference("Ban.count", 0) do
+          @admin = create(:admin_user)
+          post_auth bans_path, @mod, params: { ban: { duration: 60, reason: "xxx", user_id: @admin.id }}
+
+          assert_response 403
+          assert_equal(false, @admin.reload.is_banned?)
+        end
+      end
+
+      should "not allow mods to ban other mods" do
+        assert_difference("Ban.count", 0) do
+          @mod2 = create(:mod_user)
+          post_auth bans_path, @mod, params: { ban: { duration: 60, reason: "xxx", user_id: @mod2.id }}
+
+          assert_response 403
+          assert_equal(false, @mod2.reload.is_banned?)
+        end
+      end
+
+      should "not allow regular users to ban anyone" do
+        assert_difference("Ban.count", 0) do
+          post_auth bans_path, @user, params: { ban: { duration: 60, reason: "xxx", user_id: @mod.id }}
+
+          assert_response 403
+          assert_equal(false, @mod.reload.is_banned?)
+        end
+      end
+    end
+
+    context "update action" do
+      should "update a ban" do
+        put_auth ban_path(@ban.id), @mod, params: {ban: {reason: "xxx", duration: 60}}
+        assert_equal("xxx", @ban.reload.reason)
+        assert_redirected_to(ban_path(@ban))
+      end
+    end
+
+    context "destroy action" do
+      should "destroy a ban" do
+        assert_difference("Ban.count", -1) do
+          delete_auth ban_path(@ban.id), @mod
+          assert_redirected_to bans_path
+        end
       end
-      assert_redirected_to(bans_path)
     end
   end
 end

test/unit/ban_test.rb

@@ -23,66 +23,7 @@ class BanTest < ActiveSupport::TestCase
         assert(user.is_banned?)
       end
 
-      should "not be valid against another admin" do
+      should "be valid" do
-        user = FactoryBot.create(:admin_user)
-        ban = FactoryBot.build(:ban, :user => user, :banner => @banner)
-        ban.save
-        assert(ban.errors.any?)
-      end
-
-      should "be valid against anyone who is not an admin" do
-        user = FactoryBot.create(:moderator_user)
-        ban = FactoryBot.create(:ban, :user => user, :banner => @banner)
-        assert(ban.errors.empty?)
-
-        user = FactoryBot.create(:contributor_user)
-        ban = FactoryBot.create(:ban, :user => user, :banner => @banner)
-        assert(ban.errors.empty?)
-
-        user = FactoryBot.create(:gold_user)
-        ban = FactoryBot.create(:ban, :user => user, :banner => @banner)
-        assert(ban.errors.empty?)
-
-        user = FactoryBot.create(:user)
-        ban = FactoryBot.create(:ban, :user => user, :banner => @banner)
-        assert(ban.errors.empty?)
-      end
-    end
-
-    context "created by a moderator" do
-      setup do
-        @banner = FactoryBot.create(:moderator_user)
-        CurrentUser.user = @banner
-        CurrentUser.ip_addr = "127.0.0.1"
-      end
-
-      teardown do
-        @banner = nil
-        CurrentUser.user = nil
-        CurrentUser.ip_addr = nil
-      end
-
-      should "not be valid against an admin or moderator" do
-        user = FactoryBot.create(:admin_user)
-        ban = FactoryBot.build(:ban, :user => user, :banner => @banner)
-        ban.save
-        assert(ban.errors.any?)
-
-        user = FactoryBot.create(:moderator_user)
-        ban = FactoryBot.build(:ban, :user => user, :banner => @banner)
-        ban.save
-        assert(ban.errors.any?)
-      end
-
-      should "be valid against anyone who is not an admin or a moderator" do
-        user = FactoryBot.create(:contributor_user)
-        ban = FactoryBot.create(:ban, :user => user, :banner => @banner)
-        assert(ban.errors.empty?)
-
-        user = FactoryBot.create(:gold_user)
-        ban = FactoryBot.create(:ban, :user => user, :banner => @banner)
-        assert(ban.errors.empty?)
-
         user = FactoryBot.create(:user)
         ban = FactoryBot.create(:ban, :user => user, :banner => @banner)
         assert(ban.errors.empty?)