diff --git a/app/controllers/post_actions_controller.rb b/app/controllers/post_actions_controller.rb index 1c478504c36..f6d6399b92c 100644 --- a/app/controllers/post_actions_controller.rb +++ b/app/controllers/post_actions_controller.rb @@ -7,8 +7,13 @@ class PostActionsController < ApplicationController def create taken = PostAction.counts_for([@post], current_user)[@post.id] - guardian.ensure_post_can_act!(@post, PostActionType.types[@post_action_type_id], taken_actions: taken) - guardian.ensure_post_can_act!(@post, PostActionType.types[@post_action_type_id], is_warning: params[:is_warning]) + + guardian.ensure_post_can_act!( + @post, + PostActionType.types[@post_action_type_id], + is_warning: params[:is_warning], + taken_actions: taken + ) args = {} args[:message] = params[:message] if params[:message].present? @@ -77,7 +82,6 @@ class PostActionsController < ApplicationController finder = finder.with_deleted if guardian.is_staff? @post = finder.first - guardian.ensure_can_see!(@post) end def fetch_post_action_type_id_from_params diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 90f883f9646..cad333b5165 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -330,6 +330,8 @@ class TopicsController < ApplicationController topic = Topic.find(params[:topic_id].to_i) first_post = topic.ordered_posts.first + guardian.ensure_can_see!(first_post) + PostAction.act(current_user, first_post, PostActionType.types[:bookmark]) render nothing: true diff --git a/app/models/user_action.rb b/app/models/user_action.rb index 23678754cfc..2a5e36ec3ab 100644 --- a/app/models/user_action.rb +++ b/app/models/user_action.rb @@ -350,8 +350,24 @@ SQL builder.where('a.action_type <> :pending', pending: UserAction::PENDING) end - if !guardian.can_see_private_messages?(user_id) || ignore_private_messages - builder.where("t.archetype != :archetype", archetype: Archetype::private_message) + if !guardian.can_see_private_messages?(user_id) || ignore_private_messages || !guardian.user + builder.where("t.archetype <> :private_message", private_message: Archetype::private_message) + else + unless guardian.is_admin? + sql = <<~SQL + t.archetype <> :private_message OR + EXISTS ( + SELECT 1 FROM topic_allowed_users tu WHERE tu.topic_id = t.id AND tu.user_id = :current_user_id + ) OR + EXISTS ( + SELECT 1 FROM topic_allowed_groups tg WHERE tg.topic_id = t.id AND tg.group_id IN ( + SELECT group_id FROM group_users gu WHERE gu.user_id = :current_user_id + ) + ) + SQL + + builder.where(sql, private_message: Archetype::private_message, current_user_id: guardian.user.id) + end end unless guardian.is_admin? diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index c0f4590618c..996ee027ccc 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -4,6 +4,12 @@ module PostGuardian # Can the user act on the post in a particular way. # taken_actions = the list of actions the user has already taken def post_can_act?(post, action_key, opts={}) + + return false unless can_see_post?(post) + + # no warnings except for staff + return false if (action_key == :notify_user && !is_staff? && opts[:is_warning].present? && opts[:is_warning] == 'true') + taken = opts[:taken_actions].try(:keys).to_a is_flag = PostActionType.is_flag?(action_key) already_taken_this_action = taken.any? && taken.include?(PostActionType.types[action_key]) @@ -32,9 +38,6 @@ module PostGuardian # new users can't notify_user because they are not allowed to send private messages not(action_key == :notify_user && !@user.has_trust_level?(SiteSetting.min_trust_to_send_messages)) && - # non-staff can't send an official warning - not(action_key == :notify_user && !is_staff? && opts[:is_warning].present? && opts[:is_warning] == 'true') && - # can't send private messages if they're disabled globally not(action_key == :notify_user && !SiteSetting.enable_private_messages) && @@ -46,7 +49,7 @@ module PostGuardian end def can_defer_flags?(post) - is_staff? && post + can_see_post?(post) && is_staff? && post end # Can we see who acted on a post in a particular way? @@ -120,6 +123,8 @@ module PostGuardian # Deleting Methods def can_delete_post?(post) + can_see_post?(post) + # Can't delete the first post return false if post.is_first_post? diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index 57010a03382..b03d9ce088a 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -90,8 +90,9 @@ describe Guardian do end it "returns true for a new user flagging a private message as spam" do - post.topic.archetype = Archetype.private_message + post = Fabricate(:private_message_post, user: Fabricate(:admin)) user.trust_level = TrustLevel[0] + post.topic.allowed_users << user expect(Guardian.new(user).post_can_act?(post, :spam)).to be_truthy end diff --git a/spec/controllers/post_actions_controller_spec.rb b/spec/controllers/post_actions_controller_spec.rb index a9780ac2838..336300072d7 100644 --- a/spec/controllers/post_actions_controller_spec.rb +++ b/spec/controllers/post_actions_controller_spec.rb @@ -7,6 +7,20 @@ describe PostActionsController do expect { xhr :post, :create }.to raise_error(Discourse::NotLoggedIn) end + context 'logged in as user' do + let(:user) { Fabricate(:user) } + let(:private_message) { Fabricate(:private_message_post, user: Fabricate(:coding_horror)) } + + before do + log_in_user(user) + end + + it 'fails when the user does not have permission to see the post' do + xhr :post, :create, id: private_message.id, post_action_type_id: PostActionType.types[:bookmark] + expect(response).to be_forbidden + end + end + describe 'logged in as moderator' do before do @user = log_in(:moderator) @@ -22,13 +36,7 @@ describe PostActionsController do end it "fails when the user doesn't have permission to see the post" do - Guardian.any_instance.expects(:can_see?).with(@post).returns(false) - xhr :post, :create, id: @post.id, post_action_type_id: PostActionType.types[:like] - expect(response).to be_forbidden - end - - it "fails when the user doesn't have permission to perform that action" do - Guardian.any_instance.expects(:post_can_act?).with(@post, :like, taken_actions: nil).returns(false) + @post = Fabricate(:private_message_post, user: Fabricate(:user)) xhr :post, :create, id: @post.id, post_action_type_id: PostActionType.types[:like] expect(response).to be_forbidden end diff --git a/spec/controllers/posts_controller_spec.rb b/spec/controllers/posts_controller_spec.rb index fa904ba1283..e1f686d7dae 100644 --- a/spec/controllers/posts_controller_spec.rb +++ b/spec/controllers/posts_controller_spec.rb @@ -426,13 +426,12 @@ describe PostsController do include_examples 'action requires login', :put, :bookmark, post_id: 2 describe 'when logged in' do - let(:post) { Fabricate(:post, user: log_in) } + let(:private_message) { Fabricate(:private_message_post) } it "raises an error if the user doesn't have permission to see the post" do - Guardian.any_instance.expects(:can_see?).with(post).returns(false).once - - xhr :put, :bookmark, post_id: post.id, bookmarked: 'true' + post + xhr :put, :bookmark, post_id: private_message.id, bookmarked: 'true' expect(response).to be_forbidden end diff --git a/spec/controllers/topics_controller_spec.rb b/spec/controllers/topics_controller_spec.rb index ecfd606346e..5a7a51355d6 100644 --- a/spec/controllers/topics_controller_spec.rb +++ b/spec/controllers/topics_controller_spec.rb @@ -1224,6 +1224,15 @@ describe TopicsController do xhr :put, :remove_bookmarks, topic_id: post.topic_id expect(PostAction.where(user_id: user.id, post_action_type: bookmark).count).to eq(0) end + + it "should disallow bookmarks on posts you have no access to" do + log_in + user = Fabricate(:user) + pm = create_post(user: user, archetype: 'private_message', target_usernames: [user.username]) + + xhr :put, :bookmark, topic_id: pm.topic_id + expect(response).to be_forbidden + end end diff --git a/spec/models/user_action_spec.rb b/spec/models/user_action_spec.rb index 9e2ec982d06..79b7b4e5c89 100644 --- a/spec/models/user_action_spec.rb +++ b/spec/models/user_action_spec.rb @@ -19,6 +19,7 @@ describe UserAction do let(:private_topic) do topic = private_post.topic topic.update_columns(category_id: nil, archetype: Archetype::private_message) + TopicAllowedUser.create(topic_id: topic.id, user_id: user.id) topic end @@ -223,13 +224,13 @@ describe UserAction do end end - describe 'private messages' do + describe 'secures private messages' do let(:user) do Fabricate(:user) end - let(:target_user) do + let(:user2) do Fabricate(:user) end @@ -237,22 +238,35 @@ describe UserAction do PostCreator.create( user, raw: 'this is a private message', title: 'this is the pm title', - target_usernames: target_user.username, + target_usernames: user2.username, archetype: Archetype::private_message ) end - let!(:response) do - PostCreator.create(user, raw: 'oops I forgot to mention this', topic_id: private_message.topic_id) + def count_bookmarks + UserAction.stream( + user_id: user.id, + action_types: [UserAction::BOOKMARK], + ignore_private_messages: false, + guardian: Guardian.new(user) + ).count end - let!(:private_message2) do - PostCreator.create( target_user, - raw: 'this is a private message', - title: 'this is the pm title', - target_usernames: user.username, - archetype: Archetype::private_message - ) + it 'correctly secures stream' do + PostAction.act(user, private_message, PostActionType.types[:bookmark]) + + expect(count_bookmarks).to eq(1) + + private_message.topic.topic_allowed_users.where(user_id: user.id).destroy_all + + expect(count_bookmarks).to eq(0) + + group = Fabricate(:group) + group.add(user) + private_message.topic.topic_allowed_groups.create(group_id: group.id) + + expect(count_bookmarks).to eq(1) + end end