From 30501e943ce528a14393c0ffb073ffed6052f148 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 13 May 2013 10:48:32 +1000 Subject: [PATCH] fix search --- lib/search.rb | 110 +++++++++++++++++++-------------- lib/sql_builder.rb | 13 +++- spec/components/search_spec.rb | 66 +++++++++++--------- 3 files changed, 111 insertions(+), 78 deletions(-) diff --git a/lib/search.rb b/lib/search.rb index 42523581ed9..b68fdc5a58f 100644 --- a/lib/search.rb +++ b/lib/search.rb @@ -24,53 +24,58 @@ module Search " end - def self.topic_query_sql - "SELECT 'topic' AS type, - CAST(ft.id AS VARCHAR), - '/t/slug/' || ft.id AS url, - ft.title, - NULL AS email, - NULL AS color, - NULL AS text_color + def self.post_query(current_user, type, args) + builder = SqlBuilder.new < 1" + end + + builder.where < '#{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 - " +SQL + + allowed_categories = nil + allowed_categories = current_user.secure_category_ids if current_user + 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, - orig: sanitized_term, query: terms.join(" & "), locale: current_locale_long, limit: expected_topics * 3 + tmp = post_query(current_user, :post, args.merge(limit: expected_topics * 3)) topic_ids = Set.new db_result.map{|r| r["id"]} diff --git a/lib/sql_builder.rb b/lib/sql_builder.rb index 80ad0b211be..f92f4549746 100644 --- a/lib/sql_builder.rb +++ b/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 diff --git a/spec/components/search_spec.rb b/spec/components/search_spec.rb index 3c0df76f9db..481dd20c7e8 100644 --- a/spec/components/search_spec.rb +++ b/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 - result['url'].should == topic.relative_url + end + + 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