Merge branch '201886-refactor-search-service-redaction' into 'master'

Make search redaction more robust

Closes #201886

See merge request gitlab-org/gitlab!29166

app/services/search_service.rb

@@ -3,11 +3,6 @@
 class SearchService
   include Gitlab::Allowable
 
-  REDACTABLE_RESULTS = [
-    ActiveRecord::Relation,
-    Gitlab::Search::FoundBlob
-  ].freeze
-
   SEARCH_TERM_LIMIT = 64
   SEARCH_CHAR_LIMIT = 4096
 
@@ -68,10 +63,6 @@ class SearchService
     @search_objects ||= redact_unauthorized_results(search_results.objects(scope, params[:page]))
   end
 
-  def redactable_results
-    REDACTABLE_RESULTS
-  end
-
   private
 
   def visible_result?(object)
@@ -80,12 +71,9 @@ class SearchService
     Ability.allowed?(current_user, :"read_#{object.to_ability_name}", object)
   end
 
-  def redact_unauthorized_results(results)
-    return results unless redactable_results.any? { |redactable| results.is_a?(redactable) }
-
-    permitted_results = results.select do |object|
-      visible_result?(object)
-    end
+  def redact_unauthorized_results(results_collection)
+    results = results_collection.to_a
+    permitted_results = results.select { |object| visible_result?(object) }
 
     filtered_results = (results - permitted_results).each_with_object({}) do |object, memo|
       memo[object.id] = { ability: :"read_#{object.to_ability_name}", id: object.id, class_name: object.class.name }
@@ -93,13 +81,13 @@ class SearchService
 
     log_redacted_search_results(filtered_results.values) if filtered_results.any?
 
-    return results.id_not_in(filtered_results.keys) if results.is_a?(ActiveRecord::Relation)
+    return results_collection.id_not_in(filtered_results.keys) if results_collection.is_a?(ActiveRecord::Relation)
 
     Kaminari.paginate_array(
       permitted_results,
-      total_count: results.total_count,
-      limit: results.limit_value,
-      offset: results.offset_value
+      total_count: results_collection.total_count,
+      limit: results_collection.limit_value,
+      offset: results_collection.offset_value
     )
   end
 

changelogs/unreleased/201886-refactor-search-service-redaction.yml

+---
+title: Make search redaction more robust
+merge_request: 29166
+author:
+type: changed

ee/app/services/ee/search_service.rb

@@ -2,21 +2,12 @@
 
 module EE
   module SearchService
-    EE_REDACTABLE_RESULTS = [
-      Kaminari::PaginatableArray,
-      Elasticsearch::Model::Response::Records
-    ].freeze
-
     # This is a proper method instead of a `delegate` in order to
     # avoid adding unnecessary methods to Search::SnippetService
     def use_elasticsearch?
       search_service.use_elasticsearch?
     end
 
-    def redactable_results
-      super + EE_REDACTABLE_RESULTS
-    end
-
     def valid_query_length?
       return true if use_elasticsearch?
 

ee/lib/gitlab/elastic/group_search_results.rb

@@ -18,15 +18,6 @@ module Gitlab
         @group = group
       end
 
-      def objects(scope, page = nil)
-        case scope
-        when 'users'
-          users.page(page).per(per_page)
-        else
-          super
-        end
-      end
-
       def generic_search_results
         @generic_search_results ||= Gitlab::GroupSearchResults.new(current_user, limit_projects, group, query, default_project_filter: default_project_filter)
       end

ee/lib/gitlab/elastic/project_search_results.rb

@@ -19,23 +19,6 @@ module Gitlab
         @public_and_internal_projects = false
       end
 
-      def objects(scope, page = nil)
-        case scope
-        when 'notes'
-          notes.page(page).per(per_page).records
-        when 'blobs'
-          blobs(page: page, per_page: per_page)
-        when 'wiki_blobs'
-          wiki_blobs(page: page, per_page: per_page)
-        when 'commits'
-          commits(page: page, per_page: per_page)
-        when 'users'
-          users.page(page).per(per_page)
-        else
-          super
-        end
-      end
-
       def generic_search_results
         @generic_search_results ||= Gitlab::ProjectSearchResults.new(current_user, project, query, repository_ref)
       end

spec/services/search_service_spec.rb

@@ -3,16 +3,16 @@
 require 'spec_helper'
 
 describe SearchService do
-  let(:user) { create(:user) }
+  let_it_be(:user) { create(:user) }
 
-  let(:accessible_group) { create(:group, :private) }
-  let(:inaccessible_group) { create(:group, :private) }
-  let!(:group_member) { create(:group_member, group: accessible_group, user: user) }
+  let_it_be(:accessible_group) { create(:group, :private) }
+  let_it_be(:inaccessible_group) { create(:group, :private) }
+  let_it_be(:group_member) { create(:group_member, group: accessible_group, user: user) }
 
-  let!(:accessible_project) { create(:project, :private, name: 'accessible_project') }
-  let(:note) { create(:note_on_issue, project: accessible_project) }
+  let_it_be(:accessible_project) { create(:project, :repository, :private, name: 'accessible_project') }
+  let_it_be(:note) { create(:note_on_issue, project: accessible_project) }
 
-  let!(:inaccessible_project) { create(:project, :private, name: 'inaccessible_project') }
+  let_it_be(:inaccessible_project) { create(:project, :repository, :private, name: 'inaccessible_project') }
 
   let(:snippet) { create(:snippet, author: user) }
   let(:group_project) { create(:project, group: accessible_group, name: 'group_project') }
@@ -298,67 +298,129 @@ describe SearchService do
     end
 
     context 'redacting search results' do
-      shared_examples 'it redacts incorrect results' do
-        before do
-          allow(Ability).to receive(:allowed?).and_return(allowed)
-        end
+      let(:search) { 'anything' }
 
-        context 'when allowed' do
-          let(:allowed) { true }
+      subject(:result) { search_service.search_objects }
 
-          it 'does nothing' do
-            expect(results).not_to be_empty
-            expect(results).to all(be_an(model_class))
-          end
+      def found_blob(project)
+        Gitlab::Search::FoundBlob.new(project: project)
       end
 
-        context 'when disallowed' do
-          let(:allowed) { false }
+      def found_wiki_page(project)
+        Gitlab::Search::FoundWikiPage.new(found_blob(project))
+      end
 
-          it 'does nothing' do
-            expect(results).to be_empty
+      before do
+        expect(search_service)
+          .to receive(:search_results)
+          .and_return(double('search results', objects: unredacted_results))
       end
+
+      def ar_relation(klass, *objects)
+        klass.id_in(objects.map(&:id))
       end
+
+      def kaminari_array(*objects)
+        Kaminari.paginate_array(objects).page(1).per(20)
       end
 
       context 'issues' do
-        let(:issue) { create(:issue, project: accessible_project) }
+        let(:readable) { create(:issue, project: accessible_project) }
+        let(:unreadable) { create(:issue, project: inaccessible_project) }
+        let(:unredacted_results) { ar_relation(Issue, readable, unreadable) }
         let(:scope) { 'issues' }
-        let(:model_class) { Issue }
-        let(:ability) { :read_issue }
-        let(:search) { issue.title }
-        let(:results) { subject.search_objects }
 
-        it_behaves_like 'it redacts incorrect results'
+        it 'redacts the inaccessible issue' do
+          expect(result).to contain_exactly(readable)
+        end
       end
 
       context 'notes' do
-        let(:note) { create(:note_on_commit, project: accessible_project) }
+        let(:readable) { create(:note_on_commit, project: accessible_project) }
+        let(:unreadable) { create(:note_on_commit, project: inaccessible_project) }
+        let(:unredacted_results) { ar_relation(Note, readable, unreadable) }
         let(:scope) { 'notes' }
-        let(:model_class) { Note }
-        let(:ability) { :read_note }
-        let(:search) { note.note }
-        let(:results) do
-          described_class.new(
-            user,
-            project_id: accessible_project.id,
-            scope: scope,
-            search: note.note
-          ).search_objects
-        end
 
-        it_behaves_like 'it redacts incorrect results'
+        it 'redacts the inaccessible note' do
+          expect(result).to contain_exactly(readable)
+        end
       end
 
       context 'merge_requests' do
+        let(:readable) { create(:merge_request, source_project: accessible_project, author: user) }
+        let(:unreadable) { create(:merge_request, source_project: inaccessible_project) }
+        let(:unredacted_results) { ar_relation(MergeRequest, readable, unreadable) }
         let(:scope) { 'merge_requests' }
-        let(:model_class) { MergeRequest }
-        let(:ability) { :read_merge_request }
-        let(:merge_request) { create(:merge_request, source_project: accessible_project, author: user) }
-        let(:search) { merge_request.title }
-        let(:results) { subject.search_objects }
 
-        it_behaves_like 'it redacts incorrect results'
+        it 'redacts the inaccessible merge request' do
+          expect(result).to contain_exactly(readable)
+        end
+      end
+
+      context 'project repository blobs' do
+        let(:readable) { found_blob(accessible_project) }
+        let(:unreadable) { found_blob(inaccessible_project) }
+        let(:unredacted_results) { kaminari_array(readable, unreadable) }
+        let(:scope) { 'blobs' }
+
+        it 'redacts the inaccessible blob' do
+          expect(result).to contain_exactly(readable)
+        end
+      end
+
+      context 'project wiki blobs' do
+        let(:readable) { found_wiki_page(accessible_project) }
+        let(:unreadable) { found_wiki_page(inaccessible_project) }
+        let(:unredacted_results) { kaminari_array(readable, unreadable) }
+        let(:scope) { 'wiki_blobs' }
+
+        it 'redacts the inaccessible blob' do
+          expect(result).to contain_exactly(readable)
+        end
+      end
+
+      context 'project snippets' do
+        let(:readable) { create(:project_snippet, project: accessible_project) }
+        let(:unreadable) { create(:project_snippet, project: inaccessible_project) }
+        let(:unredacted_results) { ar_relation(ProjectSnippet, readable, unreadable) }
+        let(:scope) { 'snippet_blobs' }
+
+        it 'redacts the inaccessible snippet' do
+          expect(result).to contain_exactly(readable)
+        end
+      end
+
+      context 'personal snippets' do
+        let(:readable) { create(:personal_snippet, :private, author: user) }
+        let(:unreadable) { create(:personal_snippet, :private) }
+        let(:unredacted_results) { ar_relation(PersonalSnippet, readable, unreadable) }
+        let(:scope) { 'snippet_blobs' }
+
+        it 'redacts the inaccessible snippet' do
+          expect(result).to contain_exactly(readable)
+        end
+      end
+
+      context 'commits' do
+        let(:readable) { accessible_project.commit }
+        let(:unreadable) { inaccessible_project.commit }
+        let(:unredacted_results) { kaminari_array(readable, unreadable) }
+        let(:scope) { 'commits' }
+
+        it 'redacts the inaccessible commit' do
+          expect(result).to contain_exactly(readable)
+        end
+      end
+
+      context 'users' do
+        let(:other_user) { create(:user) }
+        let(:unredacted_results) { ar_relation(User, user, other_user) }
+        let(:scope) { 'users' }
+
+        it 'passes the users through' do
+          # Users are always visible to everyone
+          expect(result).to contain_exactly(user, other_user)
+        end
       end
     end
   end