Merge branch '7586-introduce_reports_diff' into 'master'

Introduce security report diffs

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

ee/app/models/ee/ci/pipeline.rb

@@ -146,7 +146,7 @@ module EE
       end
 
       def security_reports
-        ::Gitlab::Ci::Reports::Security::Reports.new.tap do |security_reports|
+        ::Gitlab::Ci::Reports::Security::Reports.new(sha).tap do |security_reports|
           builds.latest.with_reports(::Ci::JobArtifact.security_reports).each do |build|
             build.collect_security_reports!(security_reports)
           end

ee/app/services/security/compare_reports_base_service.rb

+# frozen_string_literal: true
+
+module Security
+  class CompareReportsBaseService
+    attr_reader :base_report, :head_report, :report_diff
+
+    def initialize(base_report, head_report)
+      @base_report = base_report
+      @head_report = head_report
+      @report_diff = ::Gitlab::Ci::Reports::Security::ReportsDiff.new
+    end
+
+    def execute
+      # If there is nothing to compare with, just consider all
+      # head occurrences as added
+      if base_report.occurrences.blank?
+        report_diff.added = head_report.occurrences
+
+        return report_diff
+      end
+
+      update_base_occurrence_locations
+
+      report_diff.added = head_report.occurrences - base_report.occurrences
+      report_diff.fixed = base_report.occurrences - head_report.occurrences
+      report_diff.existing = base_report.occurrences & head_report.occurrences
+
+      report_diff
+    end
+
+    private
+
+    def update_base_occurrence_locations
+      # Override this method with an update strategy in subclass if any?
+    end
+  end
+end

ee/app/services/security/compare_reports_sast_service.rb

+# frozen_string_literal: true
+
+module Security
+  class CompareReportsSastService < CompareReportsBaseService
+    include Gitlab::Utils::StrongMemoize
+
+    attr_reader :project
+
+    def initialize(base_report, head_report, project)
+      @project = project
+      super(base_report, head_report)
+    end
+
+    private
+
+    # Update location for base report occurrences by leveraging the git diff
+    def update_base_occurrence_locations
+      return unless git_diff
+
+      # Group by file path to optimize the usage of Diff::File and Diff::LineMapper
+      base_report.occurrences.group_by(&:file_path).each do |file_path, occurrences|
+        update_locations_by_file(git_diff, file_path, occurrences)
+      end
+    end
+
+    def update_locations_by_file(git_diff, file_path, occurrences)
+      diff_file = git_diff.diff_file_with_old_path(file_path)
+
+      return if diff_file.nil? || diff_file.deleted_file?
+
+      update_locations(diff_file, occurrences)
+    end
+
+    def update_locations(diff_file, occurrences)
+      line_mapper = Gitlab::Diff::LineMapper.new(diff_file)
+
+      occurrences.each do |occurrence|
+        new_path = line_mapper.diff_file.new_path
+        new_start_line = line_mapper.old_to_new(occurrence.start_line)
+        new_end_line = occurrence.end_line.present? ? line_mapper.old_to_new(occurrence.end_line) : nil
+
+        # skip if the line has been removed
+        # NB: if the line's content has changed it will be nil too
+        next if new_start_line.nil?
+
+        new_location = Gitlab::Ci::Reports::Security::Locations::Sast.new(
+          file_path: new_path,
+          start_line: new_start_line,
+          end_line: new_end_line
+        )
+        occurrence.update_location(new_location)
+      end
+    end
+
+    def git_diff
+      strong_memoize(:git_diff) do
+        compare = CompareService.new(project, head_report.commit_sha).execute(project, base_report.commit_sha, straight: false)
+        next unless compare
+
+        compare.diffs(expanded: true)
+      end
+    end
+  end
+end

ee/lib/gitlab/ci/reports/security/occurrence.rb

@@ -11,6 +11,7 @@ module Gitlab
           attr_reader :location
           attr_reader :metadata_version
           attr_reader :name
+          attr_reader :old_location
           attr_reader :project_fingerprint
           attr_reader :raw_metadata
           attr_reader :report_type
@@ -18,6 +19,8 @@ module Gitlab
           attr_reader :severity
           attr_reader :uuid
 
+          delegate :file_path, :start_line, :end_line, to: :location
+
           def initialize(compare_key:, identifiers:, location:, metadata_version:, name:, raw_metadata:, report_type:, scanner:, uuid:, confidence: nil, severity: nil) # rubocop:disable Metrics/ParameterLists
             @compare_key = compare_key
             @confidence = confidence
@@ -57,11 +60,17 @@ module Gitlab
             identifiers.first
           end
 
+          def update_location(new_location)
+            @old_location = location
+            @location = new_location
+          end
+
           def ==(other)
             other.report_type == report_type &&
               other.location == location &&
               other.primary_identifier == primary_identifier
           end
+          alias_method :eql?, :== # eql? is necessary in some cases like array intersection
 
           private
 

ee/lib/gitlab/ci/reports/security/report.rb

@@ -6,13 +6,16 @@ module Gitlab
       module Security
         class Report
           attr_reader :type
+          attr_reader :commit_sha
           attr_reader :occurrences
           attr_reader :scanners
           attr_reader :identifiers
+
           attr_accessor :error
 
-          def initialize(type)
+          def initialize(type, commit_sha)
             @type = type
+            @commit_sha = commit_sha
             @occurrences = []
             @scanners = {}
             @identifiers = {}

ee/lib/gitlab/ci/reports/security/reports.rb

@@ -5,14 +5,15 @@ module Gitlab
     module Reports
       module Security
         class Reports
-          attr_reader :reports
+          attr_reader :reports, :commit_sha
 
-          def initialize
+          def initialize(commit_sha)
             @reports = {}
+            @commit_sha = commit_sha
           end
 
           def get_report(report_type)
-            reports[report_type] ||= Report.new(report_type)
+            reports[report_type] ||= Report.new(report_type, commit_sha)
           end
         end
       end

ee/lib/gitlab/ci/reports/security/reports_diff.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Reports
+      module Security
+        class ReportsDiff
+          attr_accessor :added, :existing, :fixed
+
+          def initialize
+            @added = []
+            @existing = []
+            @fixed = []
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/factories/ci/reports/security/locations/sast.rb

@@ -13,5 +13,11 @@ FactoryBot.define do
     initialize_with do
       ::Gitlab::Ci::Reports::Security::Locations::Sast.new(attributes)
     end
+
+    trait :dynamic do
+      sequence(:file_path, 'a') { |n| "path/#{n}" }
+      start_line { Random.rand(20) }
+      end_line { start_line + Random.rand(5) }
+    end
   end
 end

ee/spec/factories/ci/reports/security/occurrences.rb

@@ -34,6 +34,10 @@ FactoryBot.define do
 
     skip_create
 
+    trait :dynamic do
+      location { FactoryBot.build(:ci_reports_security_locations_sast, :dynamic) }
+    end
+
     initialize_with do
       ::Gitlab::Ci::Reports::Security::Occurrence.new(attributes)
     end

ee/spec/factories/ci/reports/security/reports.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :ci_reports_security_report, class: ::Gitlab::Ci::Reports::Security::Report do
+    type :sast
+    commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) }
+
+    transient do
+      occurrences []
+    end
+
+    after :build do |report, evaluator|
+      evaluator.occurrences.each { |o| report.add_occurrence(o) }
+    end
+
+    skip_create
+
+    initialize_with do
+      ::Gitlab::Ci::Reports::Security::Report.new(type, commit_sha)
+    end
+  end
+end

ee/spec/lib/gitlab/ci/parsers/security/container_scanning_spec.rb

@@ -17,7 +17,7 @@ describe Gitlab::Ci::Parsers::Security::ContainerScanning do
     let(:project) { artifact.project }
     let(:pipeline) { artifact.job.pipeline }
     let(:artifact) { create(:ee_ci_job_artifact, :container_scanning) }
-    let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type) }
+    let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline.sha) }
 
     before do
       artifact.each_blob do |blob|

ee/spec/lib/gitlab/ci/parsers/security/dast_spec.rb

@@ -9,7 +9,7 @@ describe Gitlab::Ci::Parsers::Security::Dast do
     let(:project) { artifact.project }
     let(:pipeline) { artifact.job.pipeline }
     let(:artifact) { create(:ee_ci_job_artifact, :dast) }
-    let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type) }
+    let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline.sha) }
 
     before do
       artifact.each_blob do |blob|

ee/spec/lib/gitlab/ci/parsers/security/dependency_scanning_spec.rb

@@ -9,7 +9,7 @@ describe Gitlab::Ci::Parsers::Security::DependencyScanning do
     let(:project) { artifact.project }
     let(:pipeline) { artifact.job.pipeline }
     let(:artifact) { create(:ee_ci_job_artifact, :dependency_scanning) }
-    let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type) }
+    let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline.sha) }
     let(:parser) { described_class.new }
 
     where(:report_format, :occurrence_count, :identifier_count, :scanner_count, :file_path, :package_name, :package_version, :version) do

ee/spec/lib/gitlab/ci/parsers/security/sast_spec.rb

@@ -6,7 +6,7 @@ describe Gitlab::Ci::Parsers::Security::Sast do
   describe '#parse!' do
     let(:project) { artifact.project }
     let(:pipeline) { artifact.job.pipeline }
-    let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type) }
+    let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline.sha) }
     let(:parser) { described_class.new }
 
     where(report_format: %i(sast sast_deprecated))

ee/spec/lib/gitlab/ci/reports/security/occurrence_spec.rb

@@ -61,6 +61,15 @@ describe Gitlab::Ci::Reports::Security::Occurrence do
     end
   end
 
+  describe "delegation" do
+    subject { create(:ci_reports_security_occurrence) }
+    %i[file_path start_line end_line].each do |attribute|
+      it "delegates attribute #{attribute} to location" do
+        expect(subject.public_send(attribute)).to eq(subject.location.public_send(attribute))
+      end
+    end
+  end
+
   describe '#to_hash' do
     let(:occurrence) { create(:ci_reports_security_occurrence) }
 
@@ -97,7 +106,29 @@ describe Gitlab::Ci::Reports::Security::Occurrence do
     end
   end
 
-  describe '#==' do
+  describe '#update_location' do
+    let(:old_location) { create(:ci_reports_security_locations_sast, file_path: 'old_file.rb') }
+    let(:new_location) { create(:ci_reports_security_locations_sast, file_path: 'new_file.rb') }
+
+    let(:occurrence) { create(:ci_reports_security_occurrence, location: old_location) }
+
+    subject { occurrence.update_location(new_location) }
+
+    it 'assigns the new location and returns it' do
+      subject
+
+      expect(occurrence.location).to eq(new_location)
+      is_expected.to eq(new_location)
+    end
+
+    it 'assigns the old location' do
+      subject
+
+      expect(occurrence.old_location).to eq(old_location)
+    end
+  end
+
+  describe '#== and eql?' do
     using RSpec::Parameterized::TableSyntax
 
     let(:identifier) { create(:ci_reports_security_identifier) }
@@ -116,9 +147,13 @@ describe Gitlab::Ci::Reports::Security::Occurrence do
       let(:occurrence_1) { create(:ci_reports_security_occurrence, report_type: report_type_1, location: location_1.call, identifiers: [identifier_1.call]) }
       let(:occurrence_2) { create(:ci_reports_security_occurrence, report_type: report_type_2, location: location_2.call, identifiers: [identifier_2.call]) }
 
-      it "returns #{params[:equal]}" do
+      it "returns #{params[:equal]} for ==" do
         expect(occurrence_1 == occurrence_2).to eq(equal)
       end
+
+      it "returns #{params[:equal]} for eq?" do
+        expect(occurrence_1.eql?(occurrence_2)).to eq(equal)
+      end
     end
   end
 end

ee/spec/lib/gitlab/ci/reports/security/report_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 describe Gitlab::Ci::Reports::Security::Report do
   let(:pipeline) { create(:ci_pipeline) }
-  let(:report) { described_class.new('sast') }
+  let(:report) { described_class.new('sast', pipeline.sha) }
 
   it { expect(report.type).to eq('sast') }
 

ee/spec/lib/gitlab/ci/reports/security/reports_spec.rb

@@ -3,7 +3,8 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Reports::Security::Reports do
-  let(:security_reports) { described_class.new }
+  let(:commit_sha) { '20410773a37f49d599e5f0d45219b39304763538' }
+  let(:security_reports) { described_class.new(commit_sha) }
 
   describe '#get_report' do
     subject { security_reports.get_report(report_type) }
@@ -12,10 +13,11 @@ describe Gitlab::Ci::Reports::Security::Reports do
       let(:report_type) { 'sast' }
 
       it { expect(subject.type).to eq('sast') }
+      it { expect(subject.commit_sha).to eq(commit_sha) }
 
       it 'initializes a new report and returns it' do
         expect(Gitlab::Ci::Reports::Security::Report).to receive(:new)
-          .with('sast').and_call_original
+          .with('sast', commit_sha).and_call_original
 
         is_expected.to be_a(Gitlab::Ci::Reports::Security::Report)
       end

ee/spec/models/ci/build_spec.rb

@@ -120,7 +120,7 @@ describe Ci::Build do
   end
 
   describe '#collect_security_reports!' do
-    let(:security_reports) { ::Gitlab::Ci::Reports::Security::Reports.new }
+    let(:security_reports) { ::Gitlab::Ci::Reports::Security::Reports.new(pipeline.sha) }
 
     subject { job.collect_security_reports!(security_reports) }
 

ee/spec/models/ci/pipeline_spec.rb

@@ -204,6 +204,11 @@ describe Ci::Pipeline do
         create(:ee_ci_job_artifact, :container_scanning, job: build_cs_1, project: project)
       end
 
+      it 'assigns pipeline commit_sha to the reports' do
+        expect(subject.commit_sha).to eq(pipeline.sha)
+        expect(subject.reports.values.map(&:commit_sha).uniq).to contain_exactly(pipeline.sha)
+      end
+
       it 'returns security reports with collected data grouped as expected' do
         expect(subject.reports.keys).to contain_exactly('sast', 'dependency_scanning', 'container_scanning')
         expect(subject.get_report('sast').occurrences.size).to eq(66)

ee/spec/services/security/compare_reports_base_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Security::CompareReportsBaseService, '#execute' do
+  let(:occurrence_1) { create(:ci_reports_security_occurrence, :dynamic) }
+  let(:occurrence_2) { create(:ci_reports_security_occurrence, :dynamic) }
+  let(:occurrence_3) { create(:ci_reports_security_occurrence, :dynamic) }
+  let(:base_report) { create(:ci_reports_security_report, occurrences: [occurrence_1, occurrence_2]) }
+  let(:head_report) { create(:ci_reports_security_report, occurrences: [occurrence_2, occurrence_3]) }
+
+  subject { described_class.new(base_report, head_report).execute }
+
+  it 'exposes added occurrences' do
+    expect(subject.added).to contain_exactly(occurrence_3)
+  end
+
+  it 'exposes existing occurrences' do
+    expect(subject.existing).to contain_exactly(occurrence_2)
+  end
+
+  it 'exposes fixed occurrences' do
+    expect(subject.fixed).to contain_exactly(occurrence_1)
+  end
+end

ee/spec/services/security/compare_reports_sast_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Security::CompareReportsSastService, '#execute' do
+  # Setup a project with 2 reports having 1 common vulnerability whose location is updated
+  let(:project) { create(:project, :repository) }
+  let(:branch_name) { 'master' }
+  let(:identifier) { create(:ci_reports_security_identifier) }
+  let(:location) { create(:ci_reports_security_locations_sast, file_path: 'a.js', start_line: 2, end_line: 4) }
+  let(:location_updated) { create(:ci_reports_security_locations_sast, file_path: 'a.js', start_line: 3, end_line: 5) }
+  let(:occurrence_1) { create(:ci_reports_security_occurrence, :dynamic, name: 'occurrence_1') }
+  let(:occurrence_2) { create(:ci_reports_security_occurrence, name: 'occurrence_2', location: location, identifiers: [identifier]) }
+  let(:occurrence_2_updated) { create(:ci_reports_security_occurrence, name: 'occurrence_2_updated', location: location_updated, identifiers: [identifier]) }
+  let(:occurrence_3) { create(:ci_reports_security_occurrence, :dynamic, name: 'occurrence_3') }
+  let(:base_report) { create(:ci_reports_security_report, commit_sha: base_sha, occurrences: [occurrence_1, occurrence_2]) }
+  let(:head_report) { create(:ci_reports_security_report, commit_sha: head_sha, occurrences: [occurrence_2_updated, occurrence_3]) }
+
+  let(:file_content) do
+    <<-DIFF.strip_heredoc
+      var auth = "Jane";
+      if (userInput == auth) {
+          console.log(userInput);
+      }
+    DIFF
+  end
+
+  let(:file_content_updated) do
+    <<-DIFF.strip_heredoc
+      var auth = "Jane";
+      // Add a comment
+      if (userInput == auth) {
+          console.log(userInput);
+      }
+    DIFF
+  end
+
+  let(:base_sha) do
+    create_file('a.js', file_content)
+    project.commit(branch_name).id
+  end
+
+  subject { described_class.new(base_report, head_report, project).execute }
+
+  shared_examples 'report diff with existing occurrence' do
+    it 'returns added, existing and fixed occurrences' do
+      expect(subject.added).to contain_exactly(occurrence_3)
+      expect(subject.existing).to contain_exactly(occurrence_2)
+      expect(subject.fixed).to contain_exactly(occurrence_1)
+    end
+
+    it 'returns existing occurrence with old location set' do
+      expect(subject.existing.first.old_location).to eq(location)
+    end
+  end
+
+  shared_examples 'report diff without existing occurrence' do
+    it 'returns added and fixed occurrences but no existing ones' do
+      expect(subject.added).to contain_exactly(occurrence_3, occurrence_2_updated)
+      expect(subject.existing).to be_empty
+      expect(subject.fixed).to contain_exactly(occurrence_1, occurrence_2)
+    end
+  end
+
+  context 'when commit_sha are equal' do
+    let(:head_sha) { base_sha }
+
+    it_behaves_like 'report diff without existing occurrence'
+  end
+
+  context 'when there is no git diff available' do
+    let(:head_sha) do
+      update_file('a.js', file_content_updated)
+      project.commit(branch_name).id
+    end
+
+    before do
+      compare_service = spy
+      allow(CompareService).to receive(:new).and_return(compare_service)
+      allow(compare_service).to receive(:execute).and_return(nil)
+    end
+
+    it_behaves_like 'report diff without existing occurrence'
+  end
+
+  context 'when vulnerability line numbers are updated' do
+    let(:head_sha) do
+      update_file('a.js', file_content_updated)
+      project.commit(branch_name).id
+    end
+
+    it_behaves_like 'report diff with existing occurrence'
+
+    context 'without end_line' do
+      let(:location) { create(:ci_reports_security_locations_sast, file_path: 'a.js', start_line: 2, end_line: nil) }
+      let(:location_updated) { create(:ci_reports_security_locations_sast, file_path: 'a.js', start_line: 3, end_line: nil) }
+
+      it_behaves_like 'report diff with existing occurrence'
+    end
+
+    context 'when line content is updated' do
+      let(:file_content_updated) do
+        <<-DIFF.strip_heredoc
+          var auth = "Jane";
+          // Add a comment
+          if (input == auth) { // Add a comment here to change line content
+              console.log(userInput);
+          }
+        DIFF
+      end
+
+      it_behaves_like 'report diff without existing occurrence'
+    end
+  end
+
+  context 'when vulnerability file path is updated' do
+    let(:location_updated) { create(:ci_reports_security_locations_sast, file_path: 'b.js', start_line: 2, end_line: 4) }
+
+    let(:head_sha) do
+      delete_file('a.js')
+      create_file('b.js', file_content)
+      project.commit(branch_name).id
+    end
+
+    it_behaves_like 'report diff with existing occurrence'
+  end
+
+  context 'when vulnerability file path and lines are updated' do
+    let(:location_updated) { create(:ci_reports_security_locations_sast, file_path: 'b.js', start_line: 3, end_line: 5) }
+
+    let(:head_sha) do
+      delete_file('a.js')
+      create_file('b.js', file_content_updated)
+      project.commit(branch_name).id
+    end
+
+    it_behaves_like 'report diff with existing occurrence'
+  end
+
+  def create_file(file_name, content)
+    Files::CreateService.new(
+      project,
+      project.owner,
+      commit_message: 'Update',
+      start_branch: branch_name,
+      branch_name: branch_name,
+      file_path: file_name,
+      file_content: content
+    ).execute
+  end
+
+  def update_file(file_name, content)
+    Files::UpdateService.new(
+      project,
+      project.owner,
+      commit_message: 'Update',
+      start_branch: branch_name,
+      branch_name: branch_name,
+      file_path: file_name,
+      file_content: content
+    ).execute
+  end
+
+  def delete_file(file_name)
+    Files::DeleteService.new(
+      project,
+      project.owner,
+      commit_message: 'Update',
+      start_branch: branch_name,
+      branch_name: branch_name,
+      file_path: file_name
+    ).execute
+  end
+end

ee/spec/services/security/store_reports_service_spec.rb

@@ -28,7 +28,7 @@ describe Security::StoreReportsService, '#execute' do
     end
 
     context 'when StoreReportService returns an error for a report' do
-      let(:reports) { Gitlab::Ci::Reports::Security::Reports.new }
+      let(:reports) { Gitlab::Ci::Reports::Security::Reports.new(pipeline.sha) }
       let(:sast_report) { reports.get_report('sast') }
       let(:dast_report) { reports.get_report('dast') }
       let(:success) { { status: :success } }