From b45a30c40f96374919693968d2b64243604aeef0 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Mon, 14 Dec 2020 16:08:20 -0300 Subject: [PATCH] FIX: Users without shared drafts access can still have access to the category. (#11476) This is an edge-case of 9fb3629. An admin could set the shared draft category to one where both TL2 and TL3 users have access but only give shared draft access to TL3 users. If something like this happens, we need to make sure that TL2 users won't be able to see them, and they won't be listed on latest. Before this change, `SharedDrafts` were lazily created when a destination category was selected. We now create it alongside the topic and set the destination to the same shared draft category. --- .../discourse/app/controllers/topic.js | 17 +++++++++--- .../discourse/tests/fixtures/topic.js | 1 + app/controllers/list_controller.rb | 2 +- app/controllers/topics_controller.rb | 2 ++ app/serializers/topic_view_serializer.rb | 8 +++++- lib/guardian/topic_guardian.rb | 2 ++ lib/topic_creator.rb | 6 +++-- lib/topic_query.rb | 24 +++++++++++------ spec/components/topic_query_spec.rb | 27 +++++++++++++++++++ spec/requests/topics_controller_spec.rb | 5 ++++ 10 files changed, 79 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/topic.js b/app/assets/javascripts/discourse/app/controllers/topic.js index 996e5def746..a09f99261e8 100644 --- a/app/assets/javascripts/discourse/app/controllers/topic.js +++ b/app/assets/javascripts/discourse/app/controllers/topic.js @@ -110,10 +110,21 @@ export default Controller.extend(bufferedProperty("model"), { } }, - @discourseComputed("model.postStream.loaded", "model.category_id") - showSharedDraftControls(loaded, categoryId) { + @discourseComputed( + "model.postStream.loaded", + "model.category_id", + "model.is_shared_draft" + ) + showSharedDraftControls(loaded, categoryId, isSharedDraft) { let draftCat = this.site.shared_drafts_category_id; - return loaded && draftCat && categoryId && draftCat === categoryId; + + return ( + loaded && + draftCat && + categoryId && + draftCat === categoryId && + isSharedDraft + ); }, @discourseComputed("site.mobileView", "model.posts_count") diff --git a/app/assets/javascripts/discourse/tests/fixtures/topic.js b/app/assets/javascripts/discourse/tests/fixtures/topic.js index 34915dd7e02..fbb7f696c9f 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/topic.js +++ b/app/assets/javascripts/discourse/tests/fixtures/topic.js @@ -4032,6 +4032,7 @@ export default { archetype: "regular", slug: "this-is-a-test-topic", category_id: 24, + is_shared_draft: true, word_count: 15, deleted_at: null, user_id: 1, diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb index 910e2ed5d84..6fc25d672e3 100644 --- a/app/controllers/list_controller.rb +++ b/app/controllers/list_controller.rb @@ -62,7 +62,7 @@ class ListController < ApplicationController if @category.id == SiteSetting.shared_drafts_category.to_i # On shared drafts, show the destination category list.topics.each do |t| - t.includes_destination_category = true + t.includes_destination_category = t.shared_draft.present? end else # When viewing a non-shared draft category, find topics whose diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index e5a2f9387a9..fa778fda331 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -168,6 +168,8 @@ class TopicsController < ApplicationController topic = Topic.find(params[:id]) category = Category.find(params[:destination_category_id]) + raise Discourse::InvalidParameters if category.id == SiteSetting.shared_drafts_category.to_i + guardian.ensure_can_publish_topic!(topic, category) topic = TopicPublisher.new(topic, current_user, category.id).publish! diff --git a/app/serializers/topic_view_serializer.rb b/app/serializers/topic_view_serializer.rb index 5be4ccc4c05..19e288bf71f 100644 --- a/app/serializers/topic_view_serializer.rb +++ b/app/serializers/topic_view_serializer.rb @@ -74,7 +74,8 @@ class TopicViewSerializer < ApplicationSerializer :show_read_indicator, :requested_group_name, :thumbnails, - :user_last_posted_at + :user_last_posted_at, + :is_shared_draft ) has_one :details, serializer: TopicViewDetailsSerializer, root: false, embed: :objects @@ -241,6 +242,11 @@ class TopicViewSerializer < ApplicationSerializer object.topic.shared_draft.present? end + def is_shared_draft + include_destination_category_id? + end + alias_method :include_is_shared_draft?, :include_destination_category_id? + def include_pending_posts? scope.authenticated? && object.queued_posts_enabled end diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb index ed64497d7da..01cb6c815f0 100644 --- a/lib/guardian/topic_guardian.rb +++ b/lib/guardian/topic_guardian.rb @@ -176,6 +176,8 @@ module TopicGuardian return authenticated? && topic.all_allowed_users.where(id: @user.id).exists? end + return false if topic.shared_draft && !can_create_shared_draft? + category = topic.category can_see_category?(category) && (!category.read_restricted || !is_staged? || secure_category_ids.include?(category.id) || topic.user == user) diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb index 993abba96de..30c8de2dd4e 100644 --- a/lib/topic_creator.rb +++ b/lib/topic_creator.rb @@ -56,8 +56,10 @@ class TopicCreator private def create_shared_draft(topic) - return unless @opts[:shared_draft] && @opts[:category].present? - SharedDraft.create(topic_id: topic.id, category_id: @opts[:category]) + return if @opts[:shared_draft].blank? || @opts[:shared_draft] == 'false' + + category_id = @opts[:category].blank? ? SiteSetting.shared_drafts_category.to_i : @opts[:category] + SharedDraft.create(topic_id: topic.id, category_id: category_id) end def create_warning(topic) diff --git a/lib/topic_query.rb b/lib/topic_query.rb index f5253eadad7..812d03de21b 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -619,15 +619,23 @@ class TopicQuery viewing_shared = category_id && category_id == drafts_category_id can_create_shared = guardian.can_create_shared_draft? - if can_create_shared && options[:destination_category_id] - destination_category_id = get_category_id(options[:destination_category_id]) - topic_ids = SharedDraft.where(category_id: destination_category_id).pluck(:topic_id) - result.where(id: topic_ids) - elsif can_create_shared && viewing_shared - result.includes(:shared_draft).references(:shared_draft) - else - result.where('topics.category_id != ?', drafts_category_id) + if can_create_shared + if options[:destination_category_id] + destination_category_id = get_category_id(options[:destination_category_id]) + topic_ids = SharedDraft.where(category_id: destination_category_id).pluck(:topic_id) + + return result.where(id: topic_ids) + end + + if viewing_shared + return result.includes(:shared_draft).references(:shared_draft) + end + + elsif viewing_shared + return result.joins('LEFT OUTER JOIN shared_drafts sd ON sd.topic_id = topics.id').where('sd.id IS NULL') end + + result.where('topics.category_id != ?', drafts_category_id) end def apply_ordering(result, options) diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb index f4f13b6a888..67ba06024f0 100644 --- a/spec/components/topic_query_spec.rb +++ b/spec/components/topic_query_spec.rb @@ -1257,6 +1257,7 @@ describe TopicQuery do shared_drafts_category.set_permissions(group => :full) shared_drafts_category.save SiteSetting.shared_drafts_category = shared_drafts_category.id + SiteSetting.shared_drafts_min_trust_level = TrustLevel[3] end context "destination_category_id" do @@ -1269,6 +1270,24 @@ describe TopicQuery do list = TopicQuery.new(admin, destination_category_id: category.id).list_latest expect(list.topics).to include(topic) end + + it 'allow group members with enough trust level to query destination_category_id' do + member = Fabricate(:user, trust_level: TrustLevel[3]) + group.add(member) + + list = TopicQuery.new(member, destination_category_id: category.id).list_latest + + expect(list.topics).to include(topic) + end + + it "doesn't allow group members without enough trust level to query destination_category_id" do + member = Fabricate(:user, trust_level: TrustLevel[2]) + group.add(member) + + list = TopicQuery.new(member, destination_category_id: category.id).list_latest + + expect(list.topics).not_to include(topic) + end end context "latest" do @@ -1287,6 +1306,14 @@ describe TopicQuery do list = TopicQuery.new(user).list_latest expect(list.topics).not_to include(topic) end + + it "doesn't include shared draft topics for group members with access to shared drafts" do + member = Fabricate(:user, trust_level: TrustLevel[3]) + group.add(member) + + list = TopicQuery.new(member).list_latest + expect(list.topics).not_to include(topic) + end end context "unread" do diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 36ab0ee0021..4dc874c3543 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -3605,6 +3605,11 @@ RSpec.describe TopicsController do expect(result.category_id).to eq(category.id) expect(result.visible).to eq(true) end + + it 'fails if the destination category is the shared drafts category' do + put "/t/#{topic.id}/publish.json", params: { destination_category_id: shared_drafts_category.id } + expect(response.status).to eq(400) + end end end end