diff --git a/app/controllers/post_flags_controller.rb b/app/controllers/post_flags_controller.rb index 9fe07d0d2..fcb3b9dfa 100644 --- a/app/controllers/post_flags_controller.rb +++ b/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 diff --git a/app/logical/post_query_builder.rb b/app/logical/post_query_builder.rb index 14af3523f..cf309a276 100644 --- a/app/logical/post_query_builder.rb +++ b/app/logical/post_query_builder.rb @@ -335,27 +335,21 @@ class PostQueryBuilder end end - if q[:flagger_ids_neg] - q[:flagger_ids_neg].each do |flagger_id| - if CurrentUser.can_view_flagger?(flagger_id) - 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 + q[:flaggers_neg].to_a.each do |flagger| + flags = PostFlag.unscoped.creator_matches(flagger, CurrentUser.user).where("post_id = posts.id").select("1") + relation = relation.where("NOT EXISTS (#{flags.to_sql})") end - if q[:flagger_ids] - q[:flagger_ids].each do |flagger_id| - if flagger_id == "any" - relation = relation.where('EXISTS (' + PostFlag.unscoped.search(:category => "normal").where('post_id = posts.id').reorder('').select('1').to_sql + ')') - elsif flagger_id == "none" - relation = relation.where('NOT EXISTS (' + PostFlag.unscoped.search(:category => "normal").where('post_id = posts.id').reorder('').select('1').to_sql + ')') - elsif CurrentUser.can_view_flagger?(flagger_id) - post_ids = PostFlag.unscoped.search(creator_id: flagger_id, category: "normal").reorder("").select {|flag| flag.not_uploaded_by?(CurrentUser.id)}.map(&:post_id).uniq - relation = relation.where("posts.id": post_ids) - end + q[:flaggers].to_a.each do |flagger| + if flagger == "any" + flags = PostFlag.unscoped.category_matches("normal").where("post_id = posts.id").select("1") + relation = relation.where("EXISTS (#{flags.to_sql})") + elsif flagger == "none" + flags = PostFlag.unscoped.category_matches("normal").where("post_id = posts.id").select("1") + relation = relation.where("NOT EXISTS (#{flags.to_sql})") + else + flags = PostFlag.unscoped.creator_matches(flagger, CurrentUser.user).where("post_id = posts.id").select("1") + 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) - q[:flagger_ids] << user_id unless user_id.blank? + user = User.find_by_name(g2) + q[:flaggers] ||= [] + q[:flaggers] << user unless user.blank? end when "-flagger" if g2 == "none" - q[:flagger_ids] ||= [] - q[:flagger_ids] << "any" + q[:flaggers] ||= [] + q[:flaggers] << "any" elsif g2 == "any" - q[:flagger_ids] ||= [] - q[:flagger_ids] << "none" + q[:flaggers] ||= [] + q[:flaggers] << "none" else - q[:flagger_ids_neg] ||= [] - user_id = User.name_to_id(g2) - q[:flagger_ids_neg] << user_id unless user_id.blank? + user = User.find_by_name(g2) + q[:flaggers_neg] ||= [] + q[:flaggers_neg] << user unless user.blank? end when "appealer" diff --git a/app/models/post_event.rb b/app/models/post_event.rb index a3dffc352..bbad886c6 100644 --- a/app/models/post_event.rb +++ b/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 diff --git a/app/models/post_flag.rb b/app/models/post_flag.rb index f7e792be2..65a61ca4b 100644 --- a/app/models/post_flag.rb +++ b/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) - q = q.where.not(post_id: CurrentUser.user.posts) - q = q.where("creator_id = ?", params[:creator_id].to_i) - else - q = q.none - end + flagger = User.find(params[:creator_id]) + q = q.creator_matches(flagger, CurrentUser.user) + elsif params[:creator_name].present? + flagger = User.find_by_name(params[:creator_name]) + q = q.creator_matches(flagger, CurrentUser.user) end - # XXX - if params[:creator_name].present? - 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) + if params[:category] + q = q.category_matches(params[:category]) 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 diff --git a/app/models/user.rb b/app/models/user.rb index ccc63785c..6f03d467d 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/policies/post_flag_policy.rb b/app/policies/post_flag_policy.rb new file mode 100644 index 000000000..09207cbe5 --- /dev/null +++ b/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 diff --git a/app/views/post_flags/_reasons.html.erb b/app/views/post_flags/_reasons.html.erb index 18063234e..7395e182a 100644 --- a/app/views/post_flags/_reasons.html.erb +++ b/app/views/post_flags/_reasons.html.erb @@ -3,7 +3,7 @@
  • <%= format_text(flag.reason, inline: true) %> - <% if CurrentUser.can_view_flagger_on_post?(flag) %> + <% if policy(flag).can_view_flagger? %> - <%= link_to_user(flag.creator) %> <% end %> diff --git a/app/views/post_flags/_search.html.erb b/app/views/post_flags/_search.html.erb index 385349d93..d16a5dff8 100644 --- a/app/views/post_flags/_search.html.erb +++ b/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] %> diff --git a/app/views/post_flags/index.html.erb b/app/views/post_flags/index.html.erb index 3174867e7..2151799c7 100644 --- a/app/views/post_flags/index.html.erb +++ b/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? %>
    by <%= link_to_user post_flag.creator %> <%= link_to "ยป", post_flags_path(search: params[:search].merge(creator_name: post_flag.creator.name)) %> <% end %> diff --git a/test/functional/post_flags_controller_test.rb b/test/functional/post_flags_controller_test.rb index fad71fcfc..8f24aea45 100644 --- a/test/functional/post_flags_controller_test.rb +++ b/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 - @post_flag = create(:post_flag, creator: @user) + should "render" do + 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_auth post_flags_path, @user, params: {:format => "js", :post_flag => {:post_id => @post.id, :reason => "xxx"}} - end + @post = create(:post) + post_auth post_flags_path, @flagger, params: { post_flag: { post_id: @post.id, reason: "xxx" }}, as: :javascript + assert_redirected_to PostFlag.last + assert_equal(true, @post.reload.is_flagged?) end end end diff --git a/test/unit/post_flag_test.rb b/test/unit/post_flag_test.rb index 6ddfd212f..a59a0eb49 100644 --- a/test/unit/post_flag_test.rb +++ b/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