Skip to content

Commit

Permalink
Fix handling of dependencies without any licenses at all (#44)
Browse files Browse the repository at this point in the history
We made an incorrect assumption that "none" would be substituted in by
Composer (as the text output of `composer licenses` does) when this
situation happens.

Fixes 2 issues:

- `composer licenses` returns an empty list of licenses when a
dependency does not specify a license at all. Our parsing here would
result in always accepting that dependency (as we were iterating the
licenses, so had nothing to reject).
- `installed.json` does not contain the `license` key when a dependency
does not specify a license at all. Our parsing would error out entirely
on missing key when this was encountered.

This fixes both issues, rejecting any depedencies which do not specify a
license, and are not explicitly allowed as a vendor or package in
configuration.

---------

Co-authored-by: Marcin Michalski <marcin+github@michalski.dev>
  • Loading branch information
ben-challis and marmichalski committed Feb 13, 2023
1 parent 6f8809b commit 7268465
Show file tree
Hide file tree
Showing 14 changed files with 95 additions and 9 deletions.
12 changes: 12 additions & 0 deletions src/LicenseChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,18 @@ protected function execute(InputInterface $input, OutputInterface $output): int
continue;
}

if ($package->licenses === []) {
$violation = true;
$style->error(
\sprintf(
'Dependency "%s" does not have a license and is not explicitly allowed.',
$package->name->toString(),
)
);

continue;
}

foreach ($package->licenses as $license) {
if ($config->allowsLicense($license)) {
continue;
Expand Down
11 changes: 5 additions & 6 deletions src/PackagesProvider/ComposerInstalledJsonPackagesProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -74,18 +74,17 @@ static function (mixed $package) use (&$skipPackages): ?Package {
throw FailedProvidingPackages::withReason('Key "name" is not a string');
}

if (!isset($package['license'])) {
throw FailedProvidingPackages::withReason('Missing "license" key for package');
}
$licenses = $package['license'] ?? [];

if (!\is_array($package['license']) || !\array_is_list($package['license'])) {
if (!\is_array($licenses) || !\array_is_list($licenses)) {
throw FailedProvidingPackages::withReason('Key "license" is not a list');
}

/** @var array{name: non-empty-string, license: list<non-empty-string>} $package */
/** @var list<non-empty-string> $licenses */
/** @var array{name: non-empty-string, license?: list<non-empty-string>} $package */
return new Package(
new PackageName($package['name']),
\array_values($package['license']),
$licenses,
);
},
$dependencies,
Expand Down
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
8 changes: 8 additions & 0 deletions tests/data/with_unlicensed/composer.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "composer-license-checker/data",
"type": "project",
"license": "MIT",
"require": {
"lendable/unlicensed": "^1.0"
}
}
9 changes: 9 additions & 0 deletions tests/data/with_unlicensed/vendor/composer/installed.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"packages": [
{
"name": "lendable/unlicensed",
"version": "1.2.3"
}
],
"dev": true
}
Empty file.
48 changes: 45 additions & 3 deletions tests/e2e/LicenseCheckerCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ abstract class LicenseCheckerCase extends TestCase
{
private CommandTester $commandTester;

private string $path = 'tests'.\DIRECTORY_SEPARATOR.'data'.\DIRECTORY_SEPARATOR;
private string $path = 'tests'.\DIRECTORY_SEPARATOR.'data'.\DIRECTORY_SEPARATOR.'default'.\DIRECTORY_SEPARATOR;

protected function setUp(): void
{
Expand Down Expand Up @@ -70,14 +70,14 @@ public function test_failure_with_path_that_is_not_composer_project_root(): void
public function test_failure_with_invalid_allowed_licenses_file(): void
{
$exitCode = $this->commandTester->execute([
'--allow-file' => 'tests/data/invalid_allowed_licenses.php',
'--allow-file' => 'tests/data/default/invalid_allowed_licenses.php',
'--path' => $this->path,
]);
$output = $this->getOutputLines();

self::assertSame(1, $exitCode);
self::assertCount(5, $output);
self::assertSame('[ERROR] File "tests/data/invalid_allowed_licenses.php" must return an instance of', $output[3]);
self::assertSame('[ERROR] File "tests/data/default/invalid_allowed_licenses.php" must return an instance of', $output[3]);
self::assertSame(LicenseConfiguration::class.'.', $output[4]);
}

Expand Down Expand Up @@ -109,6 +109,48 @@ public function test_no_licenses_allowed(): void
);
}

public function test_with_unlicensed_package_from_non_trusted_vendor(): void
{
$handle = $this->createTempFile();
$allowFile = LicenseConfigurationFileBuilder::create($handle)->build();

$exitCode = $this->commandTester->execute(['--allow-file' => $allowFile, '--path' => 'tests/data/with_unlicensed']);
$output = $this->getOutputLines();

self::assertSame(1, $exitCode);
self::assertCount(4, $output);
self::assertSame(
'[ERROR] Dependency "lendable/unlicensed" does not have a license and is not explicitly allowed.',
$output[3],
);
}

public function test_with_unlicensed_package_from_trusted_vendor(): void
{
$handle = $this->createTempFile();
$allowFile = LicenseConfigurationFileBuilder::create($handle)->withAllowedVendor('lendable')->build();

$exitCode = $this->commandTester->execute(['--allow-file' => $allowFile, '--path' => 'tests/data/with_unlicensed']);
$output = $this->getOutputLines();

self::assertSame(0, $exitCode);
self::assertCount(4, $output);
self::assertSame('[OK] All dependencies have allowed licenses.', $output[3]);
}

public function test_with_unlicensed_package_which_is_explicitly_allowed(): void
{
$handle = $this->createTempFile();
$allowFile = LicenseConfigurationFileBuilder::create($handle)->withAllowedPackage('lendable/unlicensed')->build();

$exitCode = $this->commandTester->execute(['--allow-file' => $allowFile, '--path' => 'tests/data/with_unlicensed']);
$output = $this->getOutputLines();

self::assertSame(0, $exitCode);
self::assertCount(4, $output);
self::assertSame('[OK] All dependencies have allowed licenses.', $output[3]);
}

public function test_report_not_allowed_licenses(): void
{
$handle = $this->createTempFile();
Expand Down
16 changes: 16 additions & 0 deletions tests/support/LicenseConfigurationFileBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ final class LicenseConfigurationFileBuilder
*/
private array $allowedVendors = [];

/**
* @var list<string>
*/
private array $allowedPackages = [];

private ?bool $ignoreDev = null;

/**
Expand Down Expand Up @@ -47,6 +52,13 @@ public function withAllowedVendor(string $vendor): self
return $this;
}

public function withAllowedPackage(string $package): self
{
$this->allowedPackages[] = $package;

return $this;
}

public function withIgnoreDev(bool $ignore): self
{
$this->ignoreDev = $ignore;
Expand Down Expand Up @@ -81,6 +93,10 @@ private function buildContent(): string
$content .= \sprintf('->addAllowedVendor(\'%s\')', $allowedVendor);
}

foreach ($this->allowedPackages as $allowedPackage) {
$content .= \sprintf('->addAllowedPackage(\'%s\')', $allowedPackage);
}

if ($this->ignoreDev !== null) {
$content .= \sprintf('->ignoreDev(%s)', $this->ignoreDev ? 'true' : 'false');
}
Expand Down

0 comments on commit 7268465

Please sign in to comment.