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.

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")

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,

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

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!
 

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

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)

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)

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)

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

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