From fc97f7e0e73eb86df9701c92ab1a4059ae76d611 Mon Sep 17 00:00:00 2001
From: Sam Saffron <sam.saffron@gmail.com>
Date: Tue, 26 May 2020 10:07:00 +1000
Subject: [PATCH] FIX: properly ban non human users from draft system

Previously we had a partial fix in place where non human users
were not allowed draft sequences, this left edges around where non
human users asked for drafts yet had none.

For example system could already have a few drafts in place.

This also removes and extensibility point we added that is not in use
---
 app/models/draft.rb          |  5 +++++
 app/models/draft_sequence.rb | 18 +++++-------------
 app/models/user.rb           |  6 +++++-
 lib/plugin/instance.rb       |  9 ---------
 spec/models/draft_spec.rb    | 16 ++++++++++++++++
 5 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/app/models/draft.rb b/app/models/draft.rb
index a2fba62fe50..2165a0ad8a2 100644
--- a/app/models/draft.rb
+++ b/app/models/draft.rb
@@ -10,6 +10,8 @@ class Draft < ActiveRecord::Base
   class OutOfSequence < StandardError; end
 
   def self.set(user, key, sequence, data, owner = nil)
+    return 0 if !User.human_user_id?(user.id)
+
     if SiteSetting.backup_drafts_to_pm_length > 0 && SiteSetting.backup_drafts_to_pm_length < data.length
       backup_draft(user, key, sequence, data)
     end
@@ -94,6 +96,7 @@ class Draft < ActiveRecord::Base
   end
 
   def self.get(user, key, sequence)
+    return if !user || !user.id || !User.human_user_id?(user.id)
 
     opts = {
       user_id: user.id,
@@ -128,6 +131,8 @@ class Draft < ActiveRecord::Base
   end
 
   def self.clear(user, key, sequence)
+    return if !user || !user.id || !User.human_user_id?(user.id)
+
     current_sequence = DraftSequence.current(user, key)
 
     # bad caller is a reason to complain
diff --git a/app/models/draft_sequence.rb b/app/models/draft_sequence.rb
index 681a8bcb2ef..b79f3c92567 100644
--- a/app/models/draft_sequence.rb
+++ b/app/models/draft_sequence.rb
@@ -2,10 +2,12 @@
 
 class DraftSequence < ActiveRecord::Base
   def self.next!(user, key)
+    return nil if !user
+
     user_id = user
     user_id = user.id unless user.is_a?(Integer)
 
-    return 0 if invalid_user_id?(user_id)
+    return 0 if !User.human_user_id?(user_id)
 
     h = { user_id: user_id, draft_key: key }
     c = DraftSequence.find_by(h)
@@ -18,27 +20,17 @@ class DraftSequence < ActiveRecord::Base
   end
 
   def self.current(user, key)
-    return nil unless user
+    return nil if !user
 
     user_id = user
     user_id = user.id unless user.is_a?(Integer)
 
-    return 0 if invalid_user_id?(user_id)
+    return 0 if !User.human_user_id?(user_id)
 
     # perf critical path
     r, _ = DB.query_single('select sequence from draft_sequences where user_id = ? and draft_key = ?', user_id, key)
     r.to_i
   end
-
-  cattr_accessor :plugin_ignore_draft_sequence_callbacks
-  self.plugin_ignore_draft_sequence_callbacks = {}
-
-  def self.invalid_user_id?(user_id)
-    user_id < 0 || self.plugin_ignore_draft_sequence_callbacks.any? do |plugin, callback|
-      plugin.enabled? ? callback.call(user_id) : false
-    end
-  end
-  private_class_method :invalid_user_id?
 end
 
 # == Schema Information
diff --git a/app/models/user.rb b/app/models/user.rb
index d9678d1a1c7..0147c62b1bb 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -306,8 +306,12 @@ class User < ActiveRecord::Base
     fields.uniq
   end
 
+  def self.human_user_id?(user_id)
+    user_id > 0
+  end
+
   def human?
-    self.id > 0
+    User.human_user_id?(self.id)
   end
 
   def bot?
diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb
index ab48f7cbead..0390216ebe6 100644
--- a/lib/plugin/instance.rb
+++ b/lib/plugin/instance.rb
@@ -405,15 +405,6 @@ class Plugin::Instance
     SeedFu.fixture_paths.concat(paths)
   end
 
-  # Applies to all sites in a multisite environment. Block is not called if
-  # plugin is not enabled. Block is called with `user_id` and has to return a
-  # boolean based on whether the given `user_id` should be ignored.
-  def register_ignore_draft_sequence_callback(&block)
-    reloadable_patch do |plugin|
-      ::DraftSequence.plugin_ignore_draft_sequence_callbacks[plugin] = block
-    end
-  end
-
   def listen_for(event_name)
     return unless self.respond_to?(event_name)
     DiscourseEvent.on(event_name, &self.method(event_name))
diff --git a/spec/models/draft_spec.rb b/spec/models/draft_spec.rb
index b2066b048de..980f0795998 100644
--- a/spec/models/draft_spec.rb
+++ b/spec/models/draft_spec.rb
@@ -12,6 +12,22 @@ describe Draft do
     Fabricate(:post)
   end
 
+  context 'system user' do
+    it "can not set drafts" do
+      # fake a sequence
+      DraftSequence.create!(user_id: Discourse.system_user.id, draft_key: "abc", sequence: 10)
+
+      seq = Draft.set(Discourse.system_user, "abc", 0, { reply: 'hi' }.to_json)
+      expect(seq).to eq(0)
+
+      draft = Draft.get(Discourse.system_user, "abc", 0)
+      expect(draft).to eq(nil)
+
+      draft = Draft.get(Discourse.system_user, "abc", 1)
+      expect(draft).to eq(nil)
+    end
+  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