diff --git a/cloudsmith_cli/cli/config.py b/cloudsmith_cli/cli/config.py index ccbb3883..27b58750 100644 --- a/cloudsmith_cli/cli/config.py +++ b/cloudsmith_cli/cli/config.py @@ -319,6 +319,33 @@ def api_key(self, value): """Set value for API key.""" self._set_option("api_key", value) + @property + def api_key_from_flag(self): + """Get API key set explicitly via --api-key CLI flag.""" + return self._get_option("api_key_from_flag") + + @api_key_from_flag.setter + def api_key_from_flag(self, value): + self._set_option("api_key_from_flag", value, allow_clear=True) + + @property + def api_key_from_env(self): + """Get API key from CLOUDSMITH_API_KEY environment variable.""" + return self._get_option("api_key_from_env") + + @api_key_from_env.setter + def api_key_from_env(self, value): + self._set_option("api_key_from_env", value, allow_clear=True) + + @property + def api_key_from_file(self): + """Get API key loaded from credentials.ini.""" + return self._get_option("api_key_from_file") + + @api_key_from_file.setter + def api_key_from_file(self, value): + self._set_option("api_key_from_file", value, allow_clear=True) + @property def api_proxy(self): """Get value for API proxy.""" diff --git a/cloudsmith_cli/cli/decorators.py b/cloudsmith_cli/cli/decorators.py index bda1fd69..9a8d595d 100644 --- a/cloudsmith_cli/cli/decorators.py +++ b/cloudsmith_cli/cli/decorators.py @@ -3,6 +3,7 @@ import functools import click +from click.core import ParameterSource from cloudsmith_cli.cli import validators @@ -116,6 +117,7 @@ def wrapper(ctx, *args, **kwargs): opts.load_config_file(path=config_file, profile=profile) opts.load_creds_file(path=creds_file, profile=profile) + opts.api_key_from_file = opts.api_key kwargs["opts"] = opts return ctx.invoke(f, *args, **kwargs) @@ -226,6 +228,20 @@ def wrapper(ctx, *args, **kwargs): # pylint: disable=missing-docstring opts = config.get_or_create_options(ctx) api_key = kwargs.pop("api_key") + + source = ctx.get_parameter_source("api_key") + api_key_nonempty = api_key and api_key.strip() + if source == ParameterSource.COMMANDLINE and api_key_nonempty: + opts.api_key_from_flag = api_key + opts.api_key_from_env = None + elif source == ParameterSource.ENVIRONMENT and api_key_nonempty: + opts.api_key_from_flag = None + opts.api_key_from_env = api_key + else: + opts.api_key_from_flag = None + opts.api_key_from_env = None + + # Keep opts.api_key populated for any code that still reads it directly. if api_key: opts.api_key = api_key kwargs["opts"] = opts @@ -302,7 +318,9 @@ def wrapper(ctx, *args, **kwargs): context = CredentialContext( session=opts.session, - api_key=opts.api_key, + api_key_from_flag=opts.api_key_from_flag, + api_key_from_env=opts.api_key_from_env, + api_key_from_file=opts.api_key_from_file, api_host=opts.api_host or "https://api.cloudsmith.io", creds_file_path=ctx.meta.get("creds_file"), profile=ctx.meta.get("profile"), diff --git a/cloudsmith_cli/core/credentials/chain.py b/cloudsmith_cli/core/credentials/chain.py index 259490fc..7dda267b 100644 --- a/cloudsmith_cli/core/credentials/chain.py +++ b/cloudsmith_cli/core/credentials/chain.py @@ -18,18 +18,25 @@ class CredentialProviderChain: """Evaluates credential providers in order, returning the first valid result. If no providers are given, uses the default chain: - Keyring → CLIFlag. + CLIFlag → EnvVar → CredentialsFile → Keyring. """ def __init__(self, providers: list[CredentialProvider] | None = None): if providers is not None: self.providers = providers else: - from .providers import CLIFlagProvider, KeyringProvider + from .providers import ( + CLIFlagProvider, + CredentialsFileProvider, + EnvVarProvider, + KeyringProvider, + ) self.providers = [ - KeyringProvider(), CLIFlagProvider(), + EnvVarProvider(), + CredentialsFileProvider(), + KeyringProvider(), ] def resolve(self, context: CredentialContext) -> CredentialResult | None: diff --git a/cloudsmith_cli/core/credentials/models.py b/cloudsmith_cli/core/credentials/models.py index b6245ac2..903a9d0c 100644 --- a/cloudsmith_cli/core/credentials/models.py +++ b/cloudsmith_cli/core/credentials/models.py @@ -12,11 +12,15 @@ class CredentialContext: """Context passed to credential providers during resolution. - All values are populated directly from Click options / ``opts``. + Separate per-source fields allow the chain to evaluate sources in priority + order without conflating them. Populated from Click options in + ``resolve_credentials``. """ session: requests.Session | None = None - api_key: str | None = None + api_key_from_flag: str | None = None + api_key_from_env: str | None = None + api_key_from_file: str | None = None api_host: str = "https://api.cloudsmith.io" creds_file_path: str | None = None profile: str | None = None diff --git a/cloudsmith_cli/core/credentials/providers/__init__.py b/cloudsmith_cli/core/credentials/providers/__init__.py index 5482e397..b00404b0 100644 --- a/cloudsmith_cli/core/credentials/providers/__init__.py +++ b/cloudsmith_cli/core/credentials/providers/__init__.py @@ -1,9 +1,13 @@ """Credential providers for the Cloudsmith CLI.""" from .cli_flag import CLIFlagProvider +from .credentials_file import CredentialsFileProvider +from .env_var import EnvVarProvider from .keyring_provider import KeyringProvider __all__ = [ "CLIFlagProvider", + "CredentialsFileProvider", + "EnvVarProvider", "KeyringProvider", ] diff --git a/cloudsmith_cli/core/credentials/providers/cli_flag.py b/cloudsmith_cli/core/credentials/providers/cli_flag.py index 7c91a12c..2701c2a5 100644 --- a/cloudsmith_cli/core/credentials/providers/cli_flag.py +++ b/cloudsmith_cli/core/credentials/providers/cli_flag.py @@ -7,17 +7,18 @@ class CLIFlagProvider(CredentialProvider): - """Resolves credentials from a CLI flag value passed via CredentialContext.""" + """Resolves credentials from the --api-key CLI flag.""" name = "cli_flag" def resolve(self, context: CredentialContext) -> CredentialResult | None: - api_key = context.api_key + api_key = context.api_key_from_flag if api_key and api_key.strip(): - suffix = api_key.strip()[-4:] + api_key = api_key.strip() + suffix = api_key[-4:] return CredentialResult( - api_key=api_key.strip(), + api_key=api_key, source_name="cli_flag", - source_detail=f"key resolved via CLI options (ends with ...{suffix})", + source_detail=f"--api-key flag (ends with ...{suffix})", ) return None diff --git a/cloudsmith_cli/core/credentials/providers/credentials_file.py b/cloudsmith_cli/core/credentials/providers/credentials_file.py new file mode 100644 index 00000000..ce354f06 --- /dev/null +++ b/cloudsmith_cli/core/credentials/providers/credentials_file.py @@ -0,0 +1,24 @@ +"""Credentials file provider.""" + +from __future__ import annotations + +from ..models import CredentialContext, CredentialResult +from ..provider import CredentialProvider + + +class CredentialsFileProvider(CredentialProvider): + """Resolves credentials from the api_key stored in credentials.ini.""" + + name = "credentials_file" + + def resolve(self, context: CredentialContext) -> CredentialResult | None: + api_key = context.api_key_from_file + if api_key and api_key.strip(): + api_key = api_key.strip() + suffix = api_key[-4:] + return CredentialResult( + api_key=api_key, + source_name="credentials_file", + source_detail=f"credentials.ini (ends with ...{suffix})", + ) + return None diff --git a/cloudsmith_cli/core/credentials/providers/env_var.py b/cloudsmith_cli/core/credentials/providers/env_var.py new file mode 100644 index 00000000..6fbd7a0d --- /dev/null +++ b/cloudsmith_cli/core/credentials/providers/env_var.py @@ -0,0 +1,24 @@ +"""Environment variable credential provider.""" + +from __future__ import annotations + +from ..models import CredentialContext, CredentialResult +from ..provider import CredentialProvider + + +class EnvVarProvider(CredentialProvider): + """Resolves credentials from the CLOUDSMITH_API_KEY environment variable.""" + + name = "env_var" + + def resolve(self, context: CredentialContext) -> CredentialResult | None: + api_key = context.api_key_from_env + if api_key and api_key.strip(): + api_key = api_key.strip() + suffix = api_key[-4:] + return CredentialResult( + api_key=api_key, + source_name="env_var", + source_detail=f"CLOUDSMITH_API_KEY env var (ends with ...{suffix})", + ) + return None diff --git a/cloudsmith_cli/core/tests/test_cli_flag_provider.py b/cloudsmith_cli/core/tests/test_cli_flag_provider.py index dc4c0ff7..9d1f5db0 100644 --- a/cloudsmith_cli/core/tests/test_cli_flag_provider.py +++ b/cloudsmith_cli/core/tests/test_cli_flag_provider.py @@ -5,30 +5,42 @@ class TestCLIFlagProvider: - def test_resolves_from_context(self): + def test_resolves_from_flag(self): provider = CLIFlagProvider() - context = CredentialContext(api_key="my-api-key-1234") + context = CredentialContext(api_key_from_flag="my-api-key-1234") result = provider.resolve(context) assert result is not None assert result.api_key == "my-api-key-1234" assert result.source_name == "cli_flag" assert result.auth_type == "api_key" assert "1234" in result.source_detail + assert "--api-key" in result.source_detail def test_returns_none_when_not_set(self): provider = CLIFlagProvider() - context = CredentialContext(api_key=None) + context = CredentialContext(api_key_from_flag=None) result = provider.resolve(context) assert result is None def test_returns_none_for_empty_value(self): provider = CLIFlagProvider() - context = CredentialContext(api_key=" ") + context = CredentialContext(api_key_from_flag=" ") result = provider.resolve(context) assert result is None def test_strips_whitespace(self): provider = CLIFlagProvider() - context = CredentialContext(api_key=" my-key ") + context = CredentialContext(api_key_from_flag=" my-key ") result = provider.resolve(context) assert result.api_key == "my-key" + + def test_ignores_env_and_file_keys(self): + """CLIFlagProvider must not resolve keys from other sources.""" + provider = CLIFlagProvider() + context = CredentialContext( + api_key_from_flag=None, + api_key_from_env="env-key", + api_key_from_file="file-key", + ) + result = provider.resolve(context) + assert result is None diff --git a/cloudsmith_cli/core/tests/test_credential_chain_priority.py b/cloudsmith_cli/core/tests/test_credential_chain_priority.py new file mode 100644 index 00000000..a8b76128 --- /dev/null +++ b/cloudsmith_cli/core/tests/test_credential_chain_priority.py @@ -0,0 +1,135 @@ +"""Integration tests proving correct credential resolution priority. + +Priority (highest → lowest): + --api-key CLI flag > CLOUDSMITH_API_KEY env var > credentials.ini > keyring SSO +""" + +from unittest.mock import patch + +from cloudsmith_cli.core import keyring +from cloudsmith_cli.core.credentials.chain import CredentialProviderChain +from cloudsmith_cli.core.credentials.models import CredentialContext + + +class TestCredentialChainPriority: + def _context(self, **kwargs): + return CredentialContext( + api_host="https://api.cloudsmith.io", + **kwargs, + ) + + def test_cli_flag_beats_keyring(self): + """Explicit --api-key must win over a keyring SSO token.""" + context = self._context(api_key_from_flag="explicit-flag-key") + + with patch.object(keyring, "should_use_keyring", return_value=True): + with patch.object(keyring, "get_access_token", return_value="sso-token"): + with patch.object( + keyring, "should_refresh_access_token", return_value=False + ): + chain = CredentialProviderChain() + result = chain.resolve(context) + + assert result is not None + assert result.api_key == "explicit-flag-key" + assert result.source_name == "cli_flag" + + def test_env_var_beats_keyring(self): + """CLOUDSMITH_API_KEY env var must win over keyring SSO.""" + context = self._context(api_key_from_env="env-key") + + with patch.object(keyring, "should_use_keyring", return_value=True): + with patch.object(keyring, "get_access_token", return_value="sso-token"): + with patch.object( + keyring, "should_refresh_access_token", return_value=False + ): + chain = CredentialProviderChain() + result = chain.resolve(context) + + assert result is not None + assert result.api_key == "env-key" + assert result.source_name == "env_var" + + def test_credentials_file_beats_keyring(self): + """credentials.ini must win over keyring SSO.""" + context = self._context(api_key_from_file="file-key") + + with patch.object(keyring, "should_use_keyring", return_value=True): + with patch.object(keyring, "get_access_token", return_value="sso-token"): + with patch.object( + keyring, "should_refresh_access_token", return_value=False + ): + chain = CredentialProviderChain() + result = chain.resolve(context) + + assert result is not None + assert result.api_key == "file-key" + assert result.source_name == "credentials_file" + + def test_cli_flag_beats_env_var(self): + """--api-key CLI flag must win over CLOUDSMITH_API_KEY env var.""" + context = self._context( + api_key_from_flag="flag-key", + api_key_from_env="env-key", + ) + + chain = CredentialProviderChain() + result = chain.resolve(context) + + assert result is not None + assert result.api_key == "flag-key" + assert result.source_name == "cli_flag" + + def test_cli_flag_beats_credentials_file(self): + """--api-key CLI flag must win over credentials.ini key.""" + context = self._context( + api_key_from_flag="flag-key", + api_key_from_file="file-key", + ) + + chain = CredentialProviderChain() + result = chain.resolve(context) + + assert result is not None + assert result.api_key == "flag-key" + assert result.source_name == "cli_flag" + + def test_env_var_beats_credentials_file(self): + """CLOUDSMITH_API_KEY env var must win over credentials.ini key.""" + context = self._context( + api_key_from_env="env-key", + api_key_from_file="file-key", + ) + + chain = CredentialProviderChain() + result = chain.resolve(context) + + assert result is not None + assert result.api_key == "env-key" + assert result.source_name == "env_var" + + def test_keyring_used_when_no_explicit_key(self): + """Keyring SSO is the fallback when no explicit keys are set.""" + context = self._context() + + with patch.object(keyring, "should_use_keyring", return_value=True): + with patch.object(keyring, "get_access_token", return_value="sso-token"): + with patch.object( + keyring, "should_refresh_access_token", return_value=False + ): + chain = CredentialProviderChain() + result = chain.resolve(context) + + assert result is not None + assert result.api_key == "sso-token" + assert result.source_name == "keyring" + + def test_no_credentials_returns_none(self): + """Returns None when no source provides credentials.""" + context = self._context() + + with patch.object(keyring, "should_use_keyring", return_value=False): + chain = CredentialProviderChain() + result = chain.resolve(context) + + assert result is None diff --git a/cloudsmith_cli/core/tests/test_credential_context.py b/cloudsmith_cli/core/tests/test_credential_context.py index 49dc57d9..ca562993 100644 --- a/cloudsmith_cli/core/tests/test_credential_context.py +++ b/cloudsmith_cli/core/tests/test_credential_context.py @@ -4,6 +4,12 @@ class TestCredentialContext: + def test_all_key_sources_default_none(self): + context = CredentialContext() + assert context.api_key_from_flag is None + assert context.api_key_from_env is None + assert context.api_key_from_file is None + def test_keyring_refresh_failed_defaults_false(self): context = CredentialContext() assert context.keyring_refresh_failed is False @@ -12,3 +18,13 @@ def test_keyring_refresh_failed_can_be_set(self): context = CredentialContext() context.keyring_refresh_failed = True assert context.keyring_refresh_failed is True + + def test_per_source_fields_are_independent(self): + context = CredentialContext( + api_key_from_flag="flag-key", + api_key_from_env="env-key", + api_key_from_file="file-key", + ) + assert context.api_key_from_flag == "flag-key" + assert context.api_key_from_env == "env-key" + assert context.api_key_from_file == "file-key" diff --git a/cloudsmith_cli/core/tests/test_credential_provider_chain.py b/cloudsmith_cli/core/tests/test_credential_provider_chain.py index 58cddb00..d7893732 100644 --- a/cloudsmith_cli/core/tests/test_credential_provider_chain.py +++ b/cloudsmith_cli/core/tests/test_credential_provider_chain.py @@ -72,6 +72,8 @@ def test_empty_chain(self): def test_default_chain_order(self): chain = CredentialProviderChain() - assert len(chain.providers) == 2 - assert chain.providers[0].name == "keyring" - assert chain.providers[1].name == "cli_flag" + assert len(chain.providers) == 4 + assert chain.providers[0].name == "cli_flag" + assert chain.providers[1].name == "env_var" + assert chain.providers[2].name == "credentials_file" + assert chain.providers[3].name == "keyring" diff --git a/cloudsmith_cli/core/tests/test_credentials_file_provider.py b/cloudsmith_cli/core/tests/test_credentials_file_provider.py new file mode 100644 index 00000000..59adfbe4 --- /dev/null +++ b/cloudsmith_cli/core/tests/test_credentials_file_provider.py @@ -0,0 +1,47 @@ +"""Tests for the credentials file credential provider.""" + +from cloudsmith_cli.core.credentials.models import CredentialContext +from cloudsmith_cli.core.credentials.providers.credentials_file import ( + CredentialsFileProvider, +) + + +class TestCredentialsFileProvider: + def test_resolves_from_file(self): + provider = CredentialsFileProvider() + context = CredentialContext(api_key_from_file="file-api-key-9012") + result = provider.resolve(context) + assert result is not None + assert result.api_key == "file-api-key-9012" + assert result.source_name == "credentials_file" + assert result.auth_type == "api_key" + assert "9012" in result.source_detail + assert "credentials.ini" in result.source_detail + + def test_returns_none_when_not_set(self): + provider = CredentialsFileProvider() + context = CredentialContext(api_key_from_file=None) + result = provider.resolve(context) + assert result is None + + def test_returns_none_for_empty_string(self): + provider = CredentialsFileProvider() + context = CredentialContext(api_key_from_file=" ") + result = provider.resolve(context) + assert result is None + + def test_strips_whitespace(self): + provider = CredentialsFileProvider() + context = CredentialContext(api_key_from_file=" file-key ") + result = provider.resolve(context) + assert result.api_key == "file-key" + + def test_ignores_flag_and_env_keys(self): + provider = CredentialsFileProvider() + context = CredentialContext( + api_key_from_flag="flag-key", + api_key_from_env="env-key", + api_key_from_file=None, + ) + result = provider.resolve(context) + assert result is None diff --git a/cloudsmith_cli/core/tests/test_env_var_provider.py b/cloudsmith_cli/core/tests/test_env_var_provider.py new file mode 100644 index 00000000..9e537d10 --- /dev/null +++ b/cloudsmith_cli/core/tests/test_env_var_provider.py @@ -0,0 +1,45 @@ +"""Tests for the environment variable credential provider.""" + +from cloudsmith_cli.core.credentials.models import CredentialContext +from cloudsmith_cli.core.credentials.providers.env_var import EnvVarProvider + + +class TestEnvVarProvider: + def test_resolves_from_env(self): + provider = EnvVarProvider() + context = CredentialContext(api_key_from_env="env-api-key-5678") + result = provider.resolve(context) + assert result is not None + assert result.api_key == "env-api-key-5678" + assert result.source_name == "env_var" + assert result.auth_type == "api_key" + assert "5678" in result.source_detail + assert "CLOUDSMITH_API_KEY" in result.source_detail + + def test_returns_none_when_not_set(self): + provider = EnvVarProvider() + context = CredentialContext(api_key_from_env=None) + result = provider.resolve(context) + assert result is None + + def test_returns_none_for_empty_string(self): + provider = EnvVarProvider() + context = CredentialContext(api_key_from_env=" ") + result = provider.resolve(context) + assert result is None + + def test_strips_whitespace(self): + provider = EnvVarProvider() + context = CredentialContext(api_key_from_env=" my-env-key ") + result = provider.resolve(context) + assert result.api_key == "my-env-key" + + def test_ignores_flag_and_file_keys(self): + provider = EnvVarProvider() + context = CredentialContext( + api_key_from_flag="flag-key", + api_key_from_env=None, + api_key_from_file="file-key", + ) + result = provider.resolve(context) + assert result is None