Apply maintainer feedback

https://gitlab.com/gitlab-org/gitlab/merge_requests/19261#note_241728793
https://gitlab.com/gitlab-org/gitlab/merge_requests/19261#note_241728794
https://gitlab.com/gitlab-org/gitlab/merge_requests/19261#note_241728796
https://gitlab.com/gitlab-org/gitlab/merge_requests/19261#note_241728800
https://gitlab.com/gitlab-org/gitlab/merge_requests/19261#note_241728804
https://gitlab.com/gitlab-org/gitlab/merge_requests/19261#note_241728805
https://gitlab.com/gitlab-org/gitlab/merge_requests/19261#note_241728807
https://gitlab.com/gitlab-org/gitlab/merge_requests/19261#note_241728811
https://gitlab.com/gitlab-org/gitlab/merge_requests/19261#note_241728813
https://gitlab.com/gitlab-org/gitlab/merge_requests/19261#note_241728814

ee/app/models/sca/license_compliance.rb

@@ -2,7 +2,7 @@
 
 module SCA
   class LicenseCompliance
-    include ::Gitlab::Utils:: StrongMemoize
+    include ::Gitlab::Utils::StrongMemoize
 
     def initialize(project)
       @project = project
@@ -10,8 +10,16 @@ module SCA
 
     def policies
       strong_memoize(:policies) do
-        configured_policies = software_license_policies.index_by { |policy| policy.software_license.canonical_id }
-        combine_licenses_from(configured_policies).sort_by(&:name)
+        configured_policies = project.software_license_policies.index_by { |policy| policy.software_license.canonical_id }
+        detected_licenses = license_scan_report.licenses.map do |reported_license|
+          policy = configured_policies[reported_license.canonical_id]
+          configured_policies.delete(reported_license.canonical_id) if policy
+          build_policy(reported_license, policy)
+        end
+        undetected_licenses = configured_policies.map do |id, policy|
+          build_policy(license_scan_report.fetch(id, nil), policy)
+        end
+        (detected_licenses + undetected_licenses).sort_by(&:name)
       end
     end
 
@@ -26,23 +34,10 @@ module SCA
     private
 
     attr_reader :project
-    delegate :all_pipelines, :default_branch, :software_license_policies, to: :project
-
-    def combine_licenses_from(configured_policies)
-      detected_licenses = license_scan_report.licenses.map do |reported_license|
-        policy = configured_policies[reported_license.canonical_id]
-        configured_policies.delete(reported_license.canonical_id) if policy
-        build_policy(reported_license, policy)
-      end
-      undetected_licenses = configured_policies.map do |id, policy|
-        build_policy(license_scan_report.fetch(id, nil), policy)
-      end
-      detected_licenses + undetected_licenses
-    end
 
     def pipeline
       strong_memoize(:pipeline) do
-        all_pipelines.latest_successful_for_ref(default_branch)
+        project.all_pipelines.latest_successful_for_ref(project.default_branch)
       end
     end
 
@@ -53,10 +48,10 @@ module SCA
         pipeline.license_scanning_report.tap do |report|
           report.apply_details_from!(dependency_list_report)
         end
-      end
       rescue ::Gitlab::Ci::Parsers::LicenseCompliance::LicenseScanning::LicenseScanningParserError
         empty_report
       end
+    end
 
     def dependency_list_report
       pipeline.dependency_list_report

ee/app/models/sca/license_policy.rb

@@ -16,10 +16,13 @@ module SCA
     private
 
     def classify(policy)
-      return 'allowed' if policy&.approved?
-      return 'denied' if policy&.blacklisted?
-
+      if policy&.approved?
+        'allowed'
+      elsif policy&.blacklisted?
+        'denied'
+      else
         'unclassified'
       end
     end
+  end
 end

ee/changelogs/unreleased/34824-license-classifications.yml

----
-title: Add policy id and classification to the software license policy json entity
-merge_request: 19261
-author:
-type: changed

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

@@ -81,24 +81,33 @@ describe Projects::Security::LicensesController do
           end
 
           it { expect(response).to have_http_status(:ok) }
-          it { expect(json_response["licenses"].count).to be(3) }
-          it { expect(json_response.dig("licenses", 0, "id")).to be_nil }
-          it { expect(json_response.dig("licenses", 0, "spdx_identifier")).to eq("BSD-3-Clause") }
-          it { expect(json_response.dig("licenses", 0, "name")).to eql("BSD 3-Clause \"New\" or \"Revised\" License") }
-          it { expect(json_response.dig("licenses", 0, "url")).to eql("http://spdx.org/licenses/BSD-3-Clause.json") }
-          it { expect(json_response.dig("licenses", 0, "classification")).to eql("unclassified") }
-
-          it { expect(json_response.dig("licenses", 1, "id")).to eql(mit_policy.id) }
-          it { expect(json_response.dig("licenses", 1, "spdx_identifier")).to eq("MIT") }
-          it { expect(json_response.dig("licenses", 1, "name")).to eql(mit.name) }
-          it { expect(json_response.dig("licenses", 1, "url")).to eql("http://spdx.org/licenses/MIT.json") }
-          it { expect(json_response.dig("licenses", 1, "classification")).to eql("denied") }
-
-          it { expect(json_response.dig("licenses", 2, "id")).to be_nil }
-          it { expect(json_response.dig("licenses", 2, "spdx_identifier")).to be_nil }
-          it { expect(json_response.dig("licenses", 2, "name")).to eql("unknown") }
-          it { expect(json_response.dig("licenses", 2, "url")).to eql("") }
-          it { expect(json_response.dig("licenses", 2, "classification")).to eql("unclassified") }
+
+          it 'generates the proper JSON response' do
+            expect(json_response["licenses"].count).to be(3)
+            expect(json_response.dig("licenses", 0)).to include({
+              "id" => nil,
+              "spdx_identifier" => "BSD-3-Clause",
+              "name" => "BSD 3-Clause \"New\" or \"Revised\" License",
+              "url" => "http://spdx.org/licenses/BSD-3-Clause.json",
+              "classification" => "unclassified"
+            })
+
+            expect(json_response.dig("licenses", 1)).to include({
+              "id" => mit_policy.id,
+              "spdx_identifier" => "MIT",
+              "name" => mit.name,
+              "url" => "http://spdx.org/licenses/MIT.json",
+              "classification" => "denied"
+            })
+
+            expect(json_response.dig("licenses", 2)).to include({
+              "id" => nil,
+              "spdx_identifier" => nil,
+              "name" => "unknown",
+              "url" => "",
+              "classification" => "unclassified"
+            })
+          end
         end
 
         context 'without existing report' do

ee/spec/factories/ci/reports/license_scanning/dependency.rb

@@ -6,7 +6,7 @@ FactoryBot.define do
 
     trait :rails do
       name { 'rails' }
-      path { './vendor/bundle/ruby/2.6.0/gems/rails-5.2.3/README.md' }
+      path { '.' }
     end
   end
 end

ee/spec/models/ci/build_spec.rb

@@ -352,11 +352,11 @@ describe Ci::Build do
   end
 
   describe ".license_scan" do
-    subject { described_class.license_scan }
+    it 'returns only license artifacts' do
+      create(:ci_build, job_artifacts: [create(:ci_job_artifact, :zip)])
+      build_with_license_scan = create(:ci_build, job_artifacts: [create(:ci_job_artifact, file_type: :license_management, file_format: :raw)])
 
-    let!(:build_with_zip) { create(:ci_build, job_artifacts: [create(:ci_job_artifact, :zip)]) }
-    let!(:build_with_license_scan) { create(:ci_build, job_artifacts: [create(:ci_job_artifact, file_type: :license_management, file_format: :raw)]) }
-
-    it { expect(subject).to contain_exactly(build_with_license_scan) }
+      expect(described_class.license_scan).to contain_exactly(build_with_license_scan)
+    end
   end
 end

ee/spec/models/sca/license_compliance_spec.rb

@@ -16,15 +16,17 @@ RSpec.describe SCA::LicenseCompliance do
       it { expect(subject.policies.count).to be_zero }
 
       context "when the project has policies configured" do
-        let!(:mit) { create(:software_license, :mit) }
-        let!(:mit_policy) { create(:software_license_policy, :denied, software_license: mit, project: project) }
+        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)
 
-        it { expect(subject.policies.count).to be(1) }
-        it { expect(subject.policies[0]&.id).to eq(mit_policy.id) }
-        it { expect(subject.policies[0]&.name).to eq(mit.name) }
-        it { expect(subject.policies[0]&.url).to be_nil }
-        it { expect(subject.policies[0]&.classification).to eq("denied") }
-        it { expect(subject.policies[0]&.spdx_identifier).to eq("MIT") }
+          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("denied")
+          expect(subject.policies[0]&.spdx_identifier).to eq("MIT")
+        end
       end
     end
 
@@ -77,44 +79,40 @@ RSpec.describe SCA::LicenseCompliance do
 
       context "when a pipeline has successfully produced a license scan report" do
         let(:builds) { [license_scan_build] }
-        let!(:mit) { create(:software_license, :mit) }
-        let!(:mit_policy) { create(:software_license_policy, :denied, software_license: mit, project: project) }
-        let!(:other_license) { create(:software_license, spdx_identifier: "Other-Id") }
-        let!(:other_license_policy) { create(:software_license_policy, :allowed, software_license: other_license, project: project) }
-
         let(:license_scan_build) { create(:ci_build, :success, job_artifacts: [license_scan_artifact]) }
         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") }
 
-        before do
-          license_scan_artifact.update!(file: license_scan_file)
-        end
-
-        it { expect(subject.policies.count).to eq(4) }
-
-        it { expect(subject.policies[0]&.id).to be_nil }
-        it { expect(subject.policies[0]&.name).to eq("BSD 3-Clause \"New\" or \"Revised\" License") }
-        it { expect(subject.policies[0]&.url).to eq("http://spdx.org/licenses/BSD-3-Clause.json") }
-        it { expect(subject.policies[0]&.classification).to eq("unclassified") }
-        it { expect(subject.policies[0]&.spdx_identifier).to eq("BSD-3-Clause") }
-
-        it { expect(subject.policies[1]&.id).to eq(mit_policy.id) }
-        it { expect(subject.policies[1]&.name).to eq(mit.name) }
-        it { expect(subject.policies[1]&.url).to eq("http://spdx.org/licenses/MIT.json") }
-        it { expect(subject.policies[1]&.classification).to eq("denied") }
-        it { expect(subject.policies[1]&.spdx_identifier).to eq("MIT") }
+        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")
+          other_license_policy = create(:software_license_policy, :allowed, software_license: other_license, project: project)
 
-        it { expect(subject.policies[2]&.id).to eq(other_license_policy.id) }
-        it { expect(subject.policies[2]&.name).to eq(other_license.name) }
-        it { expect(subject.policies[2]&.url).to be_blank }
-        it { expect(subject.policies[2]&.classification).to eq("allowed") }
-        it { expect(subject.policies[2]&.spdx_identifier).to eq(other_license.spdx_identifier) }
+          license_scan_artifact.update!(file: license_scan_file)
 
-        it { expect(subject.policies[3]&.id).to be_nil }
-        it { expect(subject.policies[3]&.name).to eq("unknown") }
-        it { expect(subject.policies[3]&.url).to be_blank }
-        it { expect(subject.policies[3]&.classification).to eq("unclassified") }
-        it { expect(subject.policies[3]&.spdx_identifier).to be_nil }
+          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("denied")
+          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("allowed")
+          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
   end