Merge branch '14708-refresh-license-approval-new-blacklist-item' into 'master'

Refresh license approval checks when a license is blacklisted

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

config/sidekiq_queues.yml

@@ -116,3 +116,4 @@
   - [incident_management, 2]
   - [jira_connect, 1]
   - [update_external_pull_requests, 3]
+  - [refresh_license_compliance_checks, 2]

ee/app/models/approval_merge_request_rule.rb

@@ -55,6 +55,10 @@ class ApprovalMergeRequestRule < ApplicationRecord
   # 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) }
+  scope :license_compliance, -> { report_approver.where(report_type: :license_management) }
+  scope :with_head_pipeline, -> { includes(merge_request: [:head_pipeline]) }
+  scope :open_merge_requests, -> { merge(MergeRequest.opened) }
+  scope :for_checks_that_can_be_refreshed, -> { license_compliance.open_merge_requests.with_head_pipeline }
 
   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|
@@ -114,6 +118,12 @@ class ApprovalMergeRequestRule < ApplicationRecord
     self.approved_approver_ids = merge_request.approvals.map(&:user_id) & approvers.map(&:id)
   end
 
+  def refresh_required_approvals!(project_approval_rule)
+    return unless report_approver?
+
+    refresh_license_management_approvals(project_approval_rule) if license_management?
+  end
+
   private
 
   def validate_approval_project_rule
@@ -122,4 +132,15 @@ class ApprovalMergeRequestRule < ApplicationRecord
 
     errors.add(:approval_project_rule, 'must be for the same project')
   end
+
+  def refresh_license_management_approvals(project_approval_rule)
+    license_report = merge_request.head_pipeline&.license_management_report
+    return if license_report.blank?
+
+    if license_report.violates?(project.software_license_policies)
+      update!(approvals_required: project_approval_rule.approvals_required)
+    else
+      update!(approvals_required: 0)
+    end
+  end
 end

ee/app/models/ee/project.rb

@@ -53,6 +53,7 @@ module EE
       has_many :approver_users, through: :approvers, source: :user
       has_many :approver_groups, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
       has_many :approval_rules, class_name: 'ApprovalProjectRule'
+      has_many :approval_merge_request_rules, through: :merge_requests, source: :approval_rules
       has_many :audit_events, as: :entity
       has_many :designs, inverse_of: :project, class_name: 'DesignManagement::Design'
       has_many :path_locks

ee/app/models/software_license.rb

@@ -8,4 +8,11 @@ class SoftwareLicense < ApplicationRecord
   validates :name, presence: true
 
   scope :ordered, -> { order(:name) }
+
+  def self.create_policy_for!(project:, name:, approval_status:)
+    project.software_license_policies.create!(
+      approval_status: approval_status,
+      software_license: safe_find_or_create_by!(name: name)
+    )
+  end
 end

ee/app/services/software_license_policies/create_service.rb

@@ -7,37 +7,27 @@ module SoftwareLicensePolicies
       super(project, user, params.with_indifferent_access)
     end
 
-    # Returns the created managed license.
-    # rubocop: disable CodeReuse/ActiveRecord
     def execute
       return error("", 403) unless can?(@current_user, :admin_software_license_policy, @project)
 
-      # Load or create the software license
-      name = params.delete(:name)
-
-      software_license = SoftwareLicense.transaction do
-        SoftwareLicense.transaction(requires_new: true) do
-          SoftwareLicense.find_or_create_by(name: name)
-        end
-      rescue ActiveRecord::RecordNotUnique
-        retry
-      end
-
-      # Add the software license to params
-      params[:software_license] = software_license
-
-      begin
-        software_license_policy = @project.software_license_policies.create(params)
-      rescue ArgumentError => ex
-        return error(ex.message, 400)
-      end
+      success(software_license_policy: create_software_license_policy)
+    rescue ActiveRecord::RecordInvalid => exception
+      error(exception.record.errors.full_messages, 400)
+    rescue ArgumentError => exception
+      log_error(exception.message)
+      error(exception.message, 400)
+    end
 
-      if software_license_policy.errors.any?
-        return error(software_license_policy.errors.full_messages.join("\n"), 400)
-      end
+    private
 
-      success(software_license_policy: software_license_policy)
+    def create_software_license_policy
+      policy = SoftwareLicense.create_policy_for!(
+        project: project,
+        name: params[:name],
+        approval_status: params[:approval_status]
+      )
+      RefreshLicenseComplianceChecksWorker.perform_async(project.id)
+      policy
     end
-    # rubocop: enable CodeReuse/ActiveRecord
   end
 end

ee/app/services/software_license_policies/update_service.rb

@@ -15,6 +15,7 @@ module SoftwareLicensePolicies
 
       begin
         software_license_policy.update(params)
+        RefreshLicenseComplianceChecksWorker.perform_async(project.id)
       rescue ArgumentError => ex
         return error(ex.message, 400)
       end

ee/app/workers/all_queues.yml

@@ -67,5 +67,6 @@
 - project_import_schedule
 - project_update_repository_storage
 - rebase
+- refresh_license_compliance_checks
 - repository_update_mirror
 - repository_push_audit_event

ee/app/workers/refresh_license_compliance_checks_worker.rb

+# frozen_string_literal: true
+
+class RefreshLicenseComplianceChecksWorker
+  include ApplicationWorker
+
+  def perform(project_id)
+    project = Project.find(project_id)
+    project_approval_rule = project
+      .approval_rules
+      .report_approver
+      .find_by_name!(ApprovalRuleLike::DEFAULT_NAME_FOR_LICENSE_REPORT)
+
+    approval_rules = project.approval_merge_request_rules.for_checks_that_can_be_refreshed
+    approval_rules.find_each do |approval_rule|
+      approval_rule.refresh_required_approvals!(project_approval_rule)
+    end
+    # If the project or project approval rule is deleted
+    # before this job runs, then it is possible that
+    # the project and project approval rule record
+    # will not be found.
+  rescue ActiveRecord::RecordNotFound => error
+    logger.error(error.message)
+  end
+end

ee/changelogs/unreleased/14708-refresh-license-approval-new-blacklist-item.yml

+---
+title: Refresh license approval check when a license is blacklisted
+merge_request: 16070
+author:
+type: added

ee/spec/factories/approval_rules.rb

@@ -19,6 +19,10 @@ FactoryBot.define do
     report_type :security
     sequence(:name) { |n| "*-#{n}.js" }
 
+    trait :requires_approval do
+      approvals_required { rand(1..ApprovalProjectRule::APPROVALS_REQUIRED_MAX) }
+    end
+
     trait :license_management do
       name ApprovalRuleLike::DEFAULT_NAME_FOR_LICENSE_REPORT
       report_type :license_management

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -280,4 +280,36 @@ describe ApprovalMergeRequestRule do
       end
     end
   end
+
+  describe "#refresh_required_approvals!" do
+    before do
+      stub_licensed_features(license_management: true)
+    end
+
+    context "when the rule is a `#{ApprovalRuleLike::DEFAULT_NAME_FOR_LICENSE_REPORT}` rule" do
+      subject { create(:report_approver_rule, :requires_approval, :license_management, merge_request: open_merge_request) }
+      let(:open_merge_request) { create(:merge_request, :opened, target_project: project, source_project: project) }
+      let!(:project_approval_rule) { create(:approval_project_rule, :requires_approval, :license_management, project: project) }
+      let(:project) { create(:project) }
+      let!(:open_pipeline) { create(:ee_ci_pipeline, :success, :with_license_management_report, project: project, merge_requests_as_head_pipeline: [open_merge_request]) }
+      let!(:blacklist_policy) { create(:software_license_policy, project: project, software_license: license, approval_status: :blacklisted) }
+
+      before do
+        subject.refresh_required_approvals!(project_approval_rule)
+      end
+
+      context "when the latest license report violates the compliance policy" do
+        let(:license) { create(:software_license, name: license_report.license_names[0]) }
+        let(:license_report) { open_pipeline.license_management_report }
+
+        specify { expect(subject.approvals_required).to be(project_approval_rule.approvals_required) }
+      end
+
+      context "when the latest license report adheres to the compliance policy" do
+        let(:license) { create(:software_license, name: SecureRandom.uuid) }
+
+        specify { expect(subject.approvals_required).to be_zero }
+      end
+    end
+  end
 end

ee/spec/models/software_license_spec.rb

@@ -9,4 +9,28 @@ describe SoftwareLicense do
     it { is_expected.to include_module(Presentable) }
     it { is_expected.to validate_presence_of(:name) }
   end
+
+  describe ".create_policy_for!" do
+    subject { described_class }
+    let(:project) { create(:project) }
+
+    context "when a software license with a given name has already been created" do
+      let(:mit_license) { create(:software_license, :mit) }
+      let(:result) { subject.create_policy_for!(project: project, name: mit_license.name, approval_status: :approved) }
+
+      specify { expect(result).to be_persisted }
+      specify { expect(result).to be_approved }
+      specify { expect(result.software_license).to eql(mit_license) }
+    end
+
+    context "when a software license with a given name has NOT been created" do
+      let(:license_name) { SecureRandom.uuid }
+      let(:result) { subject.create_policy_for!(project: project, name: license_name, approval_status: :blacklisted) }
+
+      specify { expect(result).to be_persisted }
+      specify { expect(result).to be_blacklisted }
+      specify { expect(result.software_license).to be_persisted }
+      specify { expect(result.software_license.name).to eql(license_name) }
+    end
+  end
 end

ee/spec/services/software_license_policies/create_service_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe SoftwareLicensePolicies::CreateService do
-  let(:project) { create(:project)}
+  let(:project) { create(:project) }
   let(:params) { { name: 'ExamplePL/2.1', approval_status: 'blacklisted' } }
 
   let(:user) do
@@ -16,7 +16,7 @@ describe SoftwareLicensePolicies::CreateService do
     stub_licensed_features(license_management: true)
   end
 
-  subject { described_class.new(project, user, params).execute }
+  subject { described_class.new(project, user, params) }
 
   describe '#execute' do
     context 'with license management unavailable' do
@@ -25,26 +25,62 @@ describe SoftwareLicensePolicies::CreateService do
       end
 
       it 'does not creates a software license policy' do
-        expect { subject }.to change { project.software_license_policies.count }.by(0)
+        expect { subject.execute }.to change { project.software_license_policies.count }.by(0)
       end
     end
 
     context 'with a user who is allowed to admin' do
       it 'creates one software license policy correctly' do
-        expect { subject }.to change { project.software_license_policies.count }.from(0).to(1)
+        expect { subject.execute }.to change { project.software_license_policies.count }.from(0).to(1)
 
         software_license_policy = project.software_license_policies.last
         expect(software_license_policy).to be_persisted
-        expect(software_license_policy.name).to eq('ExamplePL/2.1')
+        expect(software_license_policy.name).to eq(params[:name])
         expect(software_license_policy.approval_status).to eq('blacklisted')
       end
+
+      context "when valid parameters are specified" do
+        let(:result) { subject.execute }
+
+        before do
+          allow(RefreshLicenseComplianceChecksWorker).to receive(:perform_async)
+          result
+        end
+
+        specify { expect(result[:status]).to be(:success) }
+        specify { expect(result[:software_license_policy]).to be_present }
+        specify { expect(result[:software_license_policy]).to be_persisted }
+        specify { expect(result[:software_license_policy].name).to eql(params[:name]) }
+        specify { expect(result[:software_license_policy].approval_status).to eql('blacklisted') }
+        specify { expect(RefreshLicenseComplianceChecksWorker).to have_received(:perform_async).with(project.id) }
+      end
+
+      context "when an argument error is raised" do
+        before do
+          allow_any_instance_of(Project).to receive(:software_license_policies).and_raise(ArgumentError)
+        end
+
+        specify { expect(subject.execute[:status]).to be(:error) }
+        specify { expect(subject.execute[:message]).to be_present }
+        specify { expect(subject.execute[:http_status]).to be(400) }
+      end
+
+      context "when invalid input is provided" do
+        before do
+          params[:approval_status] = nil
+        end
+
+        specify { expect(subject.execute[:status]).to be(:error) }
+        specify { expect(subject.execute[:message]).to be_present }
+        specify { expect(subject.execute[:http_status]).to be(400) }
+      end
     end
 
     context 'with a user not allowed to admin' do
       let(:user) { create(:user) }
 
       it 'does not create a software license policy' do
-        expect { subject }.to change { project.software_license_policies.count }.by(0)
+        expect { subject.execute }.to change { project.software_license_policies.count }.by(0)
       end
     end
   end

ee/spec/services/software_license_policies/update_service_spec.rb

@@ -48,10 +48,12 @@ describe SoftwareLicensePolicies::UpdateService do
 
       context 'with a user allowed to admin' do
         it 'updates the software license policy correctly' do
+          allow(RefreshLicenseComplianceChecksWorker).to receive(:perform_async)
           update_software_license_policy(opts)
 
           expect(software_license_policy).to be_valid
           expect(software_license_policy.approval_status).to eq(opts[:approval_status])
+          expect(RefreshLicenseComplianceChecksWorker).to have_received(:perform_async).with(project.id)
         end
       end
 

ee/spec/workers/refresh_license_compliance_checks_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe RefreshLicenseComplianceChecksWorker do
+  subject { described_class.new }
+
+  describe '#perform' do
+    let(:project) { create(:project) }
+
+    before do
+      stub_licensed_features(license_management: true)
+    end
+
+    context "when there are merge requests associated with the project" do
+      let!(:open_merge_request) { create(:merge_request, :opened, target_project: project, source_project: project) }
+      let!(:closed_merge_request) { create(:merge_request, :closed, target_project: project, source_project: project) }
+
+      context "when the `#{ApprovalRuleLike::DEFAULT_NAME_FOR_LICENSE_REPORT}` approval rule is enabled" do
+        let!(:open_merge_request_approval_rule) { create(:report_approver_rule, :requires_approval, :license_management, merge_request: open_merge_request) }
+        let!(:closed_merge_request_approval_rule) { create(:report_approver_rule, :license_management, merge_request: closed_merge_request, approvals_required: 0) }
+        let!(:project_approval_rule) { create(:approval_project_rule, :requires_approval, :license_management, project: project) }
+
+        context "when a license is blacklisted, that appears in some of the license management reports" do
+          let!(:open_pipeline) { create(:ee_ci_pipeline, :success, :with_license_management_report, project: project, merge_requests_as_head_pipeline: [open_merge_request]) }
+          let!(:closed_pipeline) { create(:ee_ci_pipeline, :success, :with_license_management_report, project: project, merge_requests_as_head_pipeline: [closed_merge_request]) }
+          let!(:blacklist_policy) { create(:software_license_policy, project: project, software_license: license, approval_status: :blacklisted) }
+          let(:license) { create(:software_license, name: license_report.license_names[0]) }
+          let(:license_report) { open_pipeline.license_management_report }
+
+          before do
+            subject.perform(project.id)
+          end
+
+          specify { expect(open_merge_request_approval_rule.reload.approvals_required).to eql(project_approval_rule.approvals_required) }
+          specify { expect(closed_merge_request_approval_rule.reload.approvals_required).to be_zero }
+        end
+
+        context "when none of the blacklisted licenses appear in the most recent license management reports" do
+          let!(:open_pipeline) { create(:ee_ci_pipeline, :success, :with_license_management_report, project: project, merge_requests_as_head_pipeline: [open_merge_request]) }
+          let!(:closed_pipeline) { create(:ee_ci_pipeline, :success, :with_license_management_report, project: project, merge_requests_as_head_pipeline: [closed_merge_request]) }
+          let!(:blacklist_policy) { create(:software_license_policy, project: project, software_license: license, approval_status: :blacklisted) }
+          let(:license) { create(:software_license, name: SecureRandom.uuid) }
+
+          before do
+            subject.perform(project.id)
+          end
+
+          specify { expect(open_merge_request_approval_rule.reload.approvals_required).to be_zero }
+          specify { expect(closed_merge_request_approval_rule.reload.approvals_required).to be_zero }
+        end
+      end
+    end
+
+    context "when the project does not exist" do
+      specify do
+        expect { subject.perform(SecureRandom.uuid) }.not_to raise_error
+      end
+    end
+
+    context "when the project does not have a license check rule" do
+      specify do
+        expect { subject.perform(project.id) }.not_to raise_error
+      end
+    end
+  end
+end

spec/lib/gitlab/import_export/all_models.yml

@@ -376,6 +376,7 @@ project:
 - index_status
 - feature_usage
 - approval_rules
+- approval_merge_request_rules
 - approvers
 - approver_users
 - pages_domains