refactor(no-ticket): pass resolved credentials through credential-helper plumbing - #336
Merged
Conversation
6 tasks
…per plumbing The credential-helper plumbing flattened opts.credential into loose api_key/auth_type strings, threaded the pair through every signature, then rebuilt a CredentialResult (with a fabricated source_name) just to hand it to initialise_api. The getattr fallback guarding auth_type could never fire: it is a declared dataclass field with a default. Pass the CredentialResult itself instead. The object carries its own auth scheme end to end, so a bearer credential's custom-domain lookup now goes out as Authorization: Bearer rather than being re-derived at the bottom of the stack. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
BartoszBlizniak
force-pushed
the
credential-plumbing
branch
from
August 4, 2026 08:36
b4126fd to
cb39e1d
Compare
…ential initialise_api() resets config.headers on every call but never clears config.api_key, which Configuration.set_default() makes sticky across calls. Re-initialising with a bearer credential therefore left a previously configured X-Api-Key in place, so subsequent requests carried both auth headers. This is reachable via the SSO login path: the initialise_api decorator sets X-Api-Key from credentials.ini, then refresh_api_config_after_auth() re-initialises with the bearer token. The API could then authenticate as the pre-login identity while the CLI reported a successful login. The bearer test previously called unset_api_key() to work around this; it now seeds a stale key and asserts it is cleared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-run Two defects in the Docker credential-helper install path: CredentialResult is a plain dataclass, so `if not credential` and `if org and credential` are true for a credential carrying an empty api_key — the guards they replaced (`if not api_key`) were not re-established. Both now check credential.api_key, so a blank credential no longer reaches an unauthenticated custom-domains lookup. Auto-discovery also ran before the dry_run short-circuit, so `install docker --dry-run` issued a live API call and overwrote the on-disk domain cache despite promising to make no changes. Discovery is now skipped under dry_run and reported as such in the planned actions. Also hoists the function-local is_cloudsmith_domain imports to module level and gives the without-credential test the httpretty and config-path isolation its neighbours use, so it can no longer pass by falling through to a live 401. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the credential-helper call chain to pass a resolved CredentialResult end-to-end (instead of flattening to api_key/auth_type), ensuring the authentication scheme is preserved throughout custom-domain lookups and API initialization.
Changes:
- Thread
CredentialResultthrough credential-helper plumbing (install,is_cloudsmith_domain, custom-domain discovery helpers) to avoid re-deriving auth scheme at lower layers. - Fix bearer re-initialization behavior so a previously configured
X-Api-Keydoes not leak into bearer-authenticated requests. - Make
install --dry-runfully read-only by skipping custom-domain auto-discovery (and avoiding cache mutation), with test coverage for the new behaviors.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| cloudsmith_cli/credential_helpers/docker/runtime.py | Passes the resolved credential object into domain checking for Docker helper runtime. |
| cloudsmith_cli/credential_helpers/docker/installer.py | Refactors installer autodiscovery to use CredentialResult and makes dry-run skip discovery/cache writes. |
| cloudsmith_cli/credential_helpers/custom_domains.py | Updates custom-domain lookup APIs to accept CredentialResult directly and initialize auth accordingly. |
| cloudsmith_cli/credential_helpers/common.py | Updates domain checking to require a usable credential (credential.api_key) for custom-domain API lookups. |
| cloudsmith_cli/core/api/init.py | Clears sticky X-Api-Key when switching to bearer auth to prevent mixed-auth headers. |
| cloudsmith_cli/cli/tests/commands/test_credential_helper.py | Adds/updates tests for credential plumbing, bearer header behavior, and blank-credential guards. |
| cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py | Extends installer tests for missing/blank credentials and dry-run autodiscovery behavior; asserts credential passthrough. |
| cloudsmith_cli/cli/commands/credential_helper/manage.py | Passes the resolved credential object to installers instead of flattening fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
BartoszBlizniak
approved these changes
Aug 4, 2026
cloudsmith-iduffy
added a commit
that referenced
this pull request
Aug 4, 2026
Both sides reworked the same credential-helper call sites. Master's versions are supersets in every conflict — the api_key guard on is_cloudsmith_domain, the dry-run skip for auto-discovery, and the stale-key test that seeds a key rather than clearing it — so they win; the branch's strict-mode custom-domain tests are kept alongside. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The credential-helper plumbing flattened
opts.credentialinto looseapi_key/auth_typestrings, threaded the pair through every signature, thenrebuilt a
CredentialResult(with a fabricatedsource_name) just to hand it toinitialise_api. This passes the resolvedCredentialResultitself instead, sothe object carries its own auth scheme (
X-Api-KeyvsAuthorization: Bearer)end to end. A bearer credential's custom-domain lookup now authenticates as a
bearer token rather than having the scheme re-derived at the bottom of the stack.
Three auth defects surfaced while reviewing the refactor and are fixed here:
Stale
X-Api-Keyleaked into bearer requests.initialise_api()resetsconfig.headerson every call but never clearsconfig.api_key, whichConfiguration.set_default()makes sticky. Re-initialising with a bearercredential left a previously configured
X-Api-Keyin place, so subsequentrequests carried both auth headers. This is reachable via SSO login: the
initialise_apidecorator setsX-Api-Keyfromcredentials.ini, thenrefresh_api_config_after_auth()re-initialises with the bearer token — theAPI could authenticate as the pre-login identity while the CLI reported a
successful login.
Blank-credential guard.
CredentialResultis a plain dataclass, soif not credentialandif org and credentialare true for a credentialcarrying an empty
api_key; theif not api_keyguards they replaced were notre-established. Both now check
credential.api_key, matching the conventionthe generic helper already uses, so a blank credential can no longer reach an
unauthenticated custom-domains lookup.
install --dry-runwas not read-only. Auto-discovery ran before thedry_runshort-circuit, so a preview issued a live API call and overwrote theon-disk domain cache. Discovery is now skipped under
dry_runand reported assuch in the planned actions.
Testing
New coverage: a bearer request asserted clean of a deliberately seeded stale
X-Api-Key; blank-credential anddry_runautodiscovery scenarios asserting thedomains API is never called; and the without-credential domain check given the
httpretty(allow_net_connect=False)and config-path isolation its neighboursuse, so it can no longer pass by falling through to a live 401.
Full suite: 572 passed, 40 skipped.
Type of Change