Repository: porscheinformatik/sonarqube-licensecheck
Pull request #183 (2020-07-24 13:23:31)

Support deprecated and wrong license syntax in package.json
### What's this about?
At CIDEON Software & Services GmbH & Co. KG we really appreciate you developing this useful plugin. We're using it in several projects, some being composed with nodejs.<br>
**(1) The issue** we ran into lately was the use of a deprecated license syntax in some npm packages we depend on. Instead of SPDX expressions like `{ "license": "ISC" }` we observed this format: <br>`"license" : 
        { "type" : "ISC", 
          "url" : "https://opensource.org/licenses/ISC"
        }`.<br>
**Expected behavior:** read the the SPDX license identifier from the "type" property.
**Current behavior:**  no handling implemented for that case - license is ignored.

**(2) Another deprecated format we observed** was the license being passed as an array (`"licenses":[..]`). The plugin handles this case, however, after taking a closer look at [the NPM docs](https://docs.npmjs.com/files/package.json#license), we noticed the possibility of encountering packages where there might be more than one license specified using this format: 
`"licenses" :
  [
    { "type": "MIT"
    , "url": "https://www.opensource.org/licenses/mit-license.php"
    }
  , { "type": "Apache-2.0"
    , "url": "https://opensource.org/licenses/apache2.0.php"
    }
  ]`.<br>
**Expected behavior:** check if there is more than one license specified. If so - collect the identifiers and make a conjunction using the _AND_ operator. This operator should be used because if multiple licenses are given without further information, one should assume that they all apply. In case of above example this would lead to the SPDX expression `"(MIT AND Apache-2.0)"`. If only one license is specified - simply take the identifier of the first item in the array (as currently implemented).
**Current behavior:**  identifier of the first license is used regardless of how many are present in the array.

### Conclusion
As it's crucial for us to have as little wrongly formatted license information fall through as possible, I changed the implementation to match our expected behavior. Please let me know if I got anything wrong on the part regarding the `licenses: []` array. Other than that, I think this change could be interesting for others using the plugin because, although there _shouldn't_, there still might be some npm packages out in the wild using one of the deprecated formats mentioned. 


Patch:
diff --git a/src/main/java/at/porscheinformatik/sonarqube/licensecheck/LicenseCheckPlugin.java b/src/main/java/at/porscheinformatik/sonarqube/licensecheck/LicenseCheckPlugin.java
index 943fdc2..c936f36 100644
--- a/src/main/java/at/porscheinformatik/sonarqube/licensecheck/LicenseCheckPlugin.java
+++ b/src/main/java/at/porscheinformatik/sonarqube/licensecheck/LicenseCheckPlugin.java
@@ -6,7 +6,6 @@ import java.util.List;
 import org.sonar.api.Plugin;
 import org.sonar.api.PropertyType;
 import org.sonar.api.config.PropertyDefinition;
-import org.sonar.api.scanner.fs.InputProject;
 
 import at.porscheinformatik.sonarqube.licensecheck.license.LicenseService;
 import at.porscheinformatik.sonarqube.licensecheck.license.LicenseSettingsService;
diff --git a/src/main/java/at/porscheinformatik/sonarqube/licensecheck/npm/PackageJsonDependencyScanner.java b/src/main/java/at/porscheinformatik/sonarqube/licensecheck/npm/PackageJsonDependencyScanner.java
index 342547a..e164ac3 100644
--- a/src/main/java/at/porscheinformatik/sonarqube/licensecheck/npm/PackageJsonDependencyScanner.java
+++ b/src/main/java/at/porscheinformatik/sonarqube/licensecheck/npm/PackageJsonDependencyScanner.java
@@ -99,14 +99,33 @@ public class PackageJsonDependencyScanner implements Scanner
                     String license = "";
                     if (packageJson.containsKey("license"))
                     {
-                        license = packageJson.getString("license", null);
+                        final Object licenceObj = packageJson.get("license");
+                        if (licenceObj instanceof JsonObject)
+                        {
+                            license = ((JsonObject) licenceObj).getString("type", "");
+                        }
+                        else
+                        {
+                            license = packageJson.getString("license", "");
+                        }
                     }
                     else if (packageJson.containsKey("licenses"))
                     {
-                        JsonArray licenses = packageJson.getJsonArray("licenses");
-                        if (licenses.size() > 0)
+                        final JsonArray licenses = packageJson.getJsonArray("licenses");
+                        if (licenses.size() == 1) 
+                        {
+                            license = licenses.getJsonObject(0).getString("type", "");
+                        }
+                        else if (licenses.size() > 1)
                         {
-                            license = licenses.getJsonObject(0).getString("type", null);
+                            license = "(";
+                            for (int i = 0; i < licenses.size(); i++) {
+                                final String licensePart = licenses.getJsonObject(i).getString("type", "");
+                                if (!licensePart.trim().isEmpty()) {
+                                    license += license.length() > 1 ? (" AND " + licensePart) : licensePart;
+                                }
+                            }
+                            license = license.length() == 1 ? "" : (license + ")");
                         }
                     }
 


Test patch:
diff --git a/src/test/java/at/porscheinformatik/sonarqube/licensecheck/PackageJsonDependencyScannerTest.java b/src/test/java/at/porscheinformatik/sonarqube/licensecheck/PackageJsonDependencyScannerTest.java
index 99f929d..e1f79a5 100644
--- a/src/test/java/at/porscheinformatik/sonarqube/licensecheck/PackageJsonDependencyScannerTest.java
+++ b/src/test/java/at/porscheinformatik/sonarqube/licensecheck/PackageJsonDependencyScannerTest.java
@@ -10,6 +10,7 @@ import at.porscheinformatik.sonarqube.licensecheck.npm.PackageJsonDependencyScan
 
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.hasSize;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThat;
 
 public class PackageJsonDependencyScannerTest
@@ -63,4 +64,30 @@ public class PackageJsonDependencyScannerTest
 
         assertThat(dependencies, hasSize(0));
     }
+
+    @Test
+    public void testLicenseInDeprecatedLicenseFormat()
+    {
+        final Scanner scanner = new PackageJsonDependencyScanner(false);
+
+        final Set<Dependency> dependencies = scanner.scan(new File(folder, "deprecated_project"));
+
+        assertEquals(1, dependencies.size());
+
+        final Dependency expectedDependency = new Dependency("dynamic-dedupe", "0.3.0", "MIT");
+        assertEquals(expectedDependency, dependencies.toArray()[0]);
+    }
+
+    @Test
+    public void testLicenseInDeprecatedLicensesFormat()
+    {
+        final Scanner scanner = new PackageJsonDependencyScanner(false);
+
+        final Set<Dependency> dependencies = scanner.scan(new File(folder, "deprecated_multilicense_project"));
+
+        assertEquals(1, dependencies.size());
+
+        final Dependency expectedDependency = new Dependency("some-module", "1.7.1", "(MIT AND LGPLv3)");
+        assertEquals(expectedDependency, dependencies.toArray()[0]);
+    }
 }
diff --git a/src/test/resources/deprecated_multilicense_project/node_modules/some-module/package.json b/src/test/resources/deprecated_multilicense_project/node_modules/some-module/package.json
new file mode 100644
index 0000000..86210b5
--- /dev/null
+++ b/src/test/resources/deprecated_multilicense_project/node_modules/some-module/package.json
@@ -0,0 +1,81 @@
+{
+    "_from": "some-module",
+    "_id": "some-module@1.7.1",
+    "_inBundle": false,
+    "_integrity": "sha1-BuRMIj9eTpTXjvnbI6ZRXOL5YqE=",
+    "_location": "/some-module",
+    "_phantomChildren": {},
+    "_requested": {
+      "type": "tag",
+      "registry": true,
+      "raw": "some-module",
+      "name": "some-module",
+      "escapedName": "some-module",
+      "rawSpec": "",
+      "saveSpec": null,
+      "fetchSpec": "latest"
+    },
+    "_requiredBy": [
+      "#USER",
+      "/"
+    ],
+    "_resolved": "https://registry.npmjs.org/some-module/-/some-module-0.3.0.tgz",
+    "_shasum": "06e44c223f5e4e94d78ef9db23a6515ce2f962a1",
+    "_spec": "some-module",
+    "_where": "C:\\Users\\felix.luepke\\License_Check_Test_Project\\AngularTestLicenseCheck",
+    "author": {
+      "name": "Thorsten Lorenz",
+      "email": "thlorenz@gmx.de",
+      "url": "http://thlorenz.com"
+    },
+    "bugs": {
+      "url": "https://github.com/thlorenz/some-module/issues"
+    },
+    "bundleDependencies": false,
+    "dependencies": {
+      "xtend": "^4.0.0"
+    },
+    "deprecated": false,
+    "description": "Illustrated a wrong way of multi licensing that might still be used by people.",
+    "devDependencies": {
+      "nave": "~0.4.3",
+      "tap": "~0.4.3"
+    },
+    "engine": {
+      "node": ">=0.6"
+    },
+    "homepage": "https://github.com/thlorenz/some-module",
+    "keywords": [
+      "dedupe",
+      "npm",
+      "require",
+      "extension",
+      "link"
+    ],
+    "licenses": [
+      {"type": ""
+      },
+      {"type": "MIT",
+        "url": "https://github.com/thlorenz/some-module/blob/master/LICENSE"
+      }, 
+      {"type": "LGPLv3"
+      }, 
+      {"type": "  "
+      }
+    ],
+    "main": "index.js",
+    "name": "some-module",
+    "repository": {
+      "type": "git",
+      "url": "git://github.com/thlorenz/some-module.git"
+    },
+    "scripts": {
+      "test": "if [ -e $TRAVIS ]; then npm run test-all; else npm run test-main; fi",
+      "test-0.10": "nave use 0.10 npm run test-main",
+      "test-0.8": "nave use 0.8 npm run test-main",
+      "test-all": "npm run test-main && npm run test-0.8 && npm run test-0.10",
+      "test-main": "tap test/*.js"
+    },
+    "version": "1.7.1"
+  }
+  
\ No newline at end of file
diff --git a/src/test/resources/deprecated_multilicense_project/package.json b/src/test/resources/deprecated_multilicense_project/package.json
new file mode 100644
index 0000000..cc890a2
--- /dev/null
+++ b/src/test/resources/deprecated_multilicense_project/package.json
@@ -0,0 +1,11 @@
+{
+    "name": "test",
+    "version": "1.0.0",
+    "description": "An example module to illustrate the usage of a package.json",
+    "author": "Your Name <you.name@example.org>",
+    "dependencies": {
+      "some-module": "^1.7.1"
+    },
+    "license": "MIT"
+  }
+  
\ No newline at end of file
diff --git a/src/test/resources/deprecated_project/node_modules/dynamic-dedupe/package.json b/src/test/resources/deprecated_project/node_modules/dynamic-dedupe/package.json
new file mode 100644
index 0000000..79d9313
--- /dev/null
+++ b/src/test/resources/deprecated_project/node_modules/dynamic-dedupe/package.json
@@ -0,0 +1,74 @@
+{
+    "_from": "dynamic-dedupe",
+    "_id": "dynamic-dedupe@0.3.0",
+    "_inBundle": false,
+    "_integrity": "sha1-BuRMIj9eTpTXjvnbI6ZRXOL5YqE=",
+    "_location": "/dynamic-dedupe",
+    "_phantomChildren": {},
+    "_requested": {
+      "type": "tag",
+      "registry": true,
+      "raw": "dynamic-dedupe",
+      "name": "dynamic-dedupe",
+      "escapedName": "dynamic-dedupe",
+      "rawSpec": "",
+      "saveSpec": null,
+      "fetchSpec": "latest"
+    },
+    "_requiredBy": [
+      "#USER",
+      "/"
+    ],
+    "_resolved": "https://registry.npmjs.org/dynamic-dedupe/-/dynamic-dedupe-0.3.0.tgz",
+    "_shasum": "06e44c223f5e4e94d78ef9db23a6515ce2f962a1",
+    "_spec": "dynamic-dedupe",
+    "_where": "C:\\Users\\felix.luepke\\License_Check_Test_Project\\AngularTestLicenseCheck",
+    "author": {
+      "name": "Thorsten Lorenz",
+      "email": "thlorenz@gmx.de",
+      "url": "http://thlorenz.com"
+    },
+    "bugs": {
+      "url": "https://github.com/thlorenz/dynamic-dedupe/issues"
+    },
+    "bundleDependencies": false,
+    "dependencies": {
+      "xtend": "^4.0.0"
+    },
+    "deprecated": false,
+    "description": "Dedupes node modules as they are being required  which works even when dependencies are linked via ln -s or npm link.",
+    "devDependencies": {
+      "nave": "~0.4.3",
+      "tap": "~0.4.3"
+    },
+    "engine": {
+      "node": ">=0.6"
+    },
+    "homepage": "https://github.com/thlorenz/dynamic-dedupe",
+    "keywords": [
+      "dedupe",
+      "npm",
+      "require",
+      "extension",
+      "link"
+    ],
+    "license": {
+      "type": "MIT",
+      "url": "https://github.com/thlorenz/dynamic-dedupe/blob/master/LICENSE"
+    },
+    "main": "index.js",
+    "name": "dynamic-dedupe",
+    "repository": {
+      "type": "git",
+      "url": "git://github.com/thlorenz/dynamic-dedupe.git"
+    },
+    "scripts": {
+      "test": "if [ -e $TRAVIS ]; then npm run test-all; else npm run test-main; fi",
+      "test-0.10": "nave use 0.10 npm run test-main",
+      "test-0.8": "nave use 0.8 npm run test-main",
+      "test-all": "npm run test-main && npm run test-0.8 && npm run test-0.10",
+      "test-main": "tap test/*.js"
+    },
+    "version": "0.3.0"
+  }
+  
\ No newline at end of file
diff --git a/src/test/resources/deprecated_project/package.json b/src/test/resources/deprecated_project/package.json
new file mode 100644
index 0000000..5f557d8
--- /dev/null
+++ b/src/test/resources/deprecated_project/package.json
@@ -0,0 +1,11 @@
+{
+    "name": "test",
+    "version": "1.0.0",
+    "description": "An example module to illustrate the usage of a package.json",
+    "author": "Your Name <you.name@example.org>",
+    "dependencies": {
+      "dynamic-dedupe": "^0.3.0"
+    },
+    "license": "MIT"
+  }
+  
\ No newline at end of file