Merge branch '36438-expand-redacting-logic' into 'master'

Expand redacting search to include non ES results

Closes #210011, #207940, and #36438

See merge request gitlab-org/gitlab!27065

app/policies/note_policy.rb

@@ -12,6 +12,7 @@ class NotePolicy < BasePolicy
   condition(:editable, scope: :subject) { @subject.editable? }
 
   condition(:can_read_noteable) { can?(:"read_#{@subject.noteable_ability_name}") }
+  condition(:commit_is_deleted) { @subject.for_commit? && @subject.noteable.blank? }
 
   condition(:is_visible) { @subject.system_note_with_references_visible_for?(@user) }
 
@@ -25,12 +26,16 @@ class NotePolicy < BasePolicy
 
   # If user can't read the issue/MR/etc then they should not be allowed to do anything to their own notes
   rule { ~can_read_noteable }.policy do
-    prevent :read_note
     prevent :admin_note
     prevent :resolve_note
     prevent :award_emoji
   end
 
+  # Special rule for deleted commits
+  rule { ~(can_read_noteable | commit_is_deleted) }.policy do
+    prevent :read_note
+  end
+
   rule { is_author }.policy do
     enable :read_note
     enable :admin_note

app/policies/snippet_policy.rb

+# frozen_string_literal: true
+
+class SnippetPolicy < PersonalSnippetPolicy
+end

app/services/search_service.rb

@@ -3,6 +3,11 @@
 class SearchService
   include Gitlab::Allowable
 
+  REDACTABLE_RESULTS = [
+    ActiveRecord::Relation,
+    Gitlab::Search::FoundBlob
+  ].freeze
+
   SEARCH_TERM_LIMIT = 64
   SEARCH_CHAR_LIMIT = 4096
 
@@ -60,11 +65,52 @@ class SearchService
   end
 
   def search_objects
-    @search_objects ||= search_results.objects(scope, params[:page])
+    @search_objects ||= redact_unauthorized_results(search_results.objects(scope, params[:page]))
+  end
+
+  def redactable_results
+    REDACTABLE_RESULTS
   end
 
   private
 
+  def visible_result?(object)
+    return true unless object.respond_to?(:to_ability_name) && DeclarativePolicy.has_policy?(object)
+
+    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
+
+    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 }
+    end
+
+    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)
+
+    Kaminari.paginate_array(
+      permitted_results,
+      total_count: results.total_count,
+      limit: results.limit_value,
+      offset: results.offset_value
+    )
+  end
+
+  def log_redacted_search_results(filtered_results)
+    logger.error(message: "redacted_search_results", filtered: filtered_results, current_user_id: current_user&.id, query: params[:search])
+  end
+
+  def logger
+    @logger ||= ::Gitlab::RedactedSearchResultsLogger.build
+  end
+
   def search_service
     @search_service ||=
       if project

ee/app/services/ee/search_service.rb

@@ -2,10 +2,7 @@
 
 module EE
   module SearchService
-    # All of these classes conform to the necessary pagination interface and
-    # all of these are returned in various places from search results. There
-    # doesn't seem to be a common ancestor to check.
-    REDACTABLE_RESULTS = [
+    EE_REDACTABLE_RESULTS = [
       Kaminari::PaginatableArray,
       Elasticsearch::Model::Response::Records
     ].freeze
@@ -16,40 +13,8 @@ module EE
       search_service.use_elasticsearch?
     end
 
-    def search_objects
-      results = super
-      redact_unauthorized_results(results)
-    end
-
-    def redact_unauthorized_results(results)
-      return results unless REDACTABLE_RESULTS.any? { |redactable| results.is_a?(redactable) }
-
-      filtered_results = []
-      permitted_results = results.select do |o|
-        next true unless o.respond_to?(:to_ability_name)
-
-        ability = :"read_#{o.to_ability_name}"
-        if Ability.allowed?(current_user, ability, o)
-          true
-        else
-          # Redact any search result the user may not have access to. This
-          # could be due to incorrect data in the index or a bug in our query
-          # so we log this as an error.
-          filtered_results << { ability: ability, id: o.id, class_name: o.class.name }
-          false
-        end
-      end
-
-      if filtered_results.any?
-        logger.error(message: "redacted_search_results", filtered: filtered_results, current_user_id: current_user&.id, query: params[:search])
-      end
-
-      Kaminari.paginate_array(
-        permitted_results,
-        total_count: results.total_count,
-        limit: results.limit_value,
-        offset: results.offset_value
-      )
+    def redactable_results
+      super + EE_REDACTABLE_RESULTS
     end
 
     def valid_query_length?
@@ -63,11 +28,5 @@ module EE
 
       super
     end
-
-    private
-
-    def logger
-      @logger ||= ::Gitlab::Elasticsearch::Logger.build
-    end
   end
 end

lib/gitlab/redacted_search_results_logger.rb

+# frozen_string_literal: true
+
+module Gitlab
+  class RedactedSearchResultsLogger < ::Gitlab::JsonLogger
+    def self.file_name_noext
+      'redacted_search_results'
+    end
+  end
+end

spec/policies/note_policy_spec.rb

@@ -35,6 +35,18 @@ describe NotePolicy do
       end
     end
 
+    context 'when the noteable is a deleted commit' do
+      let(:commit) { nil }
+      let(:note) { create(:note_on_commit, commit_id: '12345678', author: user, project: project) }
+
+      it 'allows to read' do
+        expect(policy).to be_allowed(:read_note)
+        expect(policy).to be_disallowed(:admin_note)
+        expect(policy).to be_disallowed(:resolve_note)
+        expect(policy).to be_disallowed(:award_emoji)
+      end
+    end
+
     context 'when the noteable is a commit' do
       let(:commit) { project.repository.head_commit }
       let(:note) { create(:note_on_commit, commit_id: commit.id, author: user, project: project) }

spec/services/search_service_spec.rb

@@ -10,13 +10,16 @@ describe SearchService do
   let!(:group_member) { create(:group_member, group: accessible_group, user: user) }
 
   let!(:accessible_project) { create(:project, :private, name: 'accessible_project') }
-  let!(:inaccessible_project) { create(:project, :private, name: 'inaccessible_project') }
   let(:note) { create(:note_on_issue, project: accessible_project) }
 
+  let!(:inaccessible_project) { create(:project, :private, name: 'inaccessible_project') }
+
   let(:snippet) { create(:snippet, author: user) }
   let(:group_project) { create(:project, group: accessible_group, name: 'group_project') }
   let(:public_project) { create(:project, :public, name: 'public_project') }
 
+  subject(:search_service) { described_class.new(user, search: search, scope: scope, page: 1) }
+
   before do
     accessible_project.add_maintainer(user)
   end
@@ -293,5 +296,70 @@ describe SearchService do
         expect(search_objects.first).to eq public_project
       end
     end
+
+    context 'redacting search results' do
+      shared_examples 'it redacts incorrect results' do
+        before do
+          allow(Ability).to receive(:allowed?).and_return(allowed)
+        end
+
+        context 'when allowed' do
+          let(:allowed) { true }
+
+          it 'does nothing' do
+            expect(results).not_to be_empty
+            expect(results).to all(be_an(model_class))
+          end
+        end
+
+        context 'when disallowed' do
+          let(:allowed) { false }
+
+          it 'does nothing' do
+            expect(results).to be_empty
+          end
+        end
+      end
+
+      context 'issues' do
+        let(:issue) { create(:issue, project: accessible_project) }
+        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'
+      end
+
+      context 'notes' do
+        let(:note) { create(:note_on_commit, project: accessible_project) }
+        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'
+      end
+
+      context 'merge_requests' do
+        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'
+      end
+    end
   end
 end