From 2ec7e9efa9d0845b271cca4f6b5517bfd692e054 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Wed, 21 Oct 2026 17:25:57 +0100 Subject: [PATCH] refactor(no-ticket): read the keyring settings through Click The keyring module read CLOUDSMITH_NO_KEYRING, CLOUDSMITH_KEYRING_BACKEND, CLOUDSMITH_KEYRING_KEY, CLOUDSMITH_KEYRING_FILE_PATH and CLOUDSMITH_KEYRING_DIR with os.environ.get(). These settings have no flags, so Click did not know about them. - `keyring_options` on the `main` group adds hidden options that read the five variables. It gives the values to the keyring module through `keyring.configure(KeyringSettings(...))`. - The keyring module reads its settings from `configure()`, not from the environment. It still copies them to PYTHON_KEYRING_BACKEND and KEYRING_PROPERTY_*, because the keyring library reads only those. - The path settings are expanded and made absolute when Click parses them, not when the keyring uses them. - CLOUDSMITH_NO_KEYRING keeps its parsing: only 1, true and yes, in any case, turn off the keyring. Other values leave it on, as before. - A root conftest fixture resets the settings before each test, so tests no longer depend on the keyring variables of the developer shell. Co-Authored-By: Claude Opus 5.5 --- cloudsmith_cli/cli/commands/main.py | 1 + cloudsmith_cli/cli/decorators.py | 62 +++++- .../cli/tests/commands/test_domains.py | 3 +- .../cli/tests/test_credential_helper_cargo.py | 4 +- cloudsmith_cli/cli/tests/test_decorators.py | 35 ++++ cloudsmith_cli/conftest.py | 7 + cloudsmith_cli/core/keyring.py | 50 +++-- cloudsmith_cli/core/tests/test_keyring.py | 185 +++++------------- .../core/tests/test_keyring_provider.py | 8 +- 9 files changed, 198 insertions(+), 157 deletions(-) diff --git a/cloudsmith_cli/cli/commands/main.py b/cloudsmith_cli/cli/commands/main.py index 432432115..98f3e3914 100644 --- a/cloudsmith_cli/cli/commands/main.py +++ b/cloudsmith_cli/cli/commands/main.py @@ -64,6 +64,7 @@ def print_version(opts): envvar="CLOUDSMITH_NO_UPDATE_CHECK", help="Disable the once-a-day check for a newer version of the CLI.", ) +@decorators.keyring_options @decorators.common_cli_config_options @decorators.common_cli_output_options @click.pass_context diff --git a/cloudsmith_cli/cli/decorators.py b/cloudsmith_cli/cli/decorators.py index edc03abcb..9c45d4dc7 100644 --- a/cloudsmith_cli/cli/decorators.py +++ b/cloudsmith_cli/cli/decorators.py @@ -11,7 +11,7 @@ from cloudsmith_cli.cli import validators from cloudsmith_cli.core.utils import ColorMode, TTYMode, color_enabled -from ..core import update_check +from ..core import keyring, update_check from ..core.certificate_errors import describe_certificate_error from ..core.credentials.chain import CredentialProviderChain from ..core.credentials.models import CredentialContext @@ -118,6 +118,66 @@ def wrapper(ctx, *args, **kwargs): return wrapper +def _absolute_path(ctx, param, value): + """Expand ``~`` and environment variables in *value* and make it absolute.""" + if not value: + return value + return os.path.abspath(os.path.expanduser(os.path.expandvars(value))) + + +def keyring_options(f): + """Add hidden options that read the keyring settings from the environment.""" + + @click.option( + "--no-keyring-env", + "no_keyring", + envvar="CLOUDSMITH_NO_KEYRING", + hidden=True, + ) + @click.option( + "--keyring-backend-env", + "keyring_backend", + envvar="CLOUDSMITH_KEYRING_BACKEND", + hidden=True, + ) + @click.option( + "--keyring-key-env", + "keyring_key", + envvar="CLOUDSMITH_KEYRING_KEY", + hidden=True, + ) + @click.option( + "--keyring-file-path-env", + "keyring_file_path", + envvar="CLOUDSMITH_KEYRING_FILE_PATH", + callback=_absolute_path, + hidden=True, + ) + @click.option( + "--keyring-dir-env", + "keyring_dir", + envvar="CLOUDSMITH_KEYRING_DIR", + callback=_absolute_path, + hidden=True, + ) + @click.pass_context + @functools.wraps(f) + def wrapper(ctx, *args, **kwargs): + keyring.configure( + keyring.KeyringSettings( + no_keyring=(kwargs.pop("no_keyring") or "").strip().lower() + in ("1", "true", "yes"), + backend=kwargs.pop("keyring_backend"), + key=kwargs.pop("keyring_key"), + file_path=kwargs.pop("keyring_file_path"), + directory=kwargs.pop("keyring_dir"), + ) + ) + return ctx.invoke(f, *args, **kwargs) + + return wrapper + + def common_cli_config_options(f): """Add common CLI config options to commands.""" diff --git a/cloudsmith_cli/cli/tests/commands/test_domains.py b/cloudsmith_cli/cli/tests/commands/test_domains.py index 7585cf8b7..9322cfb21 100644 --- a/cloudsmith_cli/cli/tests/commands/test_domains.py +++ b/cloudsmith_cli/cli/tests/commands/test_domains.py @@ -8,6 +8,7 @@ from ....cli import config as cli_config from ....cli.commands.domains import list_domains +from ....core import keyring as core_keyring from ....core.api.exceptions import ApiException from ....credential_helpers.domains import ( domain_from_payload, @@ -35,7 +36,7 @@ def hermetic_environment(monkeypatch, tmp_path): monkeypatch.delenv("CLOUDSMITH_API_HOST", raising=False) monkeypatch.delenv("CLOUDSMITH_CONFIG_FILE", raising=False) monkeypatch.delenv("CLOUDSMITH_API_KEY", raising=False) - monkeypatch.setenv("CLOUDSMITH_NO_KEYRING", "1") + core_keyring.configure(core_keyring.KeyringSettings(no_keyring=True)) monkeypatch.delattr(cli_config.OPTIONS, "value", raising=False) monkeypatch.setattr(cli_config.ConfigReader, "config_files", ["config.ini"]) monkeypatch.setattr(cli_config.ConfigReader, "config_searchpath", ["."]) diff --git a/cloudsmith_cli/cli/tests/test_credential_helper_cargo.py b/cloudsmith_cli/cli/tests/test_credential_helper_cargo.py index 92162df0b..9c8693c05 100644 --- a/cloudsmith_cli/cli/tests/test_credential_helper_cargo.py +++ b/cloudsmith_cli/cli/tests/test_credential_helper_cargo.py @@ -11,6 +11,7 @@ import pytest import tomlkit +from ...core import keyring as core_keyring from ...core.credentials.models import CredentialResult from ...credential_helpers.backends import BackendKind from ...credential_helpers.cargo.installer import CargoInstaller @@ -355,6 +356,7 @@ def test_cli_accepts_the_cargo_plugin_flag_and_extra_provider_args(runner): def test_cli_exits_non_zero_with_a_hint_when_no_credential_resolves(runner): """A refused `get` still answers Cargo in-band, then exits 1 with a hint.""" + core_keyring.configure(core_keyring.KeyringSettings(no_keyring=True)) with patch( "cloudsmith_cli.credential_helpers.cargo.runtime.is_cloudsmith_domain", return_value=True, @@ -363,7 +365,7 @@ def test_cli_exits_non_zero_with_a_hint_when_no_credential_resolves(runner): cargo, args=[], input=json.dumps(_request()) + "\n", - env={"CLOUDSMITH_API_KEY": "", "CLOUDSMITH_NO_KEYRING": "1"}, + env={"CLOUDSMITH_API_KEY": ""}, catch_exceptions=False, ) diff --git a/cloudsmith_cli/cli/tests/test_decorators.py b/cloudsmith_cli/cli/tests/test_decorators.py index a7b4a13af..d6e3a780f 100644 --- a/cloudsmith_cli/cli/tests/test_decorators.py +++ b/cloudsmith_cli/cli/tests/test_decorators.py @@ -4,6 +4,7 @@ import click.testing import pytest +from ...core import keyring as core_keyring from ...core.mcp.server import create_ssl_context from ...core.session import create_requests_session from ...core.tests.test_certificate_errors import certificate_error @@ -12,6 +13,7 @@ from ..decorators import ( initialise_api, initialise_session, + keyring_options, report_retry, resolve_credentials, ) @@ -289,3 +291,36 @@ def leaf(opts): ) assert result.exit_code == 0, result.output assert seen == [PROXY] + + +def test_keyring_options_configure_the_keyring(monkeypatch, tmp_path): + """The keyring settings come from the environment, with absolute paths.""" + + @click.command() + @keyring_options + def command(): + pass + + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("HOME", str(tmp_path / "home")) + monkeypatch.setenv("USERPROFILE", str(tmp_path / "home")) + monkeypatch.setenv("KEYRING_SUBDIR", "store") + result = click.testing.CliRunner().invoke( + command, + env={ + "CLOUDSMITH_NO_KEYRING": "yes", + "CLOUDSMITH_KEYRING_BACKEND": "keyring.backends.null.Keyring", + "CLOUDSMITH_KEYRING_KEY": "secret", + "CLOUDSMITH_KEYRING_FILE_PATH": "keyring.cfg", + "CLOUDSMITH_KEYRING_DIR": "~/$KEYRING_SUBDIR", + }, + ) + + assert result.exit_code == 0, result.output + assert core_keyring.current_settings() == core_keyring.KeyringSettings( + no_keyring=True, + backend="keyring.backends.null.Keyring", + key="secret", + file_path=str(tmp_path / "keyring.cfg"), + directory=str(tmp_path / "home" / "store"), + ) diff --git a/cloudsmith_cli/conftest.py b/cloudsmith_cli/conftest.py index ee7246677..5a7075828 100644 --- a/cloudsmith_cli/conftest.py +++ b/cloudsmith_cli/conftest.py @@ -1,6 +1,7 @@ import cloudsmith_api import pytest +from cloudsmith_cli.core import keyring as core_keyring from cloudsmith_cli.core import session, telemetry #: A syntactically valid DSN that is never contacted: tests that report use @@ -19,6 +20,12 @@ def no_real_error_reports(monkeypatch): monkeypatch.setattr(telemetry, "_transport", None) +@pytest.fixture(autouse=True) +def default_keyring_settings(): + """Start each test with the default keyring settings.""" + core_keyring.configure(core_keyring.KeyringSettings()) + + @pytest.fixture() def telemetry_config_dir(monkeypatch, tmp_path_factory): """An empty config search path, isolated from the developer's config.ini. diff --git a/cloudsmith_cli/core/keyring.py b/cloudsmith_cli/core/keyring.py index 55bb91828..d2c109e29 100644 --- a/cloudsmith_cli/core/keyring.py +++ b/cloudsmith_cli/core/keyring.py @@ -1,3 +1,4 @@ +import dataclasses import functools import getpass import os @@ -9,10 +10,37 @@ ACCESS_TOKEN_KEY = "cloudsmith_cli-access_token-{api_host}" +@dataclasses.dataclass(frozen=True) +class KeyringSettings: + """The CLOUDSMITH_*KEYRING* settings that the CLI parsed.""" + + no_keyring: bool = False + backend: str | None = None + key: str | None = None + file_path: str | None = None + directory: str | None = None + + +class _Current: + """The keyring settings of this process.""" + + settings = KeyringSettings() + + +def configure(settings): + """Use *settings* for the keyring in this process.""" + _Current.settings = settings + _prepare_keyring_backend.cache_clear() + + +def current_settings(): + """Return the keyring settings of this process.""" + return _Current.settings + + def should_use_keyring(): - """Check if keyring should be used based on CLOUDSMITH_NO_KEYRING env var.""" - env_value = os.environ.get("CLOUDSMITH_NO_KEYRING", "").strip().lower() - return env_value not in ("1", "true", "yes") + """Check if keyring should be used based on CLOUDSMITH_NO_KEYRING.""" + return not _Current.settings.no_keyring ACCESS_TOKEN_REFRESH_ATTEMPTED_AT_KEY = ( @@ -52,27 +80,23 @@ def _get_value_with_fallback(template, api_host, profile): def _sync_keyring_backend_env(): """Allow CLOUDSMITH_KEYRING_BACKEND to alias PYTHON_KEYRING_BACKEND.""" - alias_value = os.environ.get("CLOUDSMITH_KEYRING_BACKEND") + alias_value = _Current.settings.backend if alias_value and "PYTHON_KEYRING_BACKEND" not in os.environ: os.environ["PYTHON_KEYRING_BACKEND"] = alias_value def _sync_keyring_property_env(): """Allow CLOUDSMITH_KEYRING_KEY to alias KEYRING_PROPERTY_KEYRING_KEY.""" - alias_value = os.environ.get("CLOUDSMITH_KEYRING_KEY") + alias_value = _Current.settings.key if alias_value and "KEYRING_PROPERTY_KEYRING_KEY" not in os.environ: os.environ["KEYRING_PROPERTY_KEYRING_KEY"] = alias_value -def _expand_path(value): - return os.path.expanduser(os.path.expandvars(value)) - - def _sync_keyring_file_path_env(): """Allow CLOUDSMITH_KEYRING_FILE_PATH to alias KEYRING_PROPERTY_FILE_PATH.""" - alias_value = os.environ.get("CLOUDSMITH_KEYRING_FILE_PATH") + alias_value = _Current.settings.file_path if alias_value and "KEYRING_PROPERTY_FILE_PATH" not in os.environ: - os.environ["KEYRING_PROPERTY_FILE_PATH"] = _expand_path(alias_value) + os.environ["KEYRING_PROPERTY_FILE_PATH"] = alias_value def _apply_keyring_file_location(backend): @@ -89,13 +113,13 @@ def _apply_keyring_file_location(backend): if file_path: backend.file_path = file_path return - dir_value = os.environ.get("CLOUDSMITH_KEYRING_DIR") + dir_value = _Current.settings.directory if not dir_value: return filename = getattr(backend, "filename", None) if not filename: return - backend.file_path = os.path.join(_expand_path(dir_value), filename) + backend.file_path = os.path.join(dir_value, filename) def _refuse_password_prompt(): diff --git a/cloudsmith_cli/core/tests/test_keyring.py b/cloudsmith_cli/core/tests/test_keyring.py index c04159548..d2d031d61 100644 --- a/cloudsmith_cli/core/tests/test_keyring.py +++ b/cloudsmith_cli/core/tests/test_keyring.py @@ -14,6 +14,7 @@ from .. import keyring as core_keyring from ..keyring import ( + KeyringSettings, delete_oidc_token, delete_sso_tokens, get_access_token, @@ -246,11 +247,7 @@ def test_get_refresh_token_when_error_raised( @freeze_time("2024-06-01 10:00:00") def test_store_sso_tokens(self, mock_get_user, mock_set_password): - # Ensure keyring is enabled - env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) - with patch.dict(os.environ, env, clear=True): - result = store_sso_tokens(self.api_host, "access_token", "refresh_token") + result = store_sso_tokens(self.api_host, "access_token", "refresh_token") assert result is True refresh_key = ( @@ -274,8 +271,8 @@ def test_store_sso_tokens_returns_false_when_keyring_disabled( self, mock_get_user, mock_set_password ): """Verify store_sso_tokens returns False when keyring disabled.""" - with patch.dict(os.environ, {"CLOUDSMITH_NO_KEYRING": "1"}): - result = store_sso_tokens(self.api_host, "access_token", "refresh_token") + core_keyring.configure(KeyringSettings(no_keyring=True)) + result = store_sso_tokens(self.api_host, "access_token", "refresh_token") assert result is False mock_set_password.assert_not_called() @@ -326,12 +323,9 @@ def test_get_refresh_token_with_profile(self, mock_get_user, mock_get_password): @freeze_time("2024-06-01 10:00:00") def test_store_sso_tokens_with_profile(self, mock_get_user, mock_set_password): - env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) - with patch.dict(os.environ, env, clear=True): - result = store_sso_tokens( - self.api_host, "access_token", "refresh_token", profile="staging" - ) + result = store_sso_tokens( + self.api_host, "access_token", "refresh_token", profile="staging" + ) assert result is True assert mock_set_password.call_count == 3 @@ -588,25 +582,11 @@ class TestShouldUseKeyring: """Tests for the should_use_keyring function.""" def test_returns_true_by_default(self): - """When env var is not set, keyring should be used.""" - env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) - with patch.dict(os.environ, env, clear=True): - assert should_use_keyring() is True - - @pytest.mark.parametrize( - "env_value", ["1", "true", "True", "TRUE", "yes", "Yes", "YES"] - ) - def test_returns_false_when_env_var_is_truthy(self, env_value): - """Keyring should not be used when CLOUDSMITH_NO_KEYRING is truthy.""" - with patch.dict(os.environ, {"CLOUDSMITH_NO_KEYRING": env_value}): - assert should_use_keyring() is False + assert should_use_keyring() is True - @pytest.mark.parametrize("env_value", ["0", "false", "False", "no", "No", ""]) - def test_returns_true_when_env_var_is_falsy(self, env_value): - """Keyring should be used when CLOUDSMITH_NO_KEYRING is falsy.""" - with patch.dict(os.environ, {"CLOUDSMITH_NO_KEYRING": env_value}): - assert should_use_keyring() is True + def test_returns_false_when_disabled(self): + core_keyring.configure(KeyringSettings(no_keyring=True)) + assert should_use_keyring() is False class TestKeyringBackendAlias: @@ -617,9 +597,9 @@ class TestKeyringBackendAlias: def test_sets_python_keyring_backend_when_unset( self, mock_get_user, mock_get_password ): + core_keyring.configure(KeyringSettings(backend="keyring.backends.null.Keyring")) env = os.environ.copy() env.pop("PYTHON_KEYRING_BACKEND", None) - env["CLOUDSMITH_KEYRING_BACKEND"] = "keyring.backends.null.Keyring" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert ( @@ -629,9 +609,9 @@ def test_sets_python_keyring_backend_when_unset( def test_does_not_override_existing_python_keyring_backend( self, mock_get_user, mock_get_password ): + core_keyring.configure(KeyringSettings(backend="keyring.backends.null.Keyring")) env = os.environ.copy() env["PYTHON_KEYRING_BACKEND"] = "keyring.backends.SecretService.Keyring" - env["CLOUDSMITH_KEYRING_BACKEND"] = "keyring.backends.null.Keyring" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert ( @@ -642,7 +622,6 @@ def test_does_not_override_existing_python_keyring_backend( def test_no_op_when_alias_not_set(self, mock_get_user, mock_get_password): env = os.environ.copy() env.pop("PYTHON_KEYRING_BACKEND", None) - env.pop("CLOUDSMITH_KEYRING_BACKEND", None) with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert "PYTHON_KEYRING_BACKEND" not in os.environ @@ -663,9 +642,9 @@ class TestKeyringPropertyAlias: def test_sets_keyring_property_when_unset( self, mock_get_user, mock_get_password, mock_get_keyring ): + core_keyring.configure(KeyringSettings(key="super-secret")) env = os.environ.copy() env.pop("KEYRING_PROPERTY_KEYRING_KEY", None) - env["CLOUDSMITH_KEYRING_KEY"] = "super-secret" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert os.environ["KEYRING_PROPERTY_KEYRING_KEY"] == "super-secret" @@ -673,9 +652,9 @@ def test_sets_keyring_property_when_unset( def test_does_not_override_existing_keyring_property( self, mock_get_user, mock_get_password, mock_get_keyring ): + core_keyring.configure(KeyringSettings(key="alias-secret")) env = os.environ.copy() env["KEYRING_PROPERTY_KEYRING_KEY"] = "native-secret" - env["CLOUDSMITH_KEYRING_KEY"] = "alias-secret" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert os.environ["KEYRING_PROPERTY_KEYRING_KEY"] == "native-secret" @@ -685,7 +664,6 @@ def test_no_op_when_alias_not_set( ): env = os.environ.copy() env.pop("KEYRING_PROPERTY_KEYRING_KEY", None) - env.pop("CLOUDSMITH_KEYRING_KEY", None) with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert "KEYRING_PROPERTY_KEYRING_KEY" not in os.environ @@ -703,9 +681,9 @@ class TestKeyringFilePathAlias: api_host = "https://example.com" def test_sets_keyring_property_when_unset(self, mock_get_user, mock_get_password): + core_keyring.configure(KeyringSettings(file_path="/secure/path/keyring.cfg")) env = os.environ.copy() env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["CLOUDSMITH_KEYRING_FILE_PATH"] = "/secure/path/keyring.cfg" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert ( @@ -715,9 +693,9 @@ def test_sets_keyring_property_when_unset(self, mock_get_user, mock_get_password def test_does_not_override_existing_keyring_property( self, mock_get_user, mock_get_password ): + core_keyring.configure(KeyringSettings(file_path="/alias/path/keyring.cfg")) env = os.environ.copy() env["KEYRING_PROPERTY_FILE_PATH"] = "/native/path/keyring.cfg" - env["CLOUDSMITH_KEYRING_FILE_PATH"] = "/alias/path/keyring.cfg" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert ( @@ -727,41 +705,10 @@ def test_does_not_override_existing_keyring_property( def test_no_op_when_alias_not_set(self, mock_get_user, mock_get_password): env = os.environ.copy() env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env.pop("CLOUDSMITH_KEYRING_FILE_PATH", None) with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert "KEYRING_PROPERTY_FILE_PATH" not in os.environ - def test_expands_user_and_env_vars(self, mock_get_user, mock_get_password): - env = os.environ.copy() - env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["KEYRING_STORE_ROOT"] = "/secure" - env["CLOUDSMITH_KEYRING_FILE_PATH"] = "$KEYRING_STORE_ROOT/keyring.cfg" - with patch.dict(os.environ, env, clear=True): - get_access_token(self.api_host) - assert os.environ["KEYRING_PROPERTY_FILE_PATH"] == "/secure/keyring.cfg" - - def test_expands_home_directory(self, mock_get_user, mock_get_password): - env = os.environ.copy() - env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["CLOUDSMITH_KEYRING_FILE_PATH"] = "~/keyring.cfg" - with patch.dict(os.environ, env, clear=True): - get_access_token(self.api_host) - assert os.environ["KEYRING_PROPERTY_FILE_PATH"] == os.path.expanduser( - "~/keyring.cfg" - ) - - def test_expands_home_inside_env_var_value(self, mock_get_user, mock_get_password): - env = os.environ.copy() - env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["KEYRING_STORE_ROOT"] = "~" - env["CLOUDSMITH_KEYRING_FILE_PATH"] = "$KEYRING_STORE_ROOT/keyring.cfg" - with patch.dict(os.environ, env, clear=True): - get_access_token(self.api_host) - assert os.environ["KEYRING_PROPERTY_FILE_PATH"] == os.path.expanduser( - "~/keyring.cfg" - ) - class TestKeyringDirEnv: """Tests for CLOUDSMITH_KEYRING_DIR redirecting file-backed backends.""" @@ -773,9 +720,9 @@ def test_sets_backend_file_path( ): backend = mock_get_keyring.return_value backend.filename = "cryptfile_pass.cfg" + core_keyring.configure(KeyringSettings(directory="/secure/dir")) env = os.environ.copy() env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["CLOUDSMITH_KEYRING_DIR"] = "/secure/dir" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert backend.file_path == os.path.join( @@ -787,9 +734,9 @@ def test_native_file_path_wins_over_dir( ): backend = mock_get_keyring.return_value backend.filename = "cryptfile_pass.cfg" + core_keyring.configure(KeyringSettings(directory="/secure/dir")) env = os.environ.copy() env["KEYRING_PROPERTY_FILE_PATH"] = "/native/path/keyring.cfg" - env["CLOUDSMITH_KEYRING_DIR"] = "/secure/dir" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert backend.file_path == "/native/path/keyring.cfg" @@ -799,10 +746,13 @@ def test_alias_file_path_wins_over_dir( ): backend = mock_get_keyring.return_value backend.filename = "cryptfile_pass.cfg" + core_keyring.configure( + KeyringSettings( + file_path="/alias/path/keyring.cfg", directory="/secure/dir" + ) + ) env = os.environ.copy() env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["CLOUDSMITH_KEYRING_FILE_PATH"] = "/alias/path/keyring.cfg" - env["CLOUDSMITH_KEYRING_DIR"] = "/secure/dir" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert backend.file_path == "/alias/path/keyring.cfg" @@ -813,9 +763,9 @@ def test_no_op_for_backends_without_filename( backend = mock_get_keyring.return_value del backend.filename backend.file_path = "untouched" + core_keyring.configure(KeyringSettings(directory="/secure/dir")) env = os.environ.copy() env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["CLOUDSMITH_KEYRING_DIR"] = "/secure/dir" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert backend.file_path == "untouched" @@ -828,40 +778,10 @@ def test_no_op_when_dir_not_set( backend.file_path = "untouched" env = os.environ.copy() env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env.pop("CLOUDSMITH_KEYRING_DIR", None) with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert backend.file_path == "untouched" - def test_expands_user_and_env_vars( - self, mock_get_user, mock_get_password, mock_get_keyring - ): - backend = mock_get_keyring.return_value - backend.filename = "keyring_pass.cfg" - env = os.environ.copy() - env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["KEYRING_STORE_ROOT"] = "/secure" - env["CLOUDSMITH_KEYRING_DIR"] = "$KEYRING_STORE_ROOT/store" - with patch.dict(os.environ, env, clear=True): - get_access_token(self.api_host) - assert backend.file_path == os.path.join( - "/secure/store", "keyring_pass.cfg" - ) - - def test_expands_home_directory( - self, mock_get_user, mock_get_password, mock_get_keyring - ): - backend = mock_get_keyring.return_value - backend.filename = "keyring_pass.cfg" - env = os.environ.copy() - env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["CLOUDSMITH_KEYRING_DIR"] = "~/store" - with patch.dict(os.environ, env, clear=True): - get_access_token(self.api_host) - assert backend.file_path == os.path.join( - os.path.expanduser("~/store"), "keyring_pass.cfg" - ) - class TestKeyringFileRelocation: """Tests for file relocation with a real cryptfile backend.""" @@ -871,16 +791,9 @@ class TestKeyringFileRelocation: @staticmethod def _relocation_env(tmp_path): env = os.environ.copy() - for var in ( - "KEYRING_PROPERTY_FILE_PATH", - "KEYRING_PROPERTY_KEYRING_KEY", - "CLOUDSMITH_KEYRING_FILE_PATH", - "CLOUDSMITH_KEYRING_DIR", - "CLOUDSMITH_KEYRING_KEY", - ): + for var in ("KEYRING_PROPERTY_FILE_PATH", "KEYRING_PROPERTY_KEYRING_KEY"): env.pop(var, None) env["XDG_DATA_HOME"] = str(tmp_path / "default") - env["CLOUDSMITH_KEYRING_KEY"] = "test-password" return env def _roundtrip_token(self, backend, env): @@ -899,10 +812,11 @@ def test_dir_relocates_storage_before_unlock( ): backend = CryptFileKeyring() mock_get_keyring.return_value = backend - env = self._relocation_env(tmp_path) - env["CLOUDSMITH_KEYRING_DIR"] = str(tmp_path / "secure") + core_keyring.configure( + KeyringSettings(key="test-password", directory=str(tmp_path / "secure")) + ) - self._roundtrip_token(backend, env) + self._roundtrip_token(backend, self._relocation_env(tmp_path)) assert (tmp_path / "secure" / backend.filename).is_file() assert not (tmp_path / "default").exists() @@ -913,10 +827,11 @@ def test_file_path_alias_relocates_storage_before_unlock( backend = CryptFileKeyring() mock_get_keyring.return_value = backend target = tmp_path / "secure" / "tokens.cfg" - env = self._relocation_env(tmp_path) - env["CLOUDSMITH_KEYRING_FILE_PATH"] = str(target) + core_keyring.configure( + KeyringSettings(key="test-password", file_path=str(target)) + ) - self._roundtrip_token(backend, env) + self._roundtrip_token(backend, self._relocation_env(tmp_path)) assert target.is_file() assert not (tmp_path / "default").exists() @@ -963,16 +878,16 @@ def test_has_sso_tokens_returns_false_when_keyring_disabled( ): """has_sso_tokens should short-circuit when keyring is disabled.""" mock_get_password.return_value = "some_token" - with patch.dict(os.environ, {"CLOUDSMITH_NO_KEYRING": "1"}): - assert has_sso_tokens(self.api_host) is False + core_keyring.configure(KeyringSettings(no_keyring=True)) + assert has_sso_tokens(self.api_host) is False mock_get_password.assert_not_called() OIDC_ARGS = ("https://example.com", "my-org", "my-service") -def _keyring_env(tmp_path, **overrides): - return {"XDG_DATA_HOME": str(tmp_path), **overrides} +def _keyring_env(tmp_path): + return {"XDG_DATA_HOME": str(tmp_path)} @contextmanager @@ -1031,24 +946,24 @@ def test_store_without_terminal_skips_keyring( def test_supplied_key_uses_keyring_without_terminal( self, tmp_path, mock_get_user, backend_cls ): - env = _keyring_env(tmp_path, CLOUDSMITH_KEYRING_KEY="supplied-password") - with _keyring_session(backend_cls(), env): + core_keyring.configure(KeyringSettings(key="supplied-password")) + with _keyring_session(backend_cls(), _keyring_env(tmp_path)): assert store_oidc_token(*OIDC_ARGS, "token-data") is True assert get_oidc_token(*OIDC_ARGS) == "token-data" def test_wrong_key_reports_not_found(self, tmp_path, mock_get_user): _write_oidc_entry(CryptFileKeyring(), tmp_path) - env = _keyring_env(tmp_path, CLOUDSMITH_KEYRING_KEY="wrong-password") - with _keyring_session(CryptFileKeyring(), env): + core_keyring.configure(KeyringSettings(key="wrong-password")) + with _keyring_session(CryptFileKeyring(), _keyring_env(tmp_path)): assert get_oidc_token(*OIDC_ARGS) is None assert delete_oidc_token(*OIDC_ARGS) is False def test_key_unlocks_once_per_process(self, tmp_path, mock_get_user): _write_oidc_entry(CryptFileKeyring(), tmp_path) backend = CryptFileKeyring() - env = _keyring_env(tmp_path, CLOUDSMITH_KEYRING_KEY="earlier-password") + core_keyring.configure(KeyringSettings(key="earlier-password")) with ( - _keyring_session(backend, env), + _keyring_session(backend, _keyring_env(tmp_path)), patch.object(backend, "_unlock", wraps=backend._unlock) as unlock_mock, ): store_oidc_token(*OIDC_ARGS, "token-data") @@ -1074,12 +989,10 @@ def test_settings_reach_the_storage_member(self, tmp_path, mock_get_user): _write_oidc_entry(AltEncryptedKeyring(), tmp_path) storage_member = CryptFileKeyring() chain = _chain(storage_member, AltEncryptedKeyring()) - env = _keyring_env( - tmp_path, - CLOUDSMITH_KEYRING_KEY="supplied-password", - CLOUDSMITH_KEYRING_DIR=str(tmp_path / "secure"), + core_keyring.configure( + KeyringSettings(key="supplied-password", directory=str(tmp_path / "secure")) ) - with _keyring_session(chain, env): + with _keyring_session(chain, _keyring_env(tmp_path)): assert store_oidc_token(*OIDC_ARGS, "token-data") is True assert get_oidc_token(*OIDC_ARGS) == "token-data" assert (tmp_path / "secure" / storage_member.filename).is_file() @@ -1089,8 +1002,8 @@ def test_lower_member_with_an_existing_file_is_skipped( ): _write_oidc_entry(AltEncryptedKeyring(), tmp_path) chain = _chain(CryptFileKeyring(), AltEncryptedKeyring()) - env = _keyring_env(tmp_path, CLOUDSMITH_KEYRING_KEY="supplied-password") - with _keyring_session(chain, env): + core_keyring.configure(KeyringSettings(key="supplied-password")) + with _keyring_session(chain, _keyring_env(tmp_path)): assert get_oidc_token(*OIDC_ARGS) is None def test_write_fallback_cannot_prompt_for_a_new_file(self, tmp_path, mock_get_user): diff --git a/cloudsmith_cli/core/tests/test_keyring_provider.py b/cloudsmith_cli/core/tests/test_keyring_provider.py index f1729307c..17f4759f4 100644 --- a/cloudsmith_cli/core/tests/test_keyring_provider.py +++ b/cloudsmith_cli/core/tests/test_keyring_provider.py @@ -23,14 +23,13 @@ def mock_get_keyring(): class TestKeyringProvider: def test_returns_none_when_keyring_disabled(self): provider = KeyringProvider() - with patch.dict(os.environ, {"CLOUDSMITH_NO_KEYRING": "1"}): - result = provider.resolve(CredentialContext()) - assert result is None + keyring.configure(keyring.KeyringSettings(no_keyring=True)) + result = provider.resolve(CredentialContext()) + assert result is None def test_returns_none_when_no_token(self): provider = KeyringProvider() env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) with ( patch.dict(os.environ, env, clear=True), patch.object(keyring, "should_use_keyring", return_value=True), @@ -42,7 +41,6 @@ def test_returns_none_when_no_token(self): def test_returns_bearer_token(self): provider = KeyringProvider() env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) with ( patch.dict(os.environ, env, clear=True), patch.object(keyring, "should_use_keyring", return_value=True),