diff --git a/changelog.d/3350.fixed.md b/changelog.d/3350.fixed.md new file mode 100644 index 000000000..a342d7321 --- /dev/null +++ b/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) diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index bcaecf6b1..597f80564 100644 --- a/messagefoundry/config/settings.py +++ b/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: diff --git a/tests/test_open_mode_is_no_service.py b/tests/test_open_mode_is_no_service.py index 774ee4545..eeec0a0ce 100644 --- a/tests/test_open_mode_is_no_service.py +++ b/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) diff --git a/tests/test_security_cli.py b/tests/test_security_cli.py index a5de642b6..7a82465e3 100644 --- a/tests/test_security_cli.py +++ b/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` --