diff --git a/app/controllers/bans_controller.rb b/app/controllers/bans_controller.rb index 640672ad0..d8a1301dc 100644 --- a/app/controllers/bans_controller.rb +++ b/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)) - - if @ban.errors.any? - render :action => "new" - else - redirect_to ban_path(@ban), :notice => "Ban created" - end + @ban = authorize Ban.new(banner: CurrentUser.user, **permitted_attributes(Ban)) + @ban.save + respond_with(@ban) end def update - @ban = Ban.find(params[:id]) - if @ban.update(ban_params(:update)) - redirect_to ban_path(@ban), :notice => "Ban updated" - else - render :action => "edit" - end + @ban = authorize Ban.find(params[:id]) + @ban.update(permitted_attributes(@ban)) + respond_with(@ban) end def destroy - @ban = Ban.find(params[:id]) + @ban = authorize Ban.find(params[:id]) @ban.destroy - redirect_to bans_path, :notice => "Ban destroyed" - 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) + respond_with(@ban) end end diff --git a/app/models/ban.rb b/app/models/ban.rb index 8c56eb6f2..8c7530ac7 100644 --- a/app/models/ban.rb +++ b/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 diff --git a/app/policies/ban_policy.rb b/app/policies/ban_policy.rb new file mode 100644 index 000000000..3a4c5d52e --- /dev/null +++ b/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 diff --git a/app/views/bans/index.html.erb b/app/views/bans/index.html.erb index cc57f5d06..fe2c63e05 100644 --- a/app/views/bans/index.html.erb +++ b/app/views/bans/index.html.erb @@ -23,7 +23,7 @@
<%= time_ago_in_words_tagged(ban.created_at) %>
<% 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 %> diff --git a/test/functional/bans_controller_test.rb b/test/functional/bans_controller_test.rb index 0cae61dbe..97484e615 100644 --- a/test/functional/bans_controller_test.rb +++ b/test/functional/bans_controller_test.rb @@ -10,51 +10,96 @@ class BansControllerTest < ActionDispatch::IntegrationTest end end - should "get the new page" do - get_auth new_ban_path, @mod - assert_response :success - end - - 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}} + context "new action" do + should "render" do + get_auth new_ban_path, @mod + assert_response :success end - ban = Ban.last - assert_redirected_to(ban_path(ban)) end - should "update a ban" do - put_auth ban_path(@ban.id), @mod, params: {ban: {reason: "xxx", duration: 60}} - @ban.reload - assert_equal("xxx", @ban.reason) - assert_redirected_to(ban_path(@ban)) - end - - should "destroy a ban" do - assert_difference("Ban.count", -1) do - delete_auth ban_path(@ban.id), @mod + context "edit action" do + should "render" do + get_auth edit_ban_path(@ban.id), @mod + assert_response :success + end + end + + context "show action" do + should "render" do + 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 diff --git a/test/unit/ban_test.rb b/test/unit/ban_test.rb index 52ce0f659..6d731eef8 100644 --- a/test/unit/ban_test.rb +++ b/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 - 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?) - + should "be valid" do user = FactoryBot.create(:user) ban = FactoryBot.create(:ban, :user => user, :banner => @banner) assert(ban.errors.empty?)