fix(pii): honor abort in quick mode, accept any regex resource, download it per job

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

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.")
 

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
+    )