From 728e94e84ab4ce4e3b9d69ac40f6a7b17596274a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 11:08:09 +0000 Subject: [PATCH] Remember which keyring backend was found, instead of searching every run Left to itself, keyring works out which backend to use by loading every backend that every installed package registers and keeping the best of them. That search runs again on every invocation and is the single most expensive thing a keycmd run does: measured here it is the difference between 0.156s and 0.085s, and it grows with the number of packages installed and with any backend that takes its time deciding it is not viable. The answer, though, is the same every time until the packages on the machine change. So the new backend.py writes it down the first time a run needs a credential and loads that backend by name afterwards, which is the shortcut PYTHON_KEYRING_BACKEND buys without anyone having to know the variable exists. Nothing to read, nothing to set: the second run is simply faster than the first. The note goes where the platform keeps files a program can afford to lose -- %LOCALAPPDATA% on windows, ~/Library/Caches on macOS, $XDG_CACHE_HOME on linux -- and holds one line, and is trusted only as far as it can be checked: - is_backend_name keeps anything that is not a dotted class name from reaching an import, so a file that has been truncated or scribbled in is worth no more than a search - a name that no longer loads sends the run back to searching and is replaced. load_keyring asks the class for its priority on the way, which is how keyring itself decides a backend is viable, so an uninstalled backend and one whose daemon stopped are both caught - backend_name looks through the chainer, which is not a backend but the search wearing one's clothes; writing that down would leave the search in place, so the backend it would have reached first is written instead - PYTHON_KEYRING_BACKEND outranks the note and is never written over - a search that found nothing is not an answer, and is not remembered What keycmd cannot notice by itself is a backend that still loads but is no longer the one you want, so the two steps are also available on purpose: keycmd --detect-backend # search now, and remember what turns up keycmd --reset-backend # forget it, so the next run searches again Two ways of ending up with no backend used to reach the user as a traceback and are now errors that say what to do: keyring settling on fail.Keyring, which raises on the first lookup and which inside a distro points at the WSL section of the README, and a PYTHON_KEYRING_BACKEND that cannot be loaded. logs.error grew the hint lines those need. keyring stays out of the import path of a run that looks up no credential: backend.py reaches for it inside the functions that need it, the way creds.py used to, and its own name only appears at module level under TYPE_CHECKING. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CRYugGcVh3wvC5a5pzwKU3 --- CLAUDE.md | 8 +- README.md | 64 +++++++-- keycmd/backend.py | 290 ++++++++++++++++++++++++++++++++++++++++ keycmd/cli.py | 22 +++ keycmd/creds.py | 16 +-- keycmd/logs.py | 11 +- tests/conftest.py | 15 +++ tests/test_backend.py | 304 ++++++++++++++++++++++++++++++++++++++++++ tests/test_cli.py | 52 ++++++++ tests/test_logs.py | 10 ++ 10 files changed, 768 insertions(+), 24 deletions(-) create mode 100644 keycmd/backend.py create mode 100644 tests/test_backend.py diff --git a/CLAUDE.md b/CLAUDE.md index 30b98a8..908be44 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -29,15 +29,19 @@ Things that bite in this suite: - **Never assume a shell.** The `shell` fixture in `tests/conftest.py` parametrizes over every shell of the platform that is installed, so a test using it runs three times. Ask the `Shell` object for the dialect (`env_var`, `unset_env_var`, `command_not_found_statuses`) instead of branching on the platform. Shells that are not installed locally are covered by asserting on the command line keycmd builds for them. - **`wsl.exe` mangles its command line**: backslashes disappear and quotes are stripped before the distribution sees them. Pass paths translated to `/mnt/...` by `wsl_path`, unquoted and free of spaces, and keep remote scripts on one line. +- **The remembered backend is redirected, always.** The autouse `cache_home` fixture in `tests/conftest.py` points `backend.CACHE_HOME` at a folder under `tmp_path`, so that a test run neither reads nor writes the note the machine it runs on is using, and every test starts with nothing remembered. +- **Do not assume the suite runs unpinned.** `PYTHON_KEYRING_BACKEND` is how the README suggests running the suite without an OS keyring, and it outranks everything `backend.py` does, so a test about remembering has to `delenv` it first or it will be testing the path that deliberately remembers nothing. - Warnings are errors (`filterwarnings` in `pyproject.toml`), so a deprecation in a new Python release fails the suite rather than scrolling past. ## Architecture -`cli.main` wires the three halves together: `load_conf` produces the configuration, `get_env` turns it into an environment, and `run_cmd`/`run_shell` hand that environment to a shell. Errors reach the user through `logs.error`, which exits with status 1; `logs.vlog` output only appears under `--verbose` and is the first thing to reach for when debugging a configuration. +`cli.main` wires the three halves together: `load_conf` produces the configuration, `get_env` turns it into an environment, and `run_cmd`/`run_shell` hand that environment to a shell. `--detect-backend` and `--reset-backend` return before any of it, since neither has a use for a configuration or a command. Errors reach the user through `logs.error`, which exits with status 1 and takes the hint lines that go under the error with it; `logs.vlog` output only appears under `--verbose` and is the first thing to reach for when debugging a configuration. **`conf.py` — where the configuration comes from.** Later sources win, merged deeply by `merge_conf`: defaults, then `~/.keycmd`, then every `.keycmd` found walking up from the working directory (outermost first), then the first `pyproject.toml` found walking up, whose `[tool.keycmd]` table is used. Both searches cover the same ground, so `load_conf` collects them in a single pass over `walk_up`, which stops at a `.git` directory, at the home folder, and at the root of the file system, so the walk never escapes a repository. `USERPROFILE` is a module attribute so tests can point the user config elsewhere. The merged result is `cast` to `Conf` rather than validated: it is user authored, and `get_env` reports violations as user errors. -**`creds.py` — configuration to environment.** `get_env` copies `os.environ` and adds a variable per entry of `[keys]`, looking each credential up in the keyring; `[aliases]` re-expose an existing key under another name with different `b64`/`format` options, without a second keyring lookup. `expose` applies `format` first and `b64` second, which is what makes `{username}:{password}` basic auth work. +**`creds.py` — configuration to environment.** `get_env` copies `os.environ` and adds a variable per entry of `[keys]`, looking each credential up in the backend `backend.load_backend` hands it; `[aliases]` re-expose an existing key under another name with different `b64`/`format` options, without a second keyring lookup. `expose` applies `format` first and `b64` second, which is what makes `{username}:{password}` basic auth work. + +**`backend.py` — which keyring backend, and remembering the answer.** Left to itself keyring finds its backend by loading every backend every installed package registers, the single most expensive thing a run does. The answer only changes when the machine does, so `load_backend` writes it to `cache_path` — the platform's cache folder, `CACHE_HOME` being the module attribute tests redirect — and afterwards loads it with `load_keyring`, which is the same shortcut `PYTHON_KEYRING_BACKEND` buys without anyone having to know the variable exists. Everything about the note is treated as untrusted: `is_backend_name` keeps anything that is not a dotted class name from reaching an import, and a name that no longer loads (uninstalled, or a daemon that is no longer running, which `load_keyring` catches alike because it asks the class for its `priority`) sends the run back to searching. `backend_name` looks *through* the chainer, which is not a backend but the search wearing one's clothes, so writing it down would leave the search in place. `PYTHON_KEYRING_BACKEND` outranks the note and is never written over. Nothing is remembered when the search finds nothing: that and a `PYTHON_KEYRING_BACKEND` that cannot be loaded are reported as user errors with advice, rather than as the traceback that reaches the user otherwise. `detect_backend` and `reset_backend` are the deliberate versions of the two steps, for a machine that changed in a way that leaves the note valid but wrong. **`wsl.py` — the boundary between WSL and Windows.** WSL users install keycmd on Windows, which leaves it a Windows process with a Windows idea of a shell. `from_wsl` decides whether it was called from a distro — a `wsl.exe`/`wslhost.exe` ancestor decides it, a Windows shell found first decides against it, and a UNC working directory settles the rest — after which `run_shell`/`run_cmd` hand the work to `wsl.exe` rather than to a Windows shell. A UNC working directory also names the distro, which `wsl_argv` passes as `--distribution` so that a second distro does not send the command to the default one. `KEYCMD_WSL` overrides that decision in either direction. Neither side of the boundary inherits the other's environment, so `share_env` lists the exposed variables in `WSLENV`, which is the only thing that crosses. diff --git a/README.md b/README.md index 1dc42bd..978035f 100644 --- a/README.md +++ b/README.md @@ -157,16 +157,22 @@ The CLI has the following options: ``` ❯ keycmd --help -usage: keycmd [-h] [-v] [--version] [--shell] ... +usage: keycmd [-h] [-v] [--version] [--detect-backend] [--reset-backend] + [--shell] + ... positional arguments: - command command to run - -optional arguments: - -h, --help show this help message and exit - -v, --verbose enable verbose output, useful for configuration debugging - --version print version info - --shell spawn a subshell instead of running a command + command command to run + +options: + -h, --help show this help message and exit + -v, --verbose enable verbose output, useful for configuration debugging + --version print version info + --detect-backend search for the keyring backend now and remember it for + later runs + --reset-backend forget the remembered keyring backend, so the next run + searches again + --shell spawn a subshell instead of running a command ``` There are two main ways to use the CLI: @@ -401,7 +407,7 @@ keycmd: merged config: 'ARTIFACTS_TOKEN_B64': {'b64': True, 'credential': 'korijn@poetry-repository-main', 'username': 'korijn'}}} -keycmd: keyring backend: +keycmd: keyring backend: (remembered) keycmd: exposing credential korijn@poetry-repository-main with user korijn as environment variable ARTIFACTS_TOKEN (b64: False, format: None) keycmd: exposing credential korijn@poetry-repository-main with user korijn as environment variable ARTIFACTS_TOKEN_B64 (b64: True, format: None) keycmd: detected shell: C:\Windows\System32\cmd.exe @@ -419,14 +425,50 @@ See the [third party backends](https://github.com/jaraco/keyring/#third-party-ba Left to itself, keyring works out which backend to use by loading every backend registered by every installed package and picking the most suitable one. That search runs on each `keycmd` invocation and, on a machine with a few packages installed, costs more time than the whole of the rest of a `keycmd` run put together. -If that shows up in your shell, name the backend you already know you want, and keyring will load that one instead of going looking: +The answer, though, is the same every time until the packages on your machine change. So keycmd writes it down the first time it needs a credential, and loads that backend by name on every run after, which on the machine this was measured on takes a run from 0.156s to 0.085s. There is nothing to configure and nothing to read; it just gets faster after the first run. + +You can watch it happen with `--verbose`, which says where the backend came from: + +``` +keycmd: keyring backend: keyring.backends.SecretService.Keyring (found in 0.12s) # the first run +keycmd: keyring backend: keyring.backends.SecretService.Keyring (remembered) # every run after +``` + +The note lives with the rest of your cached files — `%LOCALAPPDATA%\keycmd\backend` on Windows, `~/Library/Caches/keycmd/backend` on macOS, and `$XDG_CACHE_HOME/keycmd/backend` (usually `~/.cache`) on Linux — and deleting it costs you nothing but one slow run. + +keycmd only trusts the note as far as it can check it. If the backend it names has been uninstalled, or is no longer usable because the daemon behind it is not running, the run searches again and writes down what it finds instead. What it cannot notice by itself is a backend that still loads but is no longer the one you want — you installed a better one, or removed a package and want the runner-up. That is what these two are for: + +```bash +keycmd --detect-backend # search now, and remember what turns up +keycmd --reset-backend # forget it, so the next run searches again +``` + +``` +❯ keycmd --detect-backend +keycmd: remembered keyring backend keyring.backends.SecretService.Keyring, found in 0.12s +``` + +If you would rather take the whole thing into your own hands, keyring's own `PYTHON_KEYRING_BACKEND` still works and outranks anything keycmd remembers: ```bash # in your shell profile; use the backend your platform actually uses export PYTHON_KEYRING_BACKEND=keyring.backends.SecretService.Keyring ``` -`keyring --list-backends` prints the names to choose from, and `keycmd --verbose` will tell you which one ends up being used. The setting is keyring's own, so it applies to everything else using keyring too. +`keyring --list-backends` prints the names to choose from. The setting is keyring's own, so it applies to everything else using keyring too, and with it set keycmd has nothing to remember and says so if you ask it to. + +### No backend at all + +If keyring finds no backend it can use, there is nowhere for keycmd to read credentials from, and it says so rather than failing on the first lookup: + +``` +❯ keycmd 'npm install' +keycmd: error: keyring has no backend to read credentials from +keycmd: hint: install one for this platform, or name one you have with PYTHON_KEYRING_BACKEND +keycmd: hint: see https://github.com/jaraco/keyring#third-party-backends +``` + +Inside a WSL distribution this usually means the distro's keyring daemon is not running, which is what the [WSL installation](#wsl-installation) instructions above are for; keycmd points you there when it notices it is running in one. Nothing is written down in this case, so there is nothing to reset once you have fixed it. ## Development diff --git a/keycmd/backend.py b/keycmd/backend.py new file mode 100644 index 0000000..1504d43 --- /dev/null +++ b/keycmd/backend.py @@ -0,0 +1,290 @@ +"""Which keyring backend to use, and remembering the answer + +Left to itself, keyring works out which backend to use by loading every +backend that every installed package registers and keeping the best of +them. That search runs again on each invocation and is the single most +expensive thing a keycmd run does, on a machine with a few packages +installed more than the whole of the rest of a run put together. + +The answer, though, is the same every time until the packages on the +machine change, so keycmd writes it down the first time and loads that +backend by name afterwards, which is the same shortcut +PYTHON_KEYRING_BACKEND is for without anyone having to know the variable +exists. What is written down is only ever an answer keycmd can check: +`load_keyring` refuses a backend that is not installed and one that is +not viable alike, so a note that has gone stale sends the run back to +searching rather than failing it. `--detect-backend` and +`--reset-backend` are the deliberate versions of the same two steps, for +when the machine changed in a way that the note is still a valid answer +to. +""" + +import os +import sys +from pathlib import Path +from time import perf_counter +from typing import TYPE_CHECKING, NoReturn + +from .logs import error, log, vlog, vwarn +from .wsl import in_distro + +if TYPE_CHECKING: + # keyring is the most expensive import in the package and a run that + # looks up no credential never makes it, so the annotations below are + # the only place its name may appear at module level + from keyring.backend import KeyringBackend + +# keyring's own way to be told which backend to use, which skips the +# search by itself and outranks anything keycmd wrote down +BACKEND_VAR: str = "PYTHON_KEYRING_BACKEND" + +# exposed for testing, so that a run on one platform can drive the paths +# of the others +IS_WINDOWS: bool = os.name == "nt" +IS_MACOS: bool = sys.platform == "darwin" +CACHE_HOME: Path | None = None + +# where someone whose machine turned out to have no backend can find one +BACKENDS_URL: str = "https://github.com/jaraco/keyring#third-party-backends" + + +def cache_home() -> Path: + """Where this platform keeps per user files a program can afford to lose + + Which is what this is: everything under it can be deleted at any + moment, and the next run pays for a search and writes it again. + """ + if CACHE_HOME is not None: + return CACHE_HOME + if IS_WINDOWS: + # the local one rather than the roaming one, since which backends + # a machine has is a fact about that machine + local = os.environ.get("LOCALAPPDATA") + return Path(local) if local else Path.home() / "AppData" / "Local" + if IS_MACOS: + return Path.home() / "Library" / "Caches" + xdg = os.environ.get("XDG_CACHE_HOME") + return Path(xdg) if xdg else Path.home() / ".cache" + + +def cache_path() -> Path: + """The file the remembered backend is written to""" + return cache_home() / "keycmd" / "backend" + + +def is_backend_name(name: str) -> bool: + """Does this look like the dotted class name it is supposed to be? + + What comes out of the file is fed to an import, so what goes in has + to be the shape of a name and nothing else. Anything keycmd wrote is; + a file that has since been truncated or scribbled in is not, and is + worth no more than a fresh search. + """ + parts = name.split(".") + return len(parts) > 1 and all(part.isidentifier() for part in parts) + + +def recall() -> str | None: + """The backend an earlier run wrote down, if one did""" + path = cache_path() + try: + name = path.read_text(encoding="utf-8").strip() + except FileNotFoundError: + vlog(f"no keyring backend remembered in {path}") + return None + except OSError as err: + vwarn(f"could not read {path}: {err!r}") + return None + if not is_backend_name(name): + vwarn(f"{path} does not name a keyring backend") + return None + return name + + +def remember(name: str) -> None: + """Write the backend down, so that the next run can skip the search + + Through a temporary file, so that a second keycmd running at the same + moment reads either the old name or the new one rather than half of + each. Somewhere unwritable is a reason to search every run, not a + reason to fail the one that already has its answer. + """ + path = cache_path() + temp = path.with_name(f"{path.name}.{os.getpid()}") + try: + path.parent.mkdir(parents=True, exist_ok=True) + temp.write_text(f"{name}\n", encoding="utf-8") + os.replace(temp, path) + except OSError as err: + vwarn(f"could not remember the keyring backend in {path}: {err!r}") + # clearing up after the failed write cannot be allowed to fail in + # its turn, since whatever stopped the write stops this just as + # easily, and missing_ok covers only the file that is not there + try: + temp.unlink(missing_ok=True) + except OSError: + pass + return + vlog(f"remembered keyring backend {name} in {path}") + + +def forget() -> bool: + """Drop what was written down, if anything was""" + path = cache_path() + try: + path.unlink() + except FileNotFoundError: + return False + except OSError as err: + error(f"could not forget the keyring backend in {path}: {err!r}") + vlog(f"removed {path}") + return True + + +def load_named(name: str) -> "KeyringBackend | None": + """The named backend, if it can still be loaded + + load_keyring asks the class for its priority on the way, which is how + keyring itself decides a backend is viable, so a backend that has + been uninstalled and one whose daemon is no longer running both come + back as nothing here. + """ + from keyring.core import load_keyring + + try: + return load_keyring(name) + except Exception as err: + vlog(f"remembered keyring backend {name} no longer loads: {err!r}") + return None + + +def search() -> tuple["KeyringBackend", float]: + """Let keyring find a backend, and time how long it took""" + import keyring + + start = perf_counter() + backend = keyring.get_keyring() + return backend, perf_counter() - start + + +def backend_name(backend: "KeyringBackend") -> str: + """The name that loads this backend again without searching + + The chainer is not a backend of its own but the search over all of + them wearing one's clothes, so writing it down would leave the search + in place. What is worth writing down is the backend the chainer would + have reached first, which is where the credentials of an unremembered + run come from anyway. + """ + from keyring.backends.chainer import ChainerBackend + + if isinstance(backend, ChainerBackend): + # sorted by priority, and the chainer only wins when it has more + # than one to sort, but it costs nothing to not assume that + chained = backend.backends + if chained: + backend = chained[0] + cls = type(backend) + return f"{cls.__module__}.{cls.__qualname__}" + + +def is_no_backend(backend: "KeyringBackend") -> bool: + """Is this the backend keyring settles on when it found nothing?""" + from keyring.backends.fail import Keyring + + return isinstance(backend, Keyring) + + +def no_backend() -> NoReturn: + """Report a keyring that has nowhere to read credentials from + + It raises on the first lookup otherwise, which reaches the user as a + traceback rather than as an answer to the question they have. + """ + hints = [ + f"install one for this platform, or name one you have with {BACKEND_VAR}", + f"see {BACKENDS_URL}", + ] + if in_distro(): + # the README tells WSL users to install keycmd on windows for + # exactly this reason, and this is what not having done so looks + # like from inside the distribution + hints.append( + "inside WSL this usually means no keyring daemon is running;" + " the README explains how to reach the windows credential" + " manager instead" + ) + error("keyring has no backend to read credentials from", *hints) + + +def pinned_backend(pinned: str) -> "KeyringBackend": + """The backend PYTHON_KEYRING_BACKEND names + + Which keyring loads without searching, so there is nothing here for + keycmd to remember or to have remembered. + """ + import keyring + + try: + backend = keyring.get_keyring() + except Exception as err: + error( + f"{BACKEND_VAR}={pinned} could not be loaded: {err!r}", + f"name a backend class that is installed, or unset {BACKEND_VAR}" + f" to let keycmd find one itself", + ) + vlog(f"keyring backend: {backend} (named by {BACKEND_VAR})") + return backend + + +def load_backend() -> "KeyringBackend": + """The keyring backend to read this run's credentials from""" + pinned = os.environ.get(BACKEND_VAR, "") + if pinned: + return pinned_backend(pinned) + + name = recall() + if name is not None: + backend = load_named(name) + if backend is not None: + vlog(f"keyring backend: {backend} (remembered)") + return backend + # whatever it named is gone, and the note is worth nothing now + forget() + + backend, elapsed = search() + vlog(f"keyring backend: {backend} (found in {elapsed:.2f}s)") + if is_no_backend(backend): + # nothing worth writing down, and nothing to read credentials from + no_backend() + remember(backend_name(backend)) + return backend + + +def detect_backend() -> None: + """Search for a backend now, and write down what turns up + + The search runs by itself on the first run that needs a credential. + This is for the runs after that, once the machine has changed in a + way that makes the old answer the wrong one rather than an invalid + one: a backend installed that outranks the one in use, or one + uninstalled that keyring can still load. + """ + pinned = os.environ.get(BACKEND_VAR, "") + if pinned: + log(f"{BACKEND_VAR}={pinned} already names the backend to use") + return + backend, elapsed = search() + if is_no_backend(backend): + no_backend() + name = backend_name(backend) + remember(name) + log(f"remembered keyring backend {name}, found in {elapsed:.2f}s") + + +def reset_backend() -> None: + """Forget the backend, so that the next run searches for one again""" + if forget(): + log("forgot the remembered keyring backend") + else: + log("no keyring backend was remembered") diff --git a/keycmd/cli.py b/keycmd/cli.py index 60d3fea..3668498 100644 --- a/keycmd/cli.py +++ b/keycmd/cli.py @@ -3,6 +3,7 @@ from collections.abc import Sequence from . import __version__ +from .backend import detect_backend, reset_backend from .conf import load_conf from .creds import get_env from .logs import error, log, set_verbose @@ -21,6 +22,18 @@ cli.add_argument( "--version", action="store_true", default=False, help="print version info" ) +cli.add_argument( + "--detect-backend", + action="store_true", + default=False, + help="search for the keyring backend now and remember it for later runs", +) +cli.add_argument( + "--reset-backend", + action="store_true", + default=False, + help="forget the remembered keyring backend, so the next run searches again", +) cli.add_argument( "--shell", action="store_true", @@ -41,6 +54,15 @@ def main(args: Sequence[str] | None = None) -> None: log(f"v{__version__}") return + # both are about the keyring itself, so neither has any use for the + # configuration or for a command to run it against + if parsed.detect_backend: + detect_backend() + return + if parsed.reset_backend: + reset_backend() + return + try: conf = load_conf() except tomllib.TOMLDecodeError as err: diff --git a/keycmd/creds.py b/keycmd/creds.py index 8021b0b..ade5a23 100644 --- a/keycmd/creds.py +++ b/keycmd/creds.py @@ -1,6 +1,7 @@ import base64 from os import environ +from .backend import load_backend from .conf import AliasConf, Conf, KeyConf from .logs import error, vlog from .wsl import share_env @@ -62,20 +63,15 @@ def get_env(conf: Conf) -> dict[str, str]: key_data: dict[str, KeyData] = {} keys = conf["keys"] if keys: - # keyring, and the backend it goes on to discover, together cost - # more time and memory than everything else keycmd does; a run that - # looks up no credential should not have to pay for either - import keyring - - # which backend keyring settles on decides where the credentials - # come from, so it is the first thing to check when they are not - # the ones that were expected - vlog(f"keyring backend: {keyring.get_keyring()}") + # which backend the credentials come from is the first thing to + # check when they are not the ones that were expected, and + # load_backend reports it under --verbose + backend = load_backend() for key, src in keys.items(): credential = src["credential"] username = src["username"] - password = keyring.get_password(credential, username) + password = backend.get_password(credential, username) if password is None: error( f"MISSING credential {credential}" diff --git a/keycmd/logs.py b/keycmd/logs.py index e180d93..a279331 100644 --- a/keycmd/logs.py +++ b/keycmd/logs.py @@ -35,8 +35,17 @@ def vlog_pretty(prefix: str, value: object) -> None: log(f"{prefix}{pformat(value)}") -def error(msg: object) -> NoReturn: +def error(msg: object, *hints: object) -> NoReturn: + """Report why keycmd cannot go on, and exit + + The line that says what went wrong is rarely the line that says what + to do about it, so an error can carry as many of the second kind as + it takes. Both go to stderr, which is where keycmd writes anything + that is not the output of the command it was asked to run. + """ log(f"error: {msg}", err=True) + for hint in hints: + log(f"hint: {hint}", err=True) sys.exit(1) diff --git a/tests/conftest.py b/tests/conftest.py index 82e4d26..fc90c65 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -13,6 +13,7 @@ import keyring import pytest +import keycmd.backend import keycmd.conf import keycmd.shell import keycmd.wsl @@ -155,6 +156,20 @@ def outside_wsl(monkeypatch): monkeypatch.delenv("WSL_INTEROP", raising=False) +@pytest.fixture(autouse=True) +def cache_home(tmp_path, monkeypatch): + """Keep the remembered keyring backend out of the real cache folder + + keycmd writes down the backend it found so that later runs can skip + the search, and a test run has no business reading or writing the + note the machine it runs on is using. One folder per test, so that a + test starts with nothing remembered unless it says otherwise. + """ + home = tmp_path / ".cache" + monkeypatch.setattr(keycmd.backend, "CACHE_HOME", home) + return home + + @pytest.fixture def subprocess(monkeypatch): """Run commands in a subprocess instead of replacing this process""" diff --git a/tests/test_backend.py b/tests/test_backend.py new file mode 100644 index 0000000..73fdd2f --- /dev/null +++ b/tests/test_backend.py @@ -0,0 +1,304 @@ +"""Finding a keyring backend once, and remembering which one it was + +The search keyring runs to find a backend is the single most expensive +thing a keycmd run does, so keycmd writes the answer down and loads it by +name afterwards. What is asserted here is that the note is only ever +trusted as far as it can be checked: it has to look like a class name, it +has to still load, and it never stands in the way of PYTHON_KEYRING_BACKEND. +""" + +from pathlib import Path +from typing import ClassVar + +import keyring +import pytest +from keyring.backends.chainer import ChainerBackend +from keyring.backends.fail import Keyring as NoKeyring +from keyring.backends.null import Keyring as NullKeyring + +import keycmd.backend +from keycmd.backend import ( + BACKEND_VAR, + backend_name, + cache_path, + detect_backend, + forget, + is_backend_name, + load_backend, + recall, + remember, + reset_backend, +) + +NULL = "keyring.backends.null.Keyring" + + +class FakeChainer(ChainerBackend): + """A chainer with a fixed membership, instead of the one on this machine""" + + backends: ClassVar = [NullKeyring(), NoKeyring()] + + +@pytest.fixture +def unpinned(monkeypatch): + """A machine whose search finds a backend, and nothing remembered yet""" + monkeypatch.delenv(BACKEND_VAR, raising=False) + monkeypatch.setattr(keyring, "get_keyring", NullKeyring) + return NullKeyring() + + +@pytest.fixture +def nothing_found(monkeypatch): + """A machine whose search comes up empty""" + monkeypatch.delenv(BACKEND_VAR, raising=False) + monkeypatch.setattr(keyring, "get_keyring", NoKeyring) + + +def test_cache_path_per_platform(monkeypatch): + """Each platform keeps discardable per user files somewhere of its own + + Covers the platforms the current one is not. + """ + monkeypatch.setattr(keycmd.backend, "CACHE_HOME", None) + monkeypatch.setattr(Path, "home", classmethod(lambda cls: Path("/home/user"))) + + monkeypatch.setattr(keycmd.backend, "IS_WINDOWS", True) + monkeypatch.setattr(keycmd.backend, "IS_MACOS", False) + monkeypatch.setenv("LOCALAPPDATA", "/local") + assert cache_path() == Path("/local/keycmd/backend") + monkeypatch.delenv("LOCALAPPDATA") + assert cache_path() == Path("/home/user/AppData/Local/keycmd/backend") + + monkeypatch.setattr(keycmd.backend, "IS_WINDOWS", False) + monkeypatch.setattr(keycmd.backend, "IS_MACOS", True) + assert cache_path() == Path("/home/user/Library/Caches/keycmd/backend") + + monkeypatch.setattr(keycmd.backend, "IS_MACOS", False) + monkeypatch.setenv("XDG_CACHE_HOME", "/xdg") + assert cache_path() == Path("/xdg/keycmd/backend") + monkeypatch.delenv("XDG_CACHE_HOME") + assert cache_path() == Path("/home/user/.cache/keycmd/backend") + + +@pytest.mark.parametrize( + ("name", "expected"), + [ + ("keyring.backends.null.Keyring", True), + ("a.B", True), + # a bare name is not a class in a module, and importing it would + # not give one either + ("Keyring", False), + ("", False), + (".", False), + ("keyring.backends..Keyring", False), + # the file is fed to an import, so nothing that is not a name + ("rm -rf /", False), + ("keyring backends", False), + ("a.b; import os", False), + ], +) +def test_is_backend_name(name, expected): + """Only the shape of a dotted class name is worth importing""" + assert is_backend_name(name) is expected + + +def test_backend_name(): + """The name is one keyring can be given to load the backend again""" + assert backend_name(NullKeyring()) == NULL + assert isinstance( + keyring.core.load_keyring(backend_name(NullKeyring())), NullKeyring + ) + + +def test_backend_name_chainer(): + """Writing down the chainer would leave the search it stands for in place + + So the backend it would have reached first is written down instead, + which is where the credentials come from either way. + """ + assert backend_name(FakeChainer()) == NULL + + +def test_remember_and_recall(cache_home): + assert recall() is None + remember(NULL) + assert cache_path().read_text(encoding="utf-8") == f"{NULL}\n" + assert recall() == NULL + + +def test_remember_replaces(cache_home): + remember("keyring.backends.fail.Keyring") + remember(NULL) + assert recall() == NULL + + +def test_remember_leaves_no_temporary_file(cache_home): + """The write goes through a temporary name, and does not stay there""" + remember(NULL) + assert [path.name for path in cache_path().parent.iterdir()] == ["backend"] + + +def test_remember_somewhere_unwritable(capsys, monkeypatch, tmp_path, verbose): + """A cache that cannot be written is a slow run, not a failed one + + A file where the folder should be, which no amount of privilege makes + writable, unlike a permission the suite may well be running above. + """ + blocked = tmp_path / "blocked" + blocked.write_text("not a folder", encoding="utf-8") + monkeypatch.setattr(keycmd.backend, "CACHE_HOME", blocked / "cache") + remember(NULL) + assert "could not remember the keyring backend" in capsys.readouterr().out + assert recall() is None + + +def test_recall_ignores_what_is_not_a_name(capsys, cache_home, verbose): + """A file that has been scribbled in is worth no more than a search""" + path = cache_path() + path.parent.mkdir(parents=True) + path.write_text("not a backend name\n", encoding="utf-8") + assert recall() is None + assert "does not name a keyring backend" in capsys.readouterr().out + + +def test_recall_ignores_an_unreadable_file(capsys, cache_home, verbose): + """Which is what a directory in its place looks like from here""" + cache_path().mkdir(parents=True) + assert recall() is None + assert "could not read" in capsys.readouterr().out + + +def test_forget(cache_home): + assert forget() is False + remember(NULL) + assert forget() is True + assert recall() is None + + +def test_load_backend_remembers(capsys, cache_home, unpinned, verbose): + """The first run searches, and writes down what it found""" + assert isinstance(load_backend(), NullKeyring) + assert "(found in " in capsys.readouterr().out + assert recall() == NULL + + +def test_load_backend_recalls(capsys, cache_home, unpinned, monkeypatch, verbose): + """The runs after it load that backend by name instead of searching""" + remember(NULL) + + def no_searching(): + raise AssertionError("searched for a backend with one already remembered") + + monkeypatch.setattr(keyring, "get_keyring", no_searching) + assert isinstance(load_backend(), NullKeyring) + assert "(remembered)" in capsys.readouterr().out + + +def test_load_backend_searches_again_when_the_note_is_stale( + capsys, cache_home, unpinned, verbose +): + """A backend that no longer loads sends the run back to searching + + load_keyring asks the class for its priority on the way, so this + covers a backend that was uninstalled and one whose daemon stopped + alike. + """ + remember("nope.NotAKeyring") + assert isinstance(load_backend(), NullKeyring) + out = capsys.readouterr().out + assert "no longer loads" in out + # and the note is replaced rather than left to fail every run + assert recall() == NULL + + +def test_load_backend_leaves_nothing_when_there_is_no_backend( + cache_home, nothing_found +): + """A search that found nothing is not an answer worth remembering""" + with pytest.raises(SystemExit) as exc_info: + load_backend() + assert exc_info.value.args[0] == 1 + assert recall() is None + + +def test_load_backend_no_backend_message(capsys, cache_home, nothing_found): + """A keyring with nothing behind it is a question, not a traceback""" + with pytest.raises(SystemExit): + load_backend() + err = capsys.readouterr().err + assert "no backend to read credentials from" in err + assert BACKEND_VAR in err + assert keycmd.backend.BACKENDS_URL in err + + +def test_load_backend_no_backend_in_wsl(capsys, monkeypatch, cache_home, nothing_found): + """Inside a distribution there is a likelier answer than installing one""" + monkeypatch.setenv("WSL_DISTRO_NAME", "Ubuntu") + with pytest.raises(SystemExit): + load_backend() + assert "inside WSL" in capsys.readouterr().err + + +def test_load_backend_pinned_wins(capsys, cache_home, monkeypatch, verbose): + """PYTHON_KEYRING_BACKEND outranks anything keycmd wrote down""" + remember("keyring.backends.fail.Keyring") + monkeypatch.setenv(BACKEND_VAR, NULL) + monkeypatch.setattr(keyring, "get_keyring", NullKeyring) + assert isinstance(load_backend(), NullKeyring) + assert f"keyring backend: {NullKeyring()} (named by {BACKEND_VAR})" in ( + capsys.readouterr().out + ) + # and is left alone, since it was not keycmd that put it there + assert recall() == "keyring.backends.fail.Keyring" + + +def test_load_backend_unloadable_pin(capsys, cache_home, monkeypatch): + """A name keyring cannot load names the variable that carries it""" + monkeypatch.setenv(BACKEND_VAR, "nope.NotAKeyring") + + def get_keyring(): + raise ModuleNotFoundError("No module named 'nope'") + + monkeypatch.setattr(keyring, "get_keyring", get_keyring) + with pytest.raises(SystemExit) as exc_info: + load_backend() + assert exc_info.value.args[0] == 1 + err = capsys.readouterr().err + assert f"{BACKEND_VAR}=nope.NotAKeyring could not be loaded" in err + assert "ModuleNotFoundError" in err + + +def test_detect_backend(capsys, cache_home, unpinned): + """Searching on purpose, for when the machine changed under the note""" + remember("keyring.backends.fail.Keyring") + detect_backend() + assert f"remembered keyring backend {NULL}, found in " in capsys.readouterr().out + assert recall() == NULL + + +def test_detect_backend_with_nothing_to_find(capsys, cache_home, nothing_found): + with pytest.raises(SystemExit) as exc_info: + detect_backend() + assert exc_info.value.args[0] == 1 + assert "no backend to read credentials from" in capsys.readouterr().err + assert recall() is None + + +def test_detect_backend_with_a_pin(capsys, cache_home, monkeypatch): + """There is nothing to remember when keyring is already being told""" + monkeypatch.setenv(BACKEND_VAR, NULL) + detect_backend() + assert f"{BACKEND_VAR}={NULL} already names the backend" in capsys.readouterr().out + assert recall() is None + + +def test_reset_backend(capsys, cache_home): + remember(NULL) + reset_backend() + assert "forgot the remembered keyring backend" in capsys.readouterr().out + assert recall() is None + + +def test_reset_backend_with_nothing_remembered(capsys, cache_home): + reset_backend() + assert "no keyring backend was remembered" in capsys.readouterr().out diff --git a/tests/test_cli.py b/tests/test_cli.py index 8fcfe1d..34de154 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -4,6 +4,7 @@ import pytest from keycmd import __version__ +from keycmd.backend import BACKEND_VAR, recall, remember from keycmd.cli import cli, main # modules that cost more to import than the rest of keycmd together, and @@ -51,6 +52,57 @@ def test_cli_missing_credential(capfd, local_conf, userprofile, subprocess, os_k assert "MISSING credential" in capfd.readouterr().err +def test_cli_detect_backend(capfd, cache_home, monkeypatch, os_keyring): + """Searching on purpose, and writing down what turns up""" + monkeypatch.delenv(BACKEND_VAR, raising=False) + main(["--detect-backend"]) + assert "remembered keyring backend" in capfd.readouterr().out + assert recall() is not None + + +def test_cli_reset_backend(capfd, cache_home): + """Forgetting on purpose, so that the next run searches again""" + remember("keyring.backends.null.Keyring") + main(["--reset-backend"]) + assert "forgot the remembered keyring backend" in capfd.readouterr().out + assert recall() is None + + main(["--reset-backend"]) + assert "no keyring backend was remembered" in capfd.readouterr().out + + +def test_cli_backend_flags_need_no_command(cache_home): + """Neither has any use for a command, or for a configuration to load""" + for flag in ("--detect-backend", "--reset-backend"): + args = cli.parse_args([flag]) + assert args.command == [] + + +def test_cli_remembers_the_backend( + capfd, + cache_home, + monkeypatch, + shell_credentials, + local_conf, + userprofile, + subprocess, +): + """A run that had to search writes the answer down for the next one + + Which is the whole feature, seen from where the user stands: nothing + to read, nothing to set, and the search paid for once. + """ + # a machine that already names its backend has nothing to remember, + # and this is about the machines that do not + monkeypatch.delenv(BACKEND_VAR, raising=False) + with pytest.raises(SystemExit) as exc_info: + main(["echo", "foo"]) + assert exc_info.value.args[0] == 0 + # the command keeps the output to itself, and the note is on disk + assert capfd.readouterr().out.strip() == "foo" + assert recall() is not None + + def test_cli_missing_command(capfd, ch_tmpdir, userprofile): with pytest.raises(SystemExit) as exc_info: main([]) diff --git a/tests/test_logs.py b/tests/test_logs.py index bf478e4..9be2d40 100644 --- a/tests/test_logs.py +++ b/tests/test_logs.py @@ -30,3 +30,13 @@ def test_logging(capsys, request): vlog("foo") assert capsys.readouterr().out == "" + + +def test_error_hints(capsys): + """What went wrong is rarely the same line as what to do about it""" + with pytest.raises(SystemExit) as exc_info: + error("no", "try this", "or this") + assert exc_info.value.args[0] == 1 + assert capsys.readouterr().err == ( + "keycmd: error: no\nkeycmd: hint: try this\nkeycmd: hint: or this\n" + )