SECURITY: Respect topic permissions when loading draft metadata

Co-authored-by: Sam Saffron <sam.saffron@gmail.com>

app/controllers/drafts_controller.rb

@@ -23,17 +23,6 @@ class DraftsController < ApplicationController
     }
 
     stream = Draft.stream(opts)
-    stream.each do |d|
-      parsed_data = JSON.parse(d.data)
-      if parsed_data
-        if parsed_data['reply']
-          d.raw = parsed_data['reply']
-        end
-        if parsed_data['categoryId'].present? && !d.category_id.present?
-          d.category_id = parsed_data['categoryId']
-        end
-      end
-    end
 
     render json: {
       drafts: stream ? serialize_data(stream, DraftSerializer) : [],

app/models/draft.rb

@@ -5,6 +5,8 @@ class Draft < ActiveRecord::Base
   NEW_PRIVATE_MESSAGE ||= 'new_private_message'
   EXISTING_TOPIC ||= 'topic_'
 
+  belongs_to :user
+
   class OutOfSequence < StandardError; end
 
   def self.set(user, key, sequence, data, owner = nil)
@@ -137,6 +139,72 @@ class Draft < ActiveRecord::Base
     Draft.where(user_id: user.id, draft_key: key).destroy_all
   end
 
+  def display_user
+    post&.user || topic&.user || user
+  end
+
+  def parsed_data
+    JSON.parse(data)
+  end
+
+  def topic_id
+    if draft_key.starts_with?(EXISTING_TOPIC)
+      draft_key.gsub(EXISTING_TOPIC, "").to_i
+    end
+  end
+
+  def topic_preloaded?
+    !!defined?(@topic)
+  end
+
+  def topic
+    topic_preloaded? ? @topic : @topic = Draft.allowed_draft_topics_for_user(user).find_by(id: topic_id)
+  end
+
+  def preload_topic(topic)
+    @topic = topic
+  end
+
+  def post_id
+    parsed_data["postId"]
+  end
+
+  def post_preloaded?
+    !!defined?(@post)
+  end
+
+  def post
+    post_preloaded? ? @post : @post = Draft.allowed_draft_posts_for_user(user).find_by(id: post_id)
+  end
+
+  def preload_post(post)
+    @post = post
+  end
+
+  def self.preload_data(drafts, user)
+    topic_ids = drafts.map(&:topic_id)
+    post_ids = drafts.map(&:post_id)
+
+    topics = self.allowed_draft_topics_for_user(user).where(id: topic_ids)
+    posts = self.allowed_draft_posts_for_user(user).where(id: post_ids)
+
+    drafts.each do |draft|
+      draft.preload_topic(topics.detect { |t| t.id == draft.topic_id })
+      draft.preload_post(posts.detect { |p| p.id == draft.post_id })
+    end
+  end
+
+  def self.allowed_draft_topics_for_user(user)
+    topics = Topic.listable_topics.secured(Guardian.new(user))
+    pms = Topic.private_messages_for_user(user)
+    topics.or(pms)
+  end
+
+  def self.allowed_draft_posts_for_user(user)
+    # .secured handles whispers, merge handles topic/pm visibility
+    Post.secured(Guardian.new(user)).joins(:topic).merge(self.allowed_draft_topics_for_user(user))
+  end
+
   def self.stream(opts = nil)
     opts ||= {}
 
@@ -144,36 +212,15 @@ class Draft < ActiveRecord::Base
     offset = (opts[:offset] || 0).to_i
     limit = (opts[:limit] || 30).to_i
 
-    # JOIN of topics table based on manipulating draft_key seems imperfect
+    stream = Draft.where(user_id: user_id)
-    builder = DB.build <<~SQL
+      .order(updated_at: :desc)
-      SELECT
-        d.*, t.title, t.id topic_id, t.archetype,
-        t.category_id, t.closed topic_closed, t.archived topic_archived,
-        pu.username, pu.name, pu.id user_id, pu.uploaded_avatar_id, pu.username_lower,
-        du.username draft_username, NULL as raw, NULL as cooked, NULL as post_number
-      FROM drafts d
-      LEFT JOIN LATERAL json_extract_path_text (d.data::json, 'postId') postId ON TRUE
-      LEFT JOIN posts p ON postId :: BIGINT = p.id
-      LEFT JOIN topics t ON
-        CASE
-            WHEN d.draft_key LIKE '%' || '#{EXISTING_TOPIC}' || '%'
-              THEN CAST(replace(d.draft_key, '#{EXISTING_TOPIC}', '') AS INT)
-            ELSE 0
-        END = t.id
-      JOIN users pu on pu.id = COALESCE(p.user_id, t.user_id, d.user_id)
-      JOIN users du on du.id = #{user_id}
-      /*where*/
-      /*order_by*/
-      /*offset*/
-      /*limit*/
-    SQL
-
-    builder
-      .where('d.user_id = :user_id', user_id: user_id.to_i)
-      .order_by('d.updated_at desc')
       .offset(offset)
       .limit(limit)
-      .query
+
+    # Preload posts and topics to avoid N+1 queries
+    Draft.preload_data(stream, opts[:user])
+
+    stream
   end
 
   def self.cleanup!

app/models/post.rb

@@ -69,15 +69,7 @@ class Post < ActiveRecord::Base
   register_custom_field_type(MISSING_UPLOADS_IGNORED, :boolean)
 
   scope :private_posts_for_user, ->(user) {
-    where("posts.topic_id IN (SELECT topic_id
+    where("posts.topic_id IN (#{Topic::PRIVATE_MESSAGES_SQL})", user_id: user.id)
-             FROM topic_allowed_users
-             WHERE user_id = :user_id
-             UNION ALL
-             SELECT tg.topic_id
-             FROM topic_allowed_groups tg
-             JOIN group_users gu ON gu.user_id = :user_id AND
-                                      gu.group_id = tg.group_id)",
-                                              user_id: user.id)
   }
 
   scope :by_newest, -> { order('created_at DESC, id DESC') }

app/models/topic.rb

@@ -152,6 +152,20 @@ class Topic < ActiveRecord::Base
   # Return private message topics
   scope :private_messages, -> { where(archetype: Archetype.private_message) }
 
+  PRIVATE_MESSAGES_SQL = <<~SQL
+    SELECT topic_id
+    FROM topic_allowed_users
+    WHERE user_id = :user_id
+    UNION ALL
+    SELECT tg.topic_id
+    FROM topic_allowed_groups tg
+    JOIN group_users gu ON gu.user_id = :user_id AND gu.group_id = tg.group_id
+  SQL
+
+  scope :private_messages_for_user, ->(user) {
+    private_messages.where("topics.id IN (#{PRIVATE_MESSAGES_SQL})", user_id: user.id)
+  }
+
   scope :listable_topics, -> { where('topics.archetype <> ?', Archetype.private_message) }
 
   scope :by_newest, -> { order('topics.created_at desc, topics.id desc') }

app/serializers/draft_serializer.rb

@@ -23,36 +23,68 @@ class DraftSerializer < ApplicationSerializer
              :archetype,
              :archived
 
+  def cooked
+    object.parsed_data['reply'] || ""
+  end
+
+  def draft_username
+    object.user.username
+  end
+
   def avatar_template
-    User.avatar_template(object.username, object.uploaded_avatar_id)
+    object.user.avatar_template
+  end
+
+  def username
+    object.display_user&.username
+  end
+
+  def username_lower
+    object.display_user&.username_lower
+  end
+
+  def name
+    object.display_user&.name
+  end
+
+  def title
+    object.topic&.title
   end
 
   def slug
-    Slug.for(object.title)
+    object.topic&.slug
   end
 
-  def include_slug?
+  def category_id
-    object.title.present?
+    object.topic&.category_id
   end
 
   def closed
-    object.topic_closed
+    object.topic&.closed
   end
 
   def archived
-    object.topic_archived
+    object.topic&.archived
+  end
+
+  def archetype
+    object&.topic&.archetype
+  end
+
+  def include_slug?
+    object.topic&.title&.present?
   end
 
   def include_closed?
-    object.topic_closed.present?
+    object.topic&.closed&.present?
   end
 
   def include_archived?
-    object.topic_archived.present?
+    object.topic&.archived&.present?
   end
 
   def include_category_id?
-    object.category_id.present?
+    object.topic&.category_id&.present?
   end
 
 end

spec/models/draft_spec.rb

@@ -8,6 +8,10 @@ describe Draft do
     Fabricate(:user)
   end
 
+  fab!(:post) do
+    Fabricate(:post)
+  end
+
   context 'backup_drafts_to_pm_length' do
     it "correctly backs up drafts to a personal message" do
       SiteSetting.backup_drafts_to_pm_length = 1
@@ -111,7 +115,6 @@ describe Draft do
   end
 
   it 'can cleanup old drafts' do
-    user = Fabricate(:user)
     key = Draft::NEW_TOPIC
 
     Draft.set(user, key, 0, 'draft')
@@ -164,39 +167,35 @@ describe Draft do
     it "should include the right draft username in the stream" do
       Draft.set(user, "topic_#{public_topic.id}", 0, '{"reply":"hey"}')
       draft_row = stream.first
-      expect(draft_row.draft_username).to eq(user.username)
+      expect(draft_row.user.username).to eq(user.username)
     end
 
   end
 
   context 'key expiry' do
     it 'nukes new topic draft after a topic is created' do
-      u = Fabricate(:user)
+      Draft.set(user, Draft::NEW_TOPIC, 0, 'my draft')
-      Draft.set(u, Draft::NEW_TOPIC, 0, 'my draft')
+      _t = Fabricate(:topic, user: user)
-      _t = Fabricate(:topic, user: u)
+      s = DraftSequence.current(user, Draft::NEW_TOPIC)
-      s = DraftSequence.current(u, Draft::NEW_TOPIC)
+      expect(Draft.get(user, Draft::NEW_TOPIC, s)).to eq nil
-      expect(Draft.get(u, Draft::NEW_TOPIC, s)).to eq nil
       expect(Draft.count).to eq 0
     end
 
     it 'nukes new pm draft after a pm is created' do
-      u = Fabricate(:user)
+      Draft.set(user, Draft::NEW_PRIVATE_MESSAGE, 0, 'my draft')
-      Draft.set(u, Draft::NEW_PRIVATE_MESSAGE, 0, 'my draft')
+      t = Fabricate(:topic, user: user, archetype: Archetype.private_message, category_id: nil)
-      t = Fabricate(:topic, user: u, archetype: Archetype.private_message, category_id: nil)
       s = DraftSequence.current(t.user, Draft::NEW_PRIVATE_MESSAGE)
-      expect(Draft.get(u, Draft::NEW_PRIVATE_MESSAGE, s)).to eq nil
+      expect(Draft.get(user, Draft::NEW_PRIVATE_MESSAGE, s)).to eq nil
     end
 
     it 'does not nuke new topic draft after a pm is created' do
-      u = Fabricate(:user)
+      Draft.set(user, Draft::NEW_TOPIC, 0, 'my draft')
-      Draft.set(u, Draft::NEW_TOPIC, 0, 'my draft')
+      t = Fabricate(:topic, user: user, archetype: Archetype.private_message, category_id: nil)
-      t = Fabricate(:topic, user: u, archetype: Archetype.private_message, category_id: nil)
       s = DraftSequence.current(t.user, Draft::NEW_TOPIC)
-      expect(Draft.get(u, Draft::NEW_TOPIC, s)).to eq 'my draft'
+      expect(Draft.get(user, Draft::NEW_TOPIC, s)).to eq 'my draft'
     end
 
     it 'nukes the post draft when a post is created' do
-      user = Fabricate(:user)
       topic = Fabricate(:topic)
 
       Draft.set(user, topic.draft_key, 0, 'hello')
@@ -207,23 +206,20 @@ describe Draft do
     end
 
     it 'nukes the post draft when a post is revised' do
-      p = Fabricate(:post)
+      Draft.set(post.user, post.topic.draft_key, 0, 'hello')
-      Draft.set(p.user, p.topic.draft_key, 0, 'hello')
+      post.revise(post.user, raw: 'another test')
-      p.revise(p.user, raw: 'another test')
+      s = DraftSequence.current(post.user, post.topic.draft_key)
-      s = DraftSequence.current(p.user, p.topic.draft_key)
+      expect(Draft.get(post.user, post.topic.draft_key, s)).to eq nil
-      expect(Draft.get(p.user, p.topic.draft_key, s)).to eq nil
     end
 
     it 'increases revision each time you set' do
-      u = User.first
+      Draft.set(user, 'new_topic', 0, 'hello')
-      Draft.set(u, 'new_topic', 0, 'hello')
+      Draft.set(user, 'new_topic', 0, 'goodbye')
-      Draft.set(u, 'new_topic', 0, 'goodbye')
 
-      expect(Draft.find_by(user_id: u.id, draft_key: 'new_topic').revisions).to eq(2)
+      expect(Draft.find_by(user_id: user.id, draft_key: 'new_topic').revisions).to eq(2)
     end
 
     it 'handles owner switching gracefully' do
-      user = Fabricate(:user)
       draft_seq = Draft.set(user, 'new_topic', 0, 'hello', _owner = 'ABCDEF')
       expect(draft_seq).to eq(0)
 
@@ -233,5 +229,20 @@ describe Draft do
       draft_seq = Draft.set(user, 'new_topic', 1, 'hello world', _owner = 'HIJKL')
       expect(draft_seq).to eq(1)
     end
+
+    it 'can correctly preload drafts' do
+      Draft.set(user, "#{Draft::EXISTING_TOPIC}#{post.topic_id}", 0, { raw: 'hello', postId: post.id }.to_json)
+
+      drafts = Draft.where(user_id: user.id).to_a
+
+      Draft.preload_data(drafts, user)
+
+      expect(drafts[0].topic_preloaded?).to eq(true)
+      expect(drafts[0].topic.id).to eq(post.topic_id)
+
+      expect(drafts[0].post_preloaded?).to eq(true)
+      expect(drafts[0].post.id).to eq(post.id)
+    end
+
   end
 end

spec/requests/drafts_controller_spec.rb

@@ -34,4 +34,24 @@ describe DraftsController do
     get "/drafts.json", params: { username: user_b.username }
     expect(response.status).to eq(403)
   end
+
+  it 'does not include topic details when user cannot see topic' do
+    topic = Fabricate(:private_message_topic)
+    topic_user = topic.user
+    other_user = Fabricate(:user)
+    Draft.set(topic_user, "topic_#{topic.id}", 0, '{}')
+    Draft.set(other_user, "topic_#{topic.id}", 0, '{}')
+
+    sign_in(topic_user)
+    get "/drafts.json", params: { username: topic_user.username }
+    expect(response.status).to eq(200)
+    parsed = JSON.parse(response.body)
+    expect(parsed["drafts"].first["title"]).to eq(topic.title)
+
+    sign_in(other_user)
+    get "/drafts.json", params: { username: other_user.username }
+    expect(response.status).to eq(200)
+    parsed = JSON.parse(response.body)
+    expect(parsed["drafts"].first["title"]).to eq(nil)
+  end
 end