Merge branch '238156_remove_old_security_findings_periodically' into 'master'

Remove security findings periodically

See merge request gitlab-org/gitlab!44336

app/services/ci/destroy_expired_job_artifacts_service.rb

@@ -39,6 +39,13 @@ module Ci
       return false if artifacts.empty?
 
       artifacts.each(&:destroy!)
+      run_after_destroy(artifacts)
+
+      true # This is required because of the design of `loop_until` method.
     end
+
+    def run_after_destroy(artifacts); end
   end
 end
+
+Ci::DestroyExpiredJobArtifactsService.prepend_if_ee('EE::Ci::DestroyExpiredJobArtifactsService')

ee/app/models/ee/ci/build.rb

@@ -21,7 +21,7 @@ module EE
       }.with_indifferent_access.freeze
 
       EE_RUNNER_FEATURES = {
-        vault_secrets: -> (build) { build.ci_secrets_management_available? && build.secrets?}
+        vault_secrets: -> (build) { build.ci_secrets_management_available? && build.secrets? }
       }.freeze
 
       prepended do

ee/app/models/security/finding.rb

@@ -22,5 +22,6 @@ module Security
     validates :project_fingerprint, presence: true, length: { maximum: 40 }
 
     scope :by_project_fingerprint, -> (fingerprints) { where(project_fingerprint: fingerprints) }
+    scope :by_build_ids, -> (build_ids) { joins(scan: :build).where(ci_builds: { id: build_ids }) }
   end
 end

ee/app/services/ee/ci/destroy_expired_job_artifacts_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Ci
+    module DestroyExpiredJobArtifactsService
+      def run_after_destroy(artifacts)
+        destroy_security_findings_for(artifacts) if artifacts.first.is_a?(::Ci::JobArtifact)
+      end
+
+      def destroy_security_findings_for(artifacts)
+        job_ids = artifacts.map(&:job_id)
+
+        ::Security::Finding.by_build_ids(job_ids).delete_all
+      end
+    end
+  end
+end

ee/spec/models/security/finding_spec.rb

@@ -22,4 +22,13 @@ RSpec.describe Security::Finding do
 
     it { is_expected.to match_array(expected_findings) }
   end
+
+  describe '.by_build_ids' do
+    let!(:finding_1) { create(:security_finding) }
+    let!(:finding_2) { create(:security_finding) }
+
+    subject { described_class.by_build_ids(finding_1.scan.build_id) }
+
+    it { is_expected.to eq([finding_1]) }
+  end
 end

ee/spec/services/ee/ci/destroy_expired_job_artifacts_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared_state do
+  include ExclusiveLeaseHelpers
+
+  describe '.execute' do
+    subject { service.execute }
+
+    let(:service) { described_class.new }
+
+    let_it_be(:artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
+    let_it_be(:security_scan) { create(:security_scan, build: artifact.job) }
+    let_it_be(:security_finding) { create(:security_finding, scan: security_scan) }
+
+    before(:all) do
+      artifact.job.pipeline.unlocked!
+    end
+
+    context 'when artifact is expired' do
+      context 'when artifact is not locked' do
+        it 'destroys job artifact and the security finding' do
+          expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
+                            .and change { Security::Finding.count }.from(1).to(0)
+        end
+      end
+
+      context 'when artifact is locked' do
+        before do
+          artifact.job.pipeline.reload.artifacts_locked!
+        end
+
+        it 'does not destroy job artifact' do
+          expect { subject }.to not_change { Ci::JobArtifact.count }
+                            .and not_change { Security::Finding.count }.from(1)
+        end
+      end
+    end
+
+    context 'when artifact is not expired' do
+      before do
+        artifact.update_column(:expire_at, 1.day.since)
+      end
+
+      it 'does not destroy expired job artifacts' do
+        expect { subject }.to not_change { Ci::JobArtifact.count }
+                          .and not_change { Security::Finding.count }.from(1)
+      end
+    end
+
+    context 'when artifact is permanent' do
+      before do
+        artifact.update_column(:expire_at, nil)
+      end
+
+      it 'does not destroy expired job artifacts' do
+        expect { subject }.to not_change { Ci::JobArtifact.count }
+                          .and not_change { Security::Finding.count }.from(1)
+      end
+    end
+
+    context 'when failed to destroy artifact' do
+      before do
+        stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_LIMIT', 10)
+
+        allow_next_found_instance_of(Ci::JobArtifact) do |artifact|
+          allow(artifact).to receive(:destroy!).and_raise(ActiveRecord::RecordNotDestroyed)
+        end
+      end
+
+      it 'raises an exception and stop destroying' do
+        expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed)
+                          .and not_change { Security::Finding.count }.from(1)
+      end
+    end
+
+    context 'when there are artifacts more than batch sizes' do
+      before do
+        stub_const('Ci::DestroyExpiredJobArtifactsService::BATCH_SIZE', 1)
+
+        second_artifact.job.pipeline.unlocked!
+      end
+
+      let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
+      let!(:second_security_scan) { create(:security_scan, build: second_artifact.job) }
+      let!(:second_security_finding) { create(:security_finding, scan: second_security_scan) }
+
+      it 'destroys all expired artifacts' do
+        expect { subject }.to change { Ci::JobArtifact.count }.by(-2)
+                          .and change { Security::Finding.count }.from(2).to(0)
+      end
+    end
+
+    context 'when some artifacts are locked' do
+      before do
+        pipeline = create(:ci_pipeline, locked: :artifacts_locked)
+        job = create(:ci_build, pipeline: pipeline)
+        create(:ci_job_artifact, expire_at: 1.day.ago, job: job)
+        security_scan = create(:security_scan, build: job)
+        create(:security_finding, scan: security_scan)
+      end
+
+      it 'destroys only unlocked artifacts' do
+        expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
+                          .and change { Security::Finding.count }.from(2).to(1)
+      end
+    end
+  end
+end

spec/services/ci/destroy_expired_job_artifacts_service_spec.rb

@@ -9,9 +9,10 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
     subject { service.execute }
 
     let(:service) { described_class.new }
-    let!(:artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
 
-    before do
+    let_it_be(:artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
+
+    before(:all) do
       artifact.job.pipeline.unlocked!
     end
 
@@ -38,7 +39,9 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
     end
 
     context 'when artifact is not expired' do
-      let!(:artifact) { create(:ci_job_artifact, expire_at: 1.day.since) }
+      before do
+        artifact.update_column(:expire_at, 1.day.since)
+      end
 
       it 'does not destroy expired job artifacts' do
         expect { subject }.not_to change { Ci::JobArtifact.count }
@@ -46,7 +49,9 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
     end
 
     context 'when artifact is permanent' do
-      let!(:artifact) { create(:ci_job_artifact, expire_at: nil) }
+      before do
+        artifact.update_column(:expire_at, nil)
+      end
 
       it 'does not destroy expired job artifacts' do
         expect { subject }.not_to change { Ci::JobArtifact.count }