BC: Rework licenses validation (#249)

- complete SPDX license expression validation/detection
- license models do no validations
- license factory rework

Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com>

Author: jkowalleck

.php-cs-fixer.dist.php

@@ -47,7 +47,9 @@
             'phpdoc_order' => true,
             'phpdoc_to_comment' => [
                 'ignored_tags' => [
-                //    'psalm-var', // needed when PSALM introduced some issues that only manual hints can solve
+                    // needed when PSALM introduced some issues that only manual hints can solve
+                    // 'psalm-var',
+                    // 'psalm-suppress',
                 ],
             ],
         ]

HISTORY.md

@@ -11,6 +11,7 @@ All notable changes to this project will be documented in this file.
   * Removed support for PHP v7.4 ([#114] via [#125])
   * Removed support for PHP v8.0 (via [#204])
   * Changed models' aggregation properties to be no longer optional ([#66] via [#131])
+  * CHanged models to be less restrictive ([#247] via [#249])
   * Streamlined repository data structures to follow a common method naming scheme (via [#131])
   * Enumeration-like classes were converted to native [PHP Enumerations](https://www.php.net/manual/en/language.types.enumerations.php) ([#140] via [#204])
 * Added
@@ -50,9 +51,16 @@ All notable changes to this project will be documented in this file.
     * BREAKING: method `isValidValue()` was removed (via [#204])
 * `CycloneDX\Core\Factories` namespace
   * `LicenseFactory` class
+    * BREAKING: check whether something is a valid SPDX Expression is now complete, was best effort implementation ([#247] via [#249])  
+      This affects all methods that potentially would create `LicenseExpression` models.  
+      Utilizes [`composer/spdx-licenses`](https://packagist.org/packages/composer/spdx-licenses).
+    * BREAKING: changed constructor method `__construct()` (via [#249])
     * BREAKING: removed method `makeDisjunctiveFromExpression()` ([#163] vial [#166])
+    * BREAKING: removed method `setSpdxLicenseValidator()` (via [#249])
+    * BREAKING: renamed method `getSpdxLicenseValidator()` -> `getLicenseIdentifiers()` (via [#249])
     * BREAKING: renamed method `makeDisjunctiveWithId()` -> `makeSpdxLicense()` ([#164] vial [#168])
     * BREAKING: renamed method `makeDisjunctiveWithName()` -> `makeNamedLicense()` ([#164] vial [#168])
+    * Added new method `getSpdxLicenses()` (via [#249])
 * `\CycloneDX\Core\Models` namespace
   * `Bom` class
     * BREAKING: changed constructor to no longer accept components ([#187] via [#188])
@@ -61,8 +69,8 @@ All notable changes to this project will be documented in this file.
       Also changed parameter & return type to non-nullable, was nullable ([#66] via [#131])
     * BREAKING: renamed methods `{get,set}MetaData()` -> `{get,set}Metadata()` ([#133] via [#131])  
       Also changed parameter & return type to non-nullable, was nullable ([#66] via [#131])
-    * Added `{get,set}Properties()` ([#228] via [#229])
-    * Added `{get,set}SerialNumber()` (via [#186])
+    * Added new methods `{get,set}Properties()` ([#228] via [#229])
+    * Added new methods `{get,set}SerialNumber()` (via [#186])
   * `Component` class
     * BREAKING: renamed methods `{get,set}DependenciesBomRefRepository()` -> `{get,set}Dependencies()` ([#133] via [#131])  
       Also changed parameter & return type to non-nullable, was nullable ([#66] via [#131])
@@ -76,10 +84,10 @@ All notable changes to this project will be documented in this file.
       This affects constructor arguments, and affects methods `{get,set}Version()`.
     * BREAKING: changed property `type` to be of type `\CycloneDX\Core\Enum\ComponentType` ([#140] via [#204])  
       This affects constructor arguments, and affects methods `{get,set}Type()`.
-    * Added `{get,set}Author()` ([#184] via [#185])
-    * Added `{get,set}Copyright()` ([#238] via [#239])
-    * Added `{get,set}Evidence()` ([#238] via [#241])
-    * Added `{get,set}Properties()` ([#228] via [#165])
+    * Added new methods `{get,set}Author()` ([#184] via [#185])
+    * Added new methods `{get,set}Copyright()` ([#238] via [#239])
+    * Added new methods `{get,set}Evidence()` ([#238] via [#241])
+    * Added new methods `{get,set}Properties()` ([#228] via [#165])
   * Added new class `ComponentEvidence` ([#238] via [#241])
   * `ExternalReference` class
     * BREAKING: renamed methods `{get,set}HashRepository()` -> `{get,set}Hashes()` ([#133] via [#131])  
@@ -93,6 +101,14 @@ All notable changes to this project will be documented in this file.
       * BREAKING: renamed class to `NamedLicense` ([#164] via [#168])
     * `DisjunctiveLicenseWithId` class
       * BREAKING: renamed class to `SpdxLicense` ([#164] via [#168])
+      * BREAKING: removed factory method `makeValidated()` ([#247] via [#249])
+        To assert valid values use `\CycloneDX\Core\Factories\LicenseFactory::makeSpdxLicense()`.
+      * Changed: constructor `__construct()` is public now, was private ([#247] via [#249])
+      * Added new method `setId()`  ([#247] via [#249])
+    * `LicenseExpression` class
+      * BREAKING: constructor `__construct()` and method `setExpression()` no longer do validation, but only assert that the parameter is no empty string. ([#247] ia [#249])  
+        To assert valid values use `\CycloneDX\Core\Factories\LicenseFactory::makeExpression()`.
+      * BREAKING: removed method `isValid()` ([#247] via [#249])
   * `MetaData` class
     * BREAKING: renamed class to `Metadata` ([#133] via [#131])  
       Even though PHP is case-insensitive with class names, autoloaders may be case-sensitive. Therefore, this is considered a breaking change.
@@ -135,10 +151,10 @@ All notable changes to this project will be documented in this file.
     * BREAKING: removed method `makeForDisjunctiveLicenseRepository()` (via [#131])
     * BREAKING: removed method `makeForHashRepositonary()` - use `makeForHashDictionary()` instead ([#133] via [#131])
     * BREAKING: removed method `setSpec()` (via [#131])
-    * Added method `makeForComponentEvidence()` ([#238] via [#241])
-    * Added method `makeForHashDictionary()` ([#133] via [#131])
-    * Added method `makeForLicense()` (via [#131])
-    * Added method `makeForLicenseRepository()` (via [#131])
+    * Added new method `makeForComponentEvidence()` ([#238] via [#241])
+    * Added new method `makeForHashDictionary()` ([#133] via [#131])
+    * Added new method `makeForLicense()` (via [#131])
+    * Added new method `makeForLicenseRepository()` (via [#131])
   * `{DOM,JSON}\Normalizers` namespaces
     * BREAKING: removed classes `DisjunctiveLicenseNormalizer` - use `LicenseNormalizer` instead (via [#131])
     * BREAKING: removed classes `LicenseExpressionNormalizer`  - use `LicenseNormalizer` instead (via [#131])
@@ -159,7 +175,10 @@ All notable changes to this project will be documented in this file.
   * `JSON\Normalizers\ExternalReferenceNormalizer` class
     * BREAKING: method `normalize()` may throw `\UnexpectedValueException` when the url is invalid to format "ini-reference" (via [#151])
 * `\CycloneDX\Core\Spdx` namespace
-  * BREAKING: renamed the class `License` -> `LicenseValidator` ([#133] via [#143])
+  * BREAKING: renamed the class `License` -> `LicenseIdentifiers` ([#133] via [#143], [#249])
+  * BREAKING: renamed method `getLicense()` -> `fixLicense()` (via [#249])
+  * BREAKING: renamed method `getLicenses()` -> `getKnownLicenses()`, and removed keys from return value (via [#249])
+  * BREAKING: renamed method `validate()` -> `isKnownLicense()` (via [#249])
 * `\CycloneDX\Core\Spec` namespace
   * BREAKING: completely reworked everything ([#139] via [#142], [#174], [#204])  
     See the code base for references
@@ -220,6 +239,8 @@ All notable changes to this project will be documented in this file.
 [#238]: https://github.com/CycloneDX/cyclonedx-php-library/issues/238
 [#239]: https://github.com/CycloneDX/cyclonedx-php-library/pull/239
 [#241]: https://github.com/CycloneDX/cyclonedx-php-library/pull/241
+[#247]: https://github.com/CycloneDX/cyclonedx-php-library/issues/247
+[#249]: https://github.com/CycloneDX/cyclonedx-php-library/pull/249
 
 ## 1.6.3 - 2022-09-15
 

composer.json

@@ -29,6 +29,7 @@
         "ext-dom": "*",
         "ext-json": "*",
         "ext-libxml": "*",
+        "composer/spdx-licenses": "^1.5",
         "opis/json-schema": "^2.0",
         "package-url/packageurl-php": "^1.0"
     },

phpunit.dist.xml

@@ -8,7 +8,12 @@
          beStrictAboutCoverageMetadata="true"
          beStrictAboutOutputDuringTests="true"
          beStrictAboutTodoAnnotatedTests="true"
+         displayDetailsOnIncompleteTests="true"
+         displayDetailsOnSkippedTests="true"
+         displayDetailsOnTestsThatTriggerNotices="false"
          displayDetailsOnTestsThatTriggerDeprecations="true"
+         displayDetailsOnTestsThatTriggerWarnings="true"
+         displayDetailsOnTestsThatTriggerErrors="true"
          failOnRisky="true"
          failOnWarning="true"
          defaultTestSuite="default"

src/Core/Factories/LicenseFactory.php

@@ -23,86 +23,83 @@
 
 namespace CycloneDX\Core\Factories;
 
+use Composer\Spdx\SpdxLicenses;
 use CycloneDX\Core\Models\License\LicenseExpression;
 use CycloneDX\Core\Models\License\NamedLicense;
 use CycloneDX\Core\Models\License\SpdxLicense;
-use CycloneDX\Core\Spdx\LicenseValidator as SpdxLicenseValidator;
+use CycloneDX\Core\Spdx\LicenseIdentifiers;
 use DomainException;
-use UnexpectedValueException;
 
 class LicenseFactory
 {
-    private ?SpdxLicenseValidator $spdxLicenseValidator;
-
-    public function __construct(?SpdxLicenseValidator $spdxLicenseValidator = null)
-    {
-        $this->spdxLicenseValidator = $spdxLicenseValidator;
+    public function __construct(
+        private readonly LicenseIdentifiers $licenseIdentifiers = new LicenseIdentifiers(),
+        private readonly SpdxLicenses $spdxLicenses = new SpdxLicenses()
+    ) {
     }
 
-    /**
-     * @psalm-assert SpdxLicenseValidator $this->spdxLicenseValidator
-     *
-     * @throws UnexpectedValueException when SpdxLicenseValidator is missing
-     */
-    public function getSpdxLicenseValidator(): SpdxLicenseValidator
+    public function getLicenseIdentifiers(): LicenseIdentifiers
     {
-        return $this->spdxLicenseValidator
-            ?? throw new UnexpectedValueException('Missing spdxLicenseValidator');
+        return $this->licenseIdentifiers;
     }
 
-    /**
-     * @psalm-assert SpdxLicenseValidator $this->spdxLicenseValidator
-     */
-    public function setSpdxLicenseValidator(SpdxLicenseValidator $spdxLicenseValidator): static
+    public function getSpdxLicenses(): SpdxLicenses
     {
-        $this->spdxLicenseValidator = $spdxLicenseValidator;
-
-        return $this;
+        return $this->spdxLicenses;
     }
 
-    /**
-     * @see makeExpression()
-     * @see makeDisjunctive()
-     */
-    public function makeFromString(string $license): NamedLicense|SpdxLicense|LicenseExpression
+    public function makeFromString(string $license): SpdxLicense|LicenseExpression|NamedLicense
     {
+        try {
+            return $this->makeSpdxLicense($license);
+        } catch (\DomainException) {
+            /* pass */
+        }
         try {
             return $this->makeExpression($license);
-        } catch (DomainException) {
-            return $this->makeDisjunctive($license);
+        } catch (\DomainException) {
+            /* pass */
         }
+
+        return $this->makeNamedLicense($license);
     }
 
-    /**
-     * @throws DomainException if the expression was invalid
-     */
-    public function makeExpression(string $license): LicenseExpression
+    public function makeDisjunctive(string $license): SpdxLicense|NamedLicense
     {
-        return new LicenseExpression($license);
+        try {
+            return $this->makeSpdxLicense($license);
+        } catch (\DomainException) {
+            return $this->makeNamedLicense($license);
+        }
     }
 
     /**
-     * @see makeSpdxLicense()
-     * @see makeNamedLicense()
+     * @throws DomainException when the SPDX license expressions was invalid
      */
-    public function makeDisjunctive(string $license): SpdxLicense|NamedLicense
+    public function makeExpression(string $license): LicenseExpression
     {
         try {
-            return $this->makeSpdxLicense($license);
-        } catch (UnexpectedValueException|DomainException) {
-            return $this->makeNamedLicense($license);
+            $valid = $this->spdxLicenses->validate($license);
+        } catch (\InvalidArgumentException) {
+            $valid = false;
         }
+        if ($valid) {
+            return new LicenseExpression($license);
+        }
+        throw new DomainException("invalid SPDX license expressions: $license");
     }
 
     /**
-     * @throws DomainException          when the SPDX license is invalid
-     * @throws UnexpectedValueException when SpdxLicenseValidator is missing
-     *
-     * @SuppressWarnings(PHPMD.StaticAccess)
+     * @throws DomainException when the SPDX license ID is unknown
      */
     public function makeSpdxLicense(string $license): SpdxLicense
     {
-        return SpdxLicense::makeValidated($license, $this->getSpdxLicenseValidator());
+        $fixed = $this->licenseIdentifiers->fixLicense($license);
+        if (null === $fixed) {
+            throw new DomainException("unknown SPDX license ID: $license");
+        }
+
+        return new SpdxLicense($fixed);
     }
 
     public function makeNamedLicense(string $license): NamedLicense

src/Core/Models/License/LicenseExpression.php

@@ -26,11 +26,18 @@
 use DomainException;
 
 /**
+ * (SPDX) License Expression.
+ *
+ * No validation is done internally.
+ * You may validate with {@see \Composer\Spdx\SpdxLicenses::isValidLicenseString()}.
+ *
  * @author jkowalleck
  */
 class LicenseExpression
 {
     /**
+     * @psalm-var non-empty-string
+     *
      * @psalm-suppress PropertyNotSetInConstructor
      */
     private string $expression;
@@ -41,32 +48,29 @@ public function getExpression(): string
     }
 
     /**
-     * @throws DomainException if the expression was invalid
+     * @psalm-assert non-empty-string $expression
+     *
+     * @throws DomainException if `$expression` is empty string
      *
      * @return $this
      */
     public function setExpression(string $expression): static
     {
-        $this->expression = self::isValid($expression)
-            ? $expression
-            : throw new DomainException("Invalid expression: $expression");
+        if ('' === $expression) {
+            throw new DomainException('expression must not be empty');
+        }
+        $this->expression = $expression;
 
         return $this;
     }
 
     /**
-     * @throws DomainException if the expression was invalid
+     * @psalm-assert non-empty-string $expression
+     *
+     * @throws DomainException if `$expression` is empty string
      */
     public function __construct(string $expression)
     {
         $this->setExpression($expression);
     }
-
-    public static function isValid(string $expression): bool
-    {
-        // smallest known: (A or B)
-        return \strlen($expression) >= 8
-            && '(' === $expression[0]
-            && ')' === $expression[-1];
-    }
 }

src/Core/Models/License/NamedLicense.php

@@ -34,10 +34,14 @@ class NamedLicense
 
     /**
      * If SPDX does not define the license used, this field may be used to provide the license name.
+     *
+     * Implementation detail: allow empty strings.
+     *
+     * @psalm-suppress PropertyNotSetInConstructor
      */
-    private ?string $name = null;
+    private string $name;
 
-    public function getName(): ?string
+    public function getName(): string
     {
         return $this->name;
     }