fix search

lib/search.rb

@@ -24,53 +24,58 @@ module Search
     "
   end
 
-  def self.topic_query_sql
+  def self.post_query(current_user, type, args)
-    "SELECT 'topic' AS type,
+    builder = SqlBuilder.new <<SQL
-            CAST(ft.id AS VARCHAR),
+    /*select*/
-            '/t/slug/' || ft.id AS url,
-            ft.title,
-            NULL AS email,
-            NULL AS color,
-            NULL AS text_color
     FROM topics AS ft
-      JOIN posts AS p ON p.topic_id = ft.id AND p.post_number = 1
+    /*join*/
       JOIN posts_search s on s.id = p.id
-    WHERE s.search_data @@ TO_TSQUERY(:locale, :query)
+      LEFT JOIN categories c ON c.id = ft.category_id
+    /*where*/
+    ORDER BY
+            TS_RANK_CD(TO_TSVECTOR(:locale, ft.title), TO_TSQUERY(:locale, :query)) desc,
+            TS_RANK_CD(search_data, TO_TSQUERY(:locale, :query)) desc,
+            bumped_at desc
+    LIMIT :limit
+SQL
+
+    builder.select "'topic' AS type"
+    builder.select("CAST(ft.id AS VARCHAR)")
+
+    if type == :topic
+      builder.select "'/t/slug/' || ft.id AS url"
+    else
+      builder.select "'/t/slug/' || ft.id || '/' || p.post_number AS url"
+    end
+
+    builder.select "ft.title, NULL AS email, NULL AS color, NULL AS text_color"
+
+  if type == :topic
+    builder.join "posts AS p ON p.topic_id = ft.id AND p.post_number = 1"
+  else
+    builder.join "posts AS p ON p.topic_id = ft.id AND p.post_number > 1"
+  end
+
+    builder.where <<SQL
+s.search_data @@ TO_TSQUERY(:locale, :query)
       AND ft.deleted_at IS NULL
+      AND p.deleted_at IS NULL
       AND ft.visible
       AND ft.archetype <> '#{Archetype.private_message}'
-    ORDER BY
+SQL
-            TS_RANK_CD(TO_TSVECTOR(:locale, ft.title), TO_TSQUERY(:locale, :query)) desc,
+
-            TS_RANK_CD(search_data, TO_TSQUERY(:locale, :query)) desc,
+    allowed_categories = nil
-            bumped_at desc
+    allowed_categories = current_user.secure_category_ids if current_user
-    LIMIT :limit
+    if allowed_categories.present?
-    "
+      builder.where("(c.id IS NULL OR c.secure = 'f' OR c.id in (:category_ids))", category_ids: allowed_categories)
+    else
+      builder.where("(c.id IS NULL OR c.secure = 'f')")
+    end
+
+    builder.exec(args)
   end
 
 
-  def self.post_query_sql
-    "SELECT cast('topic' as varchar) AS type,
-            CAST(ft.id AS VARCHAR),
-            '/t/slug/' || ft.id || '/' || p.post_number AS url,
-            ft.title,
-            NULL AS email,
-            NULL AS color,
-            NULL AS text_color
-    FROM topics AS ft
-      JOIN posts AS p ON p.topic_id = ft.id AND p.post_number <> 1
-      JOIN posts_search s on s.id = p.id
-    WHERE s.search_data @@ TO_TSQUERY(:locale, :query)
-      AND ft.deleted_at IS NULL and p.deleted_at IS NULL
-      AND ft.visible
-      AND ft.archetype <> '#{Archetype.private_message}'
-    ORDER BY
-            TS_RANK_CD(TO_TSVECTOR(:locale, ft.title), TO_TSQUERY(:locale, :query)) desc,
-            TS_RANK_CD(search_data, TO_TSQUERY(:locale, :query)) desc,
-            bumped_at desc
-    LIMIT :limit
-    "
-  end
-
   def self.category_query_sql
     "SELECT 'category' AS type,
             c.name AS id,
@@ -102,7 +107,8 @@ module Search
     end
   end
 
-  def self.query(term, type_filter=nil, min_search_term_length=3)
+  # needs current user for secure categories
+  def self.query(term, current_user, type_filter=nil, min_search_term_length=3)
 
     return nil if term.blank?
 
@@ -115,16 +121,31 @@ module Search
     terms = sanitized_term.split
     terms.map! {|t| "#{t}:*"}
 
+    args = {orig: sanitized_term, query: terms.join(" & "), locale: current_locale_long}
+
     if type_filter.present?
       raise Discourse::InvalidAccess.new("invalid type filter") unless Search.facets.include?(type_filter)
       sql = Search.send("#{type_filter}_query_sql")
-      db_result = ActiveRecord::Base.exec_sql(sql , orig: sanitized_term, query: terms.join(" & "), locale: current_locale_long, limit: Search.per_facet * Search.facets.size)
+
+      a = args.merge(limit: Search.per_facet * Search.facets.size)
+
+      if type_filter.to_s == "post"
+        db_result = post_query(current_user, :post, a)
+      elsif type_filter.to_s == "topic"
+        db_result = post_query(current_user, :topic, a)
+      else
+        db_result = ActiveRecord::Base.exec_sql(sql , a)
+      end
+
     else
 
       db_result = []
-      [user_query_sql, category_query_sql, topic_query_sql].each do |sql|
+
-        db_result += ActiveRecord::Base.exec_sql(sql , orig: sanitized_term, query: terms.join(" & "), locale: current_locale_long, limit: (Search.per_facet + 1)).to_a
+      [user_query_sql, category_query_sql].each do |sql|
+        db_result += ActiveRecord::Base.exec_sql(sql , args.merge(limit: (Search.per_facet + 1))).to_a
       end
+
+      db_result += post_query(current_user, :topic, args.merge(limit: (Search.per_facet + 1))).to_a
     end
 
     db_result = db_result.to_a
@@ -140,8 +161,7 @@ module Search
     end
 
     if expected_topics > 0
-      tmp = ActiveRecord::Base.exec_sql post_query_sql,
+      tmp = post_query(current_user, :post, args.merge(limit: expected_topics * 3))
-        orig: sanitized_term, query: terms.join(" & "), locale: current_locale_long, limit: expected_topics * 3
 
       topic_ids = Set.new db_result.map{|r| r["id"]}
 

lib/sql_builder.rb

@@ -7,7 +7,7 @@ class SqlBuilder
     @klass = klass
   end
 
-  [:set, :where2,:where,:order_by,:limit,:left_join,:join,:offset].each do |k|
+  [:set, :where2,:where,:order_by,:limit,:left_join,:join,:offset, :select].each do |k|
     define_method k do |data, args = {}|
       @args.merge!(args)
       @sections[k] ||= []
@@ -16,13 +16,14 @@ class SqlBuilder
     end
   end
 
-  def exec(args = {})
+  def to_sql
     sql = @sql.dup
-    @args.merge!(args)
 
     @sections.each do |k,v|
       joined = nil
       case k
+      when :select
+        joined = "SELECT " << v.join(" , ")
       when :where, :where2
         joined = "WHERE " << v.join(" AND ")
       when :join
@@ -41,7 +42,13 @@ class SqlBuilder
 
       sql.sub!("/*#{k}*/", joined)
     end
+    sql
+  end
 
+  def exec(args = {})
+    @args.merge!(args)
+
+    sql = to_sql
     if @klass
       @klass.find_by_sql(ActiveRecord::Base.send(:sanitize_sql_array, [sql, @args]))
     else

spec/components/search_spec.rb

@@ -68,25 +68,25 @@ describe Search do
 
   it 'returns something blank on a nil search' do
     ActiveRecord::Base.expects(:exec_sql).never
-    Search.query(nil).should be_blank
+    Search.query(nil,nil).should be_blank
   end
 
   it 'does not search when the search term is too small' do
     ActiveRecord::Base.expects(:exec_sql).never
-    Search.query('evil', nil, 5).should be_blank
+    Search.query('evil', nil,  nil, 5).should be_blank
   end
 
   it 'escapes non alphanumeric characters' do
-    Search.query('foo :!$);}]>@\#\"\'').should be_blank # There are at least three levels of sanitation for Search.query!
+    Search.query('foo :!$);}]>@\#\"\'', nil).should be_blank # There are at least three levels of sanitation for Search.query!
   end
 
   it 'works when given two terms with spaces' do
-    lambda { Search.query('evil trout') }.should_not raise_error
+    lambda { Search.query('evil trout', nil) }.should_not raise_error
   end
 
   context 'users' do
     let!(:user) { Fabricate(:user) }
-    let(:result) { first_of_type(Search.query('bruce'), 'user') }
+    let(:result) { first_of_type(Search.query('bruce', nil), 'user') }
 
     it 'returns a result' do
       result.should be_present
@@ -107,23 +107,41 @@ describe Search do
   end
 
   context 'topics' do
-    let!(:topic) { Fabricate(:topic) }
+    let(:topic) { Fabricate(:topic) }
 
     context 'searching the OP' do
 
       let!(:post) { Fabricate(:post, topic: topic, user: topic.user) }
-      let(:result) { first_of_type(Search.query('hello'), 'topic') }
+      let(:result) { first_of_type(Search.query('hello', nil), 'topic') }
 
-      it 'returns a result' do
+      it 'returns a result correctly' do
         result.should be_present
-      end
-
-      it 'has the topic title' do
         result['title'].should == topic.title
+        result['url'].should == topic.relative_url
       end
 
-      it 'has a url for the post' do
+    end
-        result['url'].should == topic.relative_url
+
+    context 'security' do
+      let!(:post) { Fabricate(:post, topic: topic, user: topic.user) }
+      def result(current_user)
+        first_of_type(Search.query('hello', current_user), 'topic')
+      end
+
+      it 'secures results correctly' do
+        category = Fabricate(:category)
+
+        topic.category_id = category.id
+        topic.save
+
+        category.deny(:all)
+        category.allow(Group[:staff])
+        category.save
+
+        result(nil).should_not be_present
+        result(Fabricate(:user)).should_not be_present
+        result(Fabricate(:admin)).should be_present
+
       end
     end
 
@@ -136,7 +154,7 @@ describe Search do
                                               end
     }
     let!(:post) {Fabricate(:post, topic: cyrillic_topic, user: cyrillic_topic.user)}
-    let(:result) { first_of_type(Search.query('запись'), 'topic') }
+    let(:result) { first_of_type(Search.query('запись',nil), 'topic') }
 
     it 'finds something when given cyrillic query' do
       result.should be_present
@@ -146,17 +164,11 @@ describe Search do
   context 'categories' do
 
     let!(:category) { Fabricate(:category) }
-    let(:result) { first_of_type(Search.query('amazing'), 'category') }
+    let(:result) { first_of_type(Search.query('amazing', nil), 'category') }
 
-    it 'returns a result' do
+    it 'returns the correct result' do
       result.should be_present
-    end
-
-    it 'has the category name' do
       result['title'].should == category.name
-    end
-
-    it 'has a url for the topic' do
       result['url'].should == "/category/#{category.slug}"
     end
 
@@ -170,26 +182,20 @@ describe Search do
 
 
     context 'user filter' do
-      let(:results) { Search.query('amazing', 'user') }
+      let(:results) { Search.query('amazing', nil, 'user') }
 
       it "returns a user result" do
         results.detect {|r| r[:type] == 'user'}.should be_present
-      end
-
-      it "returns no category results" do
         results.detect {|r| r[:type] == 'category'}.should be_blank
       end
 
     end
 
     context 'category filter' do
-      let(:results) { Search.query('amazing', 'category') }
+      let(:results) { Search.query('amazing', nil, 'category') }
 
       it "returns a user result" do
         results.detect {|r| r[:type] == 'user'}.should be_blank
-      end
-
-      it "returns no category results" do
         results.detect {|r| r[:type] == 'category'}.should be_present
       end