Add permission data to Elasticsearch notes documents

Adds permission data to json used for notes documents.
Permission data changes will kick off reindexing of
associated notes. Permissions tracked are project
visibility and access level for the note types stored
in Elasticsearch (Issue, MergeRequest, Snippet, and
Commit).

ee/app/models/ee/project.rb

@@ -108,6 +108,7 @@ module EE
       has_one :security_orchestration_policy_configuration, class_name: 'Security::OrchestrationPolicyConfiguration', foreign_key: :project_id, inverse_of: :project
 
       elastic_index_dependant_association :issues, on_change: :visibility_level
+      elastic_index_dependant_association :notes, on_change: :visibility_level
 
       scope :with_shared_runners_limit_enabled, -> do
         if ::Ci::Runner.has_shared_runners_with_non_zero_public_cost?

ee/app/models/ee/project_feature.rb

@@ -5,6 +5,12 @@ module EE
     extend ActiveSupport::Concern
 
     EE_FEATURES = %i(requirements).freeze
+    NOTES_PERMISSION_TRACKED_FIELDS = %w(
+      issues_access_level
+      repository_access_level
+      merge_requests_access_level
+      snippets_access_level
+    ).freeze
 
     prepended do
       set_available_features(EE_FEATURES)
@@ -14,7 +20,11 @@ module EE
         if project.maintaining_elasticsearch?
           project.maintain_elasticsearch_update
 
-          ElasticAssociationIndexerWorker.perform_async(self.project.class.name, project_id, ['issues']) if elasticsearch_project_associations_need_updating?
+          associations_to_update = []
+          associations_to_update << 'issues' if elasticsearch_project_issues_need_updating?
+          associations_to_update << 'notes' if elasticsearch_project_notes_need_updating?
+
+          ElasticAssociationIndexerWorker.perform_async(self.project.class.name, project_id, associations_to_update) if associations_to_update.any?
         end
       end
 
@@ -22,7 +32,11 @@ module EE
 
       private
 
-      def elasticsearch_project_associations_need_updating?
+      def elasticsearch_project_notes_need_updating?
+        self.previous_changes.keys.any? { |key| NOTES_PERMISSION_TRACKED_FIELDS.include?(key) }
+      end
+
+      def elasticsearch_project_issues_need_updating?
         self.previous_changes.key?(:issues_access_level)
       end
     end

ee/lib/elastic/instance_proxy_util.rb

@@ -54,6 +54,22 @@ module Elastic
       nil
     end
 
+    # protect against missing project_feature and set visibility to PRIVATE
+    # if the project_feature is missing on a project
+    def safely_read_project_feature_for_elasticsearch(feature)
+      if target.project.project_feature
+        target.project.project_feature.access_level(feature)
+      else
+        logger.warn(
+          message: 'Project is missing ProjectFeature',
+          project_id: target.project_id,
+          id: target.id,
+          class: target.class
+        )
+        ProjectFeature::PRIVATE
+      end
+    end
+
     def apply_field_limit(result)
       return result unless result.is_a? String
 

ee/lib/elastic/latest/issue_instance_proxy.rb

@@ -14,15 +14,7 @@ module Elastic
 
         data['assignee_id'] = safely_read_attribute_for_elasticsearch(:assignee_ids)
         data['visibility_level'] = target.project.visibility_level
-
-        # protect against missing project_feature and set visibility to PRIVATE
-        # if the project_feature is missing on a project
-        begin
-          data['issues_access_level'] = target.project.project_feature.issues_access_level
-        rescue NoMethodError => e
-          Gitlab::ErrorTracking.track_exception(e, project_id: target.project_id, issue_id: target.id)
-          data['issues_access_level'] = ProjectFeature::PRIVATE
-        end
+        data['issues_access_level'] = safely_read_project_feature_for_elasticsearch(:issues)
 
         data.merge(generic_attributes)
       end

ee/lib/elastic/latest/note_instance_proxy.rb

@@ -22,8 +22,35 @@ module Elastic
           }
         end
 
+        # do not add the permission fields unless the `remove_permissions_data_from_notes_documents`
+        # migration has completed otherwise the migration will never finish
+        if Elastic::DataMigrationService.migration_has_finished?(:remove_permissions_data_from_notes_documents)
+          data['visibility_level'] = target.project.visibility_level
+          merge_project_feature_access_level(data, noteable)
+        end
+
         data.merge(generic_attributes)
       end
+
+      private
+
+      def merge_project_feature_access_level(data, noteable)
+        return unless noteable
+
+        case noteable
+        when Snippet
+          data['snippets_access_level'] = safely_read_project_feature_for_elasticsearch(:snippets)
+        when Commit
+          data['repository_access_level'] = safely_read_project_feature_for_elasticsearch(:repository)
+        when Issue, MergeRequest
+          access_level_attribute = ProjectFeature.access_level_attribute(noteable)
+          data[access_level_attribute.to_s] = safely_read_project_feature_for_elasticsearch(noteable)
+        else
+          # do nothing for other note types (DesignManagement::Design, AlertManagement::Alert, Epic, Vulnerability )
+          # are indexed but not currently searchable so we will not add permission
+          # data for them until the search capability is implemented
+        end
+      end
     end
   end
 end

ee/spec/elastic/migrate/20210127154600_remove_permissions_data_from_notes_documents_spec.rb

@@ -46,6 +46,13 @@ RSpec.describe RemovePermissionsDataFromNotesDocuments, :elastic, :sidekiq_inlin
     context 'migration process' do
       before do
         add_permission_data_for_notes([note_on_commit, note_on_issue, note_on_merge_request, note_on_snippet])
+
+        allow(Elastic::DataMigrationService).to receive(:migration_has_finished?).and_call_original
+        # migrations are completed by default in test environments
+        # required to prevent the `as_indexed_json` method from populating the permissions fields
+        allow(Elastic::DataMigrationService).to receive(:migration_has_finished?)
+                                                  .with(:remove_permissions_data_from_notes_documents)
+                                                  .and_return(false)
       end
 
       it 'queues documents for indexing' do

ee/spec/models/concerns/elastic/issue_spec.rb

@@ -132,12 +132,11 @@ RSpec.describe Issue, :elastic do
     expect(issue.__elasticsearch__.as_indexed_json).to eq(expected_hash)
   end
 
-  it 'handles a project missing project_feature' do
+  it 'handles a project missing project_feature', :aggregate_failures do
     issue = create :issue, project: project
     allow(issue.project).to receive(:project_feature).and_return(nil)
 
-    expect(Gitlab::ErrorTracking).to receive(:track_exception)
-
+    expect { issue.__elasticsearch__.as_indexed_json }.not_to raise_error
     expect(issue.__elasticsearch__.as_indexed_json['issues_access_level']).to eq(ProjectFeature::PRIVATE)
   end
 

ee/spec/models/concerns/elastic/note_spec.rb

@@ -85,34 +85,102 @@ RSpec.describe Note, :elastic do
     expect(described_class.elastic_search('term', options: options).total_count).to eq(4)
   end
 
-  it "returns json with all needed elements" do
-    assignee = create(:user)
-    issue = create(:issue, assignees: [assignee])
-    note = create(:note, noteable: issue, project: issue.project)
-
-    expected_hash = note.attributes.extract!(
-      'id',
-      'note',
-      'project_id',
-      'noteable_type',
-      'noteable_id',
-      'created_at',
-      'updated_at',
-      'confidential'
-    ).merge({
-      'issue' => {
-        'assignee_id' => issue.assignee_ids,
-        'author_id' => issue.author_id,
-        'confidential' => issue.confidential
-      },
-      'type' => note.es_type,
-      'join_field' => {
-        'name' => note.es_type,
-        'parent' => note.es_parent
-      }
-    })
-
-    expect(note.__elasticsearch__.as_indexed_json).to eq(expected_hash)
+  describe 'json serialization' do
+    using RSpec::Parameterized::TableSyntax
+
+    it "returns json with all needed elements" do
+      assignee = create(:user)
+      project = create(:project)
+      issue = create(:issue, project: project, assignees: [assignee])
+      note = create(:note, noteable: issue, project: project)
+
+      expected_hash = note.attributes.extract!(
+        'id',
+        'note',
+        'project_id',
+        'noteable_type',
+        'noteable_id',
+        'created_at',
+        'updated_at',
+        'confidential'
+      ).merge({
+                'issue' => {
+                  'assignee_id' => issue.assignee_ids,
+                  'author_id' => issue.author_id,
+                  'confidential' => issue.confidential
+                },
+                'type' => note.es_type,
+                'join_field' => {
+                  'name' => note.es_type,
+                  'parent' => note.es_parent
+                },
+                'visibility_level' => project.visibility_level,
+                'issues_access_level' => project.issues_access_level
+              })
+
+      expect(note.__elasticsearch__.as_indexed_json).to eq(expected_hash)
+    end
+
+    it 'does not raise error for notes with null noteable references' do
+      note = create(:note_on_issue)
+      allow(note).to receive(:noteable).and_return(nil)
+
+      expect { note.__elasticsearch__.as_indexed_json }.not_to raise_error
+    end
+
+    where(:note_type, :permission, :access_level) do
+      :note_on_issue                              | ProjectFeature::ENABLED      | 'issues_access_level'
+      :note_on_project_snippet                    | ProjectFeature::DISABLED     | 'snippets_access_level'
+      :note_on_personal_snippet                   | ProjectFeature::DISABLED     | 'snippets_access_level'
+      :note_on_merge_request                      | ProjectFeature::PUBLIC       | 'merge_requests_access_level'
+      :note_on_commit                             | ProjectFeature::PRIVATE      | 'repository_access_level'
+      :diff_note_on_merge_request                 | ProjectFeature::PUBLIC       | 'merge_requests_access_level'
+      :diff_note_on_commit                        | ProjectFeature::PRIVATE      | 'repository_access_level'
+      :diff_note_on_design                        | ProjectFeature::ENABLED      | false
+      :legacy_diff_note_on_merge_request          | ProjectFeature::PUBLIC       | 'merge_requests_access_level'
+      :legacy_diff_note_on_commit                 | ProjectFeature::PRIVATE      | 'repository_access_level'
+      :note_on_alert                              | ProjectFeature::PRIVATE      | false
+      :note_on_design                             | ProjectFeature::ENABLED      | false
+      :note_on_epic                               | ProjectFeature::ENABLED      | false
+      :note_on_vulnerability                      | ProjectFeature::PRIVATE      | false
+      :discussion_note_on_vulnerability           | ProjectFeature::PRIVATE      | false
+      :discussion_note_on_merge_request           | ProjectFeature::PUBLIC       | 'merge_requests_access_level'
+      :discussion_note_on_issue                   | ProjectFeature::ENABLED      | 'issues_access_level'
+      :discussion_note_on_project_snippet         | ProjectFeature::DISABLED     | 'snippets_access_level'
+      :discussion_note_on_personal_snippet        | ProjectFeature::DISABLED     | 'snippets_access_level'
+      :note_on_merge_request                      | ProjectFeature::PUBLIC       | 'merge_requests_access_level'
+      :discussion_note_on_commit                  | ProjectFeature::PRIVATE      | 'repository_access_level'
+      :track_mr_picking_note                      | ProjectFeature::PUBLIC       | 'merge_requests_access_level'
+    end
+
+    with_them do
+      let_it_be(:project) { create(:project, :repository) }
+      let!(:note) { create(note_type, project: project) } # rubocop:disable Rails/SaveBang
+      let(:note_json) { note.__elasticsearch__.as_indexed_json }
+
+      before do
+        project.project_feature.update_attribute(access_level.to_sym, permission) if access_level.present?
+      end
+
+      it 'does not contain permissions if remove_permissions_data_from_notes_documents is not finished' do
+        allow(Elastic::DataMigrationService).to receive(:migration_has_finished?)
+                                                  .with(:remove_permissions_data_from_notes_documents)
+                                                  .and_return(false)
+
+        expect(note_json).not_to have_key(access_level) if access_level.present?
+        expect(note_json).not_to have_key('visibility_level')
+      end
+
+      it 'contains the correct permissions', :aggregate_failures do
+        if access_level
+          expect(note_json).to have_key(access_level)
+          expect(note_json[access_level]).to eq(permission)
+        end
+
+        expect(note_json).to have_key('visibility_level')
+        expect(note_json['visibility_level']).to eq(project.visibility_level)
+      end
+    end
   end
 
   it 'does not track system note updates' do

ee/spec/models/project_feature_spec.rb

@@ -28,13 +28,16 @@ RSpec.describe ProjectFeature do
       allow(project).to receive(:maintaining_elasticsearch?).and_return(true)
     end
 
-    where(:feature, :worker_expected) do
-      'issues'          | true
-      'wiki'            | false
-      'builds'          | false
-      'merge_requests'  | false
-      'repository'      | false
-      'pages'           | false
+    where(:feature, :worker_expected, :associations) do
+      'issues'                      | true        | %w[issues notes]
+      'wiki'                        | false       | nil
+      'builds'                      | false       | nil
+      'merge_requests'              | true        | %w[notes]
+      'repository'                  | true        | %w[notes]
+      'snippets'                    | true        | %w[notes]
+      'operations'                  | false       | nil
+      'security_and_compliance'     | false       | nil
+      'pages'                       | false       | nil
     end
 
     with_them do
@@ -42,12 +45,12 @@ RSpec.describe ProjectFeature do
         expect(project).to receive(:maintain_elasticsearch_update)
 
         if worker_expected
-          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, ['issues'])
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, associations)
         else
           expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async)
         end
 
-        project.project_feature.update_attribute("#{feature}_access_level".to_sym, ProjectFeature::PRIVATE)
+        project.project_feature.update_attribute("#{feature}_access_level".to_sym, ProjectFeature::DISABLED)
       end
     end
   end

ee/spec/models/project_spec.rb

@@ -2707,8 +2707,8 @@ RSpec.describe Project do
       let!(:issue) { create(:issue, project: project) }
 
       context 'when updating the visibility_level' do
-        it 'triggers ElasticAssociationIndexerWorker to update issues' do
-          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, ['issues'])
+        it 'triggers ElasticAssociationIndexerWorker to update issues and notes' do
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, %w[issues notes])
 
           project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
         end

ee/spec/services/groups/transfer_service_spec.rb

@@ -67,11 +67,11 @@ RSpec.describe Groups::TransferService, '#execute' do
 
           expect(::Gitlab::CurrentSettings).not_to receive(:invalidate_elasticsearch_indexes_cache_for_project!)
           expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project1)
-          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project1.id, ['issues'])
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project1.id, %w[issues notes])
           expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project2)
-          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project2.id, ['issues'])
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project2.id, %w[issues notes])
           expect(Elastic::ProcessBookkeepingService).not_to receive(:track!).with(project3)
-          expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async).with('Project', project3.id, ['issues'])
+          expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async).with('Project', project3.id, %w[issues notes])
 
           transfer_service.execute(new_group)
 

ee/spec/services/projects/transfer_service_spec.rb

@@ -72,9 +72,9 @@ RSpec.describe Projects::TransferService do
     context 'when visibility level changes' do
       let_it_be(:group) { create(:group, :private) }
 
-      it 'reindexes the project and associated issues' do
+      it 'reindexes the project and associated issues and notes' do
         expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project)
-        expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, ['issues'])
+        expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, %w[issues notes])
 
         subject.execute(group)
       end