diff --git a/.serena/memories/codebase_structure.md b/.serena/memories/codebase_structure.md index 58de9bb..dd3c431 100644 --- a/.serena/memories/codebase_structure.md +++ b/.serena/memories/codebase_structure.md @@ -19,7 +19,6 @@ ├── setup.py / setup.cfg / MANIFEST.in ├── requirements.txt / requirements-dev.txt ├── dot-env.template # Sample env vars -├── default-pii-regexes.txt # Default PII regex patterns ├── test_config.py # Tests configuration helper ├── wsgi.py # WSGI entry shim ├── .coveragerc # source = ckanext/datapusher_plus (fixed in PR/branch diff --git a/MANIFEST.in b/MANIFEST.in index 70cc7f2..0bbf632 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -1,5 +1,5 @@ include README.md include LICENSE include requirements.txt -recursive-include ckanext/datapusher_plus *.html *.json *.js *.less *.css *.mo *.yaml +recursive-include ckanext/datapusher_plus *.html *.json *.js *.less *.css *.mo *.yaml default-pii-regexes.txt recursive-include ckanext/datapusher_plus/migration *.ini *.py *.mako diff --git a/README.md b/README.md index 0743fdd..fc1930f 100644 --- a/README.md +++ b/README.md @@ -126,7 +126,7 @@ It features: * auto-index columns based on its cardinality/format (unique indices created for columns with all unique values, auto-index columns whose cardinality is below a given threshold; auto-index date columns) * check for duplicates, and optionally deduplicate rows * optionally screen for Personally Identifiable Information (PII), with an option to "quarantine" the PII-candidate rows in a separate resource, while still creating the screened resource. - * optional ability to specify a custom PII screening regex set, instead of the [default PII screening regex set](https://github.com/dathere/datapusher-plus/blob/master/default-pii-regexes.txt). + * optional ability to specify a custom PII screening regex set, instead of the [default PII screening regex set](https://github.com/dathere/datapusher-plus/blob/main/ckanext/datapusher_plus/default-pii-regexes.txt). Even with all these pre-processing tasks, qsv typically takes less than 5 seconds to finish all its analysis tasks, even for a 100mb CSV file. diff --git a/default-pii-regexes.txt b/ckanext/datapusher_plus/default-pii-regexes.txt similarity index 100% rename from default-pii-regexes.txt rename to ckanext/datapusher_plus/default-pii-regexes.txt diff --git a/ckanext/datapusher_plus/pii_screening.py b/ckanext/datapusher_plus/pii_screening.py index 7efe062..6950795 100644 --- a/ckanext/datapusher_plus/pii_screening.py +++ b/ckanext/datapusher_plus/pii_screening.py @@ -15,6 +15,25 @@ from ckanext.datapusher_plus.qsv_utils import QSVCommand +def _is_searchset_no_match(result) -> bool: + """True if a ``qsv searchset`` run failed only because nothing matched. + + qsv exits 1 for "no match" and prints nothing, or, when asked for + structured errors (``QSV_ERROR_FORMAT=json``), a single JSON error line + of kind ``no_match``. Any other failure prints an error message. + """ + if result.returncode != 1: + return False + stderr = (result.stderr or "").strip() + if not stderr: + return True + try: + err = json.loads(stderr.splitlines()[-1]).get("error", {}) + except (ValueError, AttributeError): + return False + return isinstance(err, dict) and err.get("kind") == "no_match" + + def screen_for_pii( tmp: str, resource: dict, @@ -49,6 +68,12 @@ def screen_for_pii( pii_regex_resource_exist = dsu.datastore_resource_exists( conf.PII_REGEX_RESOURCE_ID ) + if not pii_regex_resource_exist: + 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." + ) if pii_regex_resource_exist: pii_resource = dsu.get_resource(conf.PII_REGEX_RESOURCE_ID) pii_regex_url = pii_resource["url"] @@ -70,6 +95,8 @@ def screen_for_pii( # Count of PII candidate matches, surfaced to the caller so the # v3.0 PII-review suspend gate has a real number to threshold on. pii_candidate_count = 0 + # Both modes search the same file. It must be an absolute path: the + # worker's current directory is not the package directory. pii_regex_fname = p.absolute() if conf.PII_QUICK_SCREEN: @@ -80,21 +107,31 @@ def screen_for_pii( tmp, ignore_case=True, quick=True, + check=False, ) except utils.JobError as e: - raise utils.JobError("Cannot quickly search CSV for PII: %s", e) - pii_candidate_row = str(qsv_searchset.stderr) - if pii_candidate_row: + raise utils.JobError(f"Cannot quickly search CSV for PII: {e}") + # Exit 0: a match, with its row number on stderr. Exit 1 with no + # error message: no match, so no PII. (``--not-one`` can't be used: + # with it, qsv prints the row count to stderr even when nothing + # matched, which reads as a match.) + pii_candidate_row = "" + if qsv_searchset.returncode == 0: + pii_candidate_row = str(qsv_searchset.stderr) pii_found = True # Quick screen only detects presence, not a count. pii_candidate_count = 1 + elif not _is_searchset_no_match(qsv_searchset): + raise utils.JobError( + f"Cannot quickly search CSV for PII: {qsv_searchset.stderr}" + ) else: logger.info("Scanning for PII using %s...", pii_regex_file) qsv_searchset_csv = os.path.join(temp_dir, "qsv_searchset.csv") try: qsv_searchset = qsv.searchset( - pii_regex_file, + pii_regex_fname, tmp, ignore_case=True, flag="PII_info", @@ -103,7 +140,7 @@ def screen_for_pii( output_file=qsv_searchset_csv, ) except utils.JobError as e: - raise utils.JobError("Cannot search CSV for PII: %s", e) + raise utils.JobError(f"Cannot search CSV for PII: {e}") pii_json = json.loads(str(qsv_searchset.stderr)) pii_total_matches = int(pii_json["total_matches"]) pii_rows_with_matches = int(pii_json["rows_with_matches"]) @@ -115,14 +152,12 @@ def screen_for_pii( logger.error("PII Candidate/s Found!") if conf.PII_QUICK_SCREEN: raise utils.JobError( - "PII CANDIDATE FOUND on row %s! Job aborted.", - pii_candidate_row.rstrip(), + f"PII CANDIDATE FOUND on row {pii_candidate_row.rstrip()}! Job aborted." ) else: raise utils.JobError( - "PII CANDIDATE/S FOUND! Job aborted. Found %d PII candidate/s in %d row/s.", - pii_total_matches, - pii_rows_with_matches, + f"PII CANDIDATE/S FOUND! Job aborted. Found {pii_total_matches} PII " + f"candidate/s in {pii_rows_with_matches} row/s." ) elif pii_found and conf.PII_SHOW_CANDIDATES and not conf.PII_QUICK_SCREEN: # TODO: Create PII Candidates resource and set package to private if its not private @@ -172,7 +207,7 @@ def screen_for_pii( output_file=None, ) except utils.JobError as e: - raise utils.JobError("Cannot run stats on PII preview CSV: %s", e) + raise utils.JobError(f"Cannot run stats on PII preview CSV: {e}") pii_stats = str(qsv_pii_stats.stdout).strip() pii_stats_dict = [ @@ -223,13 +258,13 @@ def screen_for_pii( try: cur_pii.copy_expert(copy_sql, f) except psycopg2.Error as e: - raise utils.JobError("Postgres COPY failed: %s", e) + raise utils.JobError(f"Postgres COPY failed: {e}") else: pii_copied_count = cur_pii.rowcount raw_connection_pii.commit() except psycopg2.Error as e: - raise utils.JobError("Could not connect to the Datastore: %s", e) + raise utils.JobError(f"Could not connect to the Datastore: {e}") pii_resource["id"] = new_pii_resource_id pii_resource["pii_preview"] = True diff --git a/ckanext/datapusher_plus/qsv_utils.py b/ckanext/datapusher_plus/qsv_utils.py index 8029b32..4424920 100644 --- a/ckanext/datapusher_plus/qsv_utils.py +++ b/ckanext/datapusher_plus/qsv_utils.py @@ -847,6 +847,7 @@ def searchset( flag_matches_only: bool = False, json_output: bool = False, output_file: Optional[str] = None, + check: bool = True, ) -> subprocess.CompletedProcess: """ Search a CSV file for patterns defined in a regex file. @@ -860,6 +861,9 @@ def searchset( flag_matches_only: Whether to only output matching rows json_output: Whether to output JSON output_file: Path to the output file + check: Raise on a non-zero exit. qsv exits 1 when nothing + matches (outside JSON mode), so callers that treat "no + match" as a result pass False and read ``returncode``. Returns: The result of the command @@ -887,4 +891,4 @@ def searchset( if output_file: args.extend(["--output", output_file]) - return self._run_command(args) + return self._run_command(args, check=check) diff --git a/tests/test_pii_screening_search.py b/tests/test_pii_screening_search.py new file mode 100644 index 0000000..4c73968 --- /dev/null +++ b/tests/test_pii_screening_search.py @@ -0,0 +1,149 @@ +# -*- coding: utf-8 -*- +"""``screen_for_pii`` against a real qsv, with the default regex set. + +Regression coverage for three bugs that together kept PII screening from +working with its default configuration: + +* the default regex file lived at the repo root while the code looked for + it next to ``pii_screening.py`` (quick mode always failed), and full mode + passed a bare file name that only resolved when the worker's current + directory was the repo root; +* ``searchset --quick`` exits 1 when nothing matches, so a quick screen of + a file with no PII failed the job; +* ``JobError`` was called with logging-style arguments, which raised + ``TypeError`` instead. + +The tests run from a temporary directory, as a worker would, so a relative +regex path cannot pass by accident. +""" + +import os +import shutil +from pathlib import Path +from unittest import mock + +import pytest + + +def _locate_qsv(): + for c in (os.environ.get("QSV_BIN"), shutil.which("qsvdp"), shutil.which("qsv")): + if c and Path(c).is_file(): + return c + return None + + +QSV_BIN = _locate_qsv() +requires_qsv = pytest.mark.skipif(QSV_BIN is None, reason="needs a qsv binary (QSV_BIN)") + + +def test_default_regex_file_ships_next_to_the_module(): + pytest.importorskip("ckan") + from ckanext.datapusher_plus import pii_screening + + assert Path(pii_screening.__file__).with_name("default-pii-regexes.txt").is_file() + + +@pytest.fixture +def screen(tmp_path, monkeypatch): + """Run ``screen_for_pii`` on CSV text, in quick or full mode.""" + pytest.importorskip("ckan") + from ckanext.datapusher_plus import pii_screening, qsv_utils + + monkeypatch.chdir(tmp_path) + + def run(csv_text, quick): + 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_QUICK_SCREEN", quick), \ + mock.patch.object(conf, "PII_FOUND_ABORT", True), \ + mock.patch.object(conf, "PII_SHOW_CANDIDATES", False): + qsv = qsv_utils.QSVCommand(logger=mock.Mock()) + return pii_screening.screen_for_pii( + str(csv_path), {"id": "res"}, qsv, str(tmp_path), mock.Mock() + ) + + return run + + +CLEAN = "name,city\nAlice,NYC\nBob,LA\n" +WITH_SSN = "name,ssn\nAlice,none\nBob,123-45-6789\n" + + +@requires_qsv +@pytest.mark.parametrize("quick", [True, False], ids=["quick", "full"]) +def test_a_file_without_pii_passes(screen, quick): + assert screen(CLEAN, quick) == (False, 0) + + +@requires_qsv +def test_quick_screen_aborts_on_pii_with_the_row(screen): + 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) + + +@requires_qsv +def test_full_screen_aborts_on_pii_with_the_counts(screen): + from ckanext.datapusher_plus import utils + + with pytest.raises( + utils.JobError, match=r"Found 1 PII candidate/s in 1 row/s\." + ): + screen(WITH_SSN, quick=False) + + +@requires_qsv +@pytest.mark.parametrize( + "quick, message", + [(True, "Cannot quickly search CSV for PII"), (False, "Cannot search CSV for PII")], + ids=["quick", "full"], +) +def test_a_failed_search_raises_job_error(screen, quick, message, tmp_path): + from ckanext.datapusher_plus import pii_screening, utils + + # a regex file qsv cannot read makes searchset itself fail + missing = tmp_path / "missing-regexes.txt" + with mock.patch.object(pii_screening.Path, "absolute", return_value=missing), \ + pytest.raises(utils.JobError, match=message): + screen(CLEAN, quick) + + +@pytest.mark.parametrize( + "returncode, stderr, expected", + [ + (1, "", True), # qsv's text mode prints nothing for "no match" + (1, "\n", True), + (1, '{"error":{"kind":"no_match","level":"error","message":"","exit_code":1}}\n', True), + (1, 'a warning\n{"error":{"kind":"no_match","exit_code":1}}', True), + (1, '{"error":{"kind":"io","message":"No such file","exit_code":1}}', False), + (1, "io error: No such file or directory (os error 2)", False), + (0, "", False), # a success is not a "no match" + (2, "", False), + ], +) +def test_is_searchset_no_match(returncode, stderr, expected): + pytest.importorskip("ckan") + import subprocess + + from ckanext.datapusher_plus.pii_screening import _is_searchset_no_match + + result = subprocess.CompletedProcess(args=[], returncode=returncode, stdout="", stderr=stderr) + assert _is_searchset_no_match(result) is 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.""" + pytest.importorskip("ckan") + 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), \ + 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() + )