From d39281d1ed971ce6ec171db35f50d83112d65a0b Mon Sep 17 00:00:00 2001 From: Joel Natividad <1980690+jqnatividad@users.noreply.github.com> Date: Thu, 8 Oct 2026 17:11:41 -0400 Subject: [PATCH] fix(pii): honor abort in quick mode, accept any regex resource, download it per job Three PII-screening problems found in local review: - With pii_quick_screen, pii_show_candidates and pii_found_abort all on, found PII neither aborted nor produced a preview, so ingestion went on despite the abort setting. Quick screening stops at the first match and has no candidate rows to preview, so pii_show_candidates can't apply: abort. When quick screening finds PII and the job proceeds (abort off), log a warning; that case was silent too. - A custom regex resource had to have a DataStore table, though its file is downloaded, so a plain uploaded regex file was rejected. Look it up with resource_show instead; a missing resource is still a clear JobError. - The custom regexes were downloaded to user-pii-regexes.txt in the package directory, shared by every job: concurrent jobs could overwrite each other's copy mid-scan, and the package directory may not be writable. Download into the job's temp_dir. The config declarations now say how pii_show_candidates interacts with quick screening and what the regex resource must be. Co-Authored-By: Claude Opus 5.5 --- .../datapusher_plus/config_declaration.yaml | 8 ++- ckanext/datapusher_plus/pii_screening.py | 56 ++++++++++------ tests/test_pii_screening_search.py | 65 ++++++++++++++++--- 3 files changed, 98 insertions(+), 31 deletions(-) diff --git a/ckanext/datapusher_plus/config_declaration.yaml b/ckanext/datapusher_plus/config_declaration.yaml index dae7e34..129ceec 100644 --- a/ckanext/datapusher_plus/config_declaration.yaml +++ b/ckanext/datapusher_plus/config_declaration.yaml @@ -151,13 +151,17 @@ groups: editable: true default: false description: | - PII show candidates + PII show candidates. Full screening only: quick screening stops at the + first match, so it has no candidate rows to show, and + ``pii_found_abort`` then aborts regardless of this setting. - key: ckanext.datapusher_plus.pii_regex_resource_id_or_alias editable: true default: "" description: | - PII regex resource id or alias + PII regex resource id or alias: a CKAN resource whose file holds one + regex per line, used instead of the default PII regexes. Its file is + downloaded for each job, so it need not be in the DataStore. - key: ckanext.datapusher_plus.qsv_bin default: /usr/local/bin/qsvdp diff --git a/ckanext/datapusher_plus/pii_screening.py b/ckanext/datapusher_plus/pii_screening.py index 6950795..813b80f 100644 --- a/ckanext/datapusher_plus/pii_screening.py +++ b/ckanext/datapusher_plus/pii_screening.py @@ -9,6 +9,8 @@ import psycopg2 from psycopg2 import sql +import ckan.plugins.toolkit as tk + import ckanext.datapusher_plus.utils as utils import ckanext.datapusher_plus.config as conf import ckanext.datapusher_plus.datastore_utils as dsu @@ -65,28 +67,29 @@ def screen_for_pii( # a text file, with each line having a regex pattern, and an optional # label comment prefixed with "#" (e.g. #SSN, #Email, #Visa, etc.) if conf.PII_REGEX_RESOURCE_ID: - pii_regex_resource_exist = dsu.datastore_resource_exists( - conf.PII_REGEX_RESOURCE_ID - ) - if not pii_regex_resource_exist: + # Any CKAN resource will do (typically an uploaded text file): its + # file is downloaded, so it need not be in the DataStore. + try: + pii_resource = dsu.get_resource(conf.PII_REGEX_RESOURCE_ID) + except tk.ObjectNotFound: raise utils.JobError( - f"PII regex resource {conf.PII_REGEX_RESOURCE_ID!r} not found in the " - "DataStore. Check ckanext.datapusher_plus.pii_regex_resource_id_or_alias, " - "or unset it to use the default PII regexes." + f"PII regex resource {conf.PII_REGEX_RESOURCE_ID!r} not found. Check " + "ckanext.datapusher_plus.pii_regex_resource_id_or_alias, or unset it " + "to use the default PII regexes." ) - if pii_regex_resource_exist: - pii_resource = dsu.get_resource(conf.PII_REGEX_RESOURCE_ID) - pii_regex_url = pii_resource["url"] - - try: - r = requests.get(pii_regex_url) - r.raise_for_status() - pii_regex_file = pii_regex_url.split("/")[-1] - p = Path(__file__).with_name("user-pii-regexes.txt") - with p.open("wb") as f: - f.write(r.content) - except requests.RequestException as e: - raise utils.JobError(f"Failed to fetch PII regex resource: {e}") + pii_regex_url = pii_resource["url"] + + try: + r = requests.get(pii_regex_url) + r.raise_for_status() + pii_regex_file = pii_regex_url.split("/")[-1] + # Per job: concurrent jobs must not share (and overwrite) one copy, + # and the package directory may not be writable. + p = Path(temp_dir) / "user-pii-regexes.txt" + with p.open("wb") as f: + f.write(r.content) + except requests.RequestException as e: + raise utils.JobError(f"Failed to fetch PII regex resource: {e}") else: pii_regex_file = "default-pii-regexes.txt" p = Path(__file__).with_name(pii_regex_file) @@ -148,7 +151,12 @@ def screen_for_pii( pii_found = True pii_candidate_count = pii_total_matches - if pii_found and pii_found_abort and not conf.PII_SHOW_CANDIDATES: + # Quick screening finds only the first match, so there are no candidate + # rows to preview: pii_show_candidates can't apply, and must not stop + # pii_found_abort from aborting. + if pii_found and pii_found_abort and ( + conf.PII_QUICK_SCREEN or not conf.PII_SHOW_CANDIDATES + ): logger.error("PII Candidate/s Found!") if conf.PII_QUICK_SCREEN: raise utils.JobError( @@ -286,6 +294,12 @@ def screen_for_pii( logger.warning( "PII CANDIDATE/S FOUND but proceeding with job per Datapusher+ configuration." ) + elif pii_found and conf.PII_QUICK_SCREEN: + logger.warning( + "PII CANDIDATE FOUND on row %s, but proceeding with job per Datapusher+ " + "configuration.", + pii_candidate_row.rstrip(), + ) elif not pii_found: logger.info("PII Scan complete. No PII candidate/s found.") diff --git a/tests/test_pii_screening_search.py b/tests/test_pii_screening_search.py index 4c73968..2975949 100644 --- a/tests/test_pii_screening_search.py +++ b/tests/test_pii_screening_search.py @@ -51,18 +51,19 @@ def screen(tmp_path, monkeypatch): monkeypatch.chdir(tmp_path) - def run(csv_text, quick): + def run(csv_text, quick, abort=True, show_candidates=False, regex_resource=None, + logger=None): csv_path = tmp_path / "data.csv" csv_path.write_text(csv_text) conf = pii_screening.conf with mock.patch.object(qsv_utils.conf, "QSV_BIN", Path(QSV_BIN)), \ - mock.patch.object(conf, "PII_REGEX_RESOURCE_ID", None), \ + mock.patch.object(conf, "PII_REGEX_RESOURCE_ID", regex_resource), \ mock.patch.object(conf, "PII_QUICK_SCREEN", quick), \ - mock.patch.object(conf, "PII_FOUND_ABORT", True), \ - mock.patch.object(conf, "PII_SHOW_CANDIDATES", False): + mock.patch.object(conf, "PII_FOUND_ABORT", abort), \ + mock.patch.object(conf, "PII_SHOW_CANDIDATES", show_candidates): qsv = qsv_utils.QSVCommand(logger=mock.Mock()) return pii_screening.screen_for_pii( - str(csv_path), {"id": "res"}, qsv, str(tmp_path), mock.Mock() + str(csv_path), {"id": "res"}, qsv, str(tmp_path), logger or mock.Mock() ) return run @@ -136,14 +137,62 @@ def test_is_searchset_no_match(returncode, stderr, expected): def test_a_missing_custom_regex_resource_is_a_clear_job_error(tmp_path): - """A configured regex resource that isn't in the DataStore used to leave - the regex path unbound and crash with NameError.""" + """A configured regex resource that doesn't exist used to leave the regex + path unbound and crash with NameError.""" pytest.importorskip("ckan") + import ckan.plugins.toolkit as tk + from ckanext.datapusher_plus import pii_screening, utils with mock.patch.object(pii_screening.conf, "PII_REGEX_RESOURCE_ID", "no-such-resource"), \ - mock.patch.object(pii_screening.dsu, "datastore_resource_exists", return_value=None), \ + mock.patch.object(pii_screening.dsu, "get_resource", side_effect=tk.ObjectNotFound), \ pytest.raises(utils.JobError, match="PII regex resource 'no-such-resource' not found"): pii_screening.screen_for_pii( str(tmp_path / "data.csv"), {"id": "res"}, mock.Mock(), str(tmp_path), mock.Mock() ) + + +@requires_qsv +@pytest.mark.parametrize("quick", [True, False], ids=["quick", "full"]) +def test_a_custom_regex_resource_need_not_be_in_the_datastore(screen, quick, tmp_path): + """A plain uploaded regex file works: it is fetched with resource_show and + downloaded into the job's own temp dir, not into the shared package dir.""" + from ckanext.datapusher_plus import pii_screening, utils + + response = mock.Mock(content=b"^Bob$\n") + package_copy = Path(pii_screening.__file__).with_name("user-pii-regexes.txt") + with mock.patch.object( + pii_screening.dsu, "get_resource", + return_value={"url": "https://ckan.example/download/regexes.txt"}, + ) as get_resource, \ + mock.patch.object(pii_screening.dsu, "datastore_resource_exists") as in_datastore, \ + mock.patch.object(pii_screening.requests, "get", return_value=response), \ + pytest.raises(utils.JobError, match="PII CANDIDATE"): + screen(CLEAN, quick, regex_resource="custom-regexes") + + get_resource.assert_called_once_with("custom-regexes") + in_datastore.assert_not_called() + assert (tmp_path / "user-pii-regexes.txt").read_bytes() == b"^Bob$\n" + assert not package_copy.exists() + + +@requires_qsv +def test_quick_screen_aborts_even_when_candidates_would_be_shown(screen): + """Quick screening has no candidate rows to preview, so + pii_show_candidates must not stop pii_found_abort from aborting.""" + from ckanext.datapusher_plus import utils + + with pytest.raises(utils.JobError, match=r"PII CANDIDATE FOUND on row \d+! Job aborted\."): + screen(WITH_SSN, quick=True, abort=True, show_candidates=True) + + +@requires_qsv +@pytest.mark.parametrize("show_candidates", [True, False]) +def test_quick_screen_without_abort_warns_and_proceeds(screen, show_candidates): + logger = mock.Mock() + assert screen( + WITH_SSN, quick=True, abort=False, show_candidates=show_candidates, logger=logger + ) == (True, 1) + assert any( + "PII CANDIDATE FOUND on row" in str(c.args[0]) for c in logger.warning.call_args_list + )