diff --git a/app/assets/javascripts/discourse/app/widgets/topic-admin-menu.js b/app/assets/javascripts/discourse/app/widgets/topic-admin-menu.js index f5e1eb5f42f..a022ac3b404 100644 --- a/app/assets/javascripts/discourse/app/widgets/topic-admin-menu.js +++ b/app/assets/javascripts/discourse/app/widgets/topic-admin-menu.js @@ -153,7 +153,10 @@ export default createWidget("topic-admin-menu", { }); } - if (this.get("currentUser.canManageTopic")) { + if ( + this.get("currentUser.canManageTopic") || + details.get("can_moderate_category") + ) { if (details.get("can_delete")) { this.addActionButton({ className: "topic-admin-delete", diff --git a/app/assets/javascripts/discourse/tests/acceptance/topic-admin-menu-test.js b/app/assets/javascripts/discourse/tests/acceptance/topic-admin-menu-test.js index d6169b7b3c1..ed6206780f1 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/topic-admin-menu-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/topic-admin-menu-test.js @@ -28,6 +28,9 @@ acceptance("Topic - Admin Menu", function (needs) { exists(".toggle-admin-menu"), "The admin menu button was rendered" ); + + await click(".toggle-admin-menu"); + assert.ok(exists(".topic-admin-delete"), "The delete item was rendered"); }); test("Enter as a user with moderator and admin permissions", async function (assert) { diff --git a/app/assets/javascripts/discourse/tests/fixtures/topic.js b/app/assets/javascripts/discourse/tests/fixtures/topic.js index 2409a38f832..66d26f8aa6d 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/topic.js +++ b/app/assets/javascripts/discourse/tests/fixtures/topic.js @@ -5583,6 +5583,7 @@ export default { details: { notification_level: 3, notifications_reason_id: 1, + can_delete: true, can_edit: true, can_create_post: true, can_move_posts: true, @@ -5593,6 +5594,7 @@ export default { can_archive_topic: true, can_split_merge_topic: true, can_edit_staff_notes: true, + can_moderate_category: true, participants: [ { id: 3, diff --git a/app/controllers/post_action_users_controller.rb b/app/controllers/post_action_users_controller.rb index 0c59b763ccc..24cac61295e 100644 --- a/app/controllers/post_action_users_controller.rb +++ b/app/controllers/post_action_users_controller.rb @@ -9,10 +9,8 @@ class PostActionUsersController < ApplicationController page = params[:page].to_i page_size = (params[:limit] || 200).to_i - finder = Post.where(id: params[:id].to_i) - finder = finder.with_deleted if guardian.is_staff? - - post = finder.first + # Find the post, and then determine if they can see the post (if deleted) + post = Post.with_deleted.where(id: params[:id].to_i).first guardian.ensure_can_see!(post) unknown_user_ids = Set.new diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 74af0eafeab..40df173d5dc 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -299,7 +299,7 @@ class PostsController < ApplicationController def destroy post = find_post_from_params - unless current_user.staff? + unless guardian.can_moderate_topic?(post.topic) RateLimiter.new(current_user, "delete_post_per_min", SiteSetting.max_post_deletions_per_minute, 1.minute).performed! RateLimiter.new(current_user, "delete_post_per_day", SiteSetting.max_post_deletions_per_day, 1.day).performed! end @@ -320,7 +320,7 @@ class PostsController < ApplicationController def recover post = find_post_from_params - unless current_user.staff? + unless guardian.can_moderate_topic?(post.topic) RateLimiter.new(current_user, "delete_post_per_min", SiteSetting.max_post_deletions_per_minute, 1.minute).performed! RateLimiter.new(current_user, "delete_post_per_day", SiteSetting.max_post_deletions_per_day, 1.day).performed! end @@ -802,14 +802,21 @@ class PostsController < ApplicationController end def find_post_using(finder) - # Include deleted posts if the user is staff - finder = finder.with_deleted if current_user.try(:staff?) - post = finder.first + # A deleted post can be seen by staff or a category group moderator for the topic. + # But we must find the deleted post to determine which category it belongs to, so + # we must find.with_deleted + post = finder.with_deleted.first raise Discourse::NotFound unless post - # load deleted topic - post.topic = Topic.with_deleted.find(post.topic_id) if current_user.try(:staff?) - raise Discourse::NotFound unless post.topic + post.topic = Topic.with_deleted.find(post.topic_id) + + if !post.topic || + ( + (post.deleted_at.present? || post.topic.deleted_at.present?) && + !guardian.can_moderate_topic?(post.topic) + ) + raise Discourse::NotFound + end guardian.ensure_can_see!(post) post diff --git a/app/serializers/topic_view_details_serializer.rb b/app/serializers/topic_view_details_serializer.rb index 51c99d99051..ef152591c57 100644 --- a/app/serializers/topic_view_details_serializer.rb +++ b/app/serializers/topic_view_details_serializer.rb @@ -20,7 +20,8 @@ class TopicViewDetailsSerializer < ApplicationSerializer :can_close_topic, :can_archive_topic, :can_split_merge_topic, - :can_edit_staff_notes] + :can_edit_staff_notes, + :can_moderate_category] end attributes( @@ -145,6 +146,7 @@ class TopicViewDetailsSerializer < ApplicationSerializer alias :include_can_archive_topic? :can_perform_action_available_to_group_moderators? alias :include_can_split_merge_topic? :can_perform_action_available_to_group_moderators? alias :include_can_edit_staff_notes? :can_perform_action_available_to_group_moderators? + alias :include_can_moderate_category? :can_perform_action_available_to_group_moderators? def include_can_publish_page? scope.can_publish_page?(object.topic) diff --git a/lib/guardian.rb b/lib/guardian.rb index e4d34fa086d..fd594077aff 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -94,6 +94,9 @@ class Guardian end def is_category_group_moderator?(category) + return false unless category + return false unless authenticated? + @is_category_group_moderator ||= begin SiteSetting.enable_category_group_moderation? && category.present? && diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index 31a198142e5..e6e90410139 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -181,17 +181,22 @@ module PostGuardian return false if post.is_first_post? # Can't delete posts in archived topics unless you are staff - return false if !is_staff? && post.topic.archived? + can_moderate = can_moderate_topic?(post.topic) + return false if !can_moderate && post.topic&.archived? # You can delete your own posts return !post.user_deleted? if is_my_own?(post) - is_staff? + can_moderate end # Recovery Method def can_recover_post?(post) - if is_staff? + return false unless post + + topic = Topic.with_deleted.find(post.topic_id) if post.topic_id + + if can_moderate_topic?(topic) !!post.deleted_at else is_my_own?(post) && post.user_deleted && !post.deleted_at @@ -212,7 +217,7 @@ module PostGuardian return true if is_admin? return false unless can_see_topic?(post.topic) return false unless post.user == @user || Topic.visible_post_types(@user).include?(post.post_type) - return false if !is_moderator? && post.deleted_at.present? + return false if !(is_moderator? || is_category_group_moderator?(post.topic.category)) && post.deleted_at.present? true end @@ -261,8 +266,8 @@ module PostGuardian is_staff? end - def can_see_deleted_posts? - is_staff? + def can_see_deleted_posts?(category = nil) + is_staff? || is_category_group_moderator?(category) end def can_view_raw_email?(post) diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb index 77e6ff9d22f..fdd13e703fe 100644 --- a/lib/guardian/topic_guardian.rb +++ b/lib/guardian/topic_guardian.rb @@ -19,6 +19,7 @@ module TopicGuardian is_category_group_moderator?(topic.category) end + alias :can_moderate_topic? :can_review_topic? def can_create_shared_draft? is_staff? && SiteSetting.shared_drafts_enabled? @@ -115,7 +116,7 @@ module TopicGuardian # Recovery Method def can_recover_topic?(topic) - if is_staff? + if is_staff? || (topic&.category && is_category_group_moderator?(topic.category)) !!(topic && topic.deleted_at) else topic && can_recover_post?(topic.ordered_posts.first) @@ -124,7 +125,7 @@ module TopicGuardian def can_delete_topic?(topic) !topic.trashed? && - (is_staff? || (is_my_own?(topic) && topic.posts_count <= 1 && topic.created_at && topic.created_at > 24.hours.ago)) && + (is_staff? || (is_my_own?(topic) && topic.posts_count <= 1 && topic.created_at && topic.created_at > 24.hours.ago) || is_category_group_moderator?(topic.category)) && !topic.is_category_topic? && !Discourse.static_doc_topic_ids.include?(topic.id) end @@ -142,14 +143,14 @@ module TopicGuardian authenticated? && topic && @user.has_trust_level?(TrustLevel[1]) end - def can_see_deleted_topics? - is_staff? + def can_see_deleted_topics?(category) + is_staff? || is_category_group_moderator?(category) end def can_see_topic?(topic, hide_deleted = true) return false unless topic return true if is_admin? - return false if hide_deleted && topic.deleted_at && !can_see_deleted_topics? + return false if hide_deleted && topic.deleted_at && !can_see_deleted_topics?(topic.category) if topic.private_message? return authenticated? && topic.all_allowed_users.where(id: @user.id).exists? diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb index 23c3665ef59..0b569ff405d 100644 --- a/lib/post_destroyer.rb +++ b/lib/post_destroyer.rb @@ -66,7 +66,7 @@ class PostDestroyer delete_removed_posts_after = @opts[:delete_removed_posts_after] || SiteSetting.delete_removed_posts_after - if @user.staff? || delete_removed_posts_after < 1 || post_is_reviewable? + if delete_removed_posts_after < 1 || post_is_reviewable? || Guardian.new(@user).can_moderate_topic?(topic) perform_delete elsif @user.id == @post.user_id mark_for_deletion(delete_removed_posts_after) @@ -86,7 +86,7 @@ class PostDestroyer end def recover - if (@user.staff? || post_is_reviewable?) && @post.deleted_at + if (post_is_reviewable? || Guardian.new(@user).can_moderate_topic?(@post.topic)) && @post.deleted_at staff_recovered elsif @user.staff? || @user.id == @post.user_id user_recovered @@ -221,6 +221,8 @@ class PostDestroyer private def post_is_reviewable? + return true if @user.staff? + topic = @post.topic || Topic.with_deleted.find(@post.topic_id) Guardian.new(@user).can_review_topic?(topic) && Reviewable.exists?(target: @post) end diff --git a/lib/topic_view.rb b/lib/topic_view.rb index 49c9e45b49e..982e16759ca 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -704,7 +704,7 @@ class TopicView .includes({ user: :primary_group }, :reply_to_user, :deleted_by, :incoming_email, :topic) .order('sort_order') @posts = filter_post_types(@posts) - @posts = @posts.with_deleted if @guardian.can_see_deleted_posts? + @posts = @posts.with_deleted if @guardian.can_see_deleted_posts?(@topic.category) @posts end @@ -720,7 +720,7 @@ class TopicView def unfiltered_posts result = filter_post_types(@topic.posts) - result = result.with_deleted if @guardian.can_see_deleted_posts? + result = result.with_deleted if @guardian.can_see_deleted_posts?(@topic.category) result = result.where("user_id IS NOT NULL") if @exclude_deleted_users result = result.where(hidden: false) if @exclude_hidden result @@ -776,7 +776,7 @@ class TopicView # copy the filter for has_deleted? method @predelete_filtered_posts = @filtered_posts.spawn - if @guardian.can_see_deleted_posts? && !@show_deleted && has_deleted? + if @guardian.can_see_deleted_posts?(@topic.category) && !@show_deleted && has_deleted? @filtered_posts = @filtered_posts.where( "posts.deleted_at IS NULL OR posts.post_number = 1" ) diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index 5bfa887a679..1d54d690b27 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -847,12 +847,32 @@ describe Guardian do expect(Guardian.new(admin).can_banner_topic?(nil)).to be_falsey expect(Guardian.new(admin).can_banner_topic?(topic)).to be_truthy end + + it 'respects category group moderator settings' do + group_user = Fabricate(:group_user) + user_gm = group_user.user + group = group_user.group + SiteSetting.enable_category_group_moderation = true + + topic = Fabricate(:topic) + + expect(Guardian.new(user_gm).can_see?(topic)).to be_truthy + + topic.trash!(admin) + topic.reload + + expect(Guardian.new(user_gm).can_see?(topic)).to be_falsey + + topic.category.update!(reviewable_by_group_id: group.id, topic_id: post.topic.id) + expect(Guardian.new(user_gm).can_see?(topic)).to be_truthy + end end describe 'a Post' do + fab!(:post) { Fabricate(:post) } fab!(:another_admin) { Fabricate(:admin) } + it 'correctly handles post visibility' do - post = Fabricate(:post) topic = post.topic expect(Guardian.new(user).can_see?(post)).to be_truthy @@ -870,8 +890,25 @@ describe Guardian do expect(Guardian.new(admin).can_see?(post)).to be_truthy end + it 'respects category group moderator settings' do + group_user = Fabricate(:group_user) + user_gm = group_user.user + group = group_user.group + SiteSetting.enable_category_group_moderation = true + + expect(Guardian.new(user_gm).can_see?(post)).to be_truthy + + post.trash!(another_admin) + post.reload + + expect(Guardian.new(user_gm).can_see?(post)).to be_falsey + + post.topic.category.update!(reviewable_by_group_id: group.id, topic_id: post.topic.id) + expect(Guardian.new(user_gm).can_see?(post)).to be_truthy + end + it 'respects whispers' do - regular_post = Fabricate.build(:post) + regular_post = post whisper_post = Fabricate.build(:post, post_type: Post.types[:whisper]) anon_guardian = Guardian.new @@ -1194,6 +1231,29 @@ describe Guardian do end end end + + context 'category group moderation is enabled' do + fab!(:group_user) { Fabricate(:group_user) } + + before do + topic.save! + post.save! + + SiteSetting.enable_category_group_moderation = true + PostDestroyer.new(moderator, topic.first_post).destroy + topic.reload + end + + it "returns false if user is not a member of the appropriate group" do + expect(Guardian.new(group_user.user).can_recover_topic?(topic)).to be_falsey + end + + it "returns true if user is a member of the appropriate group" do + topic.category.update!(reviewable_by_group_id: group_user.group.id) + + expect(Guardian.new(group_user.user).can_recover_topic?(topic)).to be_truthy + end + end end describe "can_recover_post?" do @@ -1979,6 +2039,25 @@ describe Guardian do topic.update!(posts_count: 1, created_at: 48.hours.ago) expect(Guardian.new(topic.user).can_delete?(topic)).to be_falsey end + + context 'category group moderation is enabled' do + fab!(:group_user) { Fabricate(:group_user) } + + before do + SiteSetting.enable_category_group_moderation = true + end + + it "returns false if user is not a member of the appropriate group" do + expect(Guardian.new(group_user.user).can_delete?(topic)).to be_falsey + end + + it "returns true if user is a member of the appropriate group" do + topic.category.update!(reviewable_by_group_id: group_user.group.id) + + expect(Guardian.new(group_user.user).can_delete?(topic)).to be_truthy + end + end + end context 'a Post' do diff --git a/spec/components/post_destroyer_spec.rb b/spec/components/post_destroyer_spec.rb index 91ba651b115..c6ac8e5dd7b 100644 --- a/spec/components/post_destroyer_spec.rb +++ b/spec/components/post_destroyer_spec.rb @@ -313,19 +313,6 @@ describe PostDestroyer do end end end - - context "when the post does not have a Reviewable record" do - it "does not recover the post" do - PostDestroyer.new(Discourse.system_user, @reply).destroy - @reply.reload - expect(@reply.user_deleted).to eq(false) - expect(@reply.deleted_at).not_to eq(nil) - - PostDestroyer.new(review_user, @reply).recover - @reply.reload - expect(@reply.deleted_at).not_to eq(nil) - end - end end end end @@ -516,21 +503,6 @@ describe PostDestroyer do expect(UserHistory.count).to eq(history_count + 1) end end - - context "when the post does not have a reviewable" do - it "does not delete the post" do - author = post.user - reply = create_post(topic_id: post.topic_id, user: author) - - post_count = author.post_count - history_count = UserHistory.count - - PostDestroyer.new(review_user, reply).destroy - - expect(reply.deleted_at).not_to be_present - expect(reply.deleted_by).to eq(nil) - end - end end context "as an admin" do diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index 5e230a3f9f5..a44cd50db84 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/spec/requests/posts_controller_spec.rb @@ -45,6 +45,28 @@ shared_examples 'finding and showing post' do get url expect(response.status).to eq(200) end + + context "category group moderator" do + fab!(:group_user) { Fabricate(:group_user) } + let(:user_gm) { group_user.user } + let(:group) { group_user.group } + + before do + SiteSetting.enable_category_group_moderation = true + sign_in(user_gm) + end + + it "can find posts in the allowed category" do + post.topic.category.update!(reviewable_by_group_id: group.id, topic_id: topic.id) + get url + expect(response.status).to eq(200) + end + + it "can't find posts outside of the allowed category" do + get url + expect(response.status).to eq(404) + end + end end end