diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index d4be7cc010e4490f70e64c4e895fe39ca7107a07..ed06f309af759e5af7c0192f505924c6f9c1aab6 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -31,8 +31,15 @@ def new end def create + # dual write to v2 approval rules when creating merge requests. + + v2_approval_rules_attributes = { + v2_approval_rules_attributes: merge_request_params[:approval_rules_attributes] || {} + } + @merge_request = ::MergeRequests::CreateService - .new(project: project, current_user: current_user, params: merge_request_params) + .new(project: project, current_user: current_user, + params: merge_request_params.merge(v2_approval_rules_attributes)) .execute if @merge_request.valid? diff --git a/ee/app/graphql/types/approval_rule_type.rb b/ee/app/graphql/types/approval_rule_type.rb index cd6c204101044c5b7f3f829af1f3d6ea7194a364..9741cfa04f7ed5bf38cfd5efb69835090f024089 100644 --- a/ee/app/graphql/types/approval_rule_type.rb +++ b/ee/app/graphql/types/approval_rule_type.rb @@ -4,7 +4,8 @@ module Types class ApprovalRuleType < BaseObject graphql_name 'ApprovalRule' description 'Describes a rule for who can approve merge requests.' - authorize :read_approval_rule + # temporarily remove auth until we implement policies for v2 approval rules. + # authorize :read_approval_rule present_using ::ApprovalRulePresenter diff --git a/ee/app/models/approval_state.rb b/ee/app/models/approval_state.rb index 5c40d3e799d65cfec0dfc278302f2a97257a46e3..a3917f3f28c919086ad28ed8ba021bd9f2092a41 100644 --- a/ee/app/models/approval_state.rb +++ b/ee/app/models/approval_state.rb @@ -323,7 +323,8 @@ def wrapped_rules rules = if merge_request.merged? merge_request.applicable_post_merge_approval_rules else - merge_request.approval_rules.applicable_to_branch(target_branch) + # todo return v2 approval rules + merge_request.v2_approval_rules.applicable_to_branch(target_branch) end grouped_merge_request_rules = rules.group_by do |rule| diff --git a/ee/app/models/approval_wrapped_rule.rb b/ee/app/models/approval_wrapped_rule.rb index 313cf00796daefbb7369a0dc91571800d460d701..2d1564b02328e16944e931a3d87fa2299b5667ba 100644 --- a/ee/app/models/approval_wrapped_rule.rb +++ b/ee/app/models/approval_wrapped_rule.rb @@ -97,6 +97,8 @@ def invalid_rule? end def allow_merge_when_invalid? + return true # todo, remove when security policies added. + return true if fail_open? !approval_rule.from_scan_result_policy? || @@ -104,6 +106,8 @@ def allow_merge_when_invalid? end def scan_result_policies + return # todo, remove when security policies added. + policy_configuration_id = approval_rule.security_orchestration_policy_configuration_id return unless policy_configuration_id @@ -122,6 +126,7 @@ def scan_result_policies # and/or allow MR authors to approve their own merge # requests (in case only one approval is needed). def approvals_left + # todo test that this works. strong_memoize(:approvals_left) do next 0 if invalid_rule? && fail_open? @@ -136,7 +141,7 @@ def unactioned_approvers end def name - return approval_rule.name unless approval_rule.from_scan_result_policy? + # return approval_rule.name unless approval_rule.from_scan_result_policy? # todo, remove when security policies added. approval_rule.policy_name end diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index 334d8c93798559ca392f36d9af594c821102a0de..359f72d077a309bae90d250b4ac6012cc3b5f175 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -27,12 +27,22 @@ module MergeRequest belongs_to :iteration, foreign_key: 'sprint_id', inverse_of: :merge_requests - has_many :v2_approval_merge_request_rules, class_name: 'MergeRequests::ApprovalRulesMergeRequest', + has_many :v2_approval_rules_merge_requests, class_name: 'MergeRequests::ApprovalRulesMergeRequest', inverse_of: :merge_request - has_many :v2_approval_rules, through: :v2_approval_merge_request_rules, - class_name: 'MergeRequests::ApprovalRule', source: :approval_rule + has_many :v2_approval_rules, through: :v2_approval_rules_merge_requests, + class_name: 'MergeRequests::ApprovalRule', source: :approval_rule, inverse_of: :merge_request do + def applicable_to_branch(branch) + # todo, get preloading working here for v2 approval rules + # ActiveRecord::Associations::Preloader.new( + # records: self, + # associations: [ { approval_project_rule: [:protected_branches] }] + # ).call + + self.select { |rule| rule.applicable_to_branch?(branch) } + end + end - has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent -- Some reason + has_many :approvers, as: :target, dependent: :delete_all has_many :approver_users, through: :approvers, source: :user has_many :approver_groups, as: :target, dependent: :delete_all has_many :status_check_responses, class_name: 'MergeRequests::StatusCheckResponse', inverse_of: :merge_request @@ -101,6 +111,7 @@ def set_applicable_when_copying_rules(applicable_ids) delegate :wrapped_approval_rules, :invalid_approvers_rules, to: :approval_state accepts_nested_attributes_for :approval_rules, allow_destroy: true + accepts_nested_attributes_for :v2_approval_rules, allow_destroy: true scope :not_merged, -> { where.not(merge_requests: { state_id: ::MergeRequest.available_states[:merged] }) } diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index a3eee8878444682d879a1a8e5acfd54ce89724f7..5932ecb67c7078fa972844617479f97e05ba2075 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -84,7 +84,8 @@ def preload_protected_branches has_one :secrets_manager, class_name: '::SecretsManagement::ProjectSecretsManager' has_many :v2_approval_project_rules, class_name: 'MergeRequests::ApprovalRulesProject', inverse_of: :project - has_many :v2_approval_rules, through: :v2_approval_project_rules, class_name: 'MergeRequests::ApprovalRule', source: :approval_rule + has_many :v2_approval_rules, through: :v2_approval_project_rules, class_name: 'MergeRequests::ApprovalRule', source: :approval_rule, extend: FilterByBranch + has_many :v2_regular_or_any_approver_approval_rules, -> { v2_regular_or_any_approver.order(rule_type: :desc, id: :asc) }, through: :v2_approval_project_rules, class_name: 'MergeRequests::ApprovalRule', source: :approval_rule, extend: FilterByBranch has_many :approvers, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent -- Some reason has_many :approver_users, through: :approvers, source: :user @@ -1517,7 +1518,8 @@ def open_source_license_granted? def user_defined_rules strong_memoize(:user_defined_rules) do # Loading the relation in order to memoize it loaded - regular_or_any_approver_approval_rules.load + # todo, swap for v2 approval rules. + v2_regular_or_any_approver_approval_rules.load end end diff --git a/ee/app/models/merge_requests/approval_group_rule.rb b/ee/app/models/merge_requests/approval_group_rule.rb new file mode 100644 index 0000000000000000000000000000000000000000..9ca6783f1ee9ca0d97d46335bed56e37622b2a8f --- /dev/null +++ b/ee/app/models/merge_requests/approval_group_rule.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module MergeRequests + class ApprovalGroupRule < ApplicationRecord + self.table_name = 'merge_requests_approval_group_rules' + belongs_to :approval_rule + belongs_to :group + end +end diff --git a/ee/app/models/merge_requests/approval_rule.rb b/ee/app/models/merge_requests/approval_rule.rb index d094bd5c99623fbb82975970f5f1860809a0d8b2..40cabee995a44f26ffaca045de6f59567f195784 100644 --- a/ee/app/models/merge_requests/approval_rule.rb +++ b/ee/app/models/merge_requests/approval_rule.rb @@ -3,6 +3,13 @@ module MergeRequests class ApprovalRule < ApplicationRecord self.table_name = 'merge_requests_approval_rules' + + include MergeRequests::ApprovalRuleLike + + # todo, confirm that source rule will be used like approval project rule + # for merge request level rules. + alias_attribute :approval_project_rule_id, :source_rule_id + # When this originated_from_group there's only one group has_one :approval_rules_group, inverse_of: :approval_rule has_one :group, through: :approval_rules_group @@ -32,6 +39,26 @@ class ApprovalRule < ApplicationRecord enum :origin, { group: 0, project: 1, merge_request: 2 }, prefix: :originates_from end + # When this originated_from_project there are multiple merge_requests + has_many :approval_rules_protected_branches + has_many :protected_branches, through: :approval_rules_protected_branches + + has_many :approval_rules_security_policy_links + has_many :security_policies, through: :approval_rules_security_policy_links + + # todo add applies to all protected branches to data model. + def applies_to_all_protected_branches? + applies_to_all_protected_branches + end + + def applies_to_all_protected_branches + false + end + + def contains_hidden_groups? + false + end + def self.generate_group_rule(group) create!( name: "Group Rule (#{group.full_path})", @@ -73,5 +100,66 @@ def self.destroy_everything rule.destroy end end + + # todo, add with security policies. + def report_type + nil + end + + def applicable_to_branch?(branch) + # unless this mr is inherited from a project rule. what does that mean for us? + return true unless self.source_rule_id.present? + # todo, add this to model, or think about if we need it. + # return true if self.modified_from_project_rule + + applies_to_branch?(branch) + end + + def applies_to_branch?(branch) + return !applies_to_all_protected_branches? if protected_branches.empty? + + # Call `ProtectedBranch.matching` with `protected_branches` passed as `protected_refs` + # when `protected_branches` are already loaded to prevent executing SQL query per rule + # which can lead to N+1 issues. + # + # Without this, a SQL query per rule will be executed as `ProtectedRef.matching` + # will call `#all` on the relation even if it's already loaded. + return ProtectedBranch.matching(branch, protected_refs: protected_branches).any? if protected_branches.loaded? + + protected_branches.matching(branch).any? + end + + # todo switch this on. + # def approvers + # strong_memoize(:approvers) do + # scope_or_array = super + # + # next scope_or_array unless merge_request.author + # next scope_or_array if project.merge_requests_author_approval? + # + # if scope_or_array.respond_to?(:where) + # scope_or_array.where.not(id: merge_request.author) + # else + # scope_or_array - [merge_request.author] + # end + # end + # end + + def scanners + nil + end + + # todo add with security policies. + def vulnerabilities_allowed + nil + end + + def severity_levels + nil + end + + def vulnerability_states + nil + end end end diff --git a/ee/app/models/merge_requests/approval_rule_like.rb b/ee/app/models/merge_requests/approval_rule_like.rb new file mode 100644 index 0000000000000000000000000000000000000000..ff4fde9441920cea9ae69edd96e4af674b087c1b --- /dev/null +++ b/ee/app/models/merge_requests/approval_rule_like.rb @@ -0,0 +1,198 @@ +# frozen_string_literal: true + +module MergeRequests + module ApprovalRuleLike + # this concern can be removed and its contents can be added directly to MergeRequests::ApprovalRule + + extend ActiveSupport::Concern + include EachBatch + include Importable + + DEFAULT_NAME = 'Default' + DEFAULT_NAME_FOR_LICENSE_REPORT = 'License-Check' + DEFAULT_NAME_FOR_COVERAGE = 'Coverage-Check' + APPROVALS_REQUIRED_MAX = 100 + ALL_MEMBERS = 'All Members' + NEWLY_DETECTED = 'newly_detected' + NEW_NEEDS_TRIAGE = 'new_needs_triage' + NEW_DISMISSED = 'new_dismissed' + + NEWLY_DETECTED_STATUSES = [NEW_NEEDS_TRIAGE, NEW_DISMISSED].freeze + DEFAULT_VULNERABILITY_STATUSES = [NEW_NEEDS_TRIAGE, NEW_DISMISSED].freeze + SCAN_RESULT_POLICY_REPORT_TYPES = %w[scan_finding license_scanning any_merge_request].freeze + + included do + has_and_belongs_to_many :users, + after_add: :audit_add, after_remove: :audit_remove + has_and_belongs_to_many :groups, + class_name: 'Group', join_table: "merge_requests_approval_rules_user_groups", + after_add: :audit_add, after_remove: :audit_remove + + has_many :group_members, through: :groups + has_many :group_users, -> { distinct }, through: :groups, source: :users, disable_joins: true + + # todo are these being moved to 'security policy?' + # belongs_to :security_orchestration_policy_configuration, class_name: 'Security::OrchestrationPolicyConfiguration', optional: true + # belongs_to :scan_result_policy_read, + # class_name: 'Security::ScanResultPolicyRead', + # foreign_key: 'scan_result_policy_id', + # inverse_of: :approval_merge_request_rules, + # optional: true + + # enum report_type: { + # vulnerability: 1, # To be removed after all MRs (related to https://gitlab.com/gitlab-org/gitlab/-/issues/356996) get merged + # license_scanning: 2, + # code_coverage: 3, + # scan_finding: 4, + # any_merge_request: 5 + # } + + # todo name is not being populated for any approver rule, figure out why. + # validates :name, presence: true + validates :approvals_required, numericality: { less_than_or_equal_to: APPROVALS_REQUIRED_MAX, greater_than_or_equal_to: 0 } + + # todo is this under security policies? + # validates :report_type, presence: true, if: :report_approver? + + # todo handle this with security policies + # We should not import Approval Rules when they are created from Security Policies + # validates :orchestration_policy_idx, absence: true, if: :importing? + # validates :report_type, exclusion: SCAN_RESULT_POLICY_REPORT_TYPES, if: :importing? + + scope :with_users, -> { preload(:users, :group_users) } + scope :v2_regular_or_any_approver, -> { where(rule_type: [:regular, :any_approver]) } + scope :not_regular_or_any_approver, -> { where.not(rule_type: [:regular, :any_approver]) } + scope :for_groups, ->(groups) { joins(:groups).where(merge_requests_approval_rules_groups: { group_id: groups }) } + + # todo handle this with security policies + # scope :including_scan_result_policy_read, -> { includes(:scan_result_policy_read) } + # scope :with_scan_result_policy_read, -> { where.not(scan_result_policy_id: nil) } + # scope :for_policy_index, ->(policy_idx) { where(orchestration_policy_idx: policy_idx) } + # scope :exportable, -> { not_from_scan_result_policy } # We are not exporting approval rules that were created from Security Policies + # scope :from_scan_result_policy, -> { where(report_type: SCAN_RESULT_POLICY_REPORT_TYPES) } + # scope :not_from_scan_result_policy, -> do + # where(report_type: nil).or(where.not(report_type: SCAN_RESULT_POLICY_REPORT_TYPES)) + # end + # scope :for_policy_configuration, ->(configuration_id) do + # where(security_orchestration_policy_configuration_id: configuration_id) + # end + # scope :for_approval_policy_rules, ->(policy_rules) do + # where(approval_policy_rule: policy_rules) + # end + # + # scope :by_report_types, ->(report_types) { where(report_type: report_types) } + end + + def vulnerability_attribute_false_positive + nil + end + + def vulnerability_attribute_fix_available + nil + end + + def audit_add(_model) + # todo, only add for project rule. + end + + def audit_remove(_model) + # todo, only add for project rule. + end + + def approvers + @approvers ||= filter_inactive_approvers(with_role_approvers) + end + + def approvers_include_user?(user) + return false if filter_inactive_approvers([user]).empty? + + # todo: reintroduce codeowner role approvers and scan result policy role approvers + relation_exists?(users, column: :id, value: user.id) || + relation_exists?(group_members, column: :user_id, value: user.id) + end + + def code_owner_role_approvers + User.none + end + + def code_owner? + raise NotImplementedError + end + + def regular? + raise NotImplementedError + end + + def report_approver? + raise NotImplementedError + end + + def any_approver? + raise NotImplementedError + end + + def user_defined? + regular? || any_approver? + end + + def overridden? + return false unless source_rule.present? + + source_rule.name != name || + source_rule.approvals_required != approvals_required || + source_rule.user_ids.to_set != user_ids.to_set || + source_rule.group_ids.to_set != group_ids.to_set + end + + def from_scan_result_policy? + # todo handle this with security policies + # return true if scan_finding? + # return true if license_scanning? && scan_result_policy_id.present? + # return true if any_merge_request? + + false + end + + def policy_name + name.gsub(/\s\d+\z/, '') + end + + private + + def relation_exists?(relation, column:, value:) + return relation.exists?({ column => value }) unless relation.loaded? + + relation.detect { |item| item.read_attribute(column) == value } + end + + def with_role_approvers + # todo add role_approvers to data model and re add to this method. + if users.loaded? && group_users.loaded? + users | group_users + else + User.from_union([users, group_users]) + end + end + + def scan_result_policy_read_role_approvers + return User.none + # todo handle this with security policies + # return User.none unless scan_result_policy_read + + # project.team.members_with_access_levels(scan_result_policy_read.role_approvers) + end + + def filter_inactive_approvers(approvers) + if approvers.respond_to?(:with_state) + approvers.with_state(:active) + else + approvers.select(&:active?) + end + end + + def section + # todo add this to the models + nil + end + end +end diff --git a/ee/app/models/merge_requests/approval_rules_security_policy_link.rb b/ee/app/models/merge_requests/approval_rules_security_policy_link.rb index a533de4663848dff35876a0a79fc4d0e8f1e87d8..67578a6dbefce889c46a1e4dc97b6dacba4e5645 100644 --- a/ee/app/models/merge_requests/approval_rules_security_policy_link.rb +++ b/ee/app/models/merge_requests/approval_rules_security_policy_link.rb @@ -4,6 +4,6 @@ module MergeRequests class ApprovalRulesSecurityPolicyLink < ApplicationRecord self.table_name = 'merge_requests_approval_rules_security_policy_links' belongs_to :approval_rule - belongs_to :security_policy + belongs_to :security_policy, class_name: 'Security::Policy' end end diff --git a/ee/app/services/approval_rules/base_service.rb b/ee/app/services/approval_rules/base_service.rb index de5f6f7c4c3dda4a5eba13a465f9eb5359ac80e0..4fd29e45e6db5fa4188a3425abb8605c50361542 100644 --- a/ee/app/services/approval_rules/base_service.rb +++ b/ee/app/services/approval_rules/base_service.rb @@ -17,6 +17,7 @@ def action attr_reader :rule def can_edit? + return true # todo, remove this when policies for approval rules are added. skip_authorization || can?(current_user, policy_action, rule) end diff --git a/ee/app/services/approval_rules/create_service.rb b/ee/app/services/approval_rules/create_service.rb index a973d0bfaaa4e02af8ec2f73990c78860511fd64..444785b6276a2e984503fb0b7c6234132f7a6d20 100644 --- a/ee/app/services/approval_rules/create_service.rb +++ b/ee/app/services/approval_rules/create_service.rb @@ -6,8 +6,9 @@ class CreateService < ::ApprovalRules::BaseService # @param target [Project, MergeRequest] def initialize(target, user, params) - @rule = target.approval_rules.build - @params = params + @rule = target.v2_approval_rules.build(origin: :project, project: target) + # todo, applies_to_all_protected_branches is not in model yet. + @params = params.except!(:applies_to_all_protected_branches) # If merge request approvers are specified, they take precedence over project # approvers.