From fd65288d4a1445bfff1f54185a166cc6c4818427 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Sun, 12 Jan 2025 14:07:21 -0800 Subject: [PATCH] Add `is_open_source_project` to metadata validation Reciprocal licenses can only be used in open source projects. This change updates the presubmit validation checks to accept an optional flag `allow_reciprocal_licenses`. When True, the allowlist is extended to include reciprocal licenses. Bug: 385020146 Change-Id: I0374658207bc87ffd74e033762ee4973c6e83b3b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6107863 Reviewed-by: Jordan Brown Auto-Submit: Jordan Brown Reviewed-by: Rachael Newitt Commit-Queue: Rachael Newitt --- metadata/dependency_metadata.py | 64 +++++++++++++++---- metadata/fields/custom/license.py | 28 +++++--- metadata/fields/custom/license_allowlist.py | 6 +- .../data/README.chromium.test.multi-invalid | 34 ++++++++-- .../README.chromium.test.reciprocal-license | 7 ++ metadata/tests/dependency_metadata_test.py | 28 ++++++++ metadata/tests/parse_test.py | 30 +++++++-- metadata/tests/validate_test.py | 50 +++++++++++++-- metadata/validate.py | 26 ++++++-- presubmit_canned_checks.py | 1 + 10 files changed, 223 insertions(+), 51 deletions(-) create mode 100644 metadata/tests/data/README.chromium.test.reciprocal-license diff --git a/metadata/dependency_metadata.py b/metadata/dependency_metadata.py index 4227ce8f9a..40812be611 100644 --- a/metadata/dependency_metadata.py +++ b/metadata/dependency_metadata.py @@ -22,6 +22,7 @@ import metadata.fields.custom.version as version_util import metadata.fields.known as known_fields import metadata.fields.util as util import metadata.validation_result as vr +from metadata.fields.custom.license_allowlist import OPEN_SOURCE_SPDX_LICENSES class DependencyMetadata: @@ -109,7 +110,31 @@ class DependencyMetadata: field: field_types.MetadataField) -> List[int]: return sorted(self._metadata_line_numbers[field]) - def _assess_required_fields(self) -> Set[field_types.MetadataField]: + def all_licenses_allowlisted(self, license_field_value: str, is_open_source_project: bool) -> bool: + """Returns whether all licenses in the field are allowlisted. + Assumes a non-empty license_field_value""" + licenses = license_util.process_license_value( + license_field_value, + atomic_delimiter=known_fields.LICENSE.VALUE_DELIMITER) + for lic, valid in licenses: + allowed = license_util.is_license_allowlisted(lic, is_open_source_project=is_open_source_project) + if not valid or not allowed: + return False + return True + + + def only_open_source_licenses(self, license_field_value: str) ->List[str]: + """Returns a list of licenses that are only allowed in open source projects.""" + licenses = license_util.process_license_value( + license_field_value, + atomic_delimiter=known_fields.LICENSE.VALUE_DELIMITER) + open_source_only = [] + for lic, valid in licenses: + if valid and lic in OPEN_SOURCE_SPDX_LICENSES: + open_source_only.append(lic) + return open_source_only + + def _assess_required_fields(self, is_open_source_project: bool = False) -> Set[field_types.MetadataField]: """Returns the set of required fields, based on the current metadata. """ @@ -127,30 +152,22 @@ class DependencyMetadata: # License compatibility with Android must be set if the # package is shipped and the license is not in the # allowlist. - has_allowlisted = False license_value = self._metadata.get(known_fields.LICENSE) - if license_value: - licenses = license_util.process_license_value( - license_value, - atomic_delimiter=known_fields.LICENSE.VALUE_DELIMITER) - for _, allowed in licenses: - if allowed: - has_allowlisted = True - break - - if not has_allowlisted: + if not license_value or not self.all_licenses_allowlisted(license_value, is_open_source_project): required.add(known_fields.LICENSE_ANDROID_COMPATIBLE) return required def validate(self, source_file_dir: str, - repo_root_dir: str) -> List[vr.ValidationResult]: + repo_root_dir: str, + is_open_source_project: bool = False) -> List[vr.ValidationResult]: """Validates all the metadata. Args: source_file_dir: the directory of the file that the metadata is from. repo_root_dir: the repository's root directory. + is_open_source_project: whether the project is open source. Returns: the metadata's validation results. """ @@ -210,7 +227,7 @@ class DependencyMetadata: results.append(field_result) # Check required fields are present. - required_fields = self._assess_required_fields() + required_fields = self._assess_required_fields(is_open_source_project=is_open_source_project) for field in required_fields: if field not in self._metadata: field_name = field.get_name() @@ -249,6 +266,25 @@ class DependencyMetadata: self.get_field_line_numbers(known_fields.LICENSE_FILE)) results.append(result) + + if not is_open_source_project: + license_value = self._metadata.get(known_fields.LICENSE) + if license_value is not None: + not_allowed_licenses = self.only_open_source_licenses(license_value) + if len(not_allowed_licenses) > 0: + license_result = vr.ValidationWarning( + reason=f"License has a license not in the allowlist." + " (see https://source.chromium.org/chromium/chromium/tools/depot_tools/+/main:metadata/fields/custom/license_allowlist.py).", + additional=[ + f"The following license{'s are' if len(not_allowed_licenses) > 1 else ' is'} only allowed in open source projects: " + f"{util.quoted(not_allowed_licenses)}.", + ]) + + license_result.set_tag(tag="field", value=known_fields.LICENSE.get_name()) + license_result.set_lines( + self.get_field_line_numbers(known_fields.LICENSE)) + results.append(license_result) + return results def _return_as_property(self, field: field_types.MetadataField) -> Any: diff --git a/metadata/fields/custom/license.py b/metadata/fields/custom/license.py index bf1e161fdf..f70b2d21d9 100644 --- a/metadata/fields/custom/license.py +++ b/metadata/fields/custom/license.py @@ -18,7 +18,7 @@ sys.path.insert(0, _ROOT_DIR) import metadata.fields.field_types as field_types import metadata.fields.util as util import metadata.validation_result as vr -from metadata.fields.custom.license_allowlist import ALLOWED_LICENSES +from metadata.fields.custom.license_allowlist import ALLOWED_LICENSES, ALLOWED_OPEN_SOURCE_LICENSES def process_license_value(value: str, @@ -33,13 +33,13 @@ def process_license_value(value: str, delimiter. Returns: a list of the constituent licenses within the given value, - and whether the constituent license is on the allowlist. + and whether the constituent license is a recognized license type. e.g. [("Apache, 2.0", True), ("MIT", True), ("custom", False)] """ # Check if the value is on the allowlist as-is, and thus does not # require further processing. - if is_license_allowlisted(value): + if is_license_valid(value): return [(value, True)] breakdown = [] @@ -48,22 +48,33 @@ def process_license_value(value: str, for atomic_value in value.split(atomic_delimiter): atomic_value = atomic_value.strip() breakdown.append( - (atomic_value, is_license_allowlisted(atomic_value))) + (atomic_value, is_license_valid( + atomic_value, + )) + ) return breakdown -def is_license_allowlisted(value: str) -> bool: +def is_license_valid(value: str) -> bool: """Returns whether the value is in the allowlist for license types. """ - return value in ALLOWED_LICENSES + # The open source allowlist is the most permissive. + return value in ALLOWED_OPEN_SOURCE_LICENSES +def is_license_allowlisted(value: str, is_open_source_project: bool = False) -> bool: + """Returns whether the value is in the allowlist for license + types. + """ + if is_open_source_project: + return value in ALLOWED_OPEN_SOURCE_LICENSES + return value in ALLOWED_LICENSES class LicenseField(field_types.SingleLineTextField): """Custom field for the package's license type(s). - e.g. Apache 2.0, MIT, BSD, Public Domain. + e.g. Apache-2.0, MIT, BSD-2.0 """ def __init__(self): super().__init__(name="License") @@ -75,7 +86,8 @@ class LicenseField(field_types.SingleLineTextField): """ not_allowlisted = [] licenses = process_license_value(value, - atomic_delimiter=self.VALUE_DELIMITER) + atomic_delimiter=self.VALUE_DELIMITER, + ) for license, allowed in licenses: if util.is_empty(license): return vr.ValidationError( diff --git a/metadata/fields/custom/license_allowlist.py b/metadata/fields/custom/license_allowlist.py index 491f7491cf..7f710ed0cd 100644 --- a/metadata/fields/custom/license_allowlist.py +++ b/metadata/fields/custom/license_allowlist.py @@ -60,10 +60,6 @@ ALLOWED_SPDX_LICENSES = frozenset([ "X11", "Zlib", "libtiff", - # reciprocal. TODO(b/385020146): Only allow for opensource projects. - "APSL-2.0", - "MPL-1.1", - "MPL-2.0", ]) # These are licenses that are not in the SPDX license list, but are identified @@ -110,7 +106,7 @@ EXTENDED_LICENSE_CLASSIFIERS = frozenset([ ]) # These licenses are only allowed in open source projects due to their -# reciprocal requirements. TODO(b/385020146): Enforce this restriction. +# reciprocal requirements. OPEN_SOURCE_SPDX_LICENSES = frozenset([ # reciprocal. "APSL-2.0", diff --git a/metadata/tests/data/README.chromium.test.multi-invalid b/metadata/tests/data/README.chromium.test.multi-invalid index d723c953fb..c4a2891da0 100644 --- a/metadata/tests/data/README.chromium.test.multi-invalid +++ b/metadata/tests/data/README.chromium.test.multi-invalid @@ -4,7 +4,7 @@ URL: https://www.example.com/metadata, https://www.example.com/parser Version: 1.0.12 Date: 2020-12-03 -License: Apache-2.0 and MIT and not_an_spdx +License: Apache-2.0, MIT License File: LICENSE Security Critical: yes Shipped: yes @@ -21,7 +21,7 @@ EXCEPT: -------------------- DEPENDENCY DIVIDER -------------------- -Name: Test-B README for Chromium metadata (4 errors, 1 warning) +Name: Test-B README for Chromium metadata (3 errors, 1 warning) SHORT NAME: metadata-test-invalid URL: file://home/drive/chromium/src/metadata Version:0 @@ -34,10 +34,21 @@ Description: Local Modifications: None. +These are the expected errors (here for reference only): +1. Description is empty. + +2. Required field 'License File' is missing. + +3. URL is invalid. + +warnings: + +1. Version is '0'. + -------------------- DEPENDENCY DIVIDER -------------------- -------------------- DEPENDENCY DIVIDER -------------------- -Name: Test-C README for Chromium metadata (5 errors, 1 warning) +Name: Test-C README for Chromium metadata (4 errors, 1 warning) URL: https://www.example.com/first URL: https://www.example.com/second Version: N/A @@ -47,4 +58,19 @@ Security Critical: yes Description: Test metadata with multiple entries for one field, and -missing a mandatory field. \ No newline at end of file +missing a mandatory field. +These are the expected errors (here for reference only): + +1. Required field 'License Android Compatible' is missing. + +2. Required field 'License File' is missing. + +3. Required field 'Shipped' is missing. + +4. Repeated fields: URL (2) + +warnings: +1. License has a license not in the allowlist. +(see https://source.chromium.org/chromium/chromiu +m/tools/depot_tools/+/main:metadata/fields/custom/license_al +lowlist.py). Licenses not allowlisted: 'Custom license'. diff --git a/metadata/tests/data/README.chromium.test.reciprocal-license b/metadata/tests/data/README.chromium.test.reciprocal-license new file mode 100644 index 0000000000..a8e65f45a9 --- /dev/null +++ b/metadata/tests/data/README.chromium.test.reciprocal-license @@ -0,0 +1,7 @@ +Name: Open Sauce Package +URL: https://example.com +Version: 1.0p.3N +License: MPL-2.0 +License File: LICENSE +Security Critical: no +Shipped: yes diff --git a/metadata/tests/dependency_metadata_test.py b/metadata/tests/dependency_metadata_test.py index 87acdc4708..bec793315b 100644 --- a/metadata/tests/dependency_metadata_test.py +++ b/metadata/tests/dependency_metadata_test.py @@ -1,3 +1,4 @@ + #!/usr/bin/env vpython3 # Copyright (c) 2023 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be @@ -368,5 +369,32 @@ class DependencyValidationTest(unittest.TestCase): ) self.assertEqual(len(results), 0) + def test_all_licenses_allowlisted(self): + """Test that a single allowlisted license returns True.""" + dependency = dm.DependencyMetadata() + # "MPL-2.0" is a reciprocal license, i.e. only allowed in open source projects. + self.assertTrue(dependency.all_licenses_allowlisted("MIT", False)) + self.assertTrue(dependency.all_licenses_allowlisted("MIT, Apache-2.0", False)) + self.assertTrue(dependency.all_licenses_allowlisted("MPL-2.0", True)) + self.assertFalse(dependency.all_licenses_allowlisted("InvalidLicense", False)) + self.assertFalse(dependency.all_licenses_allowlisted("MIT, InvalidLicense", False)) + self.assertFalse(dependency.all_licenses_allowlisted("", False)) + self.assertFalse(dependency.all_licenses_allowlisted("MPL-2.0", False)) + + + def test_only_open_source_licenses(self): + """Test that only open source licenses are returned.""" + dependency = dm.DependencyMetadata() + self.assertEqual(dependency.only_open_source_licenses(""), []) + self.assertEqual(dependency.only_open_source_licenses("MIT"), []) + self.assertEqual(dependency.only_open_source_licenses("MPL-2.0"), ["MPL-2.0"]) + result = dependency.only_open_source_licenses("MIT, MPL-2.0") + self.assertEqual(result, ["MPL-2.0"]) + result = dependency.only_open_source_licenses("MPL-2.0, APSL-2.0") + self.assertEqual(set(result), {"MPL-2.0", "APSL-2.0"}) + # Test with mix of invalid and valid licenses + result = dependency.only_open_source_licenses("InvalidLicense, MPL-2.0") + self.assertEqual(result, ["MPL-2.0"]) + if __name__ == "__main__": unittest.main() diff --git a/metadata/tests/parse_test.py b/metadata/tests/parse_test.py index 8131102ee3..cc8f03ef17 100644 --- a/metadata/tests/parse_test.py +++ b/metadata/tests/parse_test.py @@ -77,7 +77,7 @@ class ParseTest(unittest.TestCase): " https://www.example.com/parser"), ("Version", "1.0.12"), ("Date", "2020-12-03"), - ('License', 'Apache-2.0 and MIT and not_an_spdx'), + ('License', 'Apache-2.0, MIT'), ("License File", "LICENSE"), ("Security Critical", "yes"), ("Shipped", "yes"), @@ -96,7 +96,7 @@ class ParseTest(unittest.TestCase): all_metadata[1].get_entries(), [ ("Name", - "Test-B README for Chromium metadata (4 errors, 1 warning)"), + "Test-B README for Chromium metadata (3 errors, 1 warning)"), ("SHORT NAME", "metadata-test-invalid"), ("URL", "file://home/drive/chromium/src/metadata"), ("Version", "0"), @@ -108,7 +108,7 @@ class ParseTest(unittest.TestCase): ("Local Modifications", "None."), ], ) - self.assertEqual((24, 35), + self.assertEqual((24, 46), all_metadata[1].get_first_and_last_line_number()) # Check repeated fields persist in the metadata's entries. @@ -116,18 +116,34 @@ class ParseTest(unittest.TestCase): all_metadata[2].get_entries(), [ ("Name", - "Test-C README for Chromium metadata (5 errors, 1 warning)"), + "Test-C README for Chromium metadata (4 errors, 1 warning)"), ("URL", "https://www.example.com/first"), ("URL", "https://www.example.com/second"), ("Version", "N/A"), ("Date", "2020-12-03"), ("License", "Custom license"), ("Security Critical", "yes"), - ("Description", "Test metadata with multiple entries for one " - "field, and\nmissing a mandatory field."), + ("Description", """Test metadata with multiple entries for one field, and +missing a mandatory field. +These are the expected errors (here for reference only): + +1. Required field 'License Android Compatible' is missing. + +2. Required field 'License File' is missing. + +3. Required field 'Shipped' is missing. + +4. Repeated fields: URL (2) + +warnings: +1. License has a license not in the allowlist. +(see https://source.chromium.org/chromium/chromiu +m/tools/depot_tools/+/main:metadata/fields/custom/license_al +lowlist.py). Licenses not allowlisted: 'Custom license'."""), + ], ) - self.assertEqual((40, 50), + self.assertEqual((51, 76), all_metadata[2].get_first_and_last_line_number()) def test_parse_multiple_local_modifications(self): diff --git a/metadata/tests/validate_test.py b/metadata/tests/validate_test.py index 72e0ba44d8..ef1c39d064 100644 --- a/metadata/tests/validate_test.py +++ b/metadata/tests/validate_test.py @@ -56,7 +56,7 @@ class ValidateContentTest(unittest.TestCase): source_file_dir=_SOURCE_FILE_DIR, repo_root_dir=_THIS_DIR, ) - self.assertEqual(len(results), 11) + self.assertEqual(len(results), 9) error_count = 0 warning_count = 0 for result in results: @@ -64,8 +64,8 @@ class ValidateContentTest(unittest.TestCase): error_count += 1 else: warning_count += 1 - self.assertEqual(error_count, 8) - self.assertEqual(warning_count, 3) + self.assertEqual(error_count, 7) + self.assertEqual(warning_count, 2) class ValidateFileTest(unittest.TestCase): @@ -94,7 +94,7 @@ class ValidateFileTest(unittest.TestCase): filepath=_INVALID_METADATA_FILEPATH, repo_root_dir=_THIS_DIR, ) - self.assertEqual(len(results), 11) + self.assertEqual(len(results), 9) error_count = 0 warning_count = 0 for result in results: @@ -102,8 +102,8 @@ class ValidateFileTest(unittest.TestCase): error_count += 1 else: warning_count += 1 - self.assertEqual(error_count, 8) - self.assertEqual(warning_count, 3) + self.assertEqual(error_count, 7) + self.assertEqual(warning_count, 2) class CheckFileTest(unittest.TestCase): @@ -141,7 +141,7 @@ class CheckFileTest(unittest.TestCase): # self.assertEqual(len(errors), 7) # self.assertEqual(len(warnings), 2) self.assertEqual(len(errors), 0) - self.assertEqual(len(warnings), 11) + self.assertEqual(len(warnings), 9) class ValidationResultTest(unittest.TestCase): @@ -219,5 +219,41 @@ class ValidationWithLineNumbers(unittest.TestCase): self.assertEqual(r.get_lines(), [13]) +class ValidateReciprocalLicenseTest(unittest.TestCase): + """Tests that validate_content handles allowing reciprocal licenses correctly.""" + def test_reciprocal_licenses(self): + # Test content with a reciprocal license (MPL-2.0). + reciprocal_license_metadata_filepath = os.path.join(_THIS_DIR, "data", + "README.chromium.test.reciprocal-license") + # Without is_open_source_project, should get a warning. + results = metadata.validate.validate_content( + content=gclient_utils.FileRead(reciprocal_license_metadata_filepath), + source_file_dir=_SOURCE_FILE_DIR, + repo_root_dir=_THIS_DIR, + is_open_source_project=False + ) + + license_errors = [] + for result in results: + if not result.is_fatal() and "License has a license not in the allowlist" in result.get_reason(): + license_errors.append(result) + + self.assertEqual(len(license_errors), 1, "Should create an error when a reciprocal license is used in a non-open source project") + + # With is_open_source_project=True, should be no warnings. + results = metadata.validate.validate_content( + content=gclient_utils.FileRead(reciprocal_license_metadata_filepath), + source_file_dir=_SOURCE_FILE_DIR, + repo_root_dir=_THIS_DIR, + is_open_source_project=True + ) + + license_errors = [] + for result in results: + if not result.is_fatal() and "License has a license not in the allowlist" in result.get_reason(): + license_errors.append(result) + + self.assertEqual(len(license_errors), 0, "Should not create an error when a reciprocal license is used in an open source project") + if __name__ == "__main__": unittest.main() diff --git a/metadata/validate.py b/metadata/validate.py index a85c8a81ed..b58fb77f45 100644 --- a/metadata/validate.py +++ b/metadata/validate.py @@ -25,8 +25,10 @@ _TRANSITION_PRESCRIPT = ( "validation is enforced.\nThird party metadata issue:") -def validate_content(content: str, source_file_dir: str, - repo_root_dir: str) -> List[vr.ValidationResult]: +def validate_content(content: str, + source_file_dir: str, + repo_root_dir: str, + is_open_source_project: bool = False) -> List[vr.ValidationResult]: """Validate the content as a metadata file. Args: @@ -37,6 +39,7 @@ def validate_content(content: str, source_file_dir: str, construct file paths to license files. repo_root_dir: the repository's root directory; this is needed to construct file paths to license files. + is_open_source_project: whether the project is open source. Returns: the validation results for the given content, sorted based severity then message. @@ -49,7 +52,10 @@ def validate_content(content: str, source_file_dir: str, for dependency in dependencies: dependency_results = dependency.validate( - source_file_dir=source_file_dir, repo_root_dir=repo_root_dir) + source_file_dir=source_file_dir, + repo_root_dir=repo_root_dir, + is_open_source_project=is_open_source_project, + ) results.extend(dependency_results) return sorted(results) @@ -68,6 +74,7 @@ def validate_file( filepath: str, repo_root_dir: str, reader: Callable[[str], Union[str, bytes]] = None, + is_open_source_project: bool = False, ) -> List[vr.ValidationResult]: """Validate the item located at the given filepath is a valid dependency metadata file. @@ -79,6 +86,8 @@ def validate_file( to construct file paths to license files. reader (optional): callable function/method to read the content of the file. + is_open_source_project: whether to allow reciprocal licenses. + This should only be True for open source projects. Returns: the validation results for the given filepath and its contents, if it exists. @@ -104,15 +113,17 @@ def validate_file( source_file_dir = os.path.dirname(filepath) return validate_content(content=content, source_file_dir=source_file_dir, - repo_root_dir=repo_root_dir) + repo_root_dir=repo_root_dir, + is_open_source_project=is_open_source_project) def check_file( filepath: str, repo_root_dir: str, reader: Callable[[str], Union[str, bytes]] = None, + is_open_source_project: bool = False, ) -> Tuple[List[str], List[str]]: - """Run metadata validation on the given filepath, and return all + """Run metadata validation on the given filepath, and return all validation errors and validation warnings. Args: @@ -122,6 +133,8 @@ def check_file( to construct file paths to license files. reader (optional): callable function/method to read the content of the file. + is_open_source_project: whether to allow reciprocal licenses. + This should only be True for open source projects. Returns: error_messages: the fatal validation issues present in the file; @@ -131,7 +144,8 @@ def check_file( """ results = validate_file(filepath=filepath, repo_root_dir=repo_root_dir, - reader=reader) + reader=reader, + is_open_source_project=is_open_source_project) error_messages = [] warning_messages = [] diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 77d6a8ae70..8c29149556 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -894,6 +894,7 @@ def CheckChromiumDependencyMetadata(input_api, output_api, file_filter=None): filepath=f.AbsoluteLocalPath(), repo_root_dir=repo_root_dir, reader=input_api.ReadFile, + is_open_source_project=True, ) for warning in warnings: