refactor: change authentication order and seperate out existing crede… (#296)

* feat: add credential provider chain concept

* fix: review feedback

* fix: metadata test

* fix: claude local review

* refactor: change authentication order and seperate out existing credential providers

* fix: test

* fix: decorator copilot feedback

* fix: move to top level import

---------

Co-authored-by: Ian Duffy <iduffy@cloudsmith.io>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>

Author: 3 people

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."""

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"),

cloudsmith_cli/core/credentials/chain.py

@@ -10,6 +10,12 @@
 
 from .models import CredentialContext, CredentialResult
 from .provider import CredentialProvider
+from .providers import (
+    CLIFlagProvider,
+    CredentialsFileProvider,
+    EnvVarProvider,
+    KeyringProvider,
+)
 
 logger = logging.getLogger(__name__)
 
@@ -18,18 +24,18 @@ 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
-
             self.providers = [
-                KeyringProvider(),
                 CLIFlagProvider(),
+                EnvVarProvider(),
+                CredentialsFileProvider(),
+                KeyringProvider(),
             ]
 
     def resolve(self, context: CredentialContext) -> CredentialResult | None:

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

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",
 ]

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

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

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

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