From 460243d7a319781f1742ccb08c859943098910a5 Mon Sep 17 00:00:00 2001 From: Kane York Date: Fri, 11 Sep 2015 08:29:44 -0700 Subject: [PATCH 1/2] FIX: Give 403 for deleted topics, +lots of tests --- lib/topic_view.rb | 4 +- spec/controllers/topics_controller_spec.rb | 120 +++++++++++++++++++++ spec/spec_helper.rb | 1 - 3 files changed, 122 insertions(+), 3 deletions(-) diff --git a/lib/topic_view.rb b/lib/topic_view.rb index 54483efd5df..333e155b404 100644 --- a/lib/topic_view.rb +++ b/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 diff --git a/spec/controllers/topics_controller_spec.rb b/spec/controllers/topics_controller_spec.rb index fd927ebac3f..e87a27f4824 100644 --- a/spec/controllers/topics_controller_spec.rb +++ b/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 diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 63b3b45b02a..701c0de01b7 100644 --- a/spec/spec_helper.rb +++ b/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 From 7e50af75473658e83a00697f85e2e855c960646a Mon Sep 17 00:00:00 2001 From: Kane York Date: Fri, 11 Sep 2015 09:10:08 -0700 Subject: [PATCH 2/2] Temporarily lock eslint to 1.3.1 --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 62961abc4a2..a3571378a32 100644 --- a/.travis.yml +++ b/.travis.yml @@ -41,7 +41,7 @@ cache: before_install: - gem install bundler - - npm i -g eslint babel-eslint + - npm i -g eslint@1.3.1 babel-eslint - eslint app/assets/javascripts - eslint --ext .es6 app/assets/javascripts - eslint --ext .es6 test/javascripts