diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index 4b6a6658bb938c30f144d4f3d07bdf923941d460..00fad9886ae88dc5941e3466a1dd88080408b098 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -108,6 +108,7 @@ def lock_for_confirmation!(id) 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? diff --git a/ee/app/models/ee/project_feature.rb b/ee/app/models/ee/project_feature.rb index ecf506b801e9823acfab469d915ece307f9a5617..29eee25fc64ef46512b32d5c930e4a0aac79286f 100644 --- a/ee/app/models/ee/project_feature.rb +++ b/ee/app/models/ee/project_feature.rb @@ -5,6 +5,7 @@ module ProjectFeature extend ActiveSupport::Concern EE_FEATURES = %i(requirements security_and_compliance).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 +15,11 @@ module ProjectFeature 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 @@ -23,7 +28,11 @@ module ProjectFeature 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 diff --git a/ee/lib/elastic/instance_proxy_util.rb b/ee/lib/elastic/instance_proxy_util.rb index 3f85185a84dfdb21e823a1dad2a4d22879c73df4..eaf5be4e9c9da7ce64015a48dd78e3b760ab49f2 100644 --- a/ee/lib/elastic/instance_proxy_util.rb +++ b/ee/lib/elastic/instance_proxy_util.rb @@ -54,6 +54,22 @@ def safely_read_attribute_for_elasticsearch(attr_name) 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 diff --git a/ee/lib/elastic/latest/issue_instance_proxy.rb b/ee/lib/elastic/latest/issue_instance_proxy.rb index 084cbff4a0398f0cb80934e104b938cee6b84786..967483f071656bc687e718e8aaa7c64cabed5988 100644 --- a/ee/lib/elastic/latest/issue_instance_proxy.rb +++ b/ee/lib/elastic/latest/issue_instance_proxy.rb @@ -14,15 +14,7 @@ def as_indexed_json(options = {}) 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 diff --git a/ee/lib/elastic/latest/note_instance_proxy.rb b/ee/lib/elastic/latest/note_instance_proxy.rb index 42bbec9cc5c07c83ab8f34cfe7697107fca9f0f3..8a3a1371b80dad9d95dab9d6fdc14f9297acfcc3 100644 --- a/ee/lib/elastic/latest/note_instance_proxy.rb +++ b/ee/lib/elastic/latest/note_instance_proxy.rb @@ -22,8 +22,31 @@ def as_indexed_json(options = {}) } 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) + else + access_level_attribute = ProjectFeature.access_level_attribute(noteable) + data[access_level_attribute.to_s] = safely_read_project_feature_for_elasticsearch(noteable) + end + end end end end diff --git a/ee/spec/elastic/migrate/20210127154600_remove_permissions_data_from_notes_documents_spec.rb b/ee/spec/elastic/migrate/20210127154600_remove_permissions_data_from_notes_documents_spec.rb index 07a673aa657ac5f3809fbaa556405a5259a76c06..4be8d3e4723aa358d6b04baff1311b5cea0d01cb 100644 --- a/ee/spec/elastic/migrate/20210127154600_remove_permissions_data_from_notes_documents_spec.rb +++ b/ee/spec/elastic/migrate/20210127154600_remove_permissions_data_from_notes_documents_spec.rb @@ -11,6 +11,13 @@ before do stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true) allow(migration).to receive(:helper).and_return(helper) + + 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 describe 'migration_options' do diff --git a/ee/spec/models/concerns/elastic/issue_spec.rb b/ee/spec/models/concerns/elastic/issue_spec.rb index fc38ae2ba6a334eff5e70a975206ec6f981ae494..99e53d6fcae6b6bbc56b8e0f2ab43a33042dbb1c 100644 --- a/ee/spec/models/concerns/elastic/issue_spec.rb +++ b/ee/spec/models/concerns/elastic/issue_spec.rb @@ -132,12 +132,11 @@ 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 diff --git a/ee/spec/models/concerns/elastic/note_spec.rb b/ee/spec/models/concerns/elastic/note_spec.rb index 1abb3bbcd6d348695d80ae906326962f985e7538..8e5b48bd749eb9458a5781d226b99d248b491e63 100644 --- a/ee/spec/models/concerns/elastic/note_spec.rb +++ b/ee/spec/models/concerns/elastic/note_spec.rb @@ -85,34 +85,80 @@ 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_merge_request | ProjectFeature::PUBLIC | 'merge_requests_access_level' + :note_on_commit | ProjectFeature::PRIVATE | 'repository_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) + 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) + expect(note_json).not_to have_key('visibility_level') + end + + it 'contains the correct permissions', :aggregate_failures do + expect(note_json).to have_key(access_level) + expect(note_json[access_level]).to eq(permission) + expect(note_json).to have_key('visibility_level') + end + end end it 'does not track system note updates' do diff --git a/ee/spec/models/project_feature_spec.rb b/ee/spec/models/project_feature_spec.rb index 476a35b3231c3df07025dddf687c4081f9f79c53..0eff55b3746ec793019e8e6d2742d12ecb75524d 100644 --- a/ee/spec/models/project_feature_spec.rb +++ b/ee/spec/models/project_feature_spec.rb @@ -28,13 +28,14 @@ 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] + 'pages' | false | nil end with_them do @@ -42,7 +43,7 @@ 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 diff --git a/ee/spec/models/project_spec.rb b/ee/spec/models/project_spec.rb index 1294d013060379445b6dfc31bbfe0fad06050eed..56f219e84e73efc05da5f9a3c9aecf372ede83d2 100644 --- a/ee/spec/models/project_spec.rb +++ b/ee/spec/models/project_spec.rb @@ -2663,8 +2663,8 @@ def stub_default_url_options(host) 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 diff --git a/ee/spec/services/groups/transfer_service_spec.rb b/ee/spec/services/groups/transfer_service_spec.rb index edf88f439a0b270a3787003f8a0479cc36880916..f5041558010cb0c0eb2b3190bff426c7666cbbb0 100644 --- a/ee/spec/services/groups/transfer_service_spec.rb +++ b/ee/spec/services/groups/transfer_service_spec.rb @@ -67,11 +67,11 @@ 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) diff --git a/ee/spec/services/projects/transfer_service_spec.rb b/ee/spec/services/projects/transfer_service_spec.rb index c171d6f825cd7bca9a4ac4617c29ba0a1f16e147..90d7d5c4541d22762e7c08dc975543fb7d68d578 100644 --- a/ee/spec/services/projects/transfer_service_spec.rb +++ b/ee/spec/services/projects/transfer_service_spec.rb @@ -72,9 +72,9 @@ 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