diff --git a/scripts/requires_cla.py b/scripts/requires_cla.py index a2b8830..0d85405 100644 --- a/scripts/requires_cla.py +++ b/scripts/requires_cla.py @@ -218,7 +218,17 @@ def get_license_decision(repo_full: str, token: Optional[str] = None, api_base_u def requires_CLA(repo_full: str, token: Optional[str] = None, api_base_url: str = "https://api.github.com", timeout_s: int = 20, licenses_data=None, permissive_data=None, allowlist_data=None) -> bool: res = get_license_decision(repo_full, token, api_base_url, timeout_s, licenses_data, permissive_data, allowlist_data) - return bool(res.get("requires_CLA", True)) + # Only an explicit False means DCO. get_license_decision reports None when + # it identified a licence but could not decide whether it is permissive + # (an SPDX expression that does not parse, for example), and bool(None) is + # False - so an undecided licence used to be downgraded to DCO. The + # paths that fail to identify a licence at all already return True; this + # makes the identified-but-undecided path agree with them. + # + # None is kept in get_license_decision itself on purpose: the org licence + # reports read it as "unknown", which is the honest answer there. This + # facade is the enforcement boundary, so this is where it must fail closed. + return res.get("requires_CLA") is not False if __name__ == "__main__": import sys, json diff --git a/tests/test_policy_selector.py b/tests/test_policy_selector.py index 1b2c960..cbec5af 100644 --- a/tests/test_policy_selector.py +++ b/tests/test_policy_selector.py @@ -1004,6 +1004,117 @@ def test_repositories_scalar_does_not_explode_into_characters(self): "a bare string must be rejected, not iterated per character") +@unittest.skipIf(requires_cla is None, "requires_cla unavailable (optional dep missing)") +class TestRequiresClaFailsClosed(unittest.TestCase): + """requires_CLA() is the only thing the gate asks, and it used to return + bool(res.get("requires_CLA", True)). get_license_decision reports None + when it identified a licence but could not decide whether it is + permissive, and bool(None) is False - so an undecided licence was quietly + downgraded to DCO, the weaker document. + + Every other test in this file replaces requires_CLA wholesale, so the + facade itself had no coverage at all. These stub one layer lower, at + get_license_decision (or at the single network call inside it), so the + real facade runs. + """ + + # A non-empty dict, so _load_allowlist uses it rather than falling through + # to the real cla/allowlist.yml on disk - and with no overrides, so nothing + # here depends on what that file currently says. + NO_OVERRIDES = {"license_overrides": {"require_cla": [], "allow_dco": []}} + + def setUp(self): + saved_decision = requires_cla.get_license_decision + saved_api = requires_cla.dorl.get_repo_license_api + self.addCleanup(setattr, requires_cla, "get_license_decision", saved_decision) + self.addCleanup(setattr, requires_cla.dorl, "get_repo_license_api", saved_api) + + def _decision(self, value): + requires_cla.get_license_decision = lambda *a, **k: value + return requires_cla.requires_CLA("vmware/repo") + + def test_undecided_licence_requires_cla(self): + """The defect.""" + self.assertIs(self._decision({"requires_CLA": None}), True) + + def test_explicit_false_still_means_dco(self): + """Guard against over-correcting: a permissive licence must stay DCO, + or every permissive repo in the org would start asking for a CLA.""" + self.assertIs(self._decision({"requires_CLA": False}), False) + + def test_explicit_true_still_means_cla(self): + self.assertIs(self._decision({"requires_CLA": True}), True) + + def test_missing_key_requires_cla(self): + """Unchanged behaviour, pinned so a rewrite can't lose it.""" + self.assertIs(self._decision({}), True) + + def test_the_detector_really_returns_none_for_an_unparseable_expression(self): + """Proves the None path exists in shipped code, so the tests above are + not guarding something hypothetical. An SPDX expression that parses to + no clauses is the one input that reaches it.""" + for expr in ("WITH", "()"): + with self.subTest(expr=expr): + self.assertEqual( + requires_cla.ld.is_permissive_with_reason(expr, expr, [], []), + (None, "expr_empty_or_unparsed")) + + def test_undecided_licence_from_the_api_requires_cla_end_to_end(self): + """Runs the real get_license_decision with only the GitHub call faked: + the repo reports a licence whose SPDX id is an expression that does + not parse. Also pins that the decision record still says None - the + org licence reports read that as "unknown", and the fix deliberately + lives in the facade rather than erasing that information.""" + async def fake_license_api(session, owner, repo): + return {"license": {"spdx_id": "WITH", "name": "WITH"}} + requires_cla.dorl.get_repo_license_api = fake_license_api + kwargs = dict(licenses_data=[], permissive_data=[], allowlist_data=self.NO_OVERRIDES) + + decision = requires_cla.get_license_decision("vmware/repo", **kwargs) + self.assertIsNone(decision["requires_CLA"]) + self.assertEqual(decision["policy_reason"], "expr_empty_or_unparsed") + + self.assertIs(requires_cla.requires_CLA("vmware/repo", **kwargs), True) + + def test_permissive_licence_from_the_api_stays_dco_end_to_end(self): + """Positive control for the test above: the same path with a licence + that is genuinely permissive must still come out DCO, which shows the + end-to-end test can tell the two cases apart.""" + async def fake_license_api(session, owner, repo): + return {"license": {"spdx_id": "MIT", "name": "MIT License"}} + requires_cla.dorl.get_repo_license_api = fake_license_api + self.assertIs(requires_cla.requires_CLA( + "vmware/repo", licenses_data=[], permissive_data=[], + allowlist_data=self.NO_OVERRIDES), False) + + +@unittest.skipIf(requires_cla is None, "requires_cla unavailable (optional dep missing)") +class TestGateFailsClosedOnAnUndecidedLicence(ProcessSinglePrHarness): + """The same defect seen from the gate: process_single_pr with the real + requires_CLA facade in place (the harness stubs it out by default), and + only get_license_decision faked.""" + + def setUp(self): + super().setUp() + policy_selector.requires_cla.requires_CLA = self._saved_requires + saved = policy_selector.requires_cla.get_license_decision + self.addCleanup(setattr, policy_selector.requires_cla, "get_license_decision", saved) + + def _run_with_decision(self, value): + policy_selector.requires_cla.get_license_decision = lambda *a, **k: {"requires_CLA": value} + return self.run_pr(paginated_routes={"/issues/5/comments": [], "/pulls/5/commits": []}) + + def test_undecided_licence_asks_for_a_cla(self): + fake = self._run_with_decision(None) + self.assertEqual(fake.statuses()[0]["description"], "CLA Missing") + + def test_permissive_licence_still_asks_for_a_dco(self): + """Positive control: shows the real facade is wired in, not a stub + that always says CLA.""" + fake = self._run_with_decision(False) + self.assertEqual(fake.statuses()[0]["description"], "DCO Missing") + + class TestOverrideMatchesTheRealCatalogue(unittest.TestCase): """Guard the seam between two files that must agree but are spelled differently.