From 86782ed23be4eab86c137d5f6e58a3241114e760 Mon Sep 17 00:00:00 2001 From: Lucas Charles Date: Fri, 31 May 2019 14:36:22 -0700 Subject: [PATCH] Sync report_approver approval_rules to pipeline security reports Adds SyncSecurityReportsToReportApprovalRulesWorker responsible for conditionally zero'ing out `approvals_required` when a report contains no high-severity or critical-severity vulnerabilities --- ee/app/controllers/ee/projects_controller.rb | 8 ++ ee/app/models/approval_merge_request_rule.rb | 21 +++- ee/app/models/approval_project_rule.rb | 27 +---- ee/app/models/approval_state.rb | 13 +- ee/app/models/concerns/approval_rule_like.rb | 1 + ee/app/models/ee/ci/pipeline.rb | 5 +- ee/app/models/ee/merge_request.rb | 2 +- ee/app/models/ee/project.rb | 6 + ee/app/models/license.rb | 1 + .../services/approval_rules/create_service.rb | 7 ++ .../project_rule_destroy_service.rb | 35 ++++++ .../ee/merge_requests/create_service.rb | 7 ++ .../ee/merge_requests/refresh_service.rb | 5 +- .../sync_report_approver_approval_rules.rb | 45 +++++++ .../sync_reports_to_approval_rules_service.rb | 36 ++++++ ee/app/workers/all_queues.yml | 1 + ...reports_to_report_approval_rules_worker.rb | 15 +++ ...rt_approver-to-approval_rules-services.yml | 5 + ee/lib/api/project_approval_rules.rb | 6 +- ee/lib/ee/api/entities.rb | 2 +- ee/lib/gitlab/ci/reports/security/report.rb | 6 + ee/spec/factories/approval_rules.rb | 6 + .../approval_merge_request_rule_spec.rb | 26 +++- ee/spec/models/approval_project_rule_spec.rb | 35 ++++-- ee/spec/models/approval_state_spec.rb | 92 ++++++++++---- ee/spec/models/merge_request_spec.rb | 4 + .../api/project_approval_rules_spec.rb | 15 +++ .../approval_rules/create_service_spec.rb | 16 +++ .../project_rule_destroy_service_spec.rb | 44 +++++++ .../ee/merge_requests/create_service_spec.rb | 31 +++++ .../ee/merge_requests/refresh_service_spec.rb | 16 +++ ...ync_report_approver_approval_rules_spec.rb | 56 +++++++++ ..._reports_to_approval_rules_service_spec.rb | 112 ++++++++++++++++++ ...ts_to_report_approval_rules_worker_spec.rb | 31 +++++ 34 files changed, 676 insertions(+), 62 deletions(-) create mode 100644 ee/app/services/approval_rules/project_rule_destroy_service.rb create mode 100644 ee/app/services/merge_requests/sync_report_approver_approval_rules.rb create mode 100644 ee/app/services/security/sync_reports_to_approval_rules_service.rb create mode 100644 ee/app/workers/sync_security_reports_to_report_approval_rules_worker.rb create mode 100644 ee/changelogs/unreleased/9928-add-report_approver-to-approval_rules-services.yml create mode 100644 ee/spec/services/approval_rules/project_rule_destroy_service_spec.rb create mode 100644 ee/spec/services/merge_requests/sync_report_approver_approval_rules_spec.rb create mode 100644 ee/spec/services/security/sync_reports_to_approval_rules_service_spec.rb create mode 100644 ee/spec/workers/sync_security_reports_to_report_approval_rules_worker_spec.rb diff --git a/ee/app/controllers/ee/projects_controller.rb b/ee/app/controllers/ee/projects_controller.rb index a51fae1f9ac3aa..5048c05b2facb6 100644 --- a/ee/app/controllers/ee/projects_controller.rb +++ b/ee/app/controllers/ee/projects_controller.rb @@ -5,6 +5,10 @@ module ProjectsController extend ActiveSupport::Concern extend ::Gitlab::Utils::Override + prepended do + before_action :set_report_approver_rules_feature_flag, only: [:edit] + end + override :project_params_attributes def project_params_attributes super + project_params_ee @@ -77,5 +81,9 @@ def allow_mirror_params? def allow_merge_pipelines_params? project&.feature_available?(:merge_pipelines) end + + def set_report_approver_rules_feature_flag + push_frontend_feature_flag(:report_approver_rules, default_enabled: false) + end end end diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb index 7b1c7dee87fe14..f078ae3995cf6e 100644 --- a/ee/app/models/approval_merge_request_rule.rb +++ b/ee/app/models/approval_merge_request_rule.rb @@ -7,6 +7,22 @@ class ApprovalMergeRequestRule < ApplicationRecord scope :not_matching_pattern, -> (pattern) { code_owner.where.not(name: pattern) } scope :matching_pattern, -> (pattern) { code_owner.where(name: pattern) } + scope :from_project_rule, -> (project_rule) do + joins(:approval_merge_request_rule_source) + .where( + approval_merge_request_rule_sources: { approval_project_rule_id: project_rule.id } + ) + end + scope :for_unmerged_merge_requests, -> (merge_requests = nil) do + query = joins(:merge_request).where.not(merge_requests: { state: 'merged' }) + + if merge_requests + query.where(merge_request_id: merge_requests) + else + query + end + end + validates :name, uniqueness: { scope: [:merge_request, :code_owner] } validates :report_type, presence: true, if: :report_approver? # Temporary validations until `code_owner` can be dropped in favor of `rule_type` @@ -22,7 +38,7 @@ class ApprovalMergeRequestRule < ApplicationRecord has_one :approval_project_rule, through: :approval_merge_request_rule_source alias_method :source_rule, :approval_project_rule - validate :validate_approvals_required + validate :validate_approvals_required, unless: :report_approver? enum rule_type: { regular: 1, @@ -37,6 +53,7 @@ class ApprovalMergeRequestRule < ApplicationRecord # Deprecated scope until code_owner column has been migrated to rule_type # To be removed with https://gitlab.com/gitlab-org/gitlab-ee/issues/11834 scope :code_owner, -> { where(code_owner: true).or(where(rule_type: :code_owner)) } + scope :security_report, -> { report_approver.where(report_type: :security) } def self.find_or_create_code_owner_rule(merge_request, pattern) merge_request.approval_rules.code_owner.where(name: pattern).first_or_create do |rule| @@ -55,7 +72,7 @@ def project # Temporary override to handle legacy records that have not yet been migrated # To be removed with https://gitlab.com/gitlab-org/gitlab-ee/issues/11834 def regular? - read_attribute(:rule_type) == 'regular' || code_owner == false + read_attribute(:rule_type) == 'regular' || (!report_approver? && !code_owner) end alias_method :regular, :regular? diff --git a/ee/app/models/approval_project_rule.rb b/ee/app/models/approval_project_rule.rb index 0e68cd184110b8..c1222a366f6df8 100644 --- a/ee/app/models/approval_project_rule.rb +++ b/ee/app/models/approval_project_rule.rb @@ -5,30 +5,15 @@ class ApprovalProjectRule < ApplicationRecord belongs_to :project - # To allow easier duck typing - scope :regular, -> { all } - scope :code_owner, -> { none } + enum rule_type: { + regular: 0, + code_owner: 1, # currently unused + report_approver: 2 + } - def regular - true - end - alias_method :regular?, :regular - - def code_owner - false - end - alias_method :code_owner?, :code_owner - - def report_approver - false - end - alias_method :report_approver?, :report_approver + alias_method :code_owner, :code_owner? def source_rule nil end - - def rule_type - 'regular' - end end diff --git a/ee/app/models/approval_state.rb b/ee/app/models/approval_state.rb index c056cbffbb8551..5e6e5b6d2a0706 100644 --- a/ee/app/models/approval_state.rb +++ b/ee/app/models/approval_state.rb @@ -46,12 +46,13 @@ def wrapped_approval_rules result = use_fallback? ? [fallback_rule] : regular_rules result += code_owner_rules + result += report_approver_rules result end end def has_non_fallback_rules? - has_regular_rule_with_approvers? || code_owner_rules.present? + has_regular_rule_with_approvers? || code_owner_rules.present? || report_approver_rules.present? end # Use the fallback rule if regular rules are empty @@ -110,12 +111,14 @@ def approvers # @param regular [Boolean] # @param code_owner [Boolean] + # @param report_approver [Boolean] # @param target [:approvers, :users] # @param unactioned [Boolean] - def filtered_approvers(regular: true, code_owner: true, target: :approvers, unactioned: false) + def filtered_approvers(regular: true, code_owner: true, report_approver: true, target: :approvers, unactioned: false) rules = [] rules.concat(regular_rules) if regular rules.concat(code_owner_rules) if code_owner + rules.concat(report_approver_rules) if report_approver users = rules.flat_map(&target) users.uniq! @@ -202,6 +205,12 @@ def code_owner_rules end end + def report_approver_rules + strong_memoize(:report_approver_rules) do + wrap_rules(merge_request.approval_rules.select(&:report_approver?)) + end + end + def wrap_rules(rules) rules.map { |rule| ApprovalWrappedRule.new(merge_request, rule) } end diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index fc58d358423a0e..ec01a842fdeb94 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -4,6 +4,7 @@ module ApprovalRuleLike extend ActiveSupport::Concern DEFAULT_NAME = 'Default' + DEFAULT_NAME_FOR_SECURITY_REPORT = 'Vulnerability-Check' APPROVALS_REQUIRED_MAX = 100 included do diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index c5965817dede33..bb522cedff40e0 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -89,10 +89,11 @@ module Pipeline state_machine :status do after_transition any => ::Ci::Pipeline.completed_statuses do |pipeline| - next unless pipeline.has_reports?(::Ci::JobArtifact.security_reports) && pipeline.default_branch? + next unless pipeline.has_reports?(::Ci::JobArtifact.security_reports) pipeline.run_after_commit do - StoreSecurityReportsWorker.perform_async(pipeline.id) + StoreSecurityReportsWorker.perform_async(pipeline.id) if pipeline.default_branch? + SyncSecurityReportsToReportApprovalRulesWorker.perform_async(pipeline.id) end end diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index ee474a3cfaee6b..25a1a1d9849237 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -90,7 +90,7 @@ def validate_approval_rule_source local_project_rule_ids.compact! invalid = if new_record? - local_project_rule_ids.to_set != project.approval_rule_ids.to_set + local_project_rule_ids.to_set != project.visible_regular_approval_rules.pluck(:id).to_set else (local_project_rule_ids - project.approval_rule_ids).present? end diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index ed8735744d3a1d..af9daea988884b 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -322,6 +322,12 @@ def approvals_before_merge super end + def visible_approval_rules + strong_memoize(:visible_approval_rules) do + visible_regular_approval_rules + approval_rules.report_approver + end + end + def visible_regular_approval_rules strong_memoize(:visible_regular_approval_rules) do regular_rules = approval_rules.regular.order(:id) diff --git a/ee/app/models/license.rb b/ee/app/models/license.rb index 1f05abc21fff98..c4979251dec965 100644 --- a/ee/app/models/license.rb +++ b/ee/app/models/license.rb @@ -110,6 +110,7 @@ class License < ApplicationRecord insights web_ide_terminal incident_management + report_approver_rules group_ip_restriction ] EEU_FEATURES.freeze diff --git a/ee/app/services/approval_rules/create_service.rb b/ee/app/services/approval_rules/create_service.rb index 63d3393ae0885a..6e45abdb03875a 100644 --- a/ee/app/services/approval_rules/create_service.rb +++ b/ee/app/services/approval_rules/create_service.rb @@ -5,6 +5,13 @@ class CreateService < ::ApprovalRules::BaseService # @param target [Project, MergeRequest] def initialize(target, user, params) @rule = target.approval_rules.build + + # report_approver rule_type is currently auto-set according to rulename + # Techdebt to be addressed with: https://gitlab.com/gitlab-org/gitlab-ee/issues/12759 + if target.is_a?(Project) && params[:name] == ApprovalProjectRule::DEFAULT_NAME_FOR_SECURITY_REPORT + params.reverse_merge!(rule_type: :report_approver) + end + super(@rule.project, user, params) end end diff --git a/ee/app/services/approval_rules/project_rule_destroy_service.rb b/ee/app/services/approval_rules/project_rule_destroy_service.rb new file mode 100644 index 00000000000000..65e813b041f132 --- /dev/null +++ b/ee/app/services/approval_rules/project_rule_destroy_service.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module ApprovalRules + class ProjectRuleDestroyService < ::BaseService + attr_reader :rule + + def initialize(approval_rule) + @rule = approval_rule + end + + def execute + ActiveRecord::Base.transaction do + # Removes only MR rules associated with project rule + remove_associated_approval_rules_from_unmerged_merge_requests + + rule.destroy + end + + if rule.destroyed? + success + else + error(rule.errors.messages) + end + end + + private + + def remove_associated_approval_rules_from_unmerged_merge_requests + ApprovalMergeRequestRule + .from_project_rule(rule) + .for_unmerged_merge_requests + .delete_all + end + end +end diff --git a/ee/app/services/ee/merge_requests/create_service.rb b/ee/app/services/ee/merge_requests/create_service.rb index 9b0f1e7816f079..c151cf76ad4364 100644 --- a/ee/app/services/ee/merge_requests/create_service.rb +++ b/ee/app/services/ee/merge_requests/create_service.rb @@ -10,6 +10,13 @@ def after_create(issuable) super ::MergeRequests::SyncCodeOwnerApprovalRules.new(issuable).execute + ::MergeRequests::SyncReportApproverApprovalRules.new(issuable).execute + + # Attempt to sync reports if pipeline has finished before MR has been created + pipeline = issuable.find_actual_head_pipeline + if pipeline + ::SyncSecurityReportsToReportApprovalRulesWorker.perform_async(pipeline.id) + end end end end diff --git a/ee/app/services/ee/merge_requests/refresh_service.rb b/ee/app/services/ee/merge_requests/refresh_service.rb index 194d4dc761f044..37296f7f8a4cc7 100644 --- a/ee/app/services/ee/merge_requests/refresh_service.rb +++ b/ee/app/services/ee/merge_requests/refresh_service.rb @@ -38,12 +38,11 @@ def fetch_latest_merge_request_diffs end def update_approvers - return yield unless project.feature_available?(:code_owners) - results = yield merge_requests_for_source_branch.each do |merge_request| - ::MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute + ::MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute if project.feature_available?(:code_owners) + ::MergeRequests::SyncReportApproverApprovalRules.new(merge_request).execute if project.beta_feature_available?(:report_approver_rules) end results diff --git a/ee/app/services/merge_requests/sync_report_approver_approval_rules.rb b/ee/app/services/merge_requests/sync_report_approver_approval_rules.rb new file mode 100644 index 00000000000000..df8c4fa0a73839 --- /dev/null +++ b/ee/app/services/merge_requests/sync_report_approver_approval_rules.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +module MergeRequests + class SyncReportApproverApprovalRules + def initialize(merge_request, params = {}) + @merge_request = merge_request + end + + def execute + if merge_request.target_project.beta_feature_available?(:report_approver_rules) + sync_rules + end + end + + private + + attr_reader :merge_request + + def sync_rules + return if merge_request.merged? + + sync_project_approval_rules_to_merge_request_rules + end + + def sync_project_approval_rules_to_merge_request_rules + merge_request.target_project.approval_rules.report_approver.each do |project_rule| + merge_request.approval_rules.report_approver.first_or_initialize.tap do |rule| + rule.update(attributes_from(project_rule)) + end + end + end + + def attributes_from(project_rule) + project_rule.attributes + .slice('approvals_required', 'name') + .merge( + users: project_rule.users, + groups: project_rule.groups, + approval_project_rule: project_rule, + rule_type: :report_approver, + report_type: :security + ) + end + end +end diff --git a/ee/app/services/security/sync_reports_to_approval_rules_service.rb b/ee/app/services/security/sync_reports_to_approval_rules_service.rb new file mode 100644 index 00000000000000..242e41579a9058 --- /dev/null +++ b/ee/app/services/security/sync_reports_to_approval_rules_service.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module Security + # Service for syncing security reports results to report_approver approval rules + # + class SyncReportsToApprovalRulesService < ::BaseService + def initialize(pipeline) + @pipeline = pipeline + end + + def execute + reports = @pipeline.security_reports.reports + + safe = reports.any? && reports.none? do |_report_type, report| + report.unsafe_severity? + end + + return success unless safe + + if remove_required_report_approvals(@pipeline.merge_requests_as_head_pipeline) + success + else + error("Failed to update approval rules") + end + end + + private + + def remove_required_report_approvals(merge_requests) + ApprovalMergeRequestRule + .security_report + .for_unmerged_merge_requests(merge_requests) + .update_all(approvals_required: 0) + end + end +end diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index eb20ad33078840..f1df649bfdc72a 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -43,6 +43,7 @@ - geo:geo_scheduler_secondary_scheduler - pipeline_default:store_security_reports +- pipeline_default:sync_security_reports_to_report_approval_rules - pipeline_default:ci_create_cross_project_pipeline - pipeline_default:ci_pipeline_bridge_status diff --git a/ee/app/workers/sync_security_reports_to_report_approval_rules_worker.rb b/ee/app/workers/sync_security_reports_to_report_approval_rules_worker.rb new file mode 100644 index 00000000000000..2b0c45022734ca --- /dev/null +++ b/ee/app/workers/sync_security_reports_to_report_approval_rules_worker.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +# Worker for syncing report_type approval_rules approvals_required +# +class SyncSecurityReportsToReportApprovalRulesWorker + include ApplicationWorker + include PipelineQueue + + def perform(pipeline_id) + pipeline = Ci::Pipeline.find_by_id(pipeline_id) + return unless pipeline + + ::Security::SyncReportsToApprovalRulesService.new(pipeline).execute + end +end diff --git a/ee/changelogs/unreleased/9928-add-report_approver-to-approval_rules-services.yml b/ee/changelogs/unreleased/9928-add-report_approver-to-approval_rules-services.yml new file mode 100644 index 00000000000000..942dc0f34aebf5 --- /dev/null +++ b/ee/changelogs/unreleased/9928-add-report_approver-to-approval_rules-services.yml @@ -0,0 +1,5 @@ +--- +title: Enable security gates for merge requests +merge_request: 13109 +author: +type: added diff --git a/ee/lib/api/project_approval_rules.rb b/ee/lib/api/project_approval_rules.rb index 583017a5624d8f..4b4c5db334e856 100644 --- a/ee/lib/api/project_approval_rules.rb +++ b/ee/lib/api/project_approval_rules.rb @@ -29,6 +29,7 @@ class ProjectApprovalRules < ::Grape::API params do requires :name, type: String, desc: 'The name of the approval rule' requires :approvals_required, type: Integer, desc: 'The number of required approvals for this rule' + optional :rule_type, type: String, desc: 'The type of approval rule', default: 'regular' optional :users, as: :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule' optional :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule' end @@ -81,9 +82,10 @@ class ProjectApprovalRules < ::Grape::API authorize! :admin_project, user_project approval_rule = user_project.approval_rules.find(params[:approval_rule_id]) - destroy_conditionally!(approval_rule) - no_content! + destroy_conditionally!(approval_rule) do |rule| + ::ApprovalRules::ProjectRuleDestroyService.new(rule).execute + end end end end diff --git a/ee/lib/ee/api/entities.rb b/ee/lib/ee/api/entities.rb index 235ab6940ed480..392e0933ef7acd 100644 --- a/ee/lib/ee/api/entities.rb +++ b/ee/lib/ee/api/entities.rb @@ -346,7 +346,7 @@ class MergeRequestApprovalRules < Grape::Entity # Decorates Project class ProjectApprovalRules < Grape::Entity - expose :visible_regular_approval_rules, as: :rules, using: ApprovalRule + expose :visible_approval_rules, as: :rules, using: ApprovalRule expose :min_fallback_approvals, as: :fallback_approvals_required end diff --git a/ee/lib/gitlab/ci/reports/security/report.rb b/ee/lib/gitlab/ci/reports/security/report.rb index ca64b744b3482c..9cf018ae481910 100644 --- a/ee/lib/gitlab/ci/reports/security/report.rb +++ b/ee/lib/gitlab/ci/reports/security/report.rb @@ -5,6 +5,8 @@ module Ci module Reports module Security class Report + UNSAFE_SEVERITIES = %w[unknown high critical].freeze + attr_reader :type attr_reader :commit_sha attr_reader :occurrences @@ -50,6 +52,10 @@ def replace_with!(other) def merge!(other) replace_with!(::Security::MergeReportsService.new(self, other).execute) end + + def unsafe_severity? + occurrences.any? { |occurrence| UNSAFE_SEVERITIES.include?(occurrence.severity) } + end end end end diff --git a/ee/spec/factories/approval_rules.rb b/ee/spec/factories/approval_rules.rb index 81ba81f6ee0b53..83a164bd09de25 100644 --- a/ee/spec/factories/approval_rules.rb +++ b/ee/spec/factories/approval_rules.rb @@ -23,5 +23,11 @@ factory :approval_project_rule do project name ApprovalRuleLike::DEFAULT_NAME + rule_type :regular + + trait :security_report do + rule_type :report_approver + name ApprovalRuleLike::DEFAULT_NAME_FOR_SECURITY_REPORT + end end end diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb index 10edf581c13dd5..06cdc9357890e6 100644 --- a/ee/spec/models/approval_merge_request_rule_spec.rb +++ b/ee/spec/models/approval_merge_request_rule_spec.rb @@ -76,6 +76,7 @@ set(:js_rule) { create(:code_owner_rule, name: '*.js') } set(:css_rule) { create(:code_owner_rule, name: '*.css') } set(:approval_rule) { create(:approval_merge_request_rule) } + set(:report_approver_rule) { create(:report_approver_rule) } describe '.not_matching_pattern' do it 'returns the correct rules' do @@ -97,6 +98,13 @@ .to contain_exactly(rb_rule, js_rule, css_rule) end end + + describe '.security_report' do + it 'returns the correct rules' do + expect(described_class.security_report) + .to contain_exactly(report_approver_rule) + end + end end describe '.find_or_create_code_owner_rule' do @@ -241,7 +249,7 @@ describe 'approvals_required' do subject { build(:approval_merge_request_rule, merge_request: merge_request) } - it 'is a natual number' do + it 'is a natural number' do subject.assign_attributes(approvals_required: 2) expect(subject).to be_valid @@ -261,6 +269,22 @@ expect(subject.errors[:approvals_required]).to include("must be greater than or equal to 3") end + + context 'when report_approver rule' do + subject do + build(:report_approver_rule, merge_request: merge_request, approvals_required: 1).tap do |rule| + rule.approval_project_rule = project_rule + end + end + + it 'skips validation' do + expect(subject).to be_valid + + subject.approvals_required = 0 + + expect(subject).to be_valid + end + end end end end diff --git a/ee/spec/models/approval_project_rule_spec.rb b/ee/spec/models/approval_project_rule_spec.rb index 75bb8c332c093d..c09b81b2229a10 100644 --- a/ee/spec/models/approval_project_rule_spec.rb +++ b/ee/spec/models/approval_project_rule_spec.rb @@ -6,8 +6,9 @@ subject { create(:approval_project_rule) } describe '.regular' do - it 'returns all records' do + it 'returns non-report_approver records' do rules = create_list(:approval_project_rule, 2) + create(:approval_project_rule, :security_report) expect(described_class.regular).to contain_exactly(*rules) end @@ -21,23 +22,43 @@ end end - describe '#regular' do - it 'returns true' do - expect(subject.regular).to eq(true) + describe '#regular?' do + let(:security_approver_rule) { build(:approval_project_rule, :security_report) } + + it 'returns true for regular rules' do expect(subject.regular?).to eq(true) end + + it 'returns false for report_approver rules' do + expect(security_approver_rule.regular?). to eq(false) + end end - describe '#code_owner' do + describe '#code_owner?' do it 'returns false' do - expect(subject.code_owner).to eq(false) expect(subject.code_owner?).to eq(false) end end + describe '#report_approver?' do + let(:security_approver_rule) { build(:approval_project_rule, :security_report) } + + it 'returns false for regular rules' do + expect(subject.report_approver?).to eq(false) + end + + it 'returns true for report_approver rules' do + expect(security_approver_rule.report_approver?). to eq(true) + end + end + describe '#rule_type' do - it 'returns the correct type' do + it 'returns the regular type for regular rules' do expect(build(:approval_project_rule).rule_type).to eq('regular') end + + it 'returns the report_approver type for security report approvers rules' do + expect(build(:approval_project_rule, :security_report).rule_type).to eq('report_approver') + end end end diff --git a/ee/spec/models/approval_state_spec.rb b/ee/spec/models/approval_state_spec.rb index 451df27023a822..01ddf35f1cd3cf 100644 --- a/ee/spec/models/approval_state_spec.rb +++ b/ee/spec/models/approval_state_spec.rb @@ -6,7 +6,12 @@ def create_rule(additional_params = {}) default_approver = create(:user) params = additional_params.reverse_merge(merge_request: merge_request, users: [default_approver]) - factory = params.delete(:code_owner) ? :code_owner_rule : :approval_merge_request_rule + factory = + case params.delete(:rule_type) + when :code_owner then :code_owner_rule + when :report_approver then :report_approver_rule + else :approval_merge_request_rule + end create(factory, params) end @@ -265,7 +270,16 @@ def disable_feature context 'when only code owner rules present' do before do - 2.times { create_rule(users: [create(:user)], code_owner: true) } + 2.times { create_rule(users: [create(:user)], rule_type: :code_owner) } + end + + it_behaves_like 'when rules are present' + it_behaves_like 'checking fallback_approvals_required' + end + + context 'when only report approver rules present' do + before do + 2.times { create_rule(users: [create(:user)], rule_type: :report_approver) } end it_behaves_like 'when rules are present' @@ -380,13 +394,16 @@ def create_unapproved_rule end describe '#filtered_approvers' do - describe 'only direct users, without code owners' do + describe 'only direct users, without code owners or report_approvers' do it 'includes only rule user members' do create_rule(users: [approver1]) create_rule(users: [approver1], groups: [group1]) - create_rule(users: [approver2], code_owner: true) + create_rule(users: [approver2], rule_type: :code_owner) + create_rule(users: [approver3], rule_type: :report_approver) - expect(subject.filtered_approvers(code_owner: false, target: :users)).to contain_exactly(approver1) + expect( + subject.filtered_approvers(code_owner: false, report_approver: false, target: :users) + ).to contain_exactly(approver1) end end @@ -394,7 +411,17 @@ def create_unapproved_rule it 'includes only code owners' do create_rule(users: [approver1]) create_rule(users: [approver1], groups: [group1]) - create_rule(users: [approver2], code_owner: true) + create_rule(users: [approver2], rule_type: :code_owner) + + expect(subject.filtered_approvers(regular: false)).to contain_exactly(approver2) + end + end + + describe 'only report approvers' do + it 'includes only report approvers' do + create_rule(users: [approver1]) + create_rule(users: [approver1], groups: [group1]) + create_rule(users: [approver2], rule_type: :report_approver) expect(subject.filtered_approvers(regular: false)).to contain_exactly(approver2) end @@ -404,11 +431,12 @@ def create_unapproved_rule it 'excludes approved approvers' do create_rule(users: [approver1]) create_rule(users: [approver1], groups: [group1]) - create_rule(users: [approver2], code_owner: true) + create_rule(users: [approver2], rule_type: :code_owner) + create_rule(users: [approver3], rule_type: :report_approver) create(:approval, merge_request: merge_request, user: approver1) - expect(subject.filtered_approvers(unactioned: true)).to contain_exactly(approver2, group_approver1) + expect(subject.filtered_approvers(unactioned: true)).to contain_exactly(approver2, approver3, group_approver1) end end @@ -889,11 +917,13 @@ def create_rules rule1 rule2 code_owner_rule + report_approver_rule end let(:rule1) { create_unapproved_rule } let(:rule2) { create_unapproved_rule } - let(:code_owner_rule) { create_unapproved_rule(code_owner: true, approvals_required: 0) } + let(:code_owner_rule) { create_unapproved_rule(rule_type: :code_owner, approvals_required: 0) } + let(:report_approver_rule) { create_unapproved_rule(rule_type: :report_approver, approvals_required: 0) } before do stub_licensed_features multiple_approval_rules: false @@ -907,7 +937,7 @@ def create_rules expect(rule.is_a?(ApprovalWrappedRule)).to eq(true) end - expect(subject.wrapped_approval_rules.size).to eq(2) + expect(subject.wrapped_approval_rules.size).to eq(3) end end @@ -1021,7 +1051,16 @@ def create_rules context 'when only code owner rules present' do before do # setting approvals required to 0 since we don't want to block on them now - 2.times { create_rule(users: [create(:user)], code_owner: true, approvals_required: 0) } + 2.times { create_rule(users: [create(:user)], rule_type: :code_owner, approvals_required: 0) } + end + + it_behaves_like 'when rules are present' + it_behaves_like 'checking fallback_approvals_required' + end + + context 'when only report approver rules present' do + before do + 2.times { create_rule(users: [create(:user)], rule_type: :report_approver) } end it_behaves_like 'when rules are present' @@ -1114,11 +1153,12 @@ def create_rules end describe '#approvers' do - let(:code_owner_rule) { create_rule(code_owner: true, groups: [group1]) } + let(:code_owner_rule) { create_rule(rule_type: :code_owner, groups: [group1]) } + let(:report_approver_rule) { create_rule(rule_type: :report_approver, users: [approver2]) } - it 'includes approvers from first rule and code owner rule' do + it 'includes approvers from first rule, code owner rule, and report approver rule' do create_rules - approvers = rule1.users + code_owner_rule.users + [group_approver1] + approvers = rule1.users + code_owner_rule.users + [group_approver1, approver2] expect(subject.approvers).to contain_exactly(*approvers) end @@ -1129,13 +1169,16 @@ def create_rules end describe '#filtered_approvers' do - describe 'only direct users, without code owners' do + describe 'only direct users, without code owners or report approvers' do it 'includes only rule user members' do create_rule(users: [approver1]) create_rule(users: [approver1], groups: [group1]) - create_rule(users: [approver2], code_owner: true) + create_rule(users: [approver2], rule_type: :code_owner) + create_rule(users: [approver3], rule_type: :report_approver) - expect(subject.filtered_approvers(code_owner: false, target: :users)).to contain_exactly(approver1) + expect( + subject.filtered_approvers(code_owner: false, report_approver: false, target: :users) + ).to contain_exactly(approver1) end end @@ -1143,7 +1186,15 @@ def create_rules it 'includes only code owners' do create_rule(users: [approver1]) create_rule(users: [approver1], groups: [group1]) - create_rule(users: [approver2], code_owner: true) + create_rule(users: [approver2], rule_type: :code_owner) + + expect(subject.filtered_approvers(regular: false)).to contain_exactly(approver2) + end + + it 'includes only report approvers' do + create_rule(users: [approver1]) + create_rule(users: [approver1], groups: [group1]) + create_rule(users: [approver2], rule_type: :report_approver) expect(subject.filtered_approvers(regular: false)).to contain_exactly(approver2) end @@ -1153,11 +1204,12 @@ def create_rules it 'excludes approved approvers' do create_rule(users: [approver1]) create_rule(users: [approver1], groups: [group1]) - create_rule(users: [approver2], code_owner: true) + create_rule(users: [approver2], rule_type: :code_owner) + create_rule(users: [approver3], rule_type: :report_approver) create(:approval, merge_request: merge_request, user: approver1) - expect(subject.filtered_approvers(unactioned: true)).to contain_exactly(approver2) + expect(subject.filtered_approvers(unactioned: true)).to contain_exactly(approver2, approver3) end end diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb index 8c8f90d5720710..640768e41aff64 100644 --- a/ee/spec/models/merge_request_spec.rb +++ b/ee/spec/models/merge_request_spec.rb @@ -51,6 +51,10 @@ end describe 'approval_rules' do + before do + stub_licensed_features(multiple_approval_rules: true) + end + context 'when project contains approval_rules' do let!(:project_rule1) { project.approval_rules.create(name: 'p1') } let!(:project_rule2) { project.approval_rules.create(name: 'p2') } diff --git a/ee/spec/requests/api/project_approval_rules_spec.rb b/ee/spec/requests/api/project_approval_rules_spec.rb index 95525cacde3162..ad48bab4ebef5c 100644 --- a/ee/spec/requests/api/project_approval_rules_spec.rb +++ b/ee/spec/requests/api/project_approval_rules_spec.rb @@ -71,6 +71,21 @@ expect(rule['groups'].size).to eq(1) end end + + context 'report_approver rules' do + let!(:report_approver_rule) do + create(:approval_project_rule, :security_report, project: project) + end + + it 'includes report_approver rules' do + get api(url, developer) + + json = json_response + + expect(json['rules'].size).to eq(2) + expect(json['rules'].map { |rule| rule['name'] }).to contain_exactly(rule.name, report_approver_rule.name) + end + end end end diff --git a/ee/spec/services/approval_rules/create_service_spec.rb b/ee/spec/services/approval_rules/create_service_spec.rb index fa1b6f4471d5cc..816a02735b091f 100644 --- a/ee/spec/services/approval_rules/create_service_spec.rb +++ b/ee/spec/services/approval_rules/create_service_spec.rb @@ -82,6 +82,22 @@ let(:target) { project } it_behaves_like "creatable" + + context 'when name matches default for security reports' do + it 'sets rule_type as report_approver' do + result = described_class.new(target, user, { + name: ApprovalProjectRule::DEFAULT_NAME_FOR_SECURITY_REPORT, + approvals_required: 1 + }).execute + + expect(result[:status]).to eq(:success) + + rule = result[:rule] + + expect(rule.approvals_required).to eq(1) + expect(rule.rule_type).to eq('report_approver') + end + end end context 'when target is merge request' do diff --git a/ee/spec/services/approval_rules/project_rule_destroy_service_spec.rb b/ee/spec/services/approval_rules/project_rule_destroy_service_spec.rb new file mode 100644 index 00000000000000..2fd4a718b6f6be --- /dev/null +++ b/ee/spec/services/approval_rules/project_rule_destroy_service_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ApprovalRules::ProjectRuleDestroyService do + let(:project) { create(:project, :repository) } + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + + describe '#execute' do + let!(:project_rule) { create(:approval_project_rule, project: project) } + + subject { described_class.new(project_rule) } + + context 'when there is no merge request rules' do + it 'destroys project rule' do + expect { subject.execute }.to change { ApprovalProjectRule.count }.by(-1) + end + end + + context 'when there is a merge request rule' do + let!(:merge_request_rule) do + create(:approval_merge_request_rule, merge_request: merge_request).tap do |rule| + rule.approval_project_rule = project_rule + end + end + + context 'when open' do + it 'destroys merge request rules' do + expect { subject.execute }.to change { ApprovalMergeRequestRule.count }.by(-1) + end + end + + context 'when merged' do + before do + merge_request.mark_as_merged! + end + + it 'does nothing' do + expect { subject.execute }.not_to change { ApprovalMergeRequestRule.count } + end + end + end + end +end diff --git a/ee/spec/services/ee/merge_requests/create_service_spec.rb b/ee/spec/services/ee/merge_requests/create_service_spec.rb index e380e773e9d7f7..572421565accd2 100644 --- a/ee/spec/services/ee/merge_requests/create_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/create_service_spec.rb @@ -34,6 +34,37 @@ service.execute end + context 'report approvers' do + let(:sha) { project.repository.commits(opts[:source_branch], limit: 1).first.id } + let(:pipeline) { instance_double(Ci::Pipeline, id: 42, project_id: project.id, triggered_by_merge_request?: true) } + + it 'refreshes report approvers for the merge request' do + expect_next_instance_of(::MergeRequests::SyncReportApproverApprovalRules) do |service| + expect(service).to receive(:execute) + end + + service.execute + end + + it 'enqueues approval rule report syncing when pipeline exists' do + expect_next_instance_of(MergeRequest) do |merge_request| + allow(merge_request).to receive(:find_actual_head_pipeline).and_return(pipeline) + allow(merge_request).to receive(:update_head_pipeline).and_return(true) + end + expect(::SyncSecurityReportsToReportApprovalRulesWorker) + .to receive(:perform_async) + + service.execute + end + + it 'wont enqueue approval rule report syncing without pipeline' do + expect(::SyncSecurityReportsToReportApprovalRulesWorker) + .not_to receive(:perform_async) + + service.execute + end + end + it_behaves_like 'new issuable with scoped labels' do let(:parent) { project } end diff --git a/ee/spec/services/ee/merge_requests/refresh_service_spec.rb b/ee/spec/services/ee/merge_requests/refresh_service_spec.rb index 53056298c3ff62..44f7887a06106f 100644 --- a/ee/spec/services/ee/merge_requests/refresh_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/refresh_service_spec.rb @@ -44,11 +44,13 @@ let(:current_user) { merge_request.author } let(:service) { described_class.new(project, current_user) } let(:enable_code_owner) { true } + let(:enable_report_approver_rules) { true } let(:todo_service) { double(:todo_service) } let(:notification_service) { double(:notification_service) } before do stub_licensed_features(code_owners: enable_code_owner) + stub_licensed_features(report_approver_rules: enable_report_approver_rules) allow(service).to receive(:mark_pending_todos_done) allow(service).to receive(:notify_about_push) @@ -88,6 +90,20 @@ subject end end + + context 'when report_approver_rules enabled, with approval_rule enabled' do + let(:relevant_merge_requests) { [merge_request, another_merge_request] } + + it 'refreshes the report_approver rules for all relevant merge requests' do + relevant_merge_requests.each do |merge_request| + expect_next_instance_of(::MergeRequests::SyncReportApproverApprovalRules, merge_request) do |service| + expect(service).to receive(:execute) + end + end + + subject + end + end end describe 'Pipelines for merge requests' do diff --git a/ee/spec/services/merge_requests/sync_report_approver_approval_rules_spec.rb b/ee/spec/services/merge_requests/sync_report_approver_approval_rules_spec.rb new file mode 100644 index 00000000000000..58928632e235dd --- /dev/null +++ b/ee/spec/services/merge_requests/sync_report_approver_approval_rules_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe MergeRequests::SyncReportApproverApprovalRules do + let(:merge_request) { create(:merge_request) } + let!(:security_approval_project_rule) { create(:approval_project_rule, :security_report, project: merge_request.target_project, approvals_required: 2) } + + subject(:service) { described_class.new(merge_request) } + + describe '#execute' do + context 'when report_approver_rules are enabled' do + let!(:regular_approval_project_rule) { create(:approval_project_rule, project: merge_request.target_project) } + + before do + stub_feature_flags(report_approver_rules: true) + end + + it 'creates rule for report approvers' do + expect { service.execute } + .to change { merge_request.approval_rules.security_report.count }.from(0).to(1) + + rule = merge_request.approval_rules.security_report.first + + expect(rule).to be_report_approver + expect(rule.report_type).to eq 'security' + expect(rule.name).to eq(security_approval_project_rule.name) + expect(rule.approval_project_rule).to eq(security_approval_project_rule) + end + + it 'updates previous rules if defined' do + mr_rule = create(:report_approver_rule, merge_request: merge_request, approvals_required: 0) + + expect { service.execute } + .not_to change { merge_request.approval_rules.security_report.count } + + expect(mr_rule.reload).to be_report_approver + expect(mr_rule.report_type).to eq 'security' + expect(mr_rule.name).to eq(security_approval_project_rule.name) + expect(mr_rule.approvals_required).to eq security_approval_project_rule.approvals_required + expect(mr_rule.approval_project_rule).to eq(security_approval_project_rule) + end + end + + context 'when report_approver_rules are disabled' do + before do + stub_feature_flags(report_approver_rules: false) + end + + it 'copies nothing' do + expect { service.execute } + .not_to change { merge_request.approval_rules.count } + end + end + end +end diff --git a/ee/spec/services/security/sync_reports_to_approval_rules_service_spec.rb b/ee/spec/services/security/sync_reports_to_approval_rules_service_spec.rb new file mode 100644 index 00000000000000..ccdfbd7aea0f7f --- /dev/null +++ b/ee/spec/services/security/sync_reports_to_approval_rules_service_spec.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Security::SyncReportsToApprovalRulesService, '#execute' do + let(:merge_request) { create(:merge_request) } + let(:project) { merge_request.project } + let(:pipeline) { create(:ee_ci_pipeline, :success, project: project, merge_requests_as_head_pipeline: [merge_request]) } + let(:report_approver_rule) { create(:report_approver_rule, merge_request: merge_request, approvals_required: 2) } + + subject { described_class.new(pipeline).execute } + + before do + allow(Ci::Pipeline).to receive(:find).with(pipeline.id) { pipeline } + + stub_licensed_features(dependency_scanning: true, dast: true) + end + + context 'when there are reports' do + context 'when pipeline passes' do + context 'when high-severity vulnerabilities are present' do + before do + create(:ee_ci_build, :success, :dependency_scanning, name: 'ds_job', pipeline: pipeline, project: project) + end + + it "won't change approvals_required count" do + expect( + pipeline.security_reports.reports.values.all?(&:unsafe_severity?) + ).to be true + + expect { subject } + .not_to change { report_approver_rule.reload.approvals_required } + end + end + + context 'when only low-severity vulnerabilities are present' do + before do + create(:ee_ci_build, :success, :dast, name: 'dast_job', pipeline: pipeline, project: project) + end + + it 'lowers approvals_required count to zero' do + expect( + pipeline.security_reports.reports.values.none?(&:unsafe_severity?) + ).to be true + + expect { subject } + .to change { report_approver_rule.reload.approvals_required }.from(2).to(0) + end + end + + context 'when merge_requests are merged' do + let!(:merge_request) { create(:merge_request, :merged) } + + before do + create(:ee_ci_build, :success, :dast, name: 'dast_job', pipeline: pipeline, project: project) + end + + it "won't change approvals_required count" do + expect( + pipeline.security_reports.reports.values.all?(&:unsafe_severity?) + ).to be false + + expect { subject } + .not_to change { report_approver_rule.reload.approvals_required } + end + end + end + + context 'when pipeline fails' do + before do + pipeline.update!(status: :failed) + end + + context 'when high-severity vulnerabilities are present' do + before do + create(:ee_ci_build, :success, :dependency_scanning, name: 'ds_job', pipeline: pipeline, project: project) + end + + it "won't change approvals_required count" do + expect( + pipeline.security_reports.reports.values.all?(&:unsafe_severity?) + ).to be true + + expect { subject } + .not_to change { report_approver_rule.reload.approvals_required } + end + end + + context 'when only low-severity vulnerabilities are present' do + before do + create(:ee_ci_build, :success, :dast, name: 'dast_job', pipeline: pipeline, project: project) + end + + it 'lowers approvals_required count to zero' do + expect( + pipeline.security_reports.reports.values.none?(&:unsafe_severity?) + ).to be true + + expect { subject } + .to change { report_approver_rule.reload.approvals_required } + end + end + end + end + + context 'without reports' do + it "won't change approvals_required count" do + expect { subject } + .not_to change { report_approver_rule.reload.approvals_required } + end + end +end diff --git a/ee/spec/workers/sync_security_reports_to_report_approval_rules_worker_spec.rb b/ee/spec/workers/sync_security_reports_to_report_approval_rules_worker_spec.rb new file mode 100644 index 00000000000000..4d970b15d363b6 --- /dev/null +++ b/ee/spec/workers/sync_security_reports_to_report_approval_rules_worker_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe SyncSecurityReportsToReportApprovalRulesWorker do + describe '#perform' do + let(:pipeline) { double(:pipeline, id: 42) } + let(:sync_service) { double(:service, execute: true) } + + context 'when pipeline exists' do + before do + allow(Ci::Pipeline).to receive(:find_by_id).with(pipeline.id) { pipeline } + end + + it "executes SyncReportsToApprovalRulesService for given pipeline" do + expect(Security::SyncReportsToApprovalRulesService).to receive(:new) + .with(pipeline).once.and_return(sync_service) + + described_class.new.perform(pipeline.id) + end + end + + context 'when pipeline is missing' do + it 'does not execute SyncReportsToApprovalRulesService' do + expect(Security::SyncReportsToApprovalRulesService).not_to receive(:new) + + described_class.new.perform(pipeline.id) + end + end + end +end -- GitLab