security show no longer says no environment variable set a removed key (BACKLOG #3350)

changelog.d/3350.fixed.md

@@ -0,0 +1,9 @@
+- **A removed key's refusal no longer says that no environment variable set it.** The refusal a
+  section built directly gives for a removed key said the key was passed straight to the section,
+  so no environment variable set it. That section is built in code, and also from the file's
+  `[security]` table by at least `security show` and `security set`, so an operator whose file
+  still held `require_sign_in` or `handles_real_patient_data` would read that claim. It held for
+  the file the command read, not for the service, whose environment may set the same key and
+  would still refuse at start. The refusal now says only to remove the key from the arguments or
+  the file table, and no longer claims that no environment variable set it. The loader's refusal
+  still names the `MEFOR_` variable to unset. (vault BACKLOG #3350)

messagefoundry/config/settings.py

@@ -7941,15 +7941,13 @@ def _removed_key_message(
     """The refusal for a removed key: the decision (``reason``), then how to remove the key.
 
     The removal step depends on where the key came from (vault BACKLOG #3216). The loader reads a
-    file and the environment, so it names both. A section built directly reads neither: code
-    passed the key (the loader's own ``cli`` overrides count as code), or ``security show``
-    read it from the file's table. Naming an environment
-    variable there would send the reader to a place the key cannot be."""
+    file and the environment, so it names both. A section built directly reads no environment:
+    code passed the key (the loader's own ``cli`` overrides count as code), or at least
+    ``security show`` and ``security set`` passed it from the file's table. Its refusal names no
+    variable, and makes no claim that none set the key (vault BACKLOG #3350): ``security show``
+    prints it for a key in the file while the service's environment may set the same key too."""
     if built_directly:
-        fix = (
-            f"The key was passed straight to the section, so no environment variable set it. "
-            f"Remove `{key}` from the arguments or the file table the section was built from"
-        )
+        fix = f"Remove `{key}` from the arguments or the file table the section was built from"
         if hint := _REMOVED_KEY_BUILT_DIRECTLY_HINT.get((section, key)):
             fix = f"{fix}. {hint}"
     else:

tests/test_open_mode_is_no_service.py

@@ -239,8 +239,11 @@ def test_each_refusal_names_only_the_places_the_key_could_be(section: str, key:
     with pytest.raises(ValueError) as built:
         _section_models()[section].model_validate({key: False})
     assert variable not in str(built.value)
-    assert "no environment variable set it" in str(built.value)
-    assert f"Remove `{key}`" in str(built.value)
+    # Nor does it say that no variable set the key (vault BACKLOG #3350): the service's
+    # environment may set it beside the file `security show` read. The removal step follows the
+    # decision directly, so no claim about where the key came from sits between them.
+    assert f"{reason}. Remove `{key}` from" in str(built.value)
+    assert "environment variable" not in str(built.value).lower()
     # The loader's own refusal, from the environment: it names the variable that set the key.
     with pytest.raises(ValueError, match=variable):
         load_settings(environ={variable: "false"}, default_file=False)

tests/test_security_cli.py

@@ -14,6 +14,7 @@
 import pytest
 
 from messagefoundry.__main__ import main
+from messagefoundry.config.settings import _ENV_PREFIX, _REMOVED_KEYS
 
 
 def _show(path: Path, capsys: pytest.CaptureFixture[str]) -> dict:
@@ -368,6 +369,69 @@ def test_show_names_the_stray_variable_that_made_the_report_partial(
     assert "SYNTHETIC-VALUE-9f3c" not in json.dumps(out_data)
 
 
+# --- a removed key the environment may still set (vault BACKLOG #3350) ------------------------
+
+
+#: ``security show`` builds only ``[security]`` straight from the file's table, so only these keys
+#: reach the direct-build refusal through it. It reports every other removed key through the
+#: loader's refusal.
+_SECURITY_REMOVED_KEYS = sorted(key for section, key in _REMOVED_KEYS if section == "security")
+# An empty list would turn both tests below into a silent skip.
+assert _SECURITY_REMOVED_KEYS, "no removed [security] key left to test"
+
+
+def _show_refusal(path: Path, capsys: pytest.CaptureFixture[str]) -> str:
+    assert main(["security", "show", "--service-config", str(path), "--json"]) == 1
+    error = json.loads(capsys.readouterr().out)["error"]
+    assert isinstance(error, str)
+    return error
+
+
+@pytest.mark.parametrize("key", _SECURITY_REMOVED_KEYS)
+def test_show_makes_no_claim_about_where_a_removed_key_was_not_set(
+    key: str,
+    tmp_path: Path,
+    capsys: pytest.CaptureFixture[str],
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """The key is in the file, and the run is repeated with the environment setting it too.
+
+    The refusal is built from the file's table alone, so it is the same text in both runs. It used
+    to add "no environment variable set it", which would send an operator away from a variable the
+    service would still refuse at start. The removal step is pinned whole, so a provenance claim
+    placed before it, inside it or after it fails."""
+    toml = tmp_path / "mf.toml"
+    toml.write_text(f"[security]\n{key} = false\n", encoding="utf-8")
+    file_only = _show_refusal(toml, capsys)
+    monkeypatch.setenv(f"{_ENV_PREFIX}SECURITY_{key.upper()}", "false")
+    file_and_env = _show_refusal(toml, capsys)
+    assert file_and_env == file_only
+    assert file_only.startswith("invalid [security]")
+    assert (
+        f"{_REMOVED_KEYS['security', key]}. Remove `{key}` from the arguments or the file table "
+        "the section was built from (see docs/CONFIGURATION.md)."
+    ) in file_only
+    assert "environment variable" not in file_only.lower()
+
+
+@pytest.mark.parametrize("key", _SECURITY_REMOVED_KEYS)
+def test_show_names_the_variable_that_set_a_removed_key(
+    key: str,
+    tmp_path: Path,
+    capsys: pytest.CaptureFixture[str],
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """With the key in the environment only, the file builds and the partial reason is the
+    loader's refusal, which names the variable to unset."""
+    toml = tmp_path / "mf.toml"
+    toml.write_text("[auth]\nlockout_minutes = 15\n", encoding="utf-8")
+    variable = f"{_ENV_PREFIX}SECURITY_{key.upper()}"
+    monkeypatch.setenv(variable, "false")
+    data = _show(toml, capsys)
+    assert data["loosenings_partial"] is True
+    assert variable in data["loosenings_partial_reason"]
+
+
 # --- operator JSON that nests past the decoder (BACKLOG #1855) --------------------------------
 #
 # `json.loads` guards its own decode depth and raises `RecursionError`, which is a `RuntimeError` --