Merge branch 'add-current-user-compare-services-ee' into 'master'

Add current user to comparison services for permissions

See merge request gitlab-org/gitlab-ee!16252

app/models/merge_request.rb

@@ -1241,7 +1241,7 @@ class MergeRequest < ApplicationRecord
   end
 
   def compare_reports(service_class, current_user = nil)
-    with_reactive_cache(service_class.name) do |data|
+    with_reactive_cache(service_class.name, current_user&.id) do |data|
       unless service_class.new(project, current_user)
         .latest?(base_pipeline, actual_head_pipeline, data)
         raise InvalidateReactiveCache
@@ -1251,12 +1251,13 @@ class MergeRequest < ApplicationRecord
     end || { status: :parsing }
   end
 
-  def calculate_reactive_cache(identifier, *args)
+  def calculate_reactive_cache(identifier, current_user_id = nil, *args)
     service_class = identifier.constantize
 
     raise NameError, service_class unless service_class < Ci::CompareReportsBaseService
 
-    service_class.new(project).execute(base_pipeline, actual_head_pipeline)
+    current_user = User.find_by(id: current_user_id)
+    service_class.new(project, current_user).execute(base_pipeline, actual_head_pipeline)
   end
 
   def all_commits

app/services/ci/compare_reports_base_service.rb

@@ -41,7 +41,7 @@ module Ci
     end
 
     def serializer_params
-      { project: project }
+      { project: project, current_user: current_user }
     end
 
     def get_report(pipeline)

ee/app/controllers/ee/projects/merge_requests_controller.rb

@@ -52,15 +52,15 @@ module EE
       end
 
       def container_scanning_reports
-        reports_response(merge_request.compare_container_scanning_reports)
+        reports_response(merge_request.compare_container_scanning_reports(current_user))
       end
 
       def dependency_scanning_reports
-        reports_response(merge_request.compare_dependency_scanning_reports)
+        reports_response(merge_request.compare_dependency_scanning_reports(current_user))
       end
 
       def sast_reports
-        reports_response(merge_request.compare_sast_reports)
+        reports_response(merge_request.compare_sast_reports(current_user))
       end
 
       def metrics_reports

ee/app/models/ee/merge_request.rb

@@ -116,12 +116,12 @@ module EE
       actual_head_pipeline&.has_reports?(::Ci::JobArtifact.dependency_list_reports)
     end
 
-    def compare_dependency_scanning_reports
+    def compare_dependency_scanning_reports(current_user)
       unless has_dependency_scanning_reports?
         return { status: :error, status_reason: 'This merge request does not have dependency scanning reports' }
       end
 
-      compare_reports(::Ci::CompareDependencyScanningReportsService)
+      compare_reports(::Ci::CompareDependencyScanningReportsService, current_user)
     end
 
     def has_license_management_reports?
@@ -132,24 +132,24 @@ module EE
       actual_head_pipeline&.has_reports?(::Ci::JobArtifact.container_scanning_reports)
     end
 
-    def compare_container_scanning_reports
+    def compare_container_scanning_reports(current_user)
       unless has_container_scanning_reports?
         return { status: :error, status_reason: 'This merge request does not have container scanning reports' }
       end
 
-      compare_reports(::Ci::CompareContainerScanningReportsService)
+      compare_reports(::Ci::CompareContainerScanningReportsService, current_user)
     end
 
     def has_sast_reports?
       actual_head_pipeline&.has_reports?(::Ci::JobArtifact.sast_reports)
     end
 
-    def compare_sast_reports
+    def compare_sast_reports(current_user)
       unless has_sast_reports?
         return { status: :error, status_reason: 'This merge request does not have SAST reports' }
       end
 
-      compare_reports(::Ci::CompareSastReportsService)
+      compare_reports(::Ci::CompareSastReportsService, current_user)
     end
 
     def compare_license_management_reports(current_user)

ee/app/services/ci/compare_license_management_reports_service.rb

@@ -10,10 +10,6 @@ module Ci
       LicenseManagementReportsComparerSerializer
     end
 
-    def serializer_params
-      { project: project, current_user: current_user }
-    end
-
     def get_report(pipeline)
       pipeline&.license_management_report
     end

ee/changelogs/unreleased/add-current-user-compare-services-ee.yml

+---
+title: Add current_user to security report comparison services
+merge_request: 16252
+author:
+type: fixed

ee/spec/controllers/projects/merge_requests_controller_spec.rb

@@ -409,7 +409,7 @@ describe Projects::MergeRequestsController do
 
     before do
       allow_any_instance_of(::MergeRequest).to receive(:compare_reports)
-        .with(::Ci::CompareDependencyScanningReportsService).and_return(comparison_status)
+        .with(::Ci::CompareDependencyScanningReportsService, project.users.first).and_return(comparison_status)
     end
 
     context 'when comparison is being processed' do
@@ -494,7 +494,7 @@ describe Projects::MergeRequestsController do
 
     before do
       allow_any_instance_of(::MergeRequest).to receive(:compare_reports)
-        .with(::Ci::CompareContainerScanningReportsService).and_return(comparison_status)
+        .with(::Ci::CompareContainerScanningReportsService, project.users.first).and_return(comparison_status)
     end
 
     context 'when comparison is being processed' do
@@ -579,7 +579,7 @@ describe Projects::MergeRequestsController do
 
     before do
       allow_any_instance_of(::MergeRequest).to receive(:compare_reports)
-        .with(::Ci::CompareSastReportsService).and_return(comparison_status)
+        .with(::Ci::CompareSastReportsService, project.users.first).and_return(comparison_status)
     end
 
     context 'when comparison is being processed' do

ee/spec/models/merge_request_spec.rb

@@ -223,10 +223,28 @@ describe MergeRequest do
     end
   end
 
+  describe '#calculate_reactive_cache with current_user' do
+    let(:project) { create(:project, :repository) }
+    let(:current_user) { project.users.take }
+    let(:merge_request) { create(:merge_request, source_project: project) }
+    subject { merge_request.calculate_reactive_cache(service_class_name, current_user&.id) }
+
+    context 'when given a known service class name' do
+      let(:service_class_name) { 'Ci::CompareDependencyScanningReportsService' }
+
+      it 'does not raises a NameError exception' do
+        allow_any_instance_of(service_class_name.constantize).to receive(:execute).and_return(nil)
+
+        expect { subject }.not_to raise_error
+      end
+    end
+  end
+
   describe '#compare_container_scanning_reports' do
-    subject { merge_request.compare_container_scanning_reports }
+    subject { merge_request.compare_container_scanning_reports(current_user) }
 
     let(:project) { create(:project, :repository) }
+    let(:current_user) { project.users.first }
     let(:merge_request) { create(:merge_request, source_project: project) }
 
     let!(:base_pipeline) do
@@ -283,9 +301,10 @@ describe MergeRequest do
   end
 
   describe '#compare_sast_reports' do
-    subject { merge_request.compare_sast_reports }
+    subject { merge_request.compare_sast_reports(current_user) }
 
     let(:project) { create(:project, :repository) }
+    let(:current_user) { project.users.first }
     let(:merge_request) { create(:merge_request, source_project: project) }
 
     let!(:base_pipeline) do
@@ -342,9 +361,10 @@ describe MergeRequest do
   end
 
   describe '#compare_license_management_reports' do
-    subject { merge_request.compare_license_management_reports(project.users.first) }
+    subject { merge_request.compare_license_management_reports(current_user) }
 
     let(:project) { create(:project, :repository) }
+    let(:current_user) { project.users.first }
     let(:merge_request) { create(:merge_request, source_project: project) }
 
     let!(:base_pipeline) do

ee/spec/services/ci/compare_container_scanning_reports_service_spec.rb

 require 'spec_helper'
 
 describe Ci::CompareContainerScanningReportsService do
-  let(:service) { described_class.new(project) }
+  let(:current_user) { project.users.take }
+  let(:service) { described_class.new(project, current_user) }
   let(:project) { create(:project, :repository) }
 
   before do
@@ -31,6 +32,15 @@ describe Ci::CompareContainerScanningReportsService do
         expect(subject[:status]).to eq(:parsed)
       end
 
+      it 'populates fields based on current_user' do
+        payload = subject[:data]['added'].first
+        expect(payload['create_vulnerability_feedback_issue_path']).not_to be_empty
+        expect(payload['create_vulnerability_feedback_merge_request_path']).not_to be_empty
+        expect(payload['create_vulnerability_feedback_dismissal_path']).not_to be_empty
+        expect(payload['create_vulnerability_feedback_issue_path']).not_to be_empty
+        expect(service.current_user).to eq(current_user)
+      end
+
       it 'reports new vulnerability' do
         expect(subject[:data]['added'].count).to eq(1)
         expect(subject[:data]['added'].first['identifiers']).to include(a_hash_including('external_id' => 'CVE-2017-15650'))

ee/spec/services/ci/compare_dependency_scanning_reports_service_spec.rb

@@ -3,7 +3,8 @@
 require 'spec_helper'
 
 describe Ci::CompareDependencyScanningReportsService do
-  let(:service) { described_class.new(project) }
+  let(:current_user) { project.users.take }
+  let(:service) { described_class.new(project, current_user) }
   let(:project) { create(:project, :repository) }
 
   before do
@@ -33,6 +34,15 @@ describe Ci::CompareDependencyScanningReportsService do
         expect(subject[:status]).to eq(:parsed)
       end
 
+      it 'populates fields based on current_user' do
+        payload = subject[:data]['existing'].first
+        expect(payload['create_vulnerability_feedback_issue_path']).not_to be_empty
+        expect(payload['create_vulnerability_feedback_merge_request_path']).not_to be_empty
+        expect(payload['create_vulnerability_feedback_dismissal_path']).not_to be_empty
+        expect(payload['create_vulnerability_feedback_issue_path']).not_to be_empty
+        expect(service.current_user).to eq(current_user)
+      end
+
       it 'reports fixed vulnerability' do
         expect(subject[:data]['added'].count).to eq(1)
         expect(subject[:data]['added'].first['identifiers']).to include(a_hash_including('external_id' => 'CVE-2017-5946'))

ee/spec/services/ci/compare_license_management_reports_service_spec.rb

@@ -3,7 +3,8 @@
 require 'spec_helper'
 
 describe Ci::CompareLicenseManagementReportsService do
-  let(:service) { described_class.new(project, project.users.take) }
+  let(:current_user) { project.users.take }
+  let(:service) { described_class.new(project, current_user) }
   let(:project) { create(:project, :repository) }
 
   before do

ee/spec/services/ci/compare_sast_reports_service_spec.rb

@@ -3,7 +3,8 @@
 require 'spec_helper'
 
 describe Ci::CompareSastReportsService do
-  let(:service) { described_class.new(project) }
+  let(:current_user) { project.users.take }
+  let(:service) { described_class.new(project, current_user) }
   let(:project) { create(:project, :repository) }
 
   before do
@@ -34,6 +35,15 @@ describe Ci::CompareSastReportsService do
         expect(subject[:status]).to eq(:parsed)
       end
 
+      it 'populates fields based on current_user' do
+        payload = subject[:data]['existing'].first
+        expect(payload['create_vulnerability_feedback_issue_path']).not_to be_empty
+        expect(payload['create_vulnerability_feedback_merge_request_path']).not_to be_empty
+        expect(payload['create_vulnerability_feedback_dismissal_path']).not_to be_empty
+        expect(payload['create_vulnerability_feedback_issue_path']).not_to be_empty
+        expect(service.current_user).to eq(current_user)
+      end
+
       it 'reports new vulnerability' do
         expect(subject[:data]['added'].count).to eq(1)
         expect(subject[:data]['added'].first['identifiers']).to include(a_hash_including('name' => 'CWE-120'))