feat: add credential provider chain concept#275
feat: add credential provider chain concept#275cloudsmith-iduffy wants to merge 1 commit intomasterfrom
Conversation
cloudsmith-iduffy
force-pushed
the
iduffy/credential-provider-chain
branch
8 times, most recently
from
6e1792c to
8862812
Compare
cloudsmith-iduffy
force-pushed
the
iduffy/credential-provider-chain
branch
2 times, most recently
from
368db92 to
a60887d
Compare
cloudsmith-iduffy
changed the base branch from
iduffy/pre-release-cli
to
master
cloudsmith-iduffy
force-pushed
the
iduffy/credential-provider-chain
branch
from
1cef871 to
b4b2583
Compare
There was a problem hiding this comment.
Pull request overview
Introduces a pluggable “credential provider chain” abstraction for the Cloudsmith CLI and refactors API initialization to accept a resolved credential (API key or bearer token) instead of directly consulting keyring/config sources.
Changes:
- Added
CredentialContext,CredentialResult,CredentialProvider, andCredentialProviderChainplus initial providers (keyring + API-key-from-context). - Refactored CLI initialization flow to (1) create a shared
requests.Session, (2) resolve credentials via the chain, then (3) initializecloudsmith_api.Configurationwith the resolved credential. - Updated/added tests to cover the new chain/provider behavior and adjusted existing tests to avoid httpretty/urllib3 socket shutdown issues.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| cloudsmith_cli/core/tests/test_rest.py | Simplifies test isolation by patching httpretty fake socket shutdown globally for the module. |
| cloudsmith_cli/core/tests/test_keyring_provider.py | Adds unit tests for the new keyring credential provider behavior. |
| cloudsmith_cli/core/tests/test_init.py | Updates API init tests to validate credential-based auth wiring rather than keyring-driven behavior. |
| cloudsmith_cli/core/tests/test_credential_provider_chain.py | Adds tests covering provider ordering, fallthrough, and error-skipping behavior. |
| cloudsmith_cli/core/tests/test_credential_context.py | Adds basic tests for CredentialContext defaults/mutability. |
| cloudsmith_cli/core/tests/test_cli_flag_provider.py | Adds tests for resolving an API key from CredentialContext. |
| cloudsmith_cli/core/credentials/session.py | Introduces a shared session factory with retry/proxy/headers support. |
| cloudsmith_cli/core/credentials/providers/keyring_provider.py | Implements keyring-backed SSO token resolution and refresh behavior. |
| cloudsmith_cli/core/credentials/providers/cli_flag.py | Implements API key resolution from CredentialContext.api_key. |
| cloudsmith_cli/core/credentials/providers/init.py | Exposes provider classes for the chain default import path. |
| cloudsmith_cli/core/credentials/init.py | Defines the core credential chain abstractions and default provider ordering. |
| cloudsmith_cli/core/api/init.py | Refactors API initialization to apply auth based on a passed-in credential object. |
| cloudsmith_cli/cli/webserver.py | Updates SSO callback flow to pass bearer credential into initialise_api(). |
| cloudsmith_cli/cli/tests/test_webserver.py | Adjusts assertions to validate credential object usage rather than access_token=. |
| cloudsmith_cli/cli/tests/conftest.py | Updates test helpers to initialize API with a CredentialResult instead of key=. |
| cloudsmith_cli/cli/decorators.py | Adds session initialization + credential resolution decorators and wires them into API init. |
| cloudsmith_cli/cli/commands/whoami.py | Changes verbose “API key source” reporting to read from the resolved credential result. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def create_session( | ||
| proxy: str | None = None, | ||
| ssl_verify: bool = True, | ||
| user_agent: str | None = None, | ||
| headers: dict | None = None, | ||
| api_key: str | None = None, | ||
| retry: Retry | None = DEFAULT_RETRY, |
There was a problem hiding this comment.
In create_session(), the parameter name api_key is used to set a Bearer Authorization header. This is easy to misinterpret as an X-Api-Key value (and conflicts with other parts of the CLI that use "api_key" to mean X-Api-Key). Consider renaming this parameter (and docstring) to something like bearer_token/auth_token, or accepting a CredentialResult/headers instead.
| """Keyring credential provider.""" | ||
|
|
There was a problem hiding this comment.
This is a new non-test source file but it doesn't include the project’s standard copyright header near the top (see e.g. cloudsmith_cli/cli/commands/logout.py). Consider adding a "Copyright 2026 Cloudsmith Ltd" comment above the module docstring.
| credential = getattr(opts, "credential", None) | ||
| if credential: | ||
| return { | ||
| "configured": True, | ||
| "source": credential.source_detail or credential.source_name, | ||
| "source_key": credential.source_name, | ||
| } |
There was a problem hiding this comment.
get_api_key_source() now relies entirely on opts.credential.source* and no longer distinguishes where the API key came from (CLI flag vs CLOUDSMITH_API_KEY vs credentials.ini/profile). With the current provider chain (keyring + CLIFlagProvider), this will report a generic/incorrect source and loses the file-path detail that was previously shown. Consider adding dedicated providers for env-var and credentials.ini (using creds_file_path/profile) and/or passing Click parameter source into CredentialContext so the resolved CredentialResult accurately reflects the real source.
| if keyring.should_refresh_access_token(api_host): | ||
| if not context.session: | ||
| return None | ||
| refresh_token = keyring.get_refresh_token(api_host) | ||
| new_access_token, new_refresh_token = refresh_access_token( |
There was a problem hiding this comment.
KeyringProvider's token refresh success path (refresh_access_token + store_sso_tokens + returning the new access token) isn't covered by tests in this PR; only the non-refresh and failure paths are tested. Adding a test for the successful refresh branch would help prevent regressions in SSO auth refresh behavior.
| """Credential Provider Chain for Cloudsmith CLI. | ||
|
|
There was a problem hiding this comment.
This is a new non-test source file but it doesn't include the project’s standard copyright header near the top (see e.g. cloudsmith_cli/cli/commands/logout.py). Consider adding a "Copyright 2026 Cloudsmith Ltd" comment above the module docstring.
| """HTTP session factory with networking configuration and retry support.""" | ||
|
|
There was a problem hiding this comment.
This is a new non-test source file but it doesn't include the project’s standard copyright header near the top (see e.g. cloudsmith_cli/cli/commands/logout.py). Consider adding a "Copyright 2026 Cloudsmith Ltd" comment above the module docstring.
| """CLI flag credential provider.""" | ||
|
|
There was a problem hiding this comment.
This is a new non-test source file but it doesn't include the project’s standard copyright header near the top (see e.g. cloudsmith_cli/cli/commands/logout.py). Consider adding a "Copyright 2026 Cloudsmith Ltd" comment above the module docstring.
| """Credential providers for the Cloudsmith CLI.""" | ||
|
|
||
| from .cli_flag import CLIFlagProvider |
There was a problem hiding this comment.
This is a new non-test source file but it doesn't include the project’s standard copyright header near the top (see e.g. cloudsmith_cli/cli/commands/logout.py). Consider adding a "Copyright 2026 Cloudsmith Ltd" comment above the module docstring.
| return CredentialResult( | ||
| api_key=api_key.strip(), | ||
| source_name="cli_flag", | ||
| source_detail=f"--api-key flag, CLOUDSMITH_API_KEY, or credentials.ini (ends with ...{suffix})", |
There was a problem hiding this comment.
CLIFlagProvider always returns source_name="cli_flag" and a source_detail that claims the key could have come from env vars or credentials.ini, but the provider only inspects context.api_key and cannot actually determine the origin. This makes verbose output (e.g., whoami) misleading. Consider splitting this into separate providers (cli flag vs env var vs credentials file) or redesigning CredentialContext to carry a reliable source indicator so source_name/source_detail stay accurate.
| source_detail=f"--api-key flag, CLOUDSMITH_API_KEY, or credentials.ini (ends with ...{suffix})", | |
| # the exact origin of context.api_key is not known here; only the value is | |
| source_detail=f"API key supplied via CLI credential context (ends with ...{suffix})", |
| if credential.auth_type == "bearer": | ||
| config.headers["Authorization"] = f"Bearer {credential.api_key}" | ||
| if config.debug: | ||
| click.echo("SSO access token config value set") |
There was a problem hiding this comment.
The debug message "SSO access token config value set" is emitted for any credential with auth_type="bearer", even though bearer credentials may not always be SSO-derived (e.g., future OIDC/process providers). Consider making this message generic (e.g., "Bearer token config value set") or include credential.source_name to avoid misleading debug output.
| click.echo("SSO access token config value set") | |
| click.echo("Bearer token config value set") |
| from ....cli.saml import refresh_access_token | ||
| from ....core import keyring |
There was a problem hiding this comment.
nab: can these be top-level imports instead? Or are there any cyclic imports?
| from __future__ import annotations | ||
|
|
||
| import logging | ||
| from abc import ABC, abstractmethod | ||
| from dataclasses import dataclass | ||
| from typing import TYPE_CHECKING | ||
|
|
||
| if TYPE_CHECKING: | ||
| import requests | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| @dataclass | ||
| class CredentialContext: | ||
| """Context passed to credential providers during resolution. | ||
|
|
||
| All values are populated directly from Click options / ``opts``. | ||
| """ | ||
|
|
||
| session: requests.Session | None = None | ||
| api_key: str | None = None | ||
| api_host: str = "https://api.cloudsmith.io" | ||
| creds_file_path: str | None = None | ||
| profile: str | None = None | ||
| debug: bool = False | ||
| keyring_refresh_failed: bool = False | ||
|
|
||
|
|
||
| @dataclass | ||
| class CredentialResult: | ||
| """Result from a successful credential resolution.""" | ||
|
|
||
| api_key: str | ||
| source_name: str | ||
| source_detail: str | None = None | ||
| auth_type: str = "api_key" | ||
|
|
||
|
|
||
| class CredentialProvider(ABC): | ||
| """Base class for credential providers.""" | ||
|
|
||
| name: str = "base" | ||
|
|
||
| @abstractmethod | ||
| def resolve(self, context: CredentialContext) -> CredentialResult | None: | ||
| """Attempt to resolve credentials. Return CredentialResult or None.""" | ||
|
|
||
|
|
||
| class CredentialProviderChain: | ||
| """Evaluates credential providers in order, returning the first valid result. | ||
|
|
||
| If no providers are given, uses the default chain: | ||
| Keyring → CLIFlag. | ||
| """ | ||
|
|
||
| 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(), | ||
| ] | ||
|
|
||
| def resolve(self, context: CredentialContext) -> CredentialResult | None: | ||
| """Evaluate each provider in order. Return the first successful result.""" | ||
| for provider in self.providers: | ||
| try: | ||
| result = provider.resolve(context) | ||
| if result is not None: | ||
| if context.debug: | ||
| logger.debug( | ||
| "Credentials resolved by %s: %s", | ||
| provider.name, | ||
| result.source_detail or result.source_name, | ||
| ) | ||
| return result | ||
| if context.debug: | ||
| logger.debug( | ||
| "Provider %s did not resolve credentials, trying next", | ||
| provider.name, | ||
| ) | ||
| except Exception: # pylint: disable=broad-exception-caught | ||
| # Intentionally broad - one provider failing shouldn't stop others | ||
| logger.debug( | ||
| "Provider %s raised an exception, skipping", | ||
| provider.name, | ||
| exc_info=True, | ||
| ) | ||
| continue | ||
| return None |
There was a problem hiding this comment.
question: why are these in the __init__.py we try to only use this file for exposing the module's imports.
| ) | ||
|
|
||
|
|
||
| def create_session( |
There was a problem hiding this comment.
question: the current create_requests_session function can't be used?
| if providers is not None: | ||
| self.providers = providers | ||
| else: | ||
| from .providers import CLIFlagProvider, KeyringProvider |
There was a problem hiding this comment.
issue: move to top-level import
Description
The goal here is to introduce a credential provider chain similar to https://docs.aws.amazon.com/sdkref/latest/guide/standardized-credentials.html to give us a pluggable system for inserting various methods of authentication, such as api keys in envvars, cli params, config, automatic discovery of the environment you're running on, OIDC exchange, running an external process, etc.
Type of Change
Additional Notes
I manually tested this to a large extent the results of this are below
Credential Provider Chain — Test Results
Comparison of published CLI (
uvx --from=cloudsmith-cli) vs this branch.API Key Scenarios
Scenario 1: Anonymous (no credentials)
Published CLI:
Local CLI (this branch):
Scenario 2: API key via CLOUDSMITH_API_KEY env var
Published CLI:
Local CLI (this branch):
Scenario 3: API key via --api-key flag
Published CLI:
Local CLI (this branch):
Scenario 4: API key via -k short flag
Published CLI:
Local CLI (this branch):
Scenario 5: API key via credentials.ini (~/.cloudsmith/)
Published CLI:
Local CLI (this branch):
Scenario 6: API key via credentials.ini (app dir)
Published CLI:
Local CLI (this branch):
Scenario 7: API key via credentials.ini (current dir)
Published CLI:
Local CLI (this branch):
Scenario 8: API key via --credentials-file flag
Published CLI:
Local CLI (this branch):
Scenario 9: API key via CLOUDSMITH_CREDENTIALS_FILE env var
Published CLI:
Local CLI (this branch):
Scenario 10: Profile via -P flag
credentials.ini has empty [default] and key in [profile:staging]
Published CLI:
Local CLI (this branch):
Scenario 11: Profile via CLOUDSMITH_PROFILE env var
credentials.ini has empty [default] and key in [profile:staging]
Published CLI:
Local CLI (this branch):
Priority / Override Scenarios
Scenario 12: Priority: --api-key flag vs env var
Both set. Source should show CLI flag.
Published CLI:
Local CLI (this branch):
Scenario 13: Priority: env var vs credentials.ini
Both set. Source should show env var.
Published CLI:
Local CLI (this branch):
Scenario 14: Priority: --api-key flag vs credentials.ini
Both set. Source should show CLI flag.
Published CLI:
Local CLI (this branch):
Scenario 15: Priority: flag + env var + config (all set)
All three set. CLI flag should win.
Published CLI:
Local CLI (this branch):
SSO / Keyring Scenarios
Removed SSO tokens from system keyring.
Note: credentials.ini was not modified (--keyring-only).
Scenario 16: SSO token via keyring (whoami)
After
cloudsmith auth -o iduffy-demo— SSO token stored in system keyring.Local CLI (this branch):
Scenario 17: Priority: SSO token vs env var
SSO token in keyring + CLOUDSMITH_API_KEY set. SSO token should take priority.
Local CLI (this branch):
Scenario 18: Priority: SSO token vs credentials.ini
SSO token in keyring + API key in credentials.ini. SSO token should take priority.
Local CLI (this branch):
Scenario 19: CLOUDSMITH_NO_KEYRING=1 bypasses SSO
SSO token in keyring but CLOUDSMITH_NO_KEYRING=1. Should fall through to anonymous.
Local CLI (this branch):
Scenario 20: Logout clears SSO token
After
cloudsmith logout --keyring-only, SSO token should be cleared.Logout output:
Local CLI (this branch) — whoami after logout: