pundit: convert bulk update requests to pundit.

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

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

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

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

app/views/bulk_update_requests/_form.html.erb

@@ -38,7 +38,7 @@
     </div>
   <% 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 %>

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