Merge branch '339751-add-additional-policy-validation' into 'master'

Add validation to check if branches are valid for security policy

See merge request gitlab-org/gitlab!78714

ee/app/services/security/security_orchestration_policies/policy_commit_service.rb

@@ -8,6 +8,9 @@ module Security
 
         return error('Security Policy Project does not exist') unless policy_configuration.present?
 
+        validation_result = validate_policy_yaml
+        return error(validation_result[:message], :bad_request) if validation_result[:status] != :success
+
         process_policy_result = process_policy
         return process_policy_result if process_policy_result[:status] != :success
 
@@ -21,6 +24,12 @@ module Security
 
       private
 
+      def validate_policy_yaml
+        Security::SecurityOrchestrationPolicies::ValidatePolicyService
+          .new(project: project, params: { policy: policy })
+          .execute
+      end
+
       def process_policy
         ProcessPolicyService.new(
           policy_configuration: policy_configuration,

ee/app/services/security/security_orchestration_policies/process_policy_service.rb

@@ -16,7 +16,6 @@ module Security
         name = params[:name]
         operation = params[:operation]
 
-        return error("Invalid policy type", :bad_request) unless Security::OrchestrationPolicyConfiguration::AVAILABLE_POLICY_TYPES.include?(type)
         return error("Name should be same as the policy name", :bad_request) if name && operation != :replace && policy[:name] != name
 
         policy_hash = policy_configuration.policy_hash.dup || {}

ee/app/services/security/security_orchestration_policies/validate_policy_service.rb

+# frozen_string_literal: true
+
+module Security
+  module SecurityOrchestrationPolicies
+    class ValidatePolicyService < ::BaseProjectService
+      def execute
+        return success if policy_disabled?
+
+        return error(s_('SecurityOrchestration|Invalid policy type')) if invalid_policy_type?
+        return error(s_('SecurityOrchestration|Policy cannot be enabled without branch information')) if blank_branch_for_rule?
+        return error(s_('SecurityOrchestration|Policy cannot be enabled for non-existing branches (%{branches})') % { branches: missing_branch_names.join(', ') }) if missing_branch_for_rule?
+
+        success
+      end
+
+      private
+
+      def policy_disabled?
+        !policy&.[](:enabled)
+      end
+
+      def invalid_policy_type?
+        return true if policy[:type].blank?
+
+        !Security::OrchestrationPolicyConfiguration::AVAILABLE_POLICY_TYPES.include?(policy[:type].to_sym)
+      end
+
+      def blank_branch_for_rule?
+        policy[:rules].any? { |rule| rule[:clusters].blank? && rule[:branches].blank? }
+      end
+
+      def missing_branch_for_rule?
+        return false if project.blank?
+
+        missing_branch_names.present?
+      end
+
+      def missing_branch_names
+        strong_memoize(:missing_branch_names) do
+          policy[:rules]
+            .select { |rule| rule[:clusters].blank? }
+            .flat_map { |rule| rule[:branches] }
+            .compact
+            .uniq
+            .select { |pattern| RefMatcher.new(pattern).matching(branches_for_project).blank? }
+        end
+      end
+
+      def policy
+        @policy ||= params[:policy]
+      end
+
+      def branches_for_project
+        strong_memoize(:branches_for_project) do
+          repository.branch_names
+        end
+      end
+    end
+  end
+end

ee/spec/factories/security/policies.rb

@@ -17,11 +17,11 @@ FactoryBot.define do
     sequence(:name) { |n| "test-policy-#{n}" }
     description { 'This policy enforces to run DAST for every pipeline within the project' }
     enabled { true }
-    rules { [{ type: 'pipeline', branches: %w[production] }] }
+    rules { [{ type: 'pipeline', branches: %w[master] }] }
     actions { [{ scan: 'dast', site_profile: 'Site Profile', scanner_profile: 'Scanner Profile' }] }
 
     trait :with_schedule do
-      rules { [{ type: 'schedule', branches: %w[production], cadence: '*/15 * * * *' }] }
+      rules { [{ type: 'schedule', branches: %w[master], cadence: '*/15 * * * *' }] }
     end
   end
 

ee/spec/graphql/mutations/security_policy/commit_scan_execution_policy_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe Mutations::SecurityPolicy::CommitScanExecutionPolicy do
 
   describe '#resolve' do
     let_it_be(:user) { create(:user) }
-    let_it_be(:project) { create(:project, namespace: user.namespace) }
+    let_it_be(:project) { create(:project, :repository, namespace: user.namespace) }
     let_it_be(:policy_management_project) { create(:project, :repository, namespace: user.namespace) }
     let_it_be(:policy_configuration) { create(:security_orchestration_policy_configuration, security_policy_management_project: policy_management_project, project: project) }
     let_it_be(:operation_mode) { Types::MutationOperationModeEnum.enum[:append] }

ee/spec/lib/ee/gitlab/ci/config_spec.rb

@@ -40,7 +40,7 @@ RSpec.describe Gitlab::Ci::Config do
   describe 'with security orchestration policy' do
     let(:source) { 'push' }
 
-    let_it_be(:ref) { 'master' }
+    let(:ref) { 'master' }
     let_it_be_with_refind(:project) { create(:project, :repository) }
 
     let_it_be(:policies_repository) { create(:project, :repository) }
@@ -70,13 +70,15 @@ RSpec.describe Gitlab::Ci::Config do
       end
 
       context 'when policy is not applicable on branch from the pipeline' do
+        let(:ref) { 'another-branch' }
+
         it 'does not modify the config' do
           expect(config.to_hash).to eq(sample_job: { script: ["echo 'test'"] })
         end
       end
 
-      context 'when policy is not applicable on branch from the pipeline' do
-        let_it_be(:ref) { 'production' }
+      context 'when policy is applicable on branch from the pipeline' do
+        let(:ref) { 'master' }
 
         context 'when DAST profiles are not found' do
           it 'adds a job with error message' do

ee/spec/lib/gitlab/ci/config/security_orchestration_policies/processor_spec.rb

@@ -69,21 +69,23 @@ RSpec.describe Gitlab::Ci::Config::SecurityOrchestrationPolicies::Processor do
     end
 
     context 'when policy is not applicable on branch from the pipeline' do
+      let(:ref) { 'refs/head/another-branch' }
+
       it 'does not modify the config' do
         expect(subject).to eq(config)
       end
     end
 
     context 'when ref is a tag' do
-      let_it_be(:ref) { 'refs/tags/v1.1.0' }
+      let(:ref) { 'refs/tags/v1.1.0' }
 
       it 'does not modify the config' do
         expect(subject).to eq(config)
       end
     end
 
-    context 'when policy is not applicable on branch from the pipeline' do
-      let_it_be(:ref) { 'refs/heads/production' }
+    context 'when policy is applicable on branch from the pipeline' do
+      let(:ref) { 'refs/heads/master' }
 
       context 'when DAST profiles are not found' do
         it 'does not modify the config' do

ee/spec/models/security/orchestration_policy_configuration_spec.rb

@@ -226,7 +226,7 @@ RSpec.describe Security::OrchestrationPolicyConfiguration do
 
     let(:expected_active_policies) do
       [
-        build(:scan_execution_policy, name: 'Run DAST in every pipeline'),
+        build(:scan_execution_policy, name: 'Run DAST in every pipeline', rules: [{ type: 'pipeline', branches: %w[production] }]),
         build(:scan_execution_policy, name: 'Run DAST in every pipeline_v1', rules: [{ type: 'pipeline', branches: %w[master] }]),
         build(:scan_execution_policy, name: 'Run DAST in every pipeline_v3', rules: [{ type: 'pipeline', branches: %w[master] }]),
         build(:scan_execution_policy, name: 'Run DAST in every pipeline_v4', rules: [{ type: 'pipeline', branches: %w[master] }]),
@@ -291,7 +291,7 @@ RSpec.describe Security::OrchestrationPolicyConfiguration do
     end
 
     subject(:pipeline_scan_actions) do
-      security_orchestration_policy_configuration.pipeline_scan_actions('refs/heads/production')
+      security_orchestration_policy_configuration.pipeline_scan_actions('refs/heads/master')
     end
 
     it 'returns only actions for pipeline scans applicable for branch' do

ee/spec/requests/api/graphql/mutations/security_policy/commit_scan_execution_policy_spec.rb

@@ -48,7 +48,7 @@ RSpec.describe 'Create scan execution policy for a project' do
     end
 
     context 'when provided policy is invalid' do
-      let_it_be(:policy_yaml) { build(:scan_execution_policy, name: policy_name).merge(type: 'scan_execution_policy', rules: [{ type: 'invalid_type' }]).to_yaml }
+      let_it_be(:policy_yaml) { build(:scan_execution_policy, name: policy_name).merge(type: 'scan_execution_policy', rules: [{ type: 'invalid_type', branches: ['master'] }]).to_yaml }
 
       it 'returns error with detailed information' do
         post_graphql_mutation(mutation, current_user: current_user)

ee/spec/services/security/security_orchestration_policies/policy_commit_service_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe Security::SecurityOrchestrationPolicies::PolicyCommitService do
   include RepoHelpers
 
   describe '#execute' do
-    let_it_be(:project) { create(:project) }
+    let_it_be(:project) { create(:project, :repository) }
     let_it_be(:current_user) { project.first_owner }
     let_it_be(:policy_management_project) { create(:project, :repository, creator: current_user) }
     let_it_be(:policy_configuration) { create(:security_orchestration_policy_configuration, security_policy_management_project: policy_management_project, project: project) }
@@ -46,8 +46,21 @@ RSpec.describe Security::SecurityOrchestrationPolicies::PolicyCommitService do
       end
     end
 
+    context 'when defined branch is missing' do
+      let(:policy_hash) { build(:scan_execution_policy, name: 'Test Policy', rules: [{ type: 'pipeline' }]) }
+
+      let(:params) { { policy_yaml: input_policy_yaml, operation: operation } }
+
+      it 'returns error' do
+        response = service.execute
+
+        expect(response[:status]).to eq(:error)
+        expect(response[:message]).to eq('Policy cannot be enabled without branch information')
+      end
+    end
+
     context 'when security_orchestration_policies_configuration does not exist for project' do
-      let_it_be(:project) { create(:project) }
+      let_it_be(:project) { create(:project, :repository) }
 
       it 'does not create new project' do
         response = service.execute

ee/spec/services/security/security_orchestration_policies/process_policy_service_spec.rb

@@ -49,17 +49,6 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService do
       end
     end
 
-    context 'when type is invalid' do
-      let(:type) { :invalid_type }
-
-      it 'returns error' do
-        result = service.execute
-
-        expect(result[:status]).to eq(:error)
-        expect(result[:message]).to eq('Invalid policy type')
-      end
-    end
-
     context 'append policy' do
       context 'when policy is present in repository' do
         before do

ee/spec/services/security/security_orchestration_policies/validate_policy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::SecurityOrchestrationPolicies::ValidatePolicyService do
+  describe '#execute' do
+    let(:service) { described_class.new(project: project, params: { policy: policy }) }
+    let(:enabled) { true }
+    let(:policy_type) { 'scan_execution_policy' }
+    let(:rule) { { clusters: { production: {} } } }
+    let(:policy) do
+      {
+        type: policy_type,
+        enabled: enabled,
+        rules: [rule]
+      }
+    end
+
+    subject(:result) { service.execute }
+
+    shared_examples 'checks only if policy is enabled' do
+      let(:enabled) { false }
+
+      it { expect(result[:status]).to eq(:success) }
+    end
+
+    shared_examples 'checks policy type' do
+      context 'when policy type is not provided' do
+        let(:policy_type) { nil }
+
+        it { expect(result[:status]).to eq(:error) }
+        it { expect(result[:message]).to eq('Invalid policy type') }
+      end
+
+      context 'when policy type is invalid' do
+        let(:policy_type) { 'invalid_policy_type' }
+
+        it { expect(result[:status]).to eq(:error) }
+        it { expect(result[:message]).to eq('Invalid policy type') }
+      end
+
+      context 'when policy type is valid' do
+        it { expect(result[:status]).to eq(:success) }
+      end
+    end
+
+    shared_examples 'checks if branches are provided in rule' do
+      context 'when rule has clusters defined' do
+        let(:rule) do
+          {
+            clusters: {
+              production: {}
+            },
+            branches: branches
+          }
+        end
+
+        context 'when branches are missing' do
+          let(:branches) { nil }
+
+          it { expect(result[:status]).to eq(:success) }
+        end
+
+        context 'when branches are provided' do
+          let(:branches) { ['master'] }
+
+          it { expect(result[:status]).to eq(:success) }
+        end
+      end
+
+      context 'when rule does not have clusters defined' do
+        let(:rule) do
+          {
+            branches: branches
+          }
+        end
+
+        context 'when branches are missing' do
+          let(:branches) { nil }
+
+          it { expect(result[:status]).to eq(:error) }
+          it { expect(result[:message]).to eq('Policy cannot be enabled without branch information') }
+
+          it_behaves_like 'checks only if policy is enabled'
+        end
+
+        context 'when branches are provided' do
+          let(:branches) { ['master'] }
+
+          it { expect(result[:status]).to eq(:success) }
+        end
+      end
+    end
+
+    shared_examples 'checks if branches are defined in the project' do
+      context 'when rule has clusters defined' do
+        let(:rule) do
+          {
+            clusters: {
+              production: {}
+            },
+            branches: branches
+          }
+        end
+
+        context 'when branches are defined for project' do
+          let(:branches) { ['master'] }
+
+          it { expect(result[:status]).to eq(:success) }
+        end
+
+        context 'when branches are not defined for project' do
+          let(:branches) { ['non-exising-branch'] }
+
+          it { expect(result[:status]).to eq(:success) }
+        end
+
+        context 'when pattern does not match any branch defined for project' do
+          let(:branches) { ['master', 'production-*', 'test-*'] }
+
+          it { expect(result[:status]).to eq(:success) }
+        end
+      end
+
+      context 'when rule does not have clusters defined' do
+        let(:rule) do
+          {
+            branches: branches
+          }
+        end
+
+        context 'when branches are defined for project' do
+          let(:branches) { ['master'] }
+
+          it { expect(result[:status]).to eq(:success) }
+        end
+
+        context 'when branches are not defined for project' do
+          let(:branches) { ['non-exising-branch'] }
+
+          it { expect(result[:status]).to eq(:error) }
+          it { expect(result[:message]).to eq('Policy cannot be enabled for non-existing branches (non-exising-branch)') }
+
+          it_behaves_like 'checks only if policy is enabled'
+        end
+
+        context 'when branches are defined as pattern' do
+          context 'when pattern matches at least one branch defined for project' do
+            let(:branches) { ['*'] }
+
+            it { expect(result[:status]).to eq(:success) }
+          end
+
+          context 'when pattern does not match any branch defined for project' do
+            let(:branches) { ['master', 'production-*', 'test-*'] }
+
+            it { expect(result[:status]).to eq(:error) }
+            it { expect(result[:message]).to eq('Policy cannot be enabled for non-existing branches (production-*, test-*)') }
+
+            it_behaves_like 'checks only if policy is enabled'
+          end
+        end
+      end
+    end
+
+    context 'when project is not provided' do
+      let_it_be(:project) { nil }
+
+      it_behaves_like 'checks policy type'
+      it_behaves_like 'checks if branches are provided in rule'
+    end
+
+    context 'when project is provided' do
+      let_it_be(:project) { create(:project, :repository) }
+
+      it_behaves_like 'checks policy type'
+      it_behaves_like 'checks if branches are provided in rule'
+      it_behaves_like 'checks if branches are defined in the project'
+    end
+  end
+end

locale/gitlab.pot

@@ -31854,6 +31854,9 @@ msgstr ""
 msgid "SecurityOrchestration|If you are using Auto DevOps, your %{monospacedStart}auto-deploy-values.yaml%{monospacedEnd} file will not be updated if you change a policy in this section. Auto DevOps users should make changes by following the %{linkStart}Container Network Policy documentation%{linkEnd}."
 msgstr ""
 
+msgid "SecurityOrchestration|Invalid policy type"
+msgstr ""
+
 msgid "SecurityOrchestration|Latest scan"
 msgstr ""
 
@@ -31872,6 +31875,12 @@ msgstr ""
 msgid "SecurityOrchestration|Policies"
 msgstr ""
 
+msgid "SecurityOrchestration|Policy cannot be enabled for non-existing branches (%{branches})"
+msgstr ""
+
+msgid "SecurityOrchestration|Policy cannot be enabled without branch information"
+msgstr ""
+
 msgid "SecurityOrchestration|Policy description"
 msgstr ""