From db4c04d6065622bb2507b07b7cfce3bc3b63af9e Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Mon, 13 Apr 2015 11:48:31 -0400 Subject: [PATCH] FIX: Moderators shouldn't be able to see secure deleted posts --- app/controllers/posts_controller.rb | 31 +++++++++++++------ spec/controllers/posts_controller_spec.rb | 37 +++++++++++++++++++++++ spec/fabricators/category_fabricator.rb | 2 +- 3 files changed, 60 insertions(+), 10 deletions(-) diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index b0d35c669f0..0ebb3a766c8 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -335,7 +335,7 @@ class PostsController < ApplicationController offset = [params[:offset].to_i, 0].max limit = [(params[:limit] || 60).to_i, 100].min - posts = user_posts(user.id, offset, limit) + posts = user_posts(guardian, user.id, offset: offset, limit: limit) .where(id: PostAction.where(post_action_type_id: PostActionType.notify_flag_type_ids) .where(disagreed_at: nil) .select(:post_id)) @@ -351,7 +351,7 @@ class PostsController < ApplicationController offset = [params[:offset].to_i, 0].max limit = [(params[:limit] || 60).to_i, 100].min - posts = user_posts(user.id, offset, limit).where.not(deleted_at: nil) + posts = user_posts(guardian, user.id, offset: offset, limit: limit).where.not(deleted_at: nil) render_serialized(posts, AdminPostSerializer) end @@ -389,13 +389,26 @@ class PostsController < ApplicationController private - def user_posts(user_id, offset=0, limit=60) - Post.includes(:user, :topic, :deleted_by, :user_actions) - .with_deleted - .where(user_id: user_id) - .order(created_at: :desc) - .offset(offset) - .limit(limit) + def user_posts(guardian, user_id, opts) + posts = Post.includes(:user, :topic, :deleted_by, :user_actions) + .where(user_id: user_id) + .with_deleted + .order(created_at: :desc) + + if guardian.user.moderator? + + # Awful hack, but you can't seem to remove the `default_scope` when joining + # So instead I grab the topics separately + topic_ids = posts.dup.pluck(:topic_id) + secured_category_ids = guardian.secure_category_ids + topics = Topic.where(id: topic_ids).with_deleted.where.not(archetype: 'private_message') + topics = topics.secured(guardian) + + posts = posts.where(topic_id: topics.pluck(:id)) + end + + posts.offset(opts[:offset]) + .limit(opts[:limit]) end def params_key(params) diff --git a/spec/controllers/posts_controller_spec.rb b/spec/controllers/posts_controller_spec.rb index 14210d0a649..3faf22d2aeb 100644 --- a/spec/controllers/posts_controller_spec.rb +++ b/spec/controllers/posts_controller_spec.rb @@ -774,6 +774,43 @@ describe PostsController do expect(response).to be_success end + it "doesn't return secured categories for moderators if they don't have access" do + user = Fabricate(:user) + admin = Fabricate(:admin) + moderator = Fabricate(:moderator) + + group = Fabricate(:group) + group.add(user) + group.appoint_manager(user) + + secured_category = Fabricate(:private_category, group: group) + secured_post = create_post(user: user, category: secured_category) + PostDestroyer.new(admin, secured_post).destroy + + log_in(:moderator) + xhr :get, :deleted_posts, username: user.username + expect(response).to be_success + + data = JSON.parse(response.body) + expect(data.length).to eq(0) + end + + it "doesn't return PMs for moderators" do + user = Fabricate(:user) + admin = Fabricate(:admin) + moderator = Fabricate(:moderator) + + pm_post = create_post(user: user, archetype: 'private_message', target_usernames: [admin.username]) + PostDestroyer.new(admin, pm_post).destroy + + log_in(:moderator) + xhr :get, :deleted_posts, username: user.username + expect(response).to be_success + + data = JSON.parse(response.body) + expect(data.length).to eq(0) + end + it "only shows posts deleted by other users" do user = Fabricate(:user) admin = Fabricate(:admin) diff --git a/spec/fabricators/category_fabricator.rb b/spec/fabricators/category_fabricator.rb index 5a23366276e..a37f5b4c8ba 100644 --- a/spec/fabricators/category_fabricator.rb +++ b/spec/fabricators/category_fabricator.rb @@ -22,6 +22,6 @@ Fabricator(:private_category, from: :category) do user after_build do |cat, transients| cat.update!(read_restricted: true) - cat.category_groups.build(group_id: transients[:group].id, permission_type: :full) + cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full]) end end