FIX: Give 403 for deleted topics, +lots of tests

lib/topic_view.rb

@@ -355,8 +355,8 @@ class TopicView
   end
 
   def find_topic(topic_id)
-    finder = Topic.where(id: topic_id).includes(:category)
-    finder = finder.with_deleted if @guardian.can_see_deleted_topics?
+    # with_deleted covered in #check_and_raise_exceptions
+    finder = Topic.with_deleted.where(id: topic_id).includes(:category)
     finder.first
   end
 

spec/controllers/topics_controller_spec.rb

@@ -1,5 +1,23 @@
 require 'spec_helper'
 
+def topics_controller_show_gen_perm_tests(expected, ctx)
+  expected.each do |sym, status|
+    params = "topic_id: #{sym}.id, slug: #{sym}.slug"
+    if sym == :nonexist
+      params = "topic_id: nonexist_topic_id"
+    end
+    ctx.instance_eval("
+it 'returns #{status} for #{sym}' do
+  begin
+    xhr :get, :show, #{params}
+    expect(response.status).to eq(#{status})
+  rescue Discourse::NotLoggedIn
+    expect(302).to eq(#{status})
+  end
+end")
+  end
+end
+
 describe TopicsController do
 
   context 'wordpress' do
@@ -554,6 +572,108 @@ describe TopicsController do
       end
     end
 
+    context 'permission errors' do
+      let(:allowed_user) { Fabricate(:user) }
+      let(:allowed_group) { Fabricate(:group) }
+      let(:secure_category) {
+        c = Fabricate(:category)
+        c.permissions = [[allowed_group, :full]]
+        c.save
+        allowed_user.groups = [allowed_group]
+        allowed_user.save
+        c }
+      let(:normal_topic) { Fabricate(:topic) }
+      let(:secure_topic) { Fabricate(:topic, category: secure_category) }
+      let(:private_topic) { Fabricate(:private_message_topic, user: allowed_user) }
+      let(:deleted_topic) { Fabricate(:deleted_topic) }
+      let(:nonexist_topic_id) { Topic.last.id + 10000 }
+
+      context 'anonymous' do
+        expected = {
+          :normal_topic => 200,
+          :secure_topic => 403,
+          :private_topic => 302,
+          :deleted_topic => 403,
+          :nonexist => 404
+        }
+        topics_controller_show_gen_perm_tests(expected, self)
+      end
+
+      context 'anonymous with login required' do
+        before do
+          SiteSetting.login_required = true
+        end
+        expected = {
+          :normal_topic => 302,
+          :secure_topic => 302,
+          :private_topic => 302,
+          :deleted_topic => 302,
+          :nonexist => 302
+        }
+        topics_controller_show_gen_perm_tests(expected, self)
+      end
+
+      context 'normal user' do
+        before do
+          log_in(:user)
+        end
+
+        expected = {
+          :normal_topic => 200,
+          :secure_topic => 403,
+          :private_topic => 403,
+          :deleted_topic => 403,
+          :nonexist => 404
+        }
+        topics_controller_show_gen_perm_tests(expected, self)
+      end
+
+      context 'allowed user' do
+        before do
+          log_in_user(allowed_user)
+        end
+
+        expected = {
+          :normal_topic => 200,
+          :secure_topic => 200,
+          :private_topic => 200,
+          :deleted_topic => 403,
+          :nonexist => 404
+        }
+        topics_controller_show_gen_perm_tests(expected, self)
+      end
+
+      context 'moderator' do
+        before do
+          log_in(:moderator)
+        end
+
+        expected = {
+          :normal_topic => 200,
+          :secure_topic => 403,
+          :private_topic => 403,
+          :deleted_topic => 200,
+          :nonexist => 404
+        }
+        topics_controller_show_gen_perm_tests(expected, self)
+      end
+
+      context 'admin' do
+        before do
+          log_in(:admin)
+        end
+
+        expected = {
+          :normal_topic => 200,
+          :secure_topic => 200,
+          :private_topic => 200,
+          :deleted_topic => 200,
+          :nonexist => 404
+        }
+        topics_controller_show_gen_perm_tests(expected, self)
+      end
+    end
+
     it 'records a view' do
       expect { xhr :get, :show, topic_id: topic.id, slug: topic.slug }.to change(TopicViewItem, :count).by(1)
     end

spec/spec_helper.rb

@@ -144,7 +144,6 @@ Spork.prefork do
     FileUtils.cp("#{Rails.root}/spec/fixtures/images/#{filename}", "#{Rails.root}/tmp/spec/#{filename}")
     File.new("#{Rails.root}/tmp/spec/#{filename}")
   end
-
 end
 
 Spork.each_run do