Merge branch...

Merge branch '13538-license-compliance-approvals-in-merge-request-mvc-add-license-check-approval-rule-to-backend-code-to-enforce-policy' into 'master'

Add 'License-Check' approval rule to enforce license compliance policy.

See merge request gitlab-org/gitlab-ee!15196

ee/app/models/approval_merge_request_rule.rb

@@ -47,7 +47,8 @@ class ApprovalMergeRequestRule < ApplicationRecord
   }
 
   enum report_type: {
-    security: 1
+    security: 1,
+    license_management: 2
   }
 
   # Deprecated scope until code_owner column has been migrated to rule_type

ee/app/models/approval_project_rule.rb

@@ -16,4 +16,32 @@ class ApprovalProjectRule < ApplicationRecord
   def source_rule
     nil
   end
+
+  def apply_report_approver_rules_to(merge_request)
+    report_type = report_type_for(self)
+    rule = merge_request
+      .approval_rules
+      .report_approver
+      .find_or_initialize_by(report_type: report_type)
+    rule.update!(attributes_to_apply_for(report_type))
+    rule
+  end
+
+  private
+
+  def report_type_for(rule)
+    ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME[rule.name]
+  end
+
+  def attributes_to_apply_for(report_type)
+    attributes
+      .slice('approvals_required', 'name')
+      .merge(
+        users: users,
+        groups: groups,
+        approval_project_rule: self,
+        rule_type: :report_approver,
+        report_type: report_type
+      )
+  end
 end

ee/app/models/concerns/approval_rule_like.rb

@@ -4,7 +4,12 @@ module ApprovalRuleLike
   extend ActiveSupport::Concern
 
   DEFAULT_NAME = 'Default'
+  DEFAULT_NAME_FOR_LICENSE_REPORT = 'License-Check'
   DEFAULT_NAME_FOR_SECURITY_REPORT = 'Vulnerability-Check'
+  REPORT_TYPES_BY_DEFAULT_NAME = {
+    DEFAULT_NAME_FOR_LICENSE_REPORT => :license_management,
+    DEFAULT_NAME_FOR_SECURITY_REPORT => :security
+  }.freeze
   APPROVALS_REQUIRED_MAX = 100
 
   included do

ee/app/models/ee/merge_request.rb

@@ -171,5 +171,14 @@ module EE
 
       compare_reports(::Ci::CompareMetricsReportsService)
     end
+
+    def synchronize_approval_rules_from_target_project
+      return if merged?
+
+      project_rules = target_project.approval_rules.report_approver.includes(:users, :groups)
+      project_rules.find_each do |project_rule|
+        project_rule.apply_report_approver_rules_to(self)
+      end
+    end
   end
 end

ee/app/models/software_license_policy.rb

@@ -35,10 +35,8 @@ class SoftwareLicensePolicy < ApplicationRecord
   scope :including_license, -> { includes(:software_license) }
 
   scope :with_license_by_name, -> (license_name) do
-    joins(:software_license).where(software_licenses: { name: license_name })
+    with_license.where(SoftwareLicense.arel_table[:name].lower.in(Array(license_name).map(&:downcase)))
   end
 
-  def name
-    software_license.name
-  end
+  delegate :name, to: :software_license
 end

ee/app/services/approval_rules/create_service.rb

@@ -10,7 +10,7 @@ module ApprovalRules
 
       # 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
+      if target.is_a?(Project) && ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME.key?(params[:name])
         params.reverse_merge!(rule_type: :report_approver)
       end
 

ee/app/services/merge_requests/sync_report_approver_approval_rules.rb

@@ -8,38 +8,12 @@ module MergeRequests
 
     def execute
       if merge_request.target_project.feature_available?(:report_approver_rules)
-        sync_rules
+        merge_request.synchronize_approval_rules_from_target_project
       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

ee/app/services/security/sync_reports_to_approval_rules_service.rb

@@ -9,27 +9,44 @@ module Security
     end
 
     def execute
-      reports = @pipeline.security_reports.reports
+      sync_license_management_rules
+      sync_vulnerability_rules
+      success
+    rescue StandardError => error
+      log_error(
+        pipeline: pipeline&.to_param,
+        error: error.class.name,
+        message: error.message,
+        source: "#{__FILE__}:#{__LINE__}",
+        backtrace: error.backtrace
+      )
+      error("Failed to update approval rules")
+    end
+
+    private
 
+    attr_reader :pipeline
+
+    def sync_license_management_rules
+      project = pipeline.project
+      report = pipeline.license_management_report
+      return if report.violates?(project.software_license_policies)
+
+      remove_required_approvals_for(ApprovalMergeRequestRule.report_approver.license_management)
+    end
+
+    def sync_vulnerability_rules
+      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
+      remove_required_approvals_for(ApprovalMergeRequestRule.security_report) if safe
     end
 
-    private
-
-    def remove_required_report_approvals(merge_requests)
-      ApprovalMergeRequestRule
-        .security_report
-        .for_unmerged_merge_requests(merge_requests)
+    def remove_required_approvals_for(rules)
+      rules
+        .for_unmerged_merge_requests(pipeline.merge_requests_as_head_pipeline)
         .update_all(approvals_required: 0)
     end
   end

ee/changelogs/unreleased/15196-license-compliance-approvals-in-merge-request-mvc-add-license-ch.yml

+---
+title: Add 'License-Check' approval rule to enforce license compliance policy.
+merge_request: 15196
+author:
+type: added

ee/lib/gitlab/ci/reports/license_management/report.rb

@@ -22,6 +22,10 @@ module Gitlab
             @found_licenses[key] ||= ::Gitlab::Ci::Reports::LicenseManagement::License.new(license_name)
             @found_licenses[key].add_dependency(dependency_name)
           end
+
+          def violates?(software_license_policies)
+            software_license_policies.blacklisted.with_license_by_name(license_names).exists?
+          end
         end
       end
     end

ee/spec/factories/approval_rules.rb

@@ -18,6 +18,11 @@ FactoryBot.define do
     rule_type :report_approver
     report_type :security
     sequence(:name) { |n| "*-#{n}.js" }
+
+    trait :license_management do
+      name ApprovalRuleLike::DEFAULT_NAME_FOR_LICENSE_REPORT
+      report_type :license_management
+    end
   end
 
   factory :approval_project_rule do
@@ -25,9 +30,22 @@ FactoryBot.define do
     name ApprovalRuleLike::DEFAULT_NAME
     rule_type :regular
 
+    trait :requires_approval do
+      approvals_required { rand(1..ApprovalProjectRule::APPROVALS_REQUIRED_MAX) }
+    end
+
     trait :security_report do
       rule_type :report_approver
       name ApprovalRuleLike::DEFAULT_NAME_FOR_SECURITY_REPORT
     end
+
+    trait :security do
+      security_report
+    end
+
+    trait :license_management do
+      name ApprovalRuleLike::DEFAULT_NAME_FOR_LICENSE_REPORT
+      rule_type :report_approver
+    end
   end
 end

ee/spec/factories/ci/reports/license_management/report.rb

@@ -15,5 +15,11 @@ FactoryBot.define do
         report.add_dependency('Apache 2.0', 'Library3')
       end
     end
+
+    trait :mit do
+      after(:build) do |report, evaluator|
+        report.add_dependency('MIT', 'rails')
+      end
+    end
   end
 end

ee/spec/factories/software_license.rb

@@ -3,5 +3,13 @@
 FactoryBot.define do
   factory :software_license, class: SoftwareLicense do
     sequence(:name) { |n| "SOFTWARE-LICENSE-2.7/example_#{n}" }
+
+    trait :mit do
+      name { 'MIT' }
+    end
+
+    trait :apache_2_0 do
+      name { 'Apache 2.0' }
+    end
   end
 end

ee/spec/factories/software_license_policy.rb

@@ -5,5 +5,9 @@ FactoryBot.define do
     approval_status 1
     project
     software_license
+
+    trait :blacklist do
+      approval_status :blacklisted
+    end
   end
 end

ee/spec/lib/gitlab/ci/reports/license_management/report_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Ci::Reports::LicenseManagement::Report do
+  subject { build(:ci_reports_license_management_report, :mit) }
+
+  describe '#violates?' do
+    let(:project) { create(:project) }
+    let(:mit_license) { build(:software_license, :mit) }
+    let(:apache_license) { build(:software_license, :apache_2_0) }
+
+    context "when a blacklisted license is found in the report" do
+      let(:mit_blacklist) { build(:software_license_policy, :blacklist, software_license: mit_license) }
+
+      before do
+        project.software_license_policies << mit_blacklist
+      end
+
+      specify { expect(subject.violates?(project.software_license_policies)).to be(true) }
+    end
+
+    context "when a blacklisted license is discovered with a different casing for the name" do
+      let(:mit_blacklist) { build(:software_license_policy, :blacklist, software_license: mit_license) }
+
+      before do
+        mit_license.update!(name: 'mit')
+        project.software_license_policies << mit_blacklist
+      end
+
+      specify { expect(subject.violates?(project.software_license_policies)).to be(true) }
+    end
+
+    context "when none of the licenses discovered in the report violate the blacklist policy" do
+      let(:apache_blacklist) { build(:software_license_policy, :blacklist, software_license: apache_license) }
+
+      before do
+        project.software_license_policies << apache_blacklist
+      end
+
+      specify { expect(subject.violates?(project.software_license_policies)).to be(false) }
+    end
+  end
+end

ee/spec/models/approval_project_rule_spec.rb

@@ -61,4 +61,27 @@ describe ApprovalProjectRule do
       expect(build(:approval_project_rule, :security_report).rule_type).to eq('report_approver')
     end
   end
+
+  describe "#apply_report_approver_rules_to" do
+    let(:project) { merge_request.target_project }
+    let(:merge_request) { create(:merge_request) }
+    let(:user) { create(:user) }
+    let(:group) { create(:group) }
+
+    before do
+      subject.users << user
+      subject.groups << group
+    end
+
+    ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME.each do |name, value|
+      context "when the project rule is for a `#{name}`" do
+        subject { create(:approval_project_rule, value, :requires_approval, project: project) }
+        let!(:result) { subject.apply_report_approver_rules_to(merge_request) }
+
+        specify { expect(merge_request.reload.approval_rules).to match_array([result]) }
+        specify { expect(result.users).to match_array([user]) }
+        specify { expect(result.groups).to match_array([group]) }
+      end
+    end
+  end
 end

ee/spec/models/software_license_policy_spec.rb

@@ -12,4 +12,28 @@ describe SoftwareLicensePolicy do
     it { is_expected.to validate_presence_of(:approval_status) }
     it { is_expected.to validate_uniqueness_of(:software_license).scoped_to(:project_id).with_message(/has already been taken/) }
   end
+
+  describe ".with_license_by_name" do
+    subject { described_class }
+    let!(:mit_policy) { create(:software_license_policy, software_license: mit) }
+    let!(:mit) { create(:software_license, :mit) }
+    let!(:apache_policy) { create(:software_license_policy, software_license: apache) }
+    let!(:apache) { create(:software_license, :apache_2_0) }
+
+    it 'finds a license by an exact match' do
+      expect(subject.with_license_by_name(mit.name)).to match_array([mit_policy])
+    end
+
+    it 'finds a license by a case insensitive match' do
+      expect(subject.with_license_by_name('mIt')).to match_array([mit_policy])
+    end
+
+    it 'finds multiple licenses' do
+      expect(subject.with_license_by_name([mit.name, apache.name])).to match_array([mit_policy, apache_policy])
+    end
+  end
+
+  describe "#name" do
+    specify { expect(subject.name).to eql(subject.software_license.name) }
+  end
 end

ee/spec/services/approval_rules/create_service_spec.rb

@@ -83,19 +83,14 @@ describe ApprovalRules::CreateService do
 
     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')
+    ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME.keys.each do |rule_name|
+      context "when the rule name is `#{rule_name}`" do
+        subject { described_class.new(target, user, { name: rule_name, approvals_required: 1 }) }
+        let(:result) { subject.execute }
+
+        specify { expect(result[:status]).to eq(:success) }
+        specify { expect(result[:rule].approvals_required).to eq(1) }
+        specify { expect(result[:rule].rule_type).to eq('report_approver') }
       end
     end
   end

ee/spec/services/merge_requests/sync_report_approver_approval_rules_spec.rb

@@ -3,42 +3,117 @@
 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) }
+  let(:merge_request) { create(: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_licensed_features(report_approver_rules: true)
+    end
+
+    context "when a project has a single `#{ApprovalProjectRule::DEFAULT_NAME_FOR_SECURITY_REPORT}` approval rule" do
+      let!(:security_approval_project_rule) { create(:approval_project_rule, :security_report, project: merge_request.target_project, approvals_required: 2) }
+
+      context 'when report_approver_rules are enabled' do
+        let!(:regular_approval_project_rule) { create(:approval_project_rule, project: merge_request.target_project) }
+
+        it 'creates rule for report approvers' do
+          expect { service.execute }
+            .to change { merge_request.approval_rules.security_report.count }.from(0).to(1)
 
-      before do
-        stub_licensed_features(report_approver_rules: true)
+          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
+    end
 
-      it 'creates rule for report approvers' do
-        expect { service.execute }
-          .to change { merge_request.approval_rules.security_report.count }.from(0).to(1)
+    context "when a project has a single `#{ApprovalProjectRule::DEFAULT_NAME_FOR_LICENSE_REPORT}` approval rule" do
+      let!(:project_rule) { create(:approval_project_rule, :license_management, project: merge_request.target_project) }
 
-        rule = merge_request.approval_rules.security_report.first
+      context "when the rule has not been synchronized to the merge request yet" do
+        let(:result) { merge_request.reload.approval_rules.last }
 
-        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)
+        before do
+          service.execute
+        end
+
+        specify { expect(merge_request.reload.approval_rules.count).to be(1) }
+        specify { expect(result).to be_report_approver }
+        specify { expect(result.report_type).to eq('license_management') }
+        specify { expect(result.name).to eq(project_rule.name) }
+        specify { expect(result.approval_project_rule).to eq(project_rule) }
+        specify { expect(result.approvals_required).to eql(project_rule.approvals_required) }
       end
 
-      it 'updates previous rules if defined' do
-        mr_rule = create(:report_approver_rule, merge_request: merge_request, approvals_required: 0)
+      context "when the rule had previously been synchronized" do
+        let!(:previous_rule) { create(:report_approver_rule, :license_management, merge_request: merge_request) }
 
-        expect { service.execute }
-          .not_to change { merge_request.approval_rules.security_report.count }
+        before do
+          service.execute
+        end
+
+        specify { expect(merge_request.reload.approval_rules.count).to be(1) }
+        specify { expect(merge_request.reload.approval_rules[0]).to eql(previous_rule) }
+      end
+    end
+
+    context "when a project has multiple report approval rules" do
+      let!(:vulnerability_project_rule) { create(:approval_project_rule, :security_report, project: merge_request.target_project) }
+      let!(:license_compliance_project_rule) { create(:approval_project_rule, :license_management, project: merge_request.target_project) }
+
+      context "when none of the rules have been synchronized to the merge request yet" do
+        let(:vulnerability_check_rule) { merge_request.reload.approval_rules.security.last }
+        let(:license_check_rule) { merge_request.reload.approval_rules.find_by(name: ApprovalProjectRule::DEFAULT_NAME_FOR_LICENSE_REPORT) }
+
+        before do
+          vulnerability_project_rule.users << create(:user)
+          vulnerability_project_rule.groups << create(:group)
+          license_compliance_project_rule.users << create(:user)
+          license_compliance_project_rule.groups << create(:group)
+
+          service.execute
+        end
+
+        specify { expect(merge_request.reload.approval_rules.count).to be(2) }
+        specify { expect(vulnerability_check_rule).to be_report_approver }
+        specify { expect(vulnerability_check_rule.approvals_required).to eql(vulnerability_project_rule.approvals_required) }
+        specify { expect(vulnerability_check_rule).to be_security }
+        specify { expect(vulnerability_check_rule.name).to eq(vulnerability_project_rule.name) }
+        specify { expect(vulnerability_check_rule.approval_project_rule).to eq(vulnerability_project_rule) }
+        specify { expect(license_check_rule).to be_report_approver }
+        specify { expect(license_check_rule.approvals_required).to eql(license_compliance_project_rule.approvals_required) }
+        specify { expect(license_check_rule).to be_license_management }
+        specify { expect(license_check_rule.name).to eq(license_compliance_project_rule.name) }
+        specify { expect(license_check_rule.approval_project_rule).to eq(license_compliance_project_rule) }
+      end
+
+      context "when some of the rules have been synchronized to the merge request" do
+        let!(:previous_rule) { create(:report_approver_rule, :license_management, merge_request: merge_request) }
+
+        before do
+          service.execute
+        end
 
-        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)
+        specify { expect(merge_request.reload.approval_rules.count).to be(2) }
+        specify { expect(merge_request.reload.approval_rules.security_report.count).to be(1) }
+        specify { expect(merge_request.reload.approval_rules.where(report_type: :license_management)).to match_array([previous_rule]) }
       end
     end
 

ee/spec/services/security/sync_reports_to_approval_rules_service_spec.rb

@@ -13,7 +13,7 @@ describe Security::SyncReportsToApprovalRulesService, '#execute' do
   before do
     allow(Ci::Pipeline).to receive(:find).with(pipeline.id) { pipeline }
 
-    stub_licensed_features(dependency_scanning: true, dast: true)
+    stub_licensed_features(dependency_scanning: true, dast: true, license_management: true)
   end
 
   context 'when there are reports' do
@@ -64,6 +64,37 @@ describe Security::SyncReportsToApprovalRulesService, '#execute' do
             .not_to change { report_approver_rule.reload.approvals_required }
         end
       end
+
+      context "license compliance policy" do
+        let!(:software_license_policy) { create(:software_license_policy, :blacklist, project: project, software_license: blacklisted_license) }
+        let!(:license_compliance_rule) { create(:report_approver_rule, :license_management, merge_request: merge_request, approvals_required: 1) }
+        let!(:blacklisted_license) { create(:software_license) }
+
+        context "when a license violates the license compliance policy" do
+          let!(:blacklisted_license) { create(:software_license, name: license_name) }
+          let!(:ci_build) { create(:ee_ci_build, :success, :license_management, pipeline: pipeline, project: project) }
+          let!(:license_name) { ci_build.pipeline.license_management_report.license_names[0] }
+
+          specify { expect { subject }.not_to change { license_compliance_rule.reload.approvals_required } }
+          specify { expect(subject[:status]).to be(:success) }
+        end
+
+        context "when no licenses violate the license compliance policy" do
+          let!(:ci_build) { create(:ee_ci_build, :success, :license_management, pipeline: pipeline, project: project) }
+
+          specify { expect { subject }.to change { license_compliance_rule.reload.approvals_required }.from(1).to(0) }
+          specify { expect(subject[:status]).to be(:success) }
+        end
+
+        context "when an unexpected error occurs" do
+          before do
+            allow_any_instance_of(Gitlab::Ci::Reports::LicenseManagement::Report).to receive(:violates?).and_raise('heck')
+          end
+
+          specify { expect(subject[:status]).to be(:error) }
+          specify { expect(subject[:message]).to eql("Failed to update approval rules") }
+        end
+      end
     end
 
     context 'when pipeline fails' do

ee/spec/support/shared_examples/project_approval_rules_api_shared_examples.rb

@@ -45,12 +45,16 @@ shared_examples_for 'an API endpoint for creating project approval rule' do
       expect(json_response.symbolize_keys).to include(params)
     end
 
-    it 'sets rule_type as report_approver if name matches default name for security reports' do
-      expect do
-        post api(url, current_user), params: params.merge(name: ApprovalProjectRule::DEFAULT_NAME_FOR_SECURITY_REPORT)
-      end.to change { ApprovalProjectRule.report_approver.count }.from(0).to(1)
+    ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME.keys.each do |rule_name|
+      context "when creating a '#{rule_name}' approval rule" do
+        it 'specifies a `rule_type` of `report_approver`' do
+          expect do
+            post api(url, current_user), params: params.merge(name: rule_name)
+          end.to change { ApprovalProjectRule.report_approver.count }.from(0).to(1)
 
-      expect(response).to have_gitlab_http_status(201)
+          expect(response).to have_gitlab_http_status(201)
+        end
+      end
     end
   end
 end