FIX: Prevent unauthorized list of private message titles. Also remove some unused code.

app/controllers/list_controller.rb

@@ -40,7 +40,10 @@ class ListController < ApplicationController
 
   def private_messages
     list_opts = build_topic_list_options
-    list = TopicQuery.new(current_user, list_opts).list_private_messages(fetch_user_from_params)
+    target_user = fetch_user_from_params
+    guardian.ensure_can_see_private_messages!(target_user.id)
+
+    list = TopicQuery.new(current_user, list_opts).list_private_messages(target_user)
     list.more_topics_url = url_for(topics_private_messages_path(list_opts.merge(format: 'json', page: next_page)))
 
     respond(list)
@@ -48,7 +51,10 @@ class ListController < ApplicationController
 
   def private_messages_sent
     list_opts = build_topic_list_options
-    list = TopicQuery.new(current_user, list_opts).list_private_messages_sent(fetch_user_from_params)
+    target_user = fetch_user_from_params
+    guardian.ensure_can_see_private_messages!(target_user.id)
+
+    list = TopicQuery.new(current_user, list_opts).list_private_messages_sent(target_user)
     list.more_topics_url = url_for(topics_private_messages_sent_path(list_opts.merge(format: 'json', page: next_page)))
 
     respond(list)
@@ -56,7 +62,10 @@ class ListController < ApplicationController
 
   def private_messages_unread
     list_opts = build_topic_list_options
-    list = TopicQuery.new(current_user, list_opts).list_private_messages_unread(fetch_user_from_params)
+    target_user = fetch_user_from_params
+    guardian.ensure_can_see_private_messages!(target_user.id)
+
+    list = TopicQuery.new(current_user, list_opts).list_private_messages_unread(target_user)
     list.more_topics_url = url_for(topics_private_messages_unread_path(list_opts.merge(format: 'json', page: next_page)))
 
     respond(list)

app/controllers/user_actions_controller.rb

@@ -16,15 +16,7 @@ class UserActionsController < ApplicationController
       ignore_private_messages: params[:filter] ? false : true
     }
 
-    stream =
-      if opts[:action_types] == [UserAction::GOT_PRIVATE_MESSAGE] ||
-         opts[:action_types] == [UserAction::NEW_PRIVATE_MESSAGE]
-        UserAction.private_message_stream(opts[:action_types][0], opts)
-      else
-        UserAction.stream(opts)
-      end
-
-    render_serialized(stream, UserActionSerializer, root: "user_actions")
+    render_serialized(UserAction.stream(opts), UserActionSerializer, root: "user_actions")
   end
 
   def show

app/models/user_action.rb

@@ -128,44 +128,6 @@ LEFT JOIN categories c on c.id = t.category_id
     builder.map_exec(UserActionRow)
   end
 
-  # slightly different to standard stream, it collapses replies
-  def self.private_message_stream(action_type, opts)
-
-    user_id = opts[:user_id]
-    return [] unless opts[:guardian].can_see_private_messages?(user_id)
-
-    builder = SqlBuilder.new("
-SELECT
-  t.title, :action_type action_type, p.created_at, t.id topic_id,
-  :user_id AS target_user_id, au.name AS target_name, au.username AS target_username,
-  coalesce(p.post_number, 1) post_number,
-  p.reply_to_post_number,
-  pu.email, pu.username, pu.name, pu.id user_id,
-  pu.use_uploaded_avatar, pu.uploaded_avatar_template, pu.uploaded_avatar_id,
-  pu.email acting_email, pu.username acting_username, pu.name acting_name, pu.id acting_user_id,
-  pu.use_uploaded_avatar acting_use_uploaded_avatar, pu.uploaded_avatar_template acting_uploaded_avatar_template, pu.uploaded_avatar_id acting_uploaded_avatar_id,
-  p.cooked,
-  CASE WHEN coalesce(p.deleted_at, t.deleted_at) IS NULL THEN false ELSE true END deleted,
-  p.hidden,
-  p.post_type
-FROM topics t
-JOIN posts p ON p.topic_id =  t.id and p.post_number = t.highest_post_number
-JOIN users pu ON pu.id = p.user_id
-JOIN users au ON au.id = :user_id
-WHERE archetype = 'private_message' and EXISTS (
-   select 1 from user_actions a where a.user_id = :user_id and a.target_topic_id = t.id and action_type = :action_type)
-ORDER BY p.created_at desc
-
-/*offset*/
-/*limit*/
-")
-
-    builder
-      .offset((opts[:offset] || 0).to_i)
-      .limit((opts[:limit] || 60).to_i)
-      .map_exec(UserActionRow, user_id: user_id, action_type: action_type)
-  end
-
   def self.log_action!(hash)
     required_parameters = [:action_type, :user_id, :acting_user_id, :target_topic_id, :target_post_id]
     require_parameters(hash, *required_parameters)

spec/controllers/list_controller_spec.rb

@@ -119,6 +119,54 @@ describe ListController do
 
   end
 
+  context "private_messages" do
+    let!(:user) { log_in }
+
+    it "raises an error when can_see_private_messages? is false " do
+      Guardian.any_instance.expects(:can_see_private_messages?).returns(false)
+      xhr :get, :private_messages, username: @user.username
+      response.should be_forbidden
+    end
+
+    it "succeeds when can_see_private_messages? is false " do
+      Guardian.any_instance.expects(:can_see_private_messages?).returns(true)
+      xhr :get, :private_messages, username: @user.username
+      response.should be_success
+    end
+  end
+
+  context "private_messages_sent" do
+    let!(:user) { log_in }
+
+    it "raises an error when can_see_private_messages? is false " do
+      Guardian.any_instance.expects(:can_see_private_messages?).returns(false)
+      xhr :get, :private_messages_sent, username: @user.username
+      response.should be_forbidden
+    end
+
+    it "succeeds when can_see_private_messages? is false " do
+      Guardian.any_instance.expects(:can_see_private_messages?).returns(true)
+      xhr :get, :private_messages_sent, username: @user.username
+      response.should be_success
+    end
+  end
+
+  context "private_messages_unread" do
+    let!(:user) { log_in }
+
+    it "raises an error when can_see_private_messages? is false " do
+      Guardian.any_instance.expects(:can_see_private_messages?).returns(false)
+      xhr :get, :private_messages_unread, username: @user.username
+      response.should be_forbidden
+    end
+
+    it "succeeds when can_see_private_messages? is false " do
+      Guardian.any_instance.expects(:can_see_private_messages?).returns(true)
+      xhr :get, :private_messages_unread, username: @user.username
+      response.should be_success
+    end
+  end
+
   context 'hot' do
     before do
       xhr :get, :hot

spec/models/user_action_spec.rb

@@ -243,22 +243,6 @@ describe UserAction do
                         )
     end
 
-    it 'should collapse the inbox correctly' do
-
-      stream = UserAction.private_message_stream(UserAction::GOT_PRIVATE_MESSAGE, user_id: target_user.id, guardian: Guardian.new(target_user))
-      # inbox should collapse this initial and reply message into one item
-      stream.count.should == 1
-
-
-      # outbox should also collapse
-      stream = UserAction.private_message_stream(UserAction::NEW_PRIVATE_MESSAGE, user_id: user.id, guardian: Guardian.new(user))
-      stream.count.should == 1
-
-      # anon should see nothing
-      stream = UserAction.private_message_stream(UserAction::NEW_PRIVATE_MESSAGE, user_id: user.id, guardian: Guardian.new(nil))
-      stream.count.should == 0
-
-    end
   end
 
   describe 'synchronize_favorites' do