SECURITY: Users can only bookmark posts which they can see.

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

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

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
+    if !guardian.can_see_private_messages?(user_id) || ignore_private_messages || !guardian.user
-      builder.where("t.archetype != :archetype", archetype: Archetype::private_message)
+      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?

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?
 

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
 

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)
+        @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
-
-      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)
         xhr :post, :create, id: @post.id, post_action_type_id: PostActionType.types[:like]
         expect(response).to be_forbidden
       end

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
+        post
-
+        xhr :put, :bookmark, post_id: private_message.id, bookmarked: 'true'
-        xhr :put, :bookmark, post_id: post.id, bookmarked: 'true'
         expect(response).to be_forbidden
       end
 

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
 
 

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
+    def count_bookmarks
-      PostCreator.create(user, raw: 'oops I forgot to mention this', topic_id: private_message.topic_id)
+      UserAction.stream(
+        user_id: user.id,
+        action_types: [UserAction::BOOKMARK],
+        ignore_private_messages: false,
+        guardian: Guardian.new(user)
+      ).count
     end
 
-    let!(:private_message2) do
+    it 'correctly secures stream' do
-      PostCreator.create( target_user,
+      PostAction.act(user, private_message, PostActionType.types[:bookmark])
-                          raw: 'this is a private message',
+
-                          title: 'this is the pm title',
+      expect(count_bookmarks).to eq(1)
-                          target_usernames: user.username,
+
-                          archetype: Archetype::private_message
+      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