From 8de296ff9f9b4ad2f1d72215a972aaf64bd90f87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20C=CC=8Cavoj?= <mcavoj@gitlab.com> Date: Wed, 31 May 2023 17:00:03 +0200 Subject: [PATCH] Add branch_type validations Changelog: added EE: true --- .../validate_policy_service.rb | 17 ++ .../validate_policy_service_spec.rb | 152 +++++++++++++++++- locale/gitlab.pot | 3 + 3 files changed, 166 insertions(+), 6 deletions(-) diff --git a/ee/app/services/security/security_orchestration_policies/validate_policy_service.rb b/ee/app/services/security/security_orchestration_policies/validate_policy_service.rb index 2229e34960980b56..275bd8c3f2762e0d 100644 --- a/ee/app/services/security/security_orchestration_policies/validate_policy_service.rb +++ b/ee/app/services/security/security_orchestration_policies/validate_policy_service.rb @@ -15,6 +15,7 @@ def execute return error_with_title(s_('SecurityOrchestration|Invalid policy type')) if invalid_policy_type? return error_with_title(s_('SecurityOrchestration|Policy cannot be enabled without branch information')) if blank_branch_for_rule? return error_with_title(s_('SecurityOrchestration|Policy cannot be enabled for non-existing branches (%{branches})') % { branches: missing_branch_names.join(', ') }) if missing_branch_for_rule? + return error_with_title(s_('SecurityOrchestration|Branch types don\'t match any existing branches.')) if invalid_branch_types? return error_with_title(s_('SecurityOrchestration|Required approvals exceed eligible approvers'), title: s_('SecurityOrchestration|Logic error'), field: "approvers_ids") if required_approvals_exceed_eligible_approvers? @@ -138,6 +139,22 @@ def branches_for_project end end + def invalid_branch_types? + return false if container.blank? || !project_container? || + Feature.disabled?(:security_policies_branch_type, container) + + service = Security::SecurityOrchestrationPolicies::PolicyBranchesService.new(project: container) + + policy[:rules].select { |rule| rule[:branch_type].present? } + .any? do |rule| + if scan_result_policy? + service.scan_result_branches([rule]).empty? + else + service.scan_execution_branches([rule]).empty? + end + end + end + def policy_type policy[:type].to_sym end diff --git a/ee/spec/services/security/security_orchestration_policies/validate_policy_service_spec.rb b/ee/spec/services/security/security_orchestration_policies/validate_policy_service_spec.rb index cfe0a0b62cd3d613..49885752af4a7d6b 100644 --- a/ee/spec/services/security/security_orchestration_policies/validate_policy_service_spec.rb +++ b/ee/spec/services/security/security_orchestration_policies/validate_policy_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Security::SecurityOrchestrationPolicies::ValidatePolicyService, feature_category: :security_policy_management do + using RSpec::Parameterized::TableSyntax + describe '#execute' do let(:service) { described_class.new(container: container, params: { policy: policy, validate_approvals_required: validate_approvals_required }) } let(:validate_approvals_required) { true } @@ -10,12 +12,13 @@ let(:policy_type) { 'scan_execution_policy' } let(:name) { 'New policy' } let(:rule) { { agents: { production: {} } } } + let(:rules) { [rule] } let(:policy) do { type: policy_type, name: name, enabled: enabled, - rules: [rule] + rules: rules } end @@ -372,6 +375,39 @@ end end + shared_examples 'checks if branches exist for the provided branch_type' do + let(:rule) do + { + branch_type: branch_type + } + end + + with_them do + context 'when feature flag "security_policies_branch_type" is enabled' do + it { expect(result[:status]).to eq(status) } + + it 'returns a corresponding error message for error case' do + if status == :error + expect(result[:details]).to eq(["Branch types don't match any existing branches."]) + else + expect(result[:details]).to be_nil + end + end + + it_behaves_like 'checks only if policy is enabled' + end + + context 'when feature flag "security_policies_branch_type" is disabled' do + before do + stub_feature_flags(security_policies_branch_type: false) + end + + it { expect(result[:status]).to eq(:success) } + it { expect(result[:details]).to be_nil } + end + end + end + context 'when project or namespace is not provided' do let_it_be(:container) { nil } @@ -380,12 +416,116 @@ end context 'when project is provided' do - let_it_be(:container) { create(:project, :repository) } + let_it_be(:default_branch) { 'master' } + let_it_be(:protected_branch) { 'protected' } + let_it_be(:unprotected_branch) { 'feature' } + + def setup_repository(project, branches) + sha = project.repository.create_file( + project.creator, + "README.md", + "", + message: "initial commit", + branch_name: branches.first) + branches.each do |branch| + project.repository.add_branch(project.creator, branch, sha) + end + end - 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' - it_behaves_like 'checks if required approvals exceed eligible approvers' + context 'when repository is empty' do + let_it_be(:container) { create(:project, :empty_repo) } + + it_behaves_like 'checks policy type' + it_behaves_like 'checks if branches exist for the provided branch_type' do + where(:policy_type, :branch_type, :status) do + :scan_execution_policy | 'all' | :error + :scan_execution_policy | 'protected' | :error + :scan_execution_policy | 'default' | :error + :scan_result_policy | 'protected' | :error + :scan_result_policy | 'default' | :error + end + end + end + + context 'when project has a default protected branch' do + let_it_be(:container) { create(:project, :repository) } + + before(:all) do + container.protected_branches.create!(name: 'master') + end + + 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' + it_behaves_like 'checks if required approvals exceed eligible approvers' + it_behaves_like 'checks if branches exist for the provided branch_type' do + where(:policy_type, :branch_type, :status) do + :scan_execution_policy | 'all' | :success + :scan_execution_policy | 'protected' | :success + :scan_execution_policy | 'default' | :success + :scan_result_policy | 'protected' | :success + :scan_result_policy | 'default' | :success + end + end + end + + context 'when project has a non-default protected branch' do + let_it_be(:container) { create(:project, :empty_repo) } + + before(:all) do + setup_repository(container, [default_branch, protected_branch]) + container.protected_branches.create!(name: protected_branch) + end + + 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' + it_behaves_like 'checks if required approvals exceed eligible approvers' + it_behaves_like 'checks if branches exist for the provided branch_type' do + where(:policy_type, :branch_type, :status) do + :scan_execution_policy | 'all' | :success + :scan_execution_policy | 'protected' | :success + :scan_execution_policy | 'default' | :success + :scan_result_policy | 'protected' | :success + :scan_result_policy | 'default' | :error + end + end + end + + context 'when project has only a default unprotected branch' do + let_it_be(:container) { create(:project, :empty_repo) } + + before(:all) do + setup_repository(container, [unprotected_branch]) + end + + it_behaves_like 'checks policy type' + it_behaves_like 'checks if branches exist for the provided branch_type' do + where(:policy_type, :branch_type, :status) do + :scan_execution_policy | 'all' | :success + :scan_execution_policy | 'protected' | :error + :scan_execution_policy | 'default' | :success + :scan_result_policy | 'protected' | :error + :scan_result_policy | 'default' | :error + end + + context 'with multiple rules' do + where(:branch_type1, :branch_type2, :status) do + 'protected' | 'default' | :error + 'all' | 'protected' | :error + 'all' | 'default' | :success + end + + with_them do + let(:rules) do + [{ branch_type: branch_type1 }, { branch_type: branch_type2 }] + end + + it { expect(result[:status]).to eq(status) } + end + end + end + end end context 'when namespace is provided' do diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 620fc5f3dff9cd2f..036d517bbf7fd5a0 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -40979,6 +40979,9 @@ msgstr "" msgid "SecurityOrchestration|Are you sure you want to delete this policy? This action cannot be undone." msgstr "" +msgid "SecurityOrchestration|Branch types don't match any existing branches." +msgstr "" + msgid "SecurityOrchestration|Choose a project" msgstr "" -- GitLab