fix: stop recurring macOS keychain prompts and self-heal dead SSO sessions - #383
Open
cloudsmith-iduffy wants to merge 8 commits into
Open
fix: stop recurring macOS keychain prompts and self-heal dead SSO sessions#383cloudsmith-iduffy wants to merge 8 commits into
cloudsmith-iduffy wants to merge 8 commits into
Conversation
…ead SSO sessions The keyring library implements each keychain write as a delete followed by a re-create. The re-created item has a fresh access control list, so every "Always Allow" grant was lost on the next token refresh and the keychain prompts returned forever. - Add core/macos_keychain.py and update keychain items in place with SecItemUpdate, which keeps the access control list. Fall back to the normal keyring write when the item does not exist. Resolve the chainer backend to its first member before the update. - Scope keyring service names by profile. Non-default profiles read the legacy unscoped entries as a fallback, so existing sessions stay valid and migrate to scoped entries on the next refresh. The default profile keeps the unscoped names. - Clear a profile's SSO tokens when the server rejects the refresh (400/401/403/422), so the CLI returns to a clean logged-out state instead of retrying dead tokens every 30 minutes. Transient failures keep the throttled retry. Treat a refresh response without an access token as a failure, and skip the refresh when no refresh token is stored. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cloudsmith-iduffy
force-pushed
the
fix/macos-keychain-prompts
branch
from
August 23, 2026 12:04
dbaf7cf to
7e3c0e3
Compare
cloudsmith-iduffy
marked this pull request as ready for review
August 23, 2026 12:56
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the CLI’s SSO token storage/refresh behavior to (1) avoid recurring macOS Keychain permission prompts by updating items in place and (2) make SSO sessions more resilient and isolated by scoping keyring entries per profile and self-healing rejected refreshes.
Changes:
- Add a macOS-specific in-place Keychain update path (SecItemUpdate) and wire it into keyring writes.
- Scope SSO token service names by profile (with legacy fallback/migration) and propagate
profilethrough auth/logout/whoami flows. - Improve refresh behavior by wiping definitively rejected sessions and avoiding
Bearer Noneoutcomes.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cloudsmith_cli/core/tests/test_metadata.py | Updates keyring stubs to accept the new profile argument. |
| cloudsmith_cli/core/tests/test_keyring.py | Adds coverage for profile-scoped key names and macOS in-place update gating. |
| cloudsmith_cli/core/tests/test_keyring_provider.py | Adds coverage for profile propagation and rejected-refresh wipe behavior. |
| cloudsmith_cli/core/macos_keychain.py | New ctypes-based SecItemUpdate helper to update Keychain items without ACL reset. |
| cloudsmith_cli/core/keyring.py | Implements profile-scoped key formatting, legacy fallback reads, and macOS in-place update hook. |
| cloudsmith_cli/core/credentials/providers/keyring_provider.py | Passes profile through keyring calls; wipes tokens on definitive refresh rejection. |
| cloudsmith_cli/cli/webserver.py | Persists profile on the auth callback server and stores SSO tokens with profile scoping. |
| cloudsmith_cli/cli/tests/test_webserver.py | Extends tests to assert profile is forwarded to store_sso_tokens. |
| cloudsmith_cli/cli/tests/commands/test_logout.py | Ensures test environment clears CLOUDSMITH_PROFILE and asserts profile-aware keyring deletion. |
| cloudsmith_cli/cli/commands/whoami.py | Adds profile-aware SSO status reporting for verbose output. |
| cloudsmith_cli/cli/commands/logout.py | Clears keyring tokens for the active profile. |
| cloudsmith_cli/cli/commands/auth.py | Passes profile into the SAML authentication webserver flow. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Author
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: cloudsmith-iduffy <178375997+cloudsmith-iduffy@users.noreply.github.com>
Co-authored-by: cloudsmith-iduffy <178375997+cloudsmith-iduffy@users.noreply.github.com>
Contributor
Conflicts are resolved and merged with |
The credential provider chain imported all providers at module level, which pulled requests and cloudsmith_api into every CLI invocation through the keyring provider's SAML dependency. Defer that import to CredentialProviderChain.__init__ so it only loads when the chain is built. Also fix a logout test assertion that dropped the profile=None keyword argument the command actually passes.
BartoszBlizniak
requested changes
Aug 26, 2026
BartoszBlizniak
left a comment
BartoszBlizniak
left a comment
Member
There was a problem hiding this comment.
few comments before approving
Logout deleted the legacy unscoped entries together with the profile's scoped entries. The unscoped entries hold the default profile's session, so a scoped-profile logout ended both sessions. Delete the legacy entries only when the profile has no scoped entries, as the refresh-rejection path already does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The keyring provider imported refresh_access_token from cli.saml, so core depended on cli at import time. Move refresh_access_token and raise_for_api_error into core/sso.py. cli/saml.py re-exports them for its existing callers. The deferred provider import in chain.py stays. decorators.py imports chain on every CLI start, so a module-level provider import would load requests and cloudsmith_api at startup and fail test_cli_import_does_not_load_heavy_modules. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
test_returns_none_on_refresh_failure patched saml.refresh_access_token, but the provider binds the function at module level, so the patch did not intercept and the 401 path never ran. On macOS the test could reach the real keychain. Patch keyring_provider.refresh_access_token, patch delete_sso_tokens, and mock get_keyring for the whole file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
BartoszBlizniak
approved these changes
Aug 26, 2026
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
Motivation
On macOS, every SSO session produced a stream of "cloudsmith wants to use your credential information" prompts. Clicking "Always Allow" did not stop them. The prompts got worse for anyone who runs the CLI from more than one install (standalone binary, a uv environment,
uv run).The root cause is in the
keyringlibrary's macOS backend: every write is a delete followed by a re-create (SecItemDelete+SecItemAdd). A re-created item is a new keychain item with a fresh access control list, so it forgets every "Always Allow" the user ever granted. The CLI rewrites its three token items on every refresh — at most every 30 minutes — so grants never survived, and each install flavor kept stealing the items from the others.Two related problems surfaced while fixing this:
Changes
Keychain writes now update items in place. A new
core/macos_keychain.pybindsSecItemUpdatevia ctypes._set_valuetries it first on macOS and falls back to the normalkeyringwrite when the item does not exist.SecItemUpdatechanges the stored secret and keeps the item's access control list, so "Always Allow" now sticks permanently. Each binary pays its grants once, ever. Note:keyring.get_keyring()returns a chainer backend that delegates writes to its first member, so the backend gate inspectsbackends[0], not the chainer.Keyring entries are profile-scoped. Until now, every profile stored its SSO tokens under one unscoped service name per API host (for example
cloudsmith_cli-access_token-<host>). This change appends-profile-<name>to the service name for non-default profiles, giving each profile its own token set. The old unscoped entries are what the code calls legacy entries, and existing sessions live in them, so backwards compatibility works in two parts:Rejected refreshes self-heal. When the refresh endpoint definitively rejects the stored tokens (400, 401, 403, or 422 — the API returns 422 for an invalid refresh token), the CLI deletes that profile's entries and returns to a clean logged-out state with a clear "run
cloudsmith auth" message. The wipe touches only the profile's own entries; if the profile was still running on the unscoped entries via the fallback, those hold the dead tokens and are removed instead — so one profile's rejection can never destroy another profile's session. Transient failures (network errors, 5xx) keep the existing behavior: stamp the attempt time and retry after the 30-minute throttle. A refresh response without an access token is treated as a failure instead of producing anAuthorization: Bearer Noneheader, and a locally missing refresh token skips the refresh instead of posting an empty token.Verification
Verified live on macOS against a real API host: three consecutive forced refresh cycles rotated the access token each time while all three keychain items kept their original creation date — same items, updated in place, access grants intact. The item-identity check is
security find-generic-password -s <service> | grep cdat; a changingcdatmeans the item was re-created and the ACL was reset. The rejection path was also verified live: a dead refresh token produced a 422, and the CLI wiped the entries and dropped cleanly to logged-out. There are no macOS-only automated tests since CI has no macOS runners; the unit tests are cross-platform.Type of Change
Additional Notes
Phase-out plan for the legacy unscoped entries. The unscoped service names stay as the default profile's canonical storage — they are only "legacy" from the point of view of non-default profiles. The fallback exists to migrate existing sessions without a forced re-login, and it retires itself:
_get_value_with_fallback) and theinclude_legacyhandling indelete_sso_tokens. Any session that never migrated — a user who skipped the intermediate releases — resolves no credentials and gets the standard "runcloudsmith auth" message, not an error.