SECURITY: Restrict unlisted topic creation (#19258)

config/locales/server.en.yml

@@ -580,6 +580,7 @@ en:
               target_user_not_found: "One of the users you are sending this message to could not be found."
               unable_to_update: "There was an error updating that topic."
               unable_to_tag: "There was an error tagging the topic."
+              unable_to_unlist: "Sorry, you cannot create an unlisted topic."
             featured_link:
               invalid: "is invalid. URL should include http:// or https://."
         user:

lib/guardian/topic_guardian.rb

@@ -163,6 +163,8 @@ module TopicGuardian
     can_moderate?(topic) || can_perform_action_available_to_group_moderators?(topic)
   end
 
+  alias :can_create_unlisted_topic? :can_toggle_topic_visibility?
+
   def can_convert_topic?(topic)
     return false unless SiteSetting.enable_personal_messages?
     return false if topic.blank?

lib/topic_creator.rb

@@ -24,6 +24,8 @@ class TopicCreator
     # this allows us to add errors
     valid = topic.valid?
 
+    validate_visibility(topic)
+
     category = find_category
     if category.present? && guardian.can_tag?(topic)
       tags = @opts[:tags].presence || []
@@ -45,6 +47,8 @@ class TopicCreator
 
   def create
     topic = Topic.new(setup_topic_params)
+
+    validate_visibility!(topic)
     setup_tags(topic)
 
     if fields = @opts[:custom_fields]
@@ -66,6 +70,18 @@ class TopicCreator
 
   private
 
+  def validate_visibility(topic)
+    if !@opts[:skip_validations] && !topic.visible && !guardian.can_create_unlisted_topic?(topic)
+      topic.errors.add(:base, :unable_to_unlist)
+    end
+  end
+
+  def validate_visibility!(topic)
+    validate_visibility(topic)
+
+    rollback_from_errors!(topic) if topic.errors.full_messages.present?
+  end
+
   def create_shared_draft(topic)
     return if @opts[:shared_draft].blank? || @opts[:shared_draft] == 'false'
 
@@ -298,5 +314,4 @@ class TopicCreator
 
     user
   end
-
 end

spec/components/guardian/topic_guardian_spec.rb

@@ -130,6 +130,22 @@ RSpec.describe TopicGuardian do
     end
   end
 
+  describe "#can_create_unlisted_topic?" do
+    it "returns true for moderators" do
+      expect(Guardian.new(moderator).can_create_unlisted_topic?(topic)).to eq(true)
+    end
+
+    it "returns true for TL4 users" do
+      tl4_user = Fabricate(:user, trust_level: TrustLevel[4])
+
+      expect(Guardian.new(tl4_user).can_create_unlisted_topic?(topic)).to eq(true)
+    end
+
+    it "returns false for regular users" do
+      expect(Guardian.new(user).can_create_unlisted_topic?(topic)).to eq(false)
+    end
+  end
+
   # The test cases here are intentionally kept brief because majority of the cases are already handled by
   # `TopicGuardianCanSeeConsistencyCheck` which we run to ensure that the implementation between `TopicGuardian#can_see_topic_ids`
   # and `TopicGuardian#can_see_topic?` is consistent.

spec/components/post_creator_spec.rb

@@ -43,15 +43,25 @@ describe PostCreator do
       expect(post.wiki).to eq(true)
     end
 
-    it "can be created with a hidden reason" do
+    it "creates post with a hidden reason for staff user" do
       hri = Post.hidden_reasons[:flag_threshold_reached]
-      post = PostCreator.create(user, basic_topic_params.merge(hidden_reason_id: hri))
+      post = PostCreator.create(admin, basic_topic_params.merge(hidden_reason_id: hri))
       expect(post.hidden).to eq(true)
       expect(post.hidden_at).to be_present
       expect(post.hidden_reason_id).to eq(hri)
       expect(post.topic.visible).to eq(false)
     end
 
+    it "fails to create post with a hidden reason for non-staff user" do
+      hri = Post.hidden_reasons[:flag_threshold_reached]
+
+      expect do
+        post = PostCreator.create(user, basic_topic_params.merge(hidden_reason_id: hri))
+
+        expect(post).to be_nil
+      end.not_to change { Post.count }
+    end
+
     it "ensures the user can create the topic" do
       Guardian.any_instance.expects(:can_create?).with(Topic, nil).returns(false)
       expect { creator.create }.to raise_error(Discourse::InvalidAccess)

spec/components/topic_creator_spec.rb

@@ -286,5 +286,27 @@ describe TopicCreator do
         expect(topic.pinned_at).to eq_time(time2)
       end
     end
+
+    context "when invisible/unlisted" do
+      let(:unlisted_attrs) { valid_attrs.merge(visible: false) }
+
+      it "throws an exception for a non-staff user" do
+        expect do
+          TopicCreator.create(user, Guardian.new(user), unlisted_attrs)
+        end.to raise_error(ActiveRecord::Rollback)
+      end
+
+      it "is invalid for a non-staff user" do
+        expect(TopicCreator.new(user, Guardian.new(user), unlisted_attrs).valid?).to eq(false)
+      end
+
+      it "creates unlisted topic for an admin" do
+        expect(TopicCreator.create(admin, Guardian.new(admin), unlisted_attrs)).to be_valid
+      end
+
+      it "is valid for an admin" do
+        expect(TopicCreator.new(admin, Guardian.new(admin), unlisted_attrs).valid?).to eq(true)
+      end
+    end
   end
 end

spec/requests/posts_controller_spec.rb

@@ -850,6 +850,34 @@ describe PostsController do
 
         expect(response.status).to eq(422)
       end
+
+      it "creates unlisted topic with admin master key" do
+        master_key = Fabricate(:api_key).key
+
+        expect do
+          post "/posts.json",
+            params: { raw: "this is a test title", title: "this is test body", unlist_topic: true },
+            headers: { HTTP_API_USERNAME: admin.username, HTTP_API_KEY: master_key }
+        end.to change { Topic.count }.by(1)
+
+        expect(response.status).to eq(200)
+        expect(Topic.find(response.parsed_body["topic_id"]).visible).to eq(false)
+      end
+
+      it "prevents creation of unlisted topic with non-admin key" do
+        user_key = ApiKey.create!(user: user).key
+
+        expect do
+          post "/posts.json",
+            params: { raw: "this is a test title", title: "this is test body", unlist_topic: true },
+            headers: { HTTP_API_USERNAME: user.username, HTTP_API_KEY: user_key }
+        end.not_to change { Topic.count }
+
+        expect(response.status).to eq(422)
+        expect(response.parsed_body["errors"]).to include(
+          I18n.t("activerecord.errors.models.topic.attributes.base.unable_to_unlist")
+        )
+      end
     end
 
     describe "when logged in" do
@@ -1173,6 +1201,7 @@ describe PostsController do
         expect(topic.title).to eq('This is the test title for the topic')
         expect(topic.category).to eq(category)
         expect(topic.meta_data).to eq("xyz" => 'abc')
+        expect(topic.visible).to eq(true)
       end
 
       it 'can create an uncategorized topic' do
@@ -1333,6 +1362,48 @@ describe PostsController do
       end
     end
 
+    context "with topic unlisting" do
+      context "when logged in as staff" do
+        before do
+          sign_in(admin)
+        end
+
+        it "creates an unlisted topic" do
+          expect do
+            post "/posts.json", params: {
+              raw: "this is the test content",
+              title: "this is the test title for the topic",
+              unlist_topic: true
+            }
+          end.to change { Topic.count }.by(1)
+
+          expect(response.status).to eq(200)
+          expect(Topic.find(response.parsed_body["topic_id"]).visible).to eq(false)
+        end
+      end
+
+      context "when logged in as a non-staff user" do
+        before do
+          sign_in(user)
+        end
+
+        it "prevents creation of an unlisted topic" do
+          expect do
+            post "/posts.json", params: {
+              raw: "this is the test content",
+              title: "this is the test title for the topic",
+              unlist_topic: true
+            }
+          end.not_to change { Topic.count }
+
+          expect(response.status).to eq(422)
+          expect(response.parsed_body["errors"]).to include(
+            I18n.t("activerecord.errors.models.topic.attributes.base.unable_to_unlist")
+          )
+        end
+      end
+    end
+
     describe 'shared draft' do
       fab!(:destination_category) { Fabricate(:category) }