Merge branch '34826-update-software-license-policy' into 'master'

Provide API to update a software license policy

See merge request gitlab-org/gitlab!19790

ee/app/controllers/projects/security/licenses_controller.rb

@@ -4,7 +4,7 @@ module Projects
   module Security
     class LicensesController < Projects::ApplicationController
       before_action :authorize_read_licenses_list!
-      before_action :authorize_admin_software_license_policy!, only: [:create]
+      before_action :authorize_admin_software_license_policy!, only: [:create, :update]
 
       before_action do
         push_frontend_feature_flag(:licenses_list)
@@ -15,7 +15,7 @@ module Projects
           format.json do
             ::Gitlab::UsageDataCounters::LicensesList.count(:views)
 
-            license_compliance = ::SCA::LicenseCompliance.new(project)
+            license_compliance = project.license_compliance
             render json: serializer.represent(
               pageable(license_compliance.policies),
               build: license_compliance.latest_build_for_default_branch
@@ -36,6 +36,18 @@ module Projects
         end
       end
 
+      def update
+        result = ::Projects::Licenses::UpdatePolicyService
+          .new(project, current_user, software_license_policy_params)
+          .execute(params[:id])
+
+        if result[:status] == :success
+          render json: LicenseEntity.represent(result[:software_license_policy]), status: :ok
+        else
+          render_error_for(result)
+        end
+      end
+
       private
 
       def serializer

ee/app/models/ee/project.rb

@@ -695,6 +695,10 @@ module EE
       packages.where(package_type: package_type).exists?
     end
 
+    def license_compliance
+      strong_memoize(:license_compliance) { SCA::LicenseCompliance.new(self) }
+    end
+
     private
 
     def set_override_pull_mirror_available

ee/app/models/sca/license_policy.rb

@@ -6,10 +6,10 @@ module SCA
 
     def initialize(reported_license, software_policy)
       @id = software_policy&.id
-      @name = software_policy&.name || reported_license.name
+      @name = software_policy&.name || reported_license&.name
       @url = reported_license&.url
       @dependencies = reported_license&.dependencies || []
-      @spdx_identifier = software_policy&.spdx_identifier || reported_license.id
+      @spdx_identifier = software_policy&.spdx_identifier || reported_license&.id
       @classification = software_policy&.approval_status || 'unclassified'
     end
   end

ee/app/services/projects/licenses/update_policy_service.rb

+# frozen_string_literal: true
+
+module Projects
+  module Licenses
+    class UpdatePolicyService < ::BaseService
+      def execute(policy_id)
+        return error({}, :forbidden) unless can?(current_user, :admin_software_license_policy, project)
+        return classification_error unless valid_classification?
+
+        policy = project.software_license_policies.find(policy_id)
+        change_classification_of(policy)
+        success(software_license_policy: compliance_report_for(policy))
+      rescue ActiveRecord::RecordInvalid => exception
+        error(exception.record.errors, :unprocessable_entity)
+      end
+
+      private
+
+      def change_classification_of(policy)
+        if blacklisted_classification?
+          policy.blacklisted!
+        else
+          policy.approved!
+        end
+
+        RefreshLicenseComplianceChecksWorker.perform_async(project.id)
+      end
+
+      def compliance_report_for(policy)
+        project.license_compliance.report_for(policy)
+      end
+
+      def classification_error
+        errors = ActiveModel::Errors.new(SoftwareLicensePolicy.new)
+        errors.add(:approval_status, :invalid)
+        error(errors, :unprocessable_entity)
+      end
+
+      def valid_classification?
+        SoftwareLicensePolicy.approval_statuses.key?(params[:classification])
+      end
+
+      def blacklisted_classification?
+        params[:classification] == 'blacklisted'
+      end
+    end
+  end
+end

ee/config/routes/project.rb

@@ -60,7 +60,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
         end
 
         resources :subscriptions, only: [:create, :destroy]
-        resources :licenses, only: [:index, :create], controller: 'security/licenses'
+        resources :licenses, only: [:index, :create, :update], controller: 'security/licenses'
       end
       # End of the /-/ scope.
 
@@ -169,7 +169,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
         resource :configuration, only: [:show], controller: :configuration
 
         resources :dependencies, only: [:index]
-        resources :licenses, only: [:index]
+        resources :licenses, only: [:index, :update]
         # We have to define both legacy and new routes for Vulnerability Findings
         # because they are loaded upon application initialization and preloaded by
         # web server.

ee/spec/controllers/projects/security/licenses_controller_spec.rb

@@ -267,4 +267,95 @@ describe Projects::Security::LicensesController do
       end
     end
   end
+
+  describe "PATCH #update" do
+    let(:project) { create(:project, :repository, :private) }
+    let(:software_license_policy) { create(:software_license_policy, project: project, software_license: mit_license) }
+    let(:mit_license) { create(:software_license, :mit) }
+
+    let(:default_params) do
+      {
+        namespace_id: project.namespace,
+        project_id: project,
+        id: software_license_policy.id,
+        software_license_policy: { classification: "approved" }
+      }
+    end
+
+    context "when authenticated" do
+      let(:current_user) { create(:user) }
+
+      before do
+        stub_licensed_features(licenses_list: true, license_management: true)
+        sign_in(current_user)
+      end
+
+      context "when the current user is not a member of the project" do
+        before do
+          patch :update, xhr: true, params: default_params
+        end
+
+        it { expect(response).to have_http_status(:not_found) }
+      end
+
+      context "when the current user is a member of the project but not authorized to update policies" do
+        before do
+          project.add_guest(current_user)
+
+          patch :update, xhr: true, params: default_params
+        end
+
+        it { expect(response).to have_http_status(:not_found) }
+      end
+
+      context "when authorized as a maintainer" do
+        let(:json) { json_response.with_indifferent_access }
+
+        before do
+          project.add_maintainer(current_user)
+        end
+
+        context "when updating a software license policy" do
+          before do
+            patch :update, xhr: true, params: default_params.merge({
+              software_license_policy: {
+                classification: "blacklisted"
+              }
+            })
+          end
+
+          it { expect(response).to have_http_status(:ok) }
+          it { expect(software_license_policy.reload).to be_blacklisted }
+
+          it "generates the proper JSON response" do
+            expect(json[:id]).to eql(software_license_policy.id)
+            expect(json[:spdx_identifier]).to eq(mit_license.spdx_identifier)
+            expect(json[:classification]).to eq("blacklisted")
+            expect(json[:name]).to eq(mit_license.name)
+          end
+        end
+
+        context "when the parameters are invalid" do
+          before do
+            patch :update, xhr: true, params: default_params.merge({
+              software_license_policy: {
+                classification: "invalid"
+              }
+            })
+          end
+
+          it { expect(response).to have_http_status(:unprocessable_entity) }
+          it { expect(json).to eq({ "errors" => { "approval_status" => ["is invalid"] } }) }
+        end
+      end
+    end
+
+    context "when unauthenticated" do
+      before do
+        patch :update, xhr: true, params: default_params
+      end
+
+      it { expect(response).to redirect_to(new_user_session_path) }
+    end
+  end
 end

ee/spec/models/project_spec.rb

@@ -2383,4 +2383,8 @@ describe Project do
       expect(described_class.with_groups_level_repos_templates.count).to eq(4)
     end
   end
+
+  describe '#license_compliance' do
+    it { expect(subject.license_compliance).to be_instance_of(::SCA::LicenseCompliance) }
+  end
 end

ee/spec/models/sca/license_compliance_spec.rb

@@ -16,16 +16,16 @@ RSpec.describe SCA::LicenseCompliance do
       it { expect(subject.policies.count).to be_zero }
 
       context "when the project has policies configured" do
-        it 'includes an entry for each policy that was not detected in the latest report' do
+        it "includes an entry for each policy that was not detected in the latest report" do
           mit = create(:software_license, :mit)
           mit_policy = create(:software_license_policy, :denied, software_license: mit, project: project)
 
           expect(subject.policies.count).to be(1)
-          expect(subject.policies[0]&.id).to eq(mit_policy.id)
-          expect(subject.policies[0]&.name).to eq(mit.name)
-          expect(subject.policies[0]&.url).to be_nil
-          expect(subject.policies[0]&.classification).to eq("blacklisted")
-          expect(subject.policies[0]&.spdx_identifier).to eq("MIT")
+          expect(subject.policies[0].id).to eq(mit_policy.id)
+          expect(subject.policies[0].name).to eq(mit.name)
+          expect(subject.policies[0].url).to be_nil
+          expect(subject.policies[0].classification).to eq("blacklisted")
+          expect(subject.policies[0].spdx_identifier).to eq("MIT")
         end
       end
     end
@@ -74,7 +74,7 @@ RSpec.describe SCA::LicenseCompliance do
           dependency_scan_artifact.update!(file: invalid_file)
         end
 
-        it { expect(subject.policies.map(&:spdx_identifier)).to contain_exactly('BSD-3-Clause', 'MIT', nil) }
+        it { expect(subject.policies.map(&:spdx_identifier)).to contain_exactly("BSD-3-Clause", "MIT", nil) }
       end
 
       context "when a pipeline has successfully produced a v2.0 license scan report" do
@@ -84,7 +84,7 @@ RSpec.describe SCA::LicenseCompliance do
         let(:license_scan_artifact) { create(:ci_job_artifact, file_type: :license_management, file_format: :raw) }
         let(:license_scan_file) { fixture_file_upload(Rails.root.join("ee/spec/fixtures/security_reports/gl-license-management-report-v2.json"), "application/json") }
 
-        it 'adds an entry for each detected license and each policy' do
+        it "adds an entry for each detected license and each policy" do
           mit = create(:software_license, :mit)
           mit_policy = create(:software_license_policy, :denied, software_license: mit, project: project)
           other_license = create(:software_license, spdx_identifier: "Other-Id")
@@ -93,26 +93,29 @@ RSpec.describe SCA::LicenseCompliance do
           license_scan_artifact.update!(file: license_scan_file)
 
           expect(subject.policies.count).to eq(4)
-          expect(subject.policies[0]&.id).to be_nil
-          expect(subject.policies[0]&.name).to eq("BSD 3-Clause \"New\" or \"Revised\" License")
-          expect(subject.policies[0]&.url).to eq("http://spdx.org/licenses/BSD-3-Clause.json")
-          expect(subject.policies[0]&.classification).to eq("unclassified")
-          expect(subject.policies[0]&.spdx_identifier).to eq("BSD-3-Clause")
-          expect(subject.policies[1]&.id).to eq(mit_policy.id)
-          expect(subject.policies[1]&.name).to eq(mit.name)
-          expect(subject.policies[1]&.url).to eq("http://spdx.org/licenses/MIT.json")
-          expect(subject.policies[1]&.classification).to eq("blacklisted")
-          expect(subject.policies[1]&.spdx_identifier).to eq("MIT")
-          expect(subject.policies[2]&.id).to eq(other_license_policy.id)
-          expect(subject.policies[2]&.name).to eq(other_license.name)
-          expect(subject.policies[2]&.url).to be_blank
-          expect(subject.policies[2]&.classification).to eq("approved")
-          expect(subject.policies[2]&.spdx_identifier).to eq(other_license.spdx_identifier)
-          expect(subject.policies[3]&.id).to be_nil
-          expect(subject.policies[3]&.name).to eq("unknown")
-          expect(subject.policies[3]&.url).to be_blank
-          expect(subject.policies[3]&.classification).to eq("unclassified")
-          expect(subject.policies[3]&.spdx_identifier).to be_nil
+          expect(subject.policies[0].id).to be_nil
+          expect(subject.policies[0].name).to eq("BSD 3-Clause \"New\" or \"Revised\" License")
+          expect(subject.policies[0].url).to eq("http://spdx.org/licenses/BSD-3-Clause.json")
+          expect(subject.policies[0].classification).to eq("unclassified")
+          expect(subject.policies[0].spdx_identifier).to eq("BSD-3-Clause")
+
+          expect(subject.policies[1].id).to eq(mit_policy.id)
+          expect(subject.policies[1].name).to eq(mit.name)
+          expect(subject.policies[1].url).to eq("http://spdx.org/licenses/MIT.json")
+          expect(subject.policies[1].classification).to eq("blacklisted")
+          expect(subject.policies[1].spdx_identifier).to eq("MIT")
+
+          expect(subject.policies[2].id).to eq(other_license_policy.id)
+          expect(subject.policies[2].name).to eq(other_license.name)
+          expect(subject.policies[2].url).to be_blank
+          expect(subject.policies[2].classification).to eq("approved")
+          expect(subject.policies[2].spdx_identifier).to eq(other_license.spdx_identifier)
+
+          expect(subject.policies[3].id).to be_nil
+          expect(subject.policies[3].name).to eq("unknown")
+          expect(subject.policies[3].url).to be_blank
+          expect(subject.policies[3].classification).to eq("unclassified")
+          expect(subject.policies[3].spdx_identifier).to be_nil
         end
       end
 
@@ -123,7 +126,7 @@ RSpec.describe SCA::LicenseCompliance do
         let(:license_scan_artifact) { create(:ci_job_artifact, file_type: :license_management, file_format: :raw) }
         let(:license_scan_file) { fixture_file_upload(Rails.root.join("ee/spec/fixtures/security_reports/gl-license-management-report-v1.1.json"), "application/json") }
 
-        it 'adds an entry for each detected license and each policy' do
+        it "adds an entry for each detected license and each policy" do
           mit = create(:software_license, :mit)
           mit_policy = create(:software_license_policy, :denied, software_license: mit, project: project)
           other_license = create(:software_license, spdx_identifier: "Other-Id")
@@ -132,26 +135,30 @@ RSpec.describe SCA::LicenseCompliance do
           license_scan_artifact.update!(file: license_scan_file)
 
           expect(subject.policies.count).to eq(4)
-          expect(subject.policies[0]&.id).to be_nil
-          expect(subject.policies[0]&.name).to eq("BSD")
-          expect(subject.policies[0]&.url).to eq("http://spdx.org/licenses/BSD-4-Clause.json")
-          expect(subject.policies[0]&.classification).to eq("unclassified")
-          expect(subject.policies[0]&.spdx_identifier).to eq("BSD-4-Clause")
-          expect(subject.policies[1]&.id).to eq(mit_policy.id)
-          expect(subject.policies[1]&.name).to eq(mit.name)
-          expect(subject.policies[1]&.url).to eq("http://opensource.org/licenses/mit-license")
-          expect(subject.policies[1]&.classification).to eq("blacklisted")
-          expect(subject.policies[1]&.spdx_identifier).to eq("MIT")
-          expect(subject.policies[2]&.id).to eq(other_license_policy.id)
-          expect(subject.policies[2]&.name).to eq(other_license.name)
-          expect(subject.policies[2]&.url).to be_blank
-          expect(subject.policies[2]&.classification).to eq("approved")
-          expect(subject.policies[2]&.spdx_identifier).to eq(other_license.spdx_identifier)
-          expect(subject.policies[3]&.id).to be_nil
-          expect(subject.policies[3]&.name).to eq("unknown")
-          expect(subject.policies[3]&.url).to be_blank
-          expect(subject.policies[3]&.classification).to eq("unclassified")
-          expect(subject.policies[3]&.spdx_identifier).to be_nil
+
+          expect(subject.policies[0].id).to be_nil
+          expect(subject.policies[0].name).to eq("BSD")
+          expect(subject.policies[0].url).to eq("http://spdx.org/licenses/BSD-4-Clause.json")
+          expect(subject.policies[0].classification).to eq("unclassified")
+          expect(subject.policies[0].spdx_identifier).to eq("BSD-4-Clause")
+
+          expect(subject.policies[1].id).to eq(mit_policy.id)
+          expect(subject.policies[1].name).to eq(mit.name)
+          expect(subject.policies[1].url).to eq("http://opensource.org/licenses/mit-license")
+          expect(subject.policies[1].classification).to eq("blacklisted")
+          expect(subject.policies[1].spdx_identifier).to eq("MIT")
+
+          expect(subject.policies[2].id).to eq(other_license_policy.id)
+          expect(subject.policies[2].name).to eq(other_license.name)
+          expect(subject.policies[2].url).to be_blank
+          expect(subject.policies[2].classification).to eq("approved")
+          expect(subject.policies[2].spdx_identifier).to eq(other_license.spdx_identifier)
+
+          expect(subject.policies[3].id).to be_nil
+          expect(subject.policies[3].name).to eq("unknown")
+          expect(subject.policies[3].url).to be_blank
+          expect(subject.policies[3].classification).to eq("unclassified")
+          expect(subject.policies[3].spdx_identifier).to be_nil
         end
       end
     end

ee/spec/models/sca/license_policy_spec.rb

@@ -3,80 +3,114 @@
 require "spec_helper"
 
 RSpec.describe SCA::LicensePolicy do
+  subject { described_class.new(license, policy) }
+
   let(:license) { build(:license_scanning_license, :mit) }
   let(:policy) { build(:software_license_policy, software_license: software_license) }
   let(:software_license) { build(:software_license, :mit) }
 
   describe "#id" do
     context "when a software_policy is provided" do
-      it { expect(described_class.new(license, policy).id).to eq(policy.id) }
+      it { expect(subject.id).to eq(policy.id) }
     end
 
     context "when a software_policy is NOT provided" do
-      it { expect(described_class.new(license, nil).id).to be_nil }
+      let(:policy) { nil }
+
+      it { expect(subject.id).to be_nil }
     end
   end
 
   describe "#name" do
     context "when a software_policy is provided" do
-      it { expect(described_class.new(license, policy).name).to eq(policy.software_license.name) }
+      it { expect(subject.name).to eq(policy.software_license.name) }
     end
 
     context "when a software_policy is NOT provided" do
-      it { expect(described_class.new(license, nil).name).to eq(license.name) }
+      let(:policy) { nil }
+
+      it { expect(subject.name).to eq(license.name) }
+    end
+
+    context "when a reported license is NOT provided" do
+      let(:license) { nil }
+
+      it { expect(subject.name).to eq(policy.name) }
+    end
+
+    context "when a reported license and policy NOT provided" do
+      let(:policy) { nil }
+      let(:license) { nil }
+
+      it { expect(subject.name).to be_nil }
     end
   end
 
   describe "#url" do
     context "when a license is provided" do
-      it { expect(described_class.new(license, policy).url).to eq(license.url) }
+      it { expect(subject.url).to eq(license.url) }
     end
 
     context "when a license is NOT provided" do
-      it { expect(described_class.new(nil, policy).id).to be_nil }
+      let(:license) { nil }
+
+      it { expect(subject.id).to be_nil }
     end
   end
 
   describe "#dependencies" do
     context "when a license is provided" do
-      it { expect(described_class.new(license, policy).dependencies).to eq(license.dependencies) }
+      it { expect(subject.dependencies).to eq(license.dependencies) }
     end
 
     context "when a license is NOT provided" do
-      it { expect(described_class.new(nil, policy).dependencies).to be_empty }
+      let(:license) { nil }
+
+      it { expect(subject.dependencies).to be_empty }
     end
   end
 
   describe "#classification" do
-    let(:allowed_policy) { build(:software_license_policy, :allowed, software_license: software_license) }
-    let(:denied_policy) { build(:software_license_policy, :denied, software_license: software_license) }
-
     context "when a allowed software_policy is provided" do
-      it { expect(described_class.new(license, allowed_policy).classification).to eq("approved") }
+      let(:policy) { build(:software_license_policy, :allowed, software_license: software_license) }
+
+      it { expect(subject.classification).to eq("approved") }
     end
 
     context "when a denied software_policy is provided" do
-      it { expect(described_class.new(license, denied_policy).classification).to eq("blacklisted") }
+      let(:policy) { build(:software_license_policy, :denied, software_license: software_license) }
+
+      it { expect(subject.classification).to eq("blacklisted") }
     end
 
     context "when a software_policy is NOT provided" do
-      it { expect(described_class.new(license, nil).classification).to eq("unclassified") }
+      let(:policy) { nil }
+
+      it { expect(subject.classification).to eq("unclassified") }
     end
   end
 
   describe "#spdx_identifier" do
     context "when a software_policy is provided" do
-      it { expect(described_class.new(license, policy).spdx_identifier).to eq(policy.software_license.spdx_identifier) }
+      it { expect(subject.spdx_identifier).to eq(policy.software_license.spdx_identifier) }
     end
 
     context "when a software_policy is provided but does not have a SPDX Id" do
       let(:software_license) { build(:software_license, spdx_identifier: nil) }
 
-      it { expect(described_class.new(license, policy).spdx_identifier).to eq(license.id) }
+      it { expect(subject.spdx_identifier).to eq(license.id) }
     end
 
     context "when a software_policy is NOT provided" do
-      it { expect(described_class.new(license, nil).spdx_identifier).to eq(license.id) }
+      let(:policy) { nil }
+
+      it { expect(subject.spdx_identifier).to eq(license.id) }
+    end
+
+    context "when a reported license is NOT provided" do
+      let(:license) { nil }
+
+      it { expect(subject.spdx_identifier).to eq(policy.software_license.spdx_identifier) }
     end
   end
 end

ee/spec/models/software_license_spec.rb

@@ -50,6 +50,14 @@ describe SoftwareLicense do
       it { expect(subject.spdx).to contain_exactly(mit) }
     end
 
+    describe '.by_spdx' do
+      it { expect(subject.by_spdx(mit.spdx_identifier)).to contain_exactly(mit) }
+    end
+
+    describe '.spdx' do
+      it { expect(subject.spdx).to contain_exactly(mit) }
+    end
+
     describe '.by_name' do
       it { expect(subject.by_name(mit.name)).to contain_exactly(mit) }
     end

ee/spec/services/projects/licenses/update_policy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::Licenses::UpdatePolicyService do
+  subject { described_class.new(project, user, params) }
+
+  let(:project) { create(:project, :repository, :private) }
+  let(:user) { create(:user) }
+
+  describe "#execute" do
+    let(:policy) { create(:software_license_policy, :denied, project: project, software_license: mit_license) }
+    let(:mit_license) { create(:software_license, :mit) }
+
+    context "when the user is authorized" do
+      before do
+        allow(RefreshLicenseComplianceChecksWorker).to receive(:perform_async)
+        stub_licensed_features(license_management: true)
+        project.add_maintainer(user)
+      end
+
+      context "when updating a policy" do
+        let(:params) { { classification: "approved" } }
+
+        it "updates the policy" do
+          result = subject.execute(policy.id)
+
+          expect(result[:status]).to eq(:success)
+          expect(result[:software_license_policy]).to be_present
+          expect(result[:software_license_policy].classification).to eq('approved')
+          expect(RefreshLicenseComplianceChecksWorker).to have_received(:perform_async).with(project.id)
+        end
+      end
+
+      context "when the classification is invalid" do
+        let(:params) { { classification: 'invalid' } }
+
+        it "returns an error" do
+          result = subject.execute(policy.id)
+
+          expect(result[:status]).to eq(:error)
+          expect(result[:message]).to be_instance_of(ActiveModel::Errors)
+          expect(result[:http_status]).to eq(:unprocessable_entity)
+          expect(RefreshLicenseComplianceChecksWorker).not_to have_received(:perform_async).with(project.id)
+        end
+      end
+    end
+
+    context "when the user is not authorized" do
+      context "when updating a policy" do
+        let(:params) { { classification: "approved" } }
+
+        it "returns an error" do
+          result = subject.execute(policy.id)
+
+          expect(result[:status]).to eq(:error)
+          expect(result[:message]).to be_empty
+          expect(result[:http_status]).to eq(:forbidden)
+        end
+      end
+    end
+  end
+end