diff --git a/db/migrate/20241201162318_add_custom_roles_to_scan_result_policies.rb b/db/migrate/20241201162318_add_custom_roles_to_scan_result_policies.rb new file mode 100644 index 0000000000000000000000000000000000000000..b36b237e0a67bc323b8a967cddc1d4a2b876c9e3 --- /dev/null +++ b/db/migrate/20241201162318_add_custom_roles_to_scan_result_policies.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class AddCustomRolesToScanResultPolicies < Gitlab::Database::Migration[2.2] + enable_lock_retries! + milestone '17.7' + + def change + add_column :scan_result_policies, :custom_roles, :bigint, array: true, default: [], null: false + end +end diff --git a/db/migrate/20241206150204_add_custom_roles_constraint_to_scan_result_policies.rb b/db/migrate/20241206150204_add_custom_roles_constraint_to_scan_result_policies.rb new file mode 100644 index 0000000000000000000000000000000000000000..e6cf3f475ccb9ed6b592fc27acb17805c6803be9 --- /dev/null +++ b/db/migrate/20241206150204_add_custom_roles_constraint_to_scan_result_policies.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class AddCustomRolesConstraintToScanResultPolicies < Gitlab::Database::Migration[2.2] + disable_ddl_transaction! + milestone '17.7' + + CONSTRAINT_NAME = 'custom_roles_array_check' + + def up + add_check_constraint(:scan_result_policies, "ARRAY_POSITION(custom_roles, null) IS null", CONSTRAINT_NAME) + end + + def down + remove_check_constraint :scan_result_policies, CONSTRAINT_NAME + end +end diff --git a/db/schema_migrations/20241201162318 b/db/schema_migrations/20241201162318 new file mode 100644 index 0000000000000000000000000000000000000000..7a52d4f384e8653a5b0e571f5724334121f6c99c --- /dev/null +++ b/db/schema_migrations/20241201162318 @@ -0,0 +1 @@ +a8c9f960933989678f4a35b5ef48cea7c172d748e971fbf96e4d03d7038c0428 \ No newline at end of file diff --git a/db/schema_migrations/20241206150204 b/db/schema_migrations/20241206150204 new file mode 100644 index 0000000000000000000000000000000000000000..a5d0a3436cc9b01a3e72a70a5e477a4168ea559e --- /dev/null +++ b/db/schema_migrations/20241206150204 @@ -0,0 +1 @@ +ad2953fc7be16a7bc66aa7513ec452c9e0d6bda1bf3700507c78af63feaed1f1 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 93ec1bcf10a193bb39e929c079dcf2e660a17a8e..58dc22b1923e789bdd93e8bceb24a9c888c66b24 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -19492,8 +19492,10 @@ CREATE TABLE scan_result_policies ( fallback_behavior jsonb DEFAULT '{}'::jsonb NOT NULL, policy_tuning jsonb DEFAULT '{}'::jsonb NOT NULL, action_idx smallint DEFAULT 0 NOT NULL, + custom_roles bigint[] DEFAULT '{}'::bigint[] NOT NULL, CONSTRAINT age_value_null_or_positive CHECK (((age_value IS NULL) OR (age_value >= 0))), - CONSTRAINT check_scan_result_policies_rule_idx_positive CHECK (((rule_idx IS NULL) OR (rule_idx >= 0))) + CONSTRAINT check_scan_result_policies_rule_idx_positive CHECK (((rule_idx IS NULL) OR (rule_idx >= 0))), + CONSTRAINT custom_roles_array_check CHECK ((array_position(custom_roles, NULL::bigint) IS NULL)) ); CREATE SEQUENCE scan_result_policies_id_seq diff --git a/ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb b/ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb index 315ab2d69d5b68c82271d0965233b0b42c0e6214..870bd2cf12371412a68bfdd7a32abdcf0db9d8ce 100644 --- a/ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb +++ b/ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb @@ -108,6 +108,7 @@ def create_scan_result_policy(rule, approval_action, send_bot_message_action, pr license_states: rule[:license_states], match_on_inclusion_license: rule[:match_on_inclusion_license] || false, role_approvers: role_access_levels(approval_action&.dig(:role_approvers)), + custom_roles: custom_role_approvers(approval_action&.dig(:role_approvers)), vulnerability_attributes: rule[:vulnerability_attributes], project_id: project.id, age_operator: rule.dig(:vulnerability_age, :operator), @@ -197,6 +198,12 @@ def groups_ids(group_ids, group_paths) search_globally: search_groups_globally?).execute(include_inaccessible: true) end + def custom_role_approvers(role_approvers) + return [] unless role_approvers + + role_approvers.select { |role| role.is_a?(Integer) } + end + def role_access_levels(role_approvers) return [] unless role_approvers diff --git a/ee/app/services/security/security_orchestration_policies/sync_project_approval_policy_rules_service.rb b/ee/app/services/security/security_orchestration_policies/sync_project_approval_policy_rules_service.rb index c31ca6c72e91224825cdb93c0827e6ba3bd29192..5900474ad51f8f09c9a3bd5e845a2e6624caaba1 100644 --- a/ee/app/services/security/security_orchestration_policies/sync_project_approval_policy_rules_service.rb +++ b/ee/app/services/security/security_orchestration_policies/sync_project_approval_policy_rules_service.rb @@ -221,6 +221,7 @@ def scan_result_policy_read_params(approval_policy_rule) license_states: rule_content[:license_states], match_on_inclusion_license: rule_content[:match_on_inclusion_license] || false, role_approvers: role_access_levels(approval_action&.dig(:role_approvers)), + custom_roles: custom_role_approvers(approval_action&.dig(:role_approvers)), vulnerability_attributes: rule_content[:vulnerability_attributes], project_id: project.id, age_operator: rule_content.dig(:vulnerability_age, :operator), @@ -243,6 +244,13 @@ def role_access_levels(role_approvers) .filter_map { |role| roles_map[role.to_sym] if role.to_s.in?(Security::ScanResultPolicy::ALLOWED_ROLES) } end + # TODO: Will be removed with https://gitlab.com/gitlab-org/gitlab/-/issues/504296 + def custom_role_approvers(role_approvers) + return [] unless role_approvers + + role_approvers.select { |role| role.is_a?(Integer) } + end + def protected_branch_ids(approval_policy_rule) service = Security::SecurityOrchestrationPolicies::PolicyBranchesService.new(project: project) applicable_branches = service.scan_result_branches([approval_policy_rule.content.deep_symbolize_keys]) diff --git a/ee/app/validators/json_schemas/approval_policy_content.json b/ee/app/validators/json_schemas/approval_policy_content.json index 18b0498a93a29391399982921d62320bd57f35c2..73cb602e64fdd8b66121dd9274133cb0c9da3637 100644 --- a/ee/app/validators/json_schemas/approval_policy_content.json +++ b/ee/app/validators/json_schemas/approval_policy_content.json @@ -91,15 +91,22 @@ "minItems": 1, "additionalItems": false, "items": { - "type": "string", - "enum": [ - "guest", - "reporter", - "developer", - "maintainer", - "owner" - ], - "minLength": 1 + "minLength": 1, + "anyOf": [ + { + "type": "string", + "enum": [ + "guest", + "reporter", + "developer", + "maintainer", + "owner" + ] + }, + { + "type": "integer" + } + ] } }, "enabled": { diff --git a/ee/app/validators/json_schemas/security_orchestration_policy.json b/ee/app/validators/json_schemas/security_orchestration_policy.json index 0aba3eed68bbad19d995ccdc2d4a6dcf9e17b8fc..8c8a501290f740de3068b5b33675b22fe2ba8214 100644 --- a/ee/app/validators/json_schemas/security_orchestration_policy.json +++ b/ee/app/validators/json_schemas/security_orchestration_policy.json @@ -1095,15 +1095,22 @@ "minItems": 1, "additionalItems": false, "items": { - "type": "string", - "enum": [ - "guest", - "reporter", - "developer", - "maintainer", - "owner" - ], - "minLength": 1 + "minLength": 1, + "anyOf": [ + { + "type": "string", + "enum": [ + "guest", + "reporter", + "developer", + "maintainer", + "owner" + ] + }, + { + "type": "integer" + } + ] } }, "enabled": { diff --git a/ee/spec/models/security/orchestration_policy_configuration_spec.rb b/ee/spec/models/security/orchestration_policy_configuration_spec.rb index b575ebd3e2553caf0ecb300521c44f9dd3f60afa..605d57196ddbf90ac849c920ba0930086130fb2a 100644 --- a/ee/spec/models/security/orchestration_policy_configuration_spec.rb +++ b/ee/spec/models/security/orchestration_policy_configuration_spec.rb @@ -1138,10 +1138,12 @@ context "with role_approvers" do before do - action[:role_approvers] = %w[guest reporter] + action[:role_approvers] = ['guest', 'reporter', 123] end - specify { expect(errors).to be_empty } + specify do + expect(errors).to be_empty + end context "with invalid role" do before do @@ -1149,8 +1151,9 @@ end specify do - expect(errors.count).to be(1) + expect(errors.count).to be(2) expect(errors.first).to match("property '/#{type}/0/actions/0/role_approvers/0' is not one of") + expect(errors.last).to match("property '/#{type}/0/actions/0/role_approvers/0' is not of type: integer") end end end diff --git a/ee/spec/services/security/security_orchestration_policies/process_scan_result_policy_service_spec.rb b/ee/spec/services/security/security_orchestration_policies/process_scan_result_policy_service_spec.rb index 9a9a34cb205cb72976f341f09780020bfd147bea..f5f10000b8c3d3a11e9cd8b54aaa755a561b9cfa 100644 --- a/ee/spec/services/security/security_orchestration_policies/process_scan_result_policy_service_spec.rb +++ b/ee/spec/services/security/security_orchestration_policies/process_scan_result_policy_service_spec.rb @@ -117,6 +117,8 @@ end context 'with role_approvers' do + let_it_be(:custom_role) { create(:member_role, namespace: project.group) } + let(:policy) do build(:scan_result_policy, name: 'Test Policy', @@ -124,7 +126,7 @@ type: 'require_approval', approvals_required: 1, user_approvers: [approver.username], - role_approvers: ['developer'] + role_approvers: ['developer', custom_role.id] }] ) end @@ -142,7 +144,10 @@ it 'creates scan_result_policy_read' do expect { subject }.to change { Security::ScanResultPolicyRead.count }.by(1) - expect(project.approval_rules.first.scan_result_policy_read.role_approvers).to match_array([Gitlab::Access::DEVELOPER]) + + scan_result_policy_read = project.scan_result_policy_reads.first + expect(scan_result_policy_read.custom_roles).to match_array([custom_role.id]) + expect(scan_result_policy_read.role_approvers).to match_array([Gitlab::Access::DEVELOPER]) end end diff --git a/ee/spec/services/security/security_orchestration_policies/sync_project_approval_policy_rules_service_spec.rb b/ee/spec/services/security/security_orchestration_policies/sync_project_approval_policy_rules_service_spec.rb index 4f96b97f0a01756aa9eb5dc850a5f1e1ae7abeda..7b76066a939a3e7b2d133c2275278fd44cb00ac0 100644 --- a/ee/spec/services/security/security_orchestration_policies/sync_project_approval_policy_rules_service_spec.rb +++ b/ee/spec/services/security/security_orchestration_policies/sync_project_approval_policy_rules_service_spec.rb @@ -112,6 +112,9 @@ end context 'with role_approvers' do + let_it_be(:custom_role) { create(:member_role, namespace: project.group) } + let_it_be(:developer) { create(:user) } + before do security_policy.update!( content: { @@ -119,7 +122,7 @@ type: 'require_approval', approvals_required: 1, user_approvers: [approver.username], - role_approvers: ['developer'] + role_approvers: ['developer', custom_role.id] }] } ) @@ -127,8 +130,6 @@ project.add_developer(developer) # rubocop:disable RSpec/BeforeAllRoleAssignment -- Does not work in before_all end - let_it_be(:developer) { create(:user) } - it 'creates approval rules with role approvers' do expect { create_rules }.to change { project.approval_rules.count }.by(1) expect(project.approval_rules.first.approvers).to contain_exactly(approver, developer) @@ -136,8 +137,10 @@ it 'creates scan_result_policy_read' do expect { create_rules }.to change { Security::ScanResultPolicyRead.count }.by(1) - expect(project.approval_rules.first.scan_result_policy_read.role_approvers) - .to match_array([Gitlab::Access::DEVELOPER]) + + scan_result_policy_read = project.scan_result_policy_reads.first + expect(scan_result_policy_read.custom_roles).to match_array([custom_role.id]) + expect(scan_result_policy_read.role_approvers).to match_array([Gitlab::Access::DEVELOPER]) end end