From 870e59883b2fb4b85728e3ecd9bba3cf66bf9654 Mon Sep 17 00:00:00 2001 From: Sam Date: Wed, 5 Jun 2013 16:10:26 +1000 Subject: [PATCH] secure the links on the topic pages, eliminated deleted topics as well. --- app/controllers/posts_controller.rb | 2 +- app/models/category.rb | 4 +- app/models/topic.rb | 18 --------- app/models/topic_link.rb | 60 ++++++++++++++++++++++++++++ app/models/topic_link_click.rb | 19 --------- lib/sql_builder.rb | 9 +++++ lib/topic_view.rb | 12 +++--- spec/models/topic_link_click_spec.rb | 25 ------------ spec/models/topic_link_spec.rb | 58 +++++++++++++++++++++++++++ 9 files changed, 137 insertions(+), 70 deletions(-) diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index f3800fe476a..c64631a0ecc 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -90,7 +90,7 @@ class PostsController < ApplicationController post_serializer = PostSerializer.new(post, scope: guardian, root: false) post_serializer.draft_sequence = DraftSequence.current(current_user, post.topic.draft_key) - link_counts = TopicLinkClick.counts_for(post.topic, [post]) + link_counts = TopicLink.counts_for(guardian,post.topic, [post]) post_serializer.single_post_link_counts = link_counts[post.id] if link_counts.present? post_serializer.topic_slug = post.topic.slug if post.topic.present? diff --git a/app/models/category.rb b/app/models/category.rb index ba41f20ffa3..85943642c8d 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -42,6 +42,8 @@ class Category < ActiveRecord::Base delegate :post_template, to: 'self.class' + attr_accessor :displayable_topics + # Internal: Update category stats: # of topics in past year, month, week for # all categories. def self.update_stats @@ -62,8 +64,6 @@ class Category < ActiveRecord::Base topics_week = (#{topics_week})") end - attr_accessor :displayable_topics - # Internal: Generate the text of post prompting to enter category # description. def self.post_template diff --git a/app/models/topic.rb b/app/models/topic.rb index 139b2850adb..026b818b18d 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -209,8 +209,6 @@ class Topic < ActiveRecord::Base meta_data[key.to_s] end - - def self.listable_count_per_day(sinceDaysAgo=30) listable_topics.where('created_at > ?', sinceDaysAgo.days.ago).group('date(created_at)').order('date(created_at)').count end @@ -219,22 +217,6 @@ class Topic < ActiveRecord::Base archetype == Archetype.private_message end - def links_grouped - exec_sql("SELECT ftl.url, - ft.title, - ftl.link_topic_id, - ftl.reflection, - ftl.internal, - MIN(ftl.user_id) AS user_id, - SUM(clicks) AS clicks - FROM topic_links AS ftl - LEFT OUTER JOIN topics AS ft ON ftl.link_topic_id = ft.id - WHERE ftl.topic_id = ? - GROUP BY ftl.url, ft.title, ftl.link_topic_id, ftl.reflection, ftl.internal - ORDER BY clicks DESC", - id).to_a - end - # Search for similar topics def self.similar_to(title, raw) return [] unless title.present? diff --git a/app/models/topic_link.rb b/app/models/topic_link.rb index cfd9a9797e7..56d45da881a 100644 --- a/app/models/topic_link.rb +++ b/app/models/topic_link.rb @@ -22,6 +22,66 @@ class TopicLink < ActiveRecord::Base errors.add(:base, "can't link to the same topic") if (topic_id == link_topic_id) end + def self.topic_summary(guardian, topic_id) + + # Sam: complicated reports are really hard in AR + builder = SqlBuilder.new("SELECT ftl.url, + ft.title, + ftl.link_topic_id, + ftl.reflection, + ftl.internal, + MIN(ftl.user_id) AS user_id, + SUM(clicks) AS clicks + FROM topic_links AS ftl + LEFT JOIN topics AS ft ON ftl.link_topic_id = ft.id + LEFT JOIN categories AS c ON c.id = ft.category_id + /*where*/ + GROUP BY ftl.url, ft.title, ftl.link_topic_id, ftl.reflection, ftl.internal + ORDER BY clicks DESC") + + builder.where('ftl.topic_id = :topic_id', topic_id: topic_id) + builder.where('ft.deleted_at IS NULL') + + builder.secure_category(guardian.secure_category_ids) + + builder.exec.to_a + + end + + def self.counts_for(guardian,topic, posts) + return {} if posts.blank? + + # Sam: I don't know how to write this cleanly in AR, + # in particular the securing logic is tricky and would fallback to SQL anyway + builder = SqlBuilder.new("SELECT + l.post_id, + l.url, + l.clicks, + t.title, + l.internal, + l.reflection + FROM topic_links l + LEFT JOIN topics t ON t.id = l.link_topic_id + LEFT JOIN categories AS c ON c.id = t.category_id + /*where*/ + ORDER BY reflection ASC, clicks DESC") + + builder.where('t.deleted_at IS NULL') + + # not certain if pluck is right, cause it may interfere with caching + builder.where('l.post_id IN (:post_ids)', post_ids: posts.map(&:id)) + builder.secure_category(guardian.secure_category_ids) + + builder.map_exec(OpenStruct).each_with_object({}) do |l,result| + result[l.post_id] ||= [] + result[l.post_id] << {url: l.url, + clicks: l.clicks, + title: l.title, + internal: l.internal, + reflection: l.reflection} + end + end + # Extract any urls in body def self.extract_from(post) return unless post.present? diff --git a/app/models/topic_link_click.rb b/app/models/topic_link_click.rb index e5b7d2b17fa..5604505258f 100644 --- a/app/models/topic_link_click.rb +++ b/app/models/topic_link_click.rb @@ -34,25 +34,6 @@ class TopicLinkClick < ActiveRecord::Base args[:url] end - def self.counts_for(topic, posts) - return {} if posts.blank? - links = TopicLink - .includes(:link_topic) - .where(topic_id: topic.id, post_id: posts.map(&:id)) - .order('reflection asc, clicks desc') - - result = {} - links.each do |l| - result[l.post_id] ||= [] - result[l.post_id] << {url: l.url, - clicks: l.clicks, - title: l.link_topic.try(:title), - internal: l.internal, - reflection: l.reflection} - end - - result - end end # == Schema Information diff --git a/lib/sql_builder.rb b/lib/sql_builder.rb index 57ec895f8fe..3eb93e5c1ba 100644 --- a/lib/sql_builder.rb +++ b/lib/sql_builder.rb @@ -16,6 +16,15 @@ class SqlBuilder end end + def secure_category(secure_category_ids, category_alias = 'c') + if secure_category_ids.present? + where("NOT COALESCE(" << category_alias << ".secure, false) OR " << category_alias << ".id IN (:secure_category_ids)", secure_category_ids: secure_category_ids) + else + where("NOT COALESCE(" << category_alias << ".secure, false)") + end + self + end + def to_sql sql = @sql.dup diff --git a/lib/topic_view.rb b/lib/topic_view.rb index 63d7428abf0..e13cec045f5 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -4,20 +4,22 @@ require_dependency 'summarize' class TopicView - attr_reader :topic, :posts, :index_offset, :index_reverse + attr_reader :topic, :posts, :index_offset, :index_reverse, :guardian attr_accessor :draft, :draft_key, :draft_sequence def initialize(topic_id, user=nil, options={}) @topic = find_topic(topic_id) raise Discourse::NotFound if @topic.blank? + @guardian = Guardian.new(user) + # Special case: If the topic is private and the user isn't logged in, ask them # to log in! if @topic.present? && @topic.private_message? && user.blank? raise Discourse::NotLoggedIn.new end - Guardian.new(user).ensure_can_see!(@topic) + guardian.ensure_can_see!(@topic) @post_number, @page = options[:post_number], options[:page] @limit = options[:limit] || SiteSetting.posts_per_page; @@ -225,18 +227,18 @@ class TopicView @post_action_visibility ||= begin result = [] PostActionType.types.each do |k, v| - result << v if Guardian.new(@user).can_see_post_actors?(@topic, v) + result << v if guardian.can_see_post_actors?(@topic, v) end result end end def links - @links ||= @topic.links_grouped + @links ||= TopicLink.topic_summary(guardian, @topic.id) end def link_counts - @link_counts ||= TopicLinkClick.counts_for(@topic, posts) + @link_counts ||= TopicLink.counts_for(guardian,@topic, posts) end # Are we the initial page load? If so, we can return extra information like diff --git a/spec/models/topic_link_click_spec.rb b/spec/models/topic_link_click_spec.rb index 853b75f6a3e..fce47cb5978 100644 --- a/spec/models/topic_link_click_spec.rb +++ b/spec/models/topic_link_click_spec.rb @@ -12,10 +12,6 @@ describe TopicLinkClick do URI.parse('http://test.host') end - it 'returns blank from counts_for without posts' do - TopicLinkClick.counts_for(nil, nil).should be_blank - end - context 'topic_links' do before do @topic = Fabricate(:topic) @@ -46,25 +42,6 @@ describe TopicLinkClick do TopicLinkClick.first.ip.to_s.should == '192.168.1.1' end - context 'counts for' do - - before do - @counts_for = TopicLinkClick.counts_for(@topic, [@post]) - end - - it 'has a counts_for result' do - @counts_for[@post.id].should be_present - end - - it 'contains the click we made' do - @counts_for[@post.id].first[:clicks].should == 1 - end - - it 'has no clicks on another url in the post' do - @counts_for[@post.id].find {|l| l[:url] == 'http://google.com'}[:clicks].should == 0 - end - - end end context 'create_from' do @@ -82,10 +59,8 @@ describe TopicLinkClick do }.should_not change(TopicLinkClick, :count) end - end - context 'with a valid url and post_id' do before do TopicLinkClick.create_from(url: @topic_link.url, post_id: @post.id, ip: '127.0.0.1') diff --git a/spec/models/topic_link_spec.rb b/spec/models/topic_link_spec.rb index 6e144a124c6..820d45b5977 100644 --- a/spec/models/topic_link_spec.rb +++ b/spec/models/topic_link_spec.rb @@ -216,4 +216,62 @@ describe TopicLink do end end + describe 'counts_for and topic_summary' do + it 'returns blank without posts' do + TopicLink.counts_for(Guardian.new, nil, nil).should be_blank + end + + context 'with data' do + + let(:post) do + topic = Fabricate(:topic) + Fabricate(:post_with_external_links, user: topic.user, topic: topic) + end + + let(:counts_for) do + TopicLink.counts_for(Guardian.new, post.topic, [post]) + end + + + it 'has the correct results' do + TopicLink.extract_from(post) + topic_link = post.topic.topic_links.first + TopicLinkClick.create(topic_link: topic_link, ip: '192.168.1.1') + + counts_for[post.id].should be_present + counts_for[post.id].find {|l| l[:url] == 'http://google.com'}[:clicks].should == 0 + counts_for[post.id].first[:clicks].should == 1 + + array = TopicLink.topic_summary(Guardian.new, post.topic_id) + array.length.should == 4 + array[0]["clicks"].should == "1" + end + + it 'secures internal links correctly' do + category = Fabricate(:category) + secret_topic = Fabricate(:topic, category: category) + + url = "http://#{test_uri.host}/t/topic-slug/#{secret_topic.id}" + post = Fabricate(:post, raw: "hello test topic #{url}") + TopicLink.extract_from(post) + + TopicLink.topic_summary(Guardian.new, post.topic_id).count.should == 1 + TopicLink.counts_for(Guardian.new, post.topic, [post]).length.should == 1 + + category.deny(:all) + category.allow(Group[:staff]) + category.save + + admin = Fabricate(:admin) + + TopicLink.topic_summary(Guardian.new, post.topic_id).count.should == 0 + TopicLink.topic_summary(Guardian.new(admin), post.topic_id).count.should == 1 + + TopicLink.counts_for(Guardian.new, post.topic, [post]).length.should == 0 + TopicLink.counts_for(Guardian.new(admin), post.topic, [post]).length.should == 1 + end + + end + end + end