From 3f0c03a20cdb9da2d019489ca56099ad06be8285 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Mon, 30 Sep 2013 14:35:11 -0400 Subject: [PATCH] FIX: Prevent unauthorized list of private message titles. Also remove some unused code. --- app/controllers/list_controller.rb | 15 +++++-- app/controllers/user_actions_controller.rb | 10 +---- app/models/user_action.rb | 38 ----------------- spec/controllers/list_controller_spec.rb | 48 ++++++++++++++++++++++ spec/models/user_action_spec.rb | 16 -------- 5 files changed, 61 insertions(+), 66 deletions(-) diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb index ea6e6a8dc9e..7d0a6065b18 100644 --- a/app/controllers/list_controller.rb +++ b/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) diff --git a/app/controllers/user_actions_controller.rb b/app/controllers/user_actions_controller.rb index d7efb826e5e..9b617546071 100644 --- a/app/controllers/user_actions_controller.rb +++ b/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 diff --git a/app/models/user_action.rb b/app/models/user_action.rb index 06a43114ca5..a37f1e90e8d 100644 --- a/app/models/user_action.rb +++ b/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) diff --git a/spec/controllers/list_controller_spec.rb b/spec/controllers/list_controller_spec.rb index 2b5cd36b1df..e944637ec42 100644 --- a/spec/controllers/list_controller_spec.rb +++ b/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 diff --git a/spec/models/user_action_spec.rb b/spec/models/user_action_spec.rb index b4eb5783644..da75437e5ec 100644 --- a/spec/models/user_action_spec.rb +++ b/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