pundit: convert post flags to pundit.

app/controllers/post_flags_controller.rb

@@ -1,14 +1,13 @@
 class PostFlagsController < ApplicationController
-  before_action :member_only, :except => [:index, :show]
   respond_to :html, :xml, :json, :js
 
   def new
-    @post_flag = PostFlag.new(post_flag_params)
+    @post_flag = authorize PostFlag.new(permitted_attributes(PostFlag))
     respond_with(@post_flag)
   end
 
   def index
-    @post_flags = PostFlag.paginated_search(params)
+    @post_flags = authorize PostFlag.paginated_search(params)
 
     if request.format.html?
       @post_flags = @post_flags.includes(:creator, post: [:flags, :uploader, :approver])
@@ -20,21 +19,16 @@ class PostFlagsController < ApplicationController
   end
 
   def create
-    @post_flag = PostFlag.create(post_flag_params.merge(creator: CurrentUser.user))
+    @post_flag = authorize PostFlag.new(creator: CurrentUser.user, **permitted_attributes(PostFlag))
+    @post_flag.save
     flash[:notice] = @post_flag.errors.none? ? "Post flagged" : @post_flag.errors.full_messages.join("; ")
     respond_with(@post_flag)
   end
 
   def show
-    @post_flag = PostFlag.find(params[:id])
+    @post_flag = authorize PostFlag.find(params[:id])
     respond_with(@post_flag) do |fmt|
       fmt.html { redirect_to post_flags_path(search: { id: @post_flag.id }) }
     end
   end
-
-  private
-
-  def post_flag_params
-    params.fetch(:post_flag, {}).permit(%i[post_id reason])
-  end
 end

app/logical/post_query_builder.rb

@@ -335,27 +335,21 @@ class PostQueryBuilder
       end
     end
 
-    if q[:flagger_ids_neg]
+    q[:flaggers_neg].to_a.each do |flagger|
-      q[:flagger_ids_neg].each do |flagger_id|
+      flags = PostFlag.unscoped.creator_matches(flagger, CurrentUser.user).where("post_id = posts.id").select("1")
-        if CurrentUser.can_view_flagger?(flagger_id)
+      relation = relation.where("NOT EXISTS (#{flags.to_sql})")
-          post_ids = PostFlag.unscoped.search(:creator_id => flagger_id, :category => "normal").reorder("").select {|flag| flag.not_uploaded_by?(CurrentUser.id)}.map {|flag| flag.post_id}.uniq
-          if post_ids.any?
-            relation = relation.where.not("posts.id": post_ids)
-          end
-        end
-      end
     end
 
-    if q[:flagger_ids]
+    q[:flaggers].to_a.each do |flagger|
-      q[:flagger_ids].each do |flagger_id|
+      if flagger == "any"
-        if flagger_id == "any"
+        flags = PostFlag.unscoped.category_matches("normal").where("post_id = posts.id").select("1")
-          relation = relation.where('EXISTS (' + PostFlag.unscoped.search(:category => "normal").where('post_id = posts.id').reorder('').select('1').to_sql + ')')
+        relation = relation.where("EXISTS (#{flags.to_sql})")
-        elsif flagger_id == "none"
+      elsif flagger == "none"
-          relation = relation.where('NOT EXISTS (' + PostFlag.unscoped.search(:category => "normal").where('post_id = posts.id').reorder('').select('1').to_sql + ')')
+        flags = PostFlag.unscoped.category_matches("normal").where("post_id = posts.id").select("1")
-        elsif CurrentUser.can_view_flagger?(flagger_id)
+        relation = relation.where("NOT EXISTS (#{flags.to_sql})")
-          post_ids = PostFlag.unscoped.search(creator_id: flagger_id, category: "normal").reorder("").select {|flag| flag.not_uploaded_by?(CurrentUser.id)}.map(&:post_id).uniq
+      else
-          relation = relation.where("posts.id": post_ids)
+        flags = PostFlag.unscoped.creator_matches(flagger, CurrentUser.user).where("post_id = posts.id").select("1")
-        end
+        relation = relation.where("EXISTS (#{flags.to_sql})")
       end
     end
 
@@ -738,28 +732,29 @@ class PostQueryBuilder
               end
 
             when "flagger"
-              q[:flagger_ids] ||= []
-
               if g2 == "none"
-                q[:flagger_ids] << "none"
+                q[:flaggers] ||= []
+                q[:flaggers] << "none"
               elsif g2 == "any"
-                q[:flagger_ids] << "any"
+                q[:flaggers] ||= []
+                q[:flaggers] << "any"
               else
-                user_id = User.name_to_id(g2)
+                user = User.find_by_name(g2)
-                q[:flagger_ids] << user_id unless user_id.blank?
+                q[:flaggers] ||= []
+                q[:flaggers] << user unless user.blank?
               end
 
             when "-flagger"
               if g2 == "none"
-                q[:flagger_ids] ||= []
+                q[:flaggers] ||= []
-                q[:flagger_ids] << "any"
+                q[:flaggers] << "any"
               elsif g2 == "any"
-                q[:flagger_ids] ||= []
+                q[:flaggers] ||= []
-                q[:flagger_ids] << "none"
+                q[:flaggers] << "none"
               else
-                q[:flagger_ids_neg] ||= []
+                user = User.find_by_name(g2)
-                user_id = User.name_to_id(g2)
+                q[:flaggers_neg] ||= []
-                q[:flagger_ids_neg] << user_id unless user_id.blank?
+                q[:flaggers_neg] << user unless user.blank?
               end
 
             when "appealer"

app/models/post_event.rb

@@ -48,7 +48,7 @@ class PostEvent
       true
     when PostFlag
       flag = event
-      user.can_view_flagger_on_post?(flag)
+      Pundit.policy!([user, nil], flag).can_view_flagger?
     end
   end
 

app/models/post_flag.rb

@@ -26,42 +26,47 @@ class PostFlag < ApplicationRecord
   scope :old, -> { where("post_flags.created_at <= ?", 3.days.ago) }
 
   module SearchMethods
+    def creator_matches(creator, searcher)
+      policy = Pundit.policy!([searcher, nil], PostFlag.new(creator: creator))
+
+      if policy.can_view_flagger?
+        where(creator: creator).where.not(post: searcher.posts)
+      else
+        none
+      end
+    end
+
+    def category_matches(category)
+      case category
+      when "normal"
+        where("reason NOT IN (?) AND reason NOT LIKE ?", [Reasons::UNAPPROVED], Reasons::REJECTED)
+      when "unapproved"
+        where(reason: Reasons::UNAPPROVED)
+      when "rejected"
+        where("reason LIKE ?", Reasons::REJECTED)
+      when "deleted"
+        where("reason = ? OR reason LIKE ?", Reasons::UNAPPROVED, Reasons::REJECTED)
+      else
+        none
+      end
+    end
+
     def search(params)
       q = super
 
       q = q.search_attributes(params, :post, :is_resolved, :reason)
       q = q.text_attribute_matches(:reason, params[:reason_matches])
 
-      # XXX
       if params[:creator_id].present?
-        if CurrentUser.can_view_flagger?(params[:creator_id].to_i)
+        flagger = User.find(params[:creator_id])
-          q = q.where.not(post_id: CurrentUser.user.posts)
+        q = q.creator_matches(flagger, CurrentUser.user)
-          q = q.where("creator_id = ?", params[:creator_id].to_i)
+      elsif params[:creator_name].present?
-        else
+        flagger = User.find_by_name(params[:creator_name])
-          q = q.none
+        q = q.creator_matches(flagger, CurrentUser.user)
-        end
       end
 
-      # XXX
+      if params[:category]
-      if params[:creator_name].present?
+        q = q.category_matches(params[:category])
-        flagger_id = User.name_to_id(params[:creator_name].strip)
-        if flagger_id && CurrentUser.can_view_flagger?(flagger_id)
-          q = q.where.not(post_id: CurrentUser.user.posts)
-          q = q.where("creator_id = ?", flagger_id)
-        else
-          q = q.none
-        end
-      end
-
-      case params[:category]
-      when "normal"
-        q = q.where("reason NOT IN (?) AND reason NOT LIKE ?", [Reasons::UNAPPROVED], Reasons::REJECTED)
-      when "unapproved"
-        q = q.where(reason: Reasons::UNAPPROVED)
-      when "rejected"
-        q = q.where("reason LIKE ?", Reasons::REJECTED)
-      when "deleted"
-        q = q.where("reason = ? OR reason LIKE ?", Reasons::UNAPPROVED, Reasons::REJECTED)
       end
 
       q.apply_default_order(params)
@@ -71,7 +76,7 @@ class PostFlag < ApplicationRecord
   module ApiMethods
     def api_attributes
       attributes = super + [:category]
-      attributes -= [:creator_id] unless CurrentUser.can_view_flagger_on_post?(self)
+      attributes -= [:creator_id] unless Pundit.policy!([CurrentUser.user, nil], self).can_view_flagger?
       attributes
     end
   end
@@ -131,10 +136,6 @@ class PostFlag < ApplicationRecord
     post.uploader_id
   end
 
-  def not_uploaded_by?(userid)
-    uploader_id != userid
-  end
-
   def self.available_includes
     [:post]
   end

app/models/user.rb

@@ -430,14 +430,6 @@ class User < ApplicationRecord
       created_at <= 1.week.ago
     end
 
-    def can_view_flagger?(flagger_id)
-      is_moderator? || flagger_id == id
-    end
-
-    def can_view_flagger_on_post?(flag)
-      (is_moderator? && flag.not_uploaded_by?(id)) || flag.creator_id == id
-    end
-
     def upload_limit
       @upload_limit ||= UploadLimit.new(self)
     end

app/policies/post_flag_policy.rb

@@ -0,0 +1,13 @@
+class PostFlagPolicy < ApplicationPolicy
+  def can_search_flagger?
+    user.is_moderator?
+  end
+
+  def can_view_flagger?
+    (user.is_moderator? || record.creator_id == user.id) && (record.post&.uploader_id != user.id)
+  end
+
+  def permitted_attributes
+    [:post_id, :reason]
+  end
+end

app/views/post_flags/_reasons.html.erb

@@ -3,7 +3,7 @@
     <li class="post-flag-reason">
       <span class="prose"><%= format_text(flag.reason, inline: true) %></span>
 
-      <% if CurrentUser.can_view_flagger_on_post?(flag) %>
+      <% if policy(flag).can_view_flagger? %>
         - <%= link_to_user(flag.creator) %>
       <% end %>
 

app/views/post_flags/_search.html.erb

@@ -2,7 +2,7 @@
   <%= f.input :reason_matches, label: "Reason", hint: "Use * for wildcard searches", input_html: { value: params[:search][:reason_matches] } %>
   <%= f.input :post_tags_match, label: "Tags", input_html: { value: params[:search][:post_tags_match], data: { autocomplete: "tag-query" } } %>
   <%= f.input :post_id, label: "Post ID", input_html: { value: params[:search][:post_id] } %>
-  <% if CurrentUser.is_moderator? %>
+  <% if policy(PostFlag).can_search_flagger? %>
     <%= f.input :creator_name, label: "Creator", input_html: { value: params[:search][:creator_name], data: { autocomplete: "user" } } %>
   <% end %>
   <%= f.input :is_resolved, label: "Resolved?", collection: [["Yes", true], ["No", false]], include_blank: true, selected: params[:search][:is_resolved] %>

app/views/post_flags/index.html.erb

@@ -29,7 +29,7 @@
       <% end %>
       <% t.column "Flagged", width: "15%" do |post_flag| %>
         <%= compact_time post_flag.created_at %>
-        <% if CurrentUser.can_view_flagger_on_post?(post_flag) %>
+        <% if policy(post_flag).can_view_flagger? %>
           <br> by <%= link_to_user post_flag.creator %>
           <%= link_to "»", post_flags_path(search: params[:search].merge(creator_name: post_flag.creator.name)) %>
         <% end %>

test/functional/post_flags_controller_test.rb

@@ -3,46 +3,85 @@ require 'test_helper'
 class PostFlagsControllerTest < ActionDispatch::IntegrationTest
   context "The post flags controller" do
     setup do
-      @user = create(:user, created_at: 2.weeks.ago)
+      @user = create(:user)
+      @flagger = create(:gold_user, created_at: 2.weeks.ago)
+      @uploader = create(:mod_user, created_at: 2.weeks.ago)
+      @mod = create(:mod_user)
+      @post = create(:post, is_flagged: true, uploader: @uploader)
+      @post_flag = create(:post_flag, post: @post, creator: @flagger)
     end
 
     context "new action" do
       should "render" do
-        get_auth new_post_flag_path, @user
+        get_auth new_post_flag_path, @flagger
+        assert_response :success
+      end
+    end
+
+    context "show action" do
+      should "work" do
+        get post_flag_path(@post_flag), as: :json
         assert_response :success
       end
     end
 
     context "index action" do
-      setup do
+      should "render" do
-        @post_flag = create(:post_flag, creator: @user)
+        get post_flags_path
+        assert_response :success
       end
 
-      should "render" do
+      should "hide flagger names from regular users" do
         get_auth post_flags_path, @user
         assert_response :success
+        assert_select "tr#post-flag-#{@post_flag.id} .flagged-column a.user-gold", false
+      end
+
+      should "hide flagger names from uploaders" do
+        get_auth post_flags_path, @uploader
+        assert_response :success
+        assert_select "tr#post-flag-#{@post_flag.id} .flagged-column a.user-gold", false
+      end
+
+      should "show flagger names to the flagger themselves" do
+        get_auth post_flags_path, @flagger
+        assert_response :success
+        assert_select "tr#post-flag-#{@post_flag.id} .flagged-column a.user-gold", true
+      end
+
+      should "show flagger names to mods" do
+        get_auth post_flags_path, @mod
+        assert_response :success
+        assert_select "tr#post-flag-#{@post_flag.id} .flagged-column a.user-gold", true
       end
 
       context "with search parameters" do
         should "render" do
-          get_auth post_flags_path, @user, params: {:search => {:post_id => @post_flag.post_id}}
+          get_auth post_flags_path(search: { post_id: @post_flag.post_id }), @user
           assert_response :success
         end
+
+        should "hide flagged posts when the searcher is the uploader" do
+          get_auth post_flags_path(search: { creator_id: @flagger.id }), @uploader
+          assert_response :success
+          assert_select "tr#post-flag-#{@post_flag.id}", false
+        end
+
+        should "show flagged posts when the searcher is not the uploader" do
+          get_auth post_flags_path(search: { creator_id: @flagger.id }), @mod
+          assert_response :success
+          assert_select "tr#post-flag-#{@post_flag.id}", true
+        end
       end
     end
 
     context "create action" do
-      setup do
-        @user.as_current do
-          @post = create(:post)
-        end
-      end
-
       should "create a new flag" do
         assert_difference("PostFlag.count", 1) do
-          assert_difference("PostFlag.count") do
+          @post = create(:post)
-            post_auth post_flags_path, @user, params: {:format => "js", :post_flag => {:post_id => @post.id, :reason => "xxx"}}
+          post_auth post_flags_path, @flagger, params: { post_flag: { post_id: @post.id, reason: "xxx" }}, as: :javascript
-          end
+          assert_redirected_to PostFlag.last
+          assert_equal(true, @post.reload.is_flagged?)
         end
       end
     end

test/unit/post_flag_test.rb

@@ -95,22 +95,5 @@ class PostFlagTest < ActiveSupport::TestCase
         assert_equal(@alice.id, @post_flag.creator_id)
       end
     end
-
-    context "a moderator user" do
-      should "not be able to view flags on their own uploads" do
-        @dave = create(:moderator_user, created_at: 1.month.ago)
-        @modpost = create(:post, :tag_string => "mmm", :uploader => @dave)
-        @flag1 = create(:post_flag, post: @modpost, creator: @alice)
-
-        assert_equal(false, @dave.can_view_flagger_on_post?(@flag1))
-
-        as(@dave) do
-          flag2 = PostFlag.search(:creator_id => @alice.id)
-          assert_equal(0, flag2.length)
-          flag3 = PostFlag.search({})
-          assert_nil(JSON.parse(flag3.to_json)[0]["creator_id"])
-        end
-      end
-    end
   end
 end