Merge branch...

Merge branch '345173-data-tracking-add-high-conversion-trial-actions-to-continuous-onboarding-database-secure' into 'master'

Secure onboarding progress trial actions

See merge request gitlab-org/gitlab!76550

app/models/onboarding_progress.rb

@@ -20,7 +20,14 @@ class OnboardingProgress < ApplicationRecord
     :issue_created,
     :issue_auto_closed,
     :repository_imported,
-    :repository_mirrored
+    :repository_mirrored,
+    :secure_dependency_scanning_run,
+    :secure_container_scanning_run,
+    :secure_dast_run,
+    :secure_secret_detection_run,
+    :secure_coverage_fuzzing_run,
+    :secure_api_fuzzing_run,
+    :secure_cluster_image_scanning_run
   ].freeze
 
   scope :incomplete_actions, -> (actions) do
@@ -52,12 +59,19 @@ class OnboardingProgress < ApplicationRecord
       where(namespace: namespace).any?
     end
 
-    def register(namespace, action)
-      return unless root_namespace?(namespace) && ACTIONS.include?(action)
+    def register(namespace, actions)
+      actions = Array(actions)
+      return unless root_namespace?(namespace) && actions.difference(ACTIONS).empty?
 
-      action_column = column_name(action)
-      onboarding_progress = find_by(namespace: namespace, action_column => nil)
-      onboarding_progress&.update!(action_column => Time.current)
+      onboarding_progress = find_by(namespace: namespace)
+      return unless onboarding_progress
+
+      now = Time.current
+      nil_actions = actions.select { |action| onboarding_progress[column_name(action)].nil? }
+      return if nil_actions.empty?
+
+      updates = nil_actions.inject({}) { |sum, action| sum.merge!({ column_name(action) => now }) }
+      onboarding_progress.update!(updates)
     end
 
     def completed?(namespace, action)

db/migrate/20211220174504_add_secure_scanning_actions_to_onboarding_progresses.rb

+# frozen_string_literal: true
+
+class AddSecureScanningActionsToOnboardingProgresses < Gitlab::Database::Migration[1.0]
+  def change
+    change_table(:onboarding_progresses, bulk: true) do |t|
+      t.column :secure_dependency_scanning_run_at, :datetime_with_timezone
+      t.column :secure_container_scanning_run_at, :datetime_with_timezone
+      t.column :secure_dast_run_at, :datetime_with_timezone
+      t.column :secure_secret_detection_run_at, :datetime_with_timezone
+      t.column :secure_coverage_fuzzing_run_at, :datetime_with_timezone
+      t.column :secure_cluster_image_scanning_run_at, :datetime_with_timezone
+      t.column :secure_api_fuzzing_run_at, :datetime_with_timezone
+    end
+  end
+end

db/schema_migrations/20211220174504

+80c1ad5815ef68ab1a7d63566d478683b3f9a5169ed15ecd6f44f7f542d40dc8
\ No newline at end of file

db/structure.sql

@@ -16778,7 +16778,14 @@ CREATE TABLE onboarding_progresses (
     issue_auto_closed_at timestamp with time zone,
     repository_imported_at timestamp with time zone,
     repository_mirrored_at timestamp with time zone,
-    issue_created_at timestamp with time zone
+    issue_created_at timestamp with time zone,
+    secure_dependency_scanning_run_at timestamp with time zone,
+    secure_container_scanning_run_at timestamp with time zone,
+    secure_dast_run_at timestamp with time zone,
+    secure_secret_detection_run_at timestamp with time zone,
+    secure_coverage_fuzzing_run_at timestamp with time zone,
+    secure_cluster_image_scanning_run_at timestamp with time zone,
+    secure_api_fuzzing_run_at timestamp with time zone
 );
 
 CREATE SEQUENCE onboarding_progresses_id_seq

ee/app/models/security/scan.rb

@@ -30,7 +30,7 @@ module Security
     enum status: { created: 0, succeeded: 1, failed: 2 }
 
     scope :by_scan_types, -> (scan_types) { where(scan_type: sanitize_scan_types(scan_types)) }
-
+    scope :distinct_scan_types, -> { select(:scan_type).distinct.pluck(:scan_type) }
     scope :scoped_project, -> { where('security_scans.project_id = projects.id') }
 
     scope :has_dismissal_feedback, -> do

ee/app/workers/security/store_scans_worker.rb

@@ -25,10 +25,12 @@ module Security
     private
 
     def record_onboarding_progress(pipeline)
-      # We only record SAST scans since it's a Free feature and available to all users
-      return unless pipeline.security_scans.sast.any?
+      recordable_scan_actions = Security::Scan.scan_types.keys
+        .inject({}) { |hash, scan_type| hash.merge!(scan_type => "secure_#{scan_type}_run".to_sym) }
+        .merge('sast' => :security_scan_enabled) # sast has an exceptional action name
 
-      OnboardingProgressService.new(pipeline.project.namespace).execute(action: :security_scan_enabled)
+      actions = pipeline.security_scans.distinct_scan_types.map { |scan_type| recordable_scan_actions[scan_type] }
+      OnboardingProgressService.new(pipeline.project.namespace).execute(action: actions)
     end
   end
 end

ee/spec/models/security/scan_spec.rb

@@ -86,6 +86,18 @@ RSpec.describe Security::Scan do
     end
   end
 
+  describe '.distinct_scan_types' do
+    let_it_be(:sast_scan) { create(:security_scan, scan_type: :sast) }
+    let_it_be(:sast_scan2) { create(:security_scan, scan_type: :sast) }
+    let_it_be(:dast_scan) { create(:security_scan, scan_type: :dast) }
+
+    let(:expected_scans) { %w(sast dast) }
+
+    subject { described_class.distinct_scan_types }
+
+    it { is_expected.to match_array(expected_scans) }
+  end
+
   describe '.latest_successful' do
     let!(:first_successful_scan) { create(:security_scan, latest: false, status: :succeeded) }
     let!(:second_successful_scan) { create(:security_scan, latest: true, status: :succeeded) }

ee/spec/workers/security/store_scans_worker_spec.rb

@@ -38,16 +38,26 @@ RSpec.describe Security::StoreScansWorker do
         expect(Security::StoreScansService).to have_received(:execute)
       end
 
-      it_behaves_like 'records an onboarding progress action', :security_scan_enabled do
-        let(:namespace) { pipeline.project.namespace }
-      end
+      scan_types_actions = {
+        "sast" => :security_scan_enabled,
+        "dependency_scanning" => :secure_dependency_scanning_run,
+        "container_scanning" => :secure_container_scanning_run,
+        "dast" => :secure_dast_run,
+        "secret_detection" => :secure_secret_detection_run,
+        "coverage_fuzzing" => :secure_coverage_fuzzing_run,
+        "api_fuzzing" => :secure_api_fuzzing_run,
+        "cluster_image_scanning" => :secure_cluster_image_scanning_run
+      }.freeze
 
-      context 'dast scan' do
-        let_it_be(:dast_scan) { create(:security_scan, scan_type: :dast) }
-        let_it_be(:pipeline) { dast_scan.pipeline }
-        let_it_be(:dast_build) { pipeline.security_scans.dast.last&.build }
+      scan_types_actions.each do |scan_type, action|
+        context "security #{scan_type}" do
+          let_it_be(:scan) { create(:security_scan, scan_type: scan_type) }
+          let_it_be(:pipeline) { scan.pipeline }
 
-        it_behaves_like 'does not record an onboarding progress action'
+          it_behaves_like 'records an onboarding progress action', [action] do
+            let(:namespace) { pipeline.project.namespace }
+          end
+        end
       end
     end
   end

spec/models/onboarding_progress_spec.rb

@@ -131,6 +131,7 @@ RSpec.describe OnboardingProgress do
   end
 
   describe '.register' do
+    context 'for a single action' do
       subject(:register_action) { described_class.register(namespace, action) }
 
       context 'when the namespace was onboarded' do
@@ -142,6 +143,13 @@ RSpec.describe OnboardingProgress do
           expect { register_action }.to change { described_class.completed?(namespace, action) }.from(false).to(true)
         end
 
+        it 'does not override timestamp', :aggregate_failures do
+          expect(described_class.find_by_namespace_id(namespace.id).subscription_created_at).to be_nil
+          register_action
+          expect(described_class.find_by_namespace_id(namespace.id).subscription_created_at).not_to be_nil
+          expect { described_class.register(namespace, action) }.not_to change { described_class.find_by_namespace_id(namespace.id).subscription_created_at }
+        end
+
         context 'when the action does not exist' do
           let(:action) { :foo }
 
@@ -158,6 +166,55 @@ RSpec.describe OnboardingProgress do
       end
     end
 
+    context 'for multiple actions' do
+      let(:action1) { :security_scan_enabled }
+      let(:action2) { :secure_dependency_scanning_run }
+      let(:actions) { [action1, action2] }
+
+      subject(:register_action) { described_class.register(namespace, actions) }
+
+      context 'when the namespace was onboarded' do
+        before do
+          described_class.onboard(namespace)
+        end
+
+        it 'registers the actions for the namespace' do
+          expect { register_action }.to change {
+            [described_class.completed?(namespace, action1), described_class.completed?(namespace, action2)]
+          }.from([false, false]).to([true, true])
+        end
+
+        it 'does not override timestamp', :aggregate_failures do
+          described_class.register(namespace, [action1])
+          expect(described_class.find_by_namespace_id(namespace.id).security_scan_enabled_at).not_to be_nil
+          expect(described_class.find_by_namespace_id(namespace.id).secure_dependency_scanning_run_at).to be_nil
+
+          expect { described_class.register(namespace, [action1, action2]) }.not_to change {
+            described_class.find_by_namespace_id(namespace.id).security_scan_enabled_at
+          }
+          expect(described_class.find_by_namespace_id(namespace.id).secure_dependency_scanning_run_at).not_to be_nil
+        end
+
+        context 'when one of the actions does not exist' do
+          let(:action2) { :foo }
+
+          it 'does not register any action for the namespace' do
+            expect { register_action }.not_to change {
+              [described_class.completed?(namespace, action1), described_class.completed?(namespace, action2)]
+            }.from([false, nil])
+          end
+        end
+      end
+
+      context 'when the namespace was not onboarded' do
+        it 'does not register the action for the namespace' do
+          expect { register_action }.not_to change { described_class.completed?(namespace, action1) }.from(false)
+          expect { described_class.register(namespace, action) }.not_to change { described_class.completed?(namespace, action2) }.from(false)
+        end
+      end
+    end
+  end
+
   describe '.completed?' do
     subject { described_class.completed?(namespace, action) }