diff --git a/app/controllers/bulk_update_requests_controller.rb b/app/controllers/bulk_update_requests_controller.rb index b7febed42..7448e9064 100644 --- a/app/controllers/bulk_update_requests_controller.rb +++ b/app/controllers/bulk_update_requests_controller.rb @@ -1,62 +1,48 @@ class BulkUpdateRequestsController < ApplicationController respond_to :html, :xml, :json, :js - before_action :member_only, :except => [:index, :show] - before_action :admin_only, :only => [:approve] - before_action :load_bulk_update_request, :except => [:new, :create, :index] def new - @bulk_update_request = BulkUpdateRequest.new(bur_params(:create)) + @bulk_update_request = authorize BulkUpdateRequest.new(permitted_attributes(BulkUpdateRequest)) respond_with(@bulk_update_request) end def create - @bulk_update_request = BulkUpdateRequest.create(bur_params(:create).merge(user: CurrentUser.user)) + @bulk_update_request = authorize BulkUpdateRequest.new(user: CurrentUser.user, **permitted_attributes(BulkUpdateRequest)) + @bulk_update_request.save respond_with(@bulk_update_request, :location => bulk_update_requests_path) end def show + @bulk_update_request = authorize BulkUpdateRequest.find(params[:id]) respond_with(@bulk_update_request) end def edit + @bulk_update_request = authorize BulkUpdateRequest.find(params[:id]) + respond_with(@bulk_update_request) end def update - raise User::PrivilegeError unless @bulk_update_request.editable?(CurrentUser.user) - - @bulk_update_request.update(bur_params(:update)) + @bulk_update_request = authorize BulkUpdateRequest.find(params[:id]) + @bulk_update_request.update(permitted_attributes(@bulk_update_request)) respond_with(@bulk_update_request, location: bulk_update_requests_path, notice: "Bulk update request updated") end def approve + @bulk_update_request = authorize BulkUpdateRequest.find(params[:id]) @bulk_update_request.approve!(CurrentUser.user) respond_with(@bulk_update_request, :location => bulk_update_requests_path) end def destroy - raise User::PrivilegeError unless @bulk_update_request.rejectable?(CurrentUser.user) - + @bulk_update_request = authorize BulkUpdateRequest.find(params[:id]) @bulk_update_request.reject!(CurrentUser.user) respond_with(@bulk_update_request, location: bulk_update_requests_path, notice: "Bulk update request rejected") end def index - @bulk_update_requests = BulkUpdateRequest.paginated_search(params, count_pages: true) + @bulk_update_requests = authorize BulkUpdateRequest.paginated_search(params, count_pages: true) @bulk_update_requests = @bulk_update_requests.includes(:user, :approver, :forum_topic, forum_post: [:votes]) if request.format.html? respond_with(@bulk_update_requests) end - - private - - def load_bulk_update_request - @bulk_update_request = BulkUpdateRequest.find(params[:id]) - end - - def bur_params(context) - permitted_params = %i[script skip_secondary_validations] - permitted_params += %i[title reason forum_topic_id] if context == :create - permitted_params += %i[forum_topic_id forum_post_id] if context == :update && CurrentUser.is_admin? - - params.fetch(:bulk_update_request, {}).permit(permitted_params) - end end diff --git a/app/models/bulk_update_request.rb b/app/models/bulk_update_request.rb index 5e10284f0..961b22488 100644 --- a/app/models/bulk_update_request.rb +++ b/app/models/bulk_update_request.rb @@ -10,9 +10,9 @@ class BulkUpdateRequest < ApplicationRecord validates_presence_of :script validates_presence_of :title, if: ->(rec) {rec.forum_topic_id.blank?} + validates_presence_of :forum_topic, if: ->(rec) {rec.forum_topic_id.present?} validates_inclusion_of :status, :in => %w(pending approved rejected) validate :script_formatted_correctly - validate :forum_topic_id_not_invalid validate :validate_script, :on => :create before_validation :normalize_text after_create :create_forum_topic @@ -113,20 +113,6 @@ class BulkUpdateRequest < ApplicationRecord errors[:base] << e.message end - def forum_topic_id_not_invalid - if forum_topic_id - if !forum_topic - errors[:base] << "Forum topic ID is invalid" - elsif !forum_topic.visible?(CurrentUser.user) - errors[:base] << "Forum topic is private" - elsif forum_topic.is_locked - errors[:base] << "Forum topic is locked" - elsif forum_topic.is_deleted - errors[:base] << "Forum topic is deleted" - end - end - end - def validate_script AliasAndImplicationImporter.new(script, forum_topic_id, "1", skip_secondary_validations).validate! rescue RuntimeError => e @@ -138,18 +124,6 @@ class BulkUpdateRequest < ApplicationRecord include ApprovalMethods include ValidationMethods - def editable?(user) - user_id == user.id || user.is_builder? - end - - def approvable?(user) - !is_approved? && user.is_admin? - end - - def rejectable?(user) - is_pending? && editable?(user) - end - def reason_with_link "[bur:#{id}]\n\nReason: #{reason}" end diff --git a/app/policies/bulk_update_request_policy.rb b/app/policies/bulk_update_request_policy.rb new file mode 100644 index 000000000..5c9201d93 --- /dev/null +++ b/app/policies/bulk_update_request_policy.rb @@ -0,0 +1,33 @@ +class BulkUpdateRequestPolicy < ApplicationPolicy + def create? + unbanned? && (record.forum_topic.blank? || policy(record.forum_topic).reply?) + end + + def update? + unbanned? && (user.is_builder? || record.user_id == user.id) + end + + def approve? + user.is_admin? && !record.is_approved? + end + + def destroy? + record.is_pending? && update? + end + + def can_update_forum? + user.is_admin? + end + + def permitted_attributes_for_create + [:script, :skip_secondary_validations, :title, :reason, :forum_topic_id] + end + + def permitted_attributes_for_update + if can_update_forum? + [:script, :skip_secondary_validations, :forum_topic_id, :forum_post_id] + else + [:script, :skip_secondary_validations] + end + end +end diff --git a/app/views/bulk_update_requests/_bur_edit_links.html.erb b/app/views/bulk_update_requests/_bur_edit_links.html.erb index fedb802e4..644938b27 100644 --- a/app/views/bulk_update_requests/_bur_edit_links.html.erb +++ b/app/views/bulk_update_requests/_bur_edit_links.html.erb @@ -1,11 +1,11 @@ <%# bur %> -<% if bur.approvable?(CurrentUser.user) %> +<% if policy(bur).approve? %> <%= link_to "Approve", approve_bulk_update_request_path(bur), remote: true, method: :post, "data-confirm": "Are you sure you want to approve this bulk update request?" %> | <% end %> -<% if bur.rejectable?(CurrentUser.user) %> +<% if policy(bur).destroy? %> <%= link_to "Reject", bur, remote: true, method: :delete, "data-confirm": "Are you sure you want to reject this bulk update request?" %> | <% end %> -<% if bur.editable?(CurrentUser.user) %> +<% if policy(bur).update? %> <%= link_to "Edit", edit_bulk_update_request_path(bur), :"data-shortcut" => "e" %> <% end %> diff --git a/app/views/bulk_update_requests/_form.html.erb b/app/views/bulk_update_requests/_form.html.erb index 899737d41..8d48d16ea 100644 --- a/app/views/bulk_update_requests/_form.html.erb +++ b/app/views/bulk_update_requests/_form.html.erb @@ -38,7 +38,7 @@ <% end %> - <% if @bulk_update_request.persisted? && CurrentUser.is_admin? %> + <% if @bulk_update_request.persisted? && policy(@bulk_update_request).can_update_forum? %> <%= f.input :forum_topic_id %> <%= f.input :forum_post_id %> <% end %> diff --git a/test/functional/bulk_update_requests_controller_test.rb b/test/functional/bulk_update_requests_controller_test.rb index a6d377041..5a256b622 100644 --- a/test/functional/bulk_update_requests_controller_test.rb +++ b/test/functional/bulk_update_requests_controller_test.rb @@ -19,6 +19,7 @@ class BulkUpdateRequestsControllerTest < ActionDispatch::IntegrationTest should "succeed" do assert_difference("BulkUpdateRequest.count", 1) do post_auth bulk_update_requests_path, @user, params: {bulk_update_request: {skip_secondary_validations: "1", script: "create alias aaa -> bbb", title: "xxx"}} + assert_response :redirect end end end @@ -26,14 +27,26 @@ class BulkUpdateRequestsControllerTest < ActionDispatch::IntegrationTest context "#update" do should "still handle enabled secondary validations correctly" do put_auth bulk_update_request_path(@bulk_update_request.id), @user, params: {bulk_update_request: {script: "create alias zzz -> 222", skip_secondary_validations: "0"}} - @bulk_update_request.reload - assert_equal("create alias zzz -> 222", @bulk_update_request.script) + assert_response :redirect + assert_equal("create alias zzz -> 222", @bulk_update_request.reload.script) end should "still handle disabled secondary validations correctly" do put_auth bulk_update_request_path(@bulk_update_request.id), @user, params: {bulk_update_request: {script: "create alias zzz -> 222", skip_secondary_validations: "1"}} - @bulk_update_request.reload - assert_equal("create alias zzz -> 222", @bulk_update_request.script) + assert_response :redirect + assert_equal("create alias zzz -> 222", @bulk_update_request.reload.script) + end + + should "allow builders to update other people's requests" do + put_auth bulk_update_request_path(@bulk_update_request.id), create(:builder_user), params: {bulk_update_request: {script: "create alias zzz -> 222", skip_secondary_validations: "0"}} + assert_response :redirect + assert_equal("create alias zzz -> 222", @bulk_update_request.reload.script) + end + + should "not allow members to update other people's requests" do + put_auth bulk_update_request_path(@bulk_update_request.id), create(:user), params: {bulk_update_request: {script: "create alias zzz -> 222", skip_secondary_validations: "0"}} + assert_response 403 + assert_equal("create alias aaa -> bbb", @bulk_update_request.reload.script) end end @@ -55,19 +68,16 @@ class BulkUpdateRequestsControllerTest < ActionDispatch::IntegrationTest context "for the creator" do should "succeed" do delete_auth bulk_update_request_path(@bulk_update_request), @user - @bulk_update_request.reload - assert_equal("rejected", @bulk_update_request.status) + assert_response :redirect + assert_equal("rejected", @bulk_update_request.reload.status) end end context "for another member" do - setup do - @another_user = create(:user) - end - should "fail" do assert_difference("BulkUpdateRequest.count", 0) do - delete_auth bulk_update_request_path(@bulk_update_request), @another_user + delete_auth bulk_update_request_path(@bulk_update_request), create(:user) + assert_response 403 end end end @@ -75,8 +85,8 @@ class BulkUpdateRequestsControllerTest < ActionDispatch::IntegrationTest context "for an admin" do should "succeed" do delete_auth bulk_update_request_path(@bulk_update_request), @admin - @bulk_update_request.reload - assert_equal("rejected", @bulk_update_request.status) + assert_response :redirect + assert_equal("rejected", @bulk_update_request.reload.status) end end end @@ -85,16 +95,16 @@ class BulkUpdateRequestsControllerTest < ActionDispatch::IntegrationTest context "for a member" do should "fail" do post_auth approve_bulk_update_request_path(@bulk_update_request), @user - @bulk_update_request.reload - assert_equal("pending", @bulk_update_request.status) + assert_response 403 + assert_equal("pending", @bulk_update_request.reload.status) end end context "for an admin" do should "succeed" do post_auth approve_bulk_update_request_path(@bulk_update_request), @admin - @bulk_update_request.reload - assert_equal("approved", @bulk_update_request.status) + assert_response :redirect + assert_equal("approved", @bulk_update_request.reload.status) end end end