diff --git a/cloudsmith_cli/cli/decorators.py b/cloudsmith_cli/cli/decorators.py index a76305e8..f5124cfb 100644 --- a/cloudsmith_cli/cli/decorators.py +++ b/cloudsmith_cli/cli/decorators.py @@ -495,6 +495,7 @@ def wrapper(ctx, *args, **kwargs): opts.oidc_detector_order, oidc_disabled_detectors ) + is_auth_command = ctx.command.name in ("authenticate", "login") context = CredentialContext( session=opts.session, api_key_from_flag=opts.api_key_from_flag, @@ -510,24 +511,37 @@ def wrapper(ctx, *args, **kwargs): oidc_discovery_disabled=opts.oidc_discovery_disabled, oidc_detector_order=opts.oidc_detector_order, oidc_disabled_detectors=oidc_disabled_detectors, + skip_keyring_refresh=is_auth_command, ) chain = CredentialProviderChain() credential = chain.resolve(context) - if context.keyring_refresh_failed: - click.secho( - "An error occurred when attempting to refresh your SSO access token. " - "To refresh this session, run 'cloudsmith auth'", - fg="yellow", - err=True, - ) + if context.keyring_refresh_failed and not is_auth_command: if credential: - click.secho( - "Falling back to API key authentication.", - fg="yellow", - err=True, + message = ( + "Using the existing access token until it expires." + if credential.source_name == "keyring" + else "Falling back to alternative authentication." ) + elif context.keyring_refresh_rejected: + message = ( + "Your SSO session has expired. Run 'cloudsmith auth' to " + "authenticate again; continuing without SSO authentication." + ) + elif context.keyring_refresh_unrenewable: + message = ( + "The SSO session has no refresh token and its access token " + "has expired. Run 'cloudsmith auth' to authenticate again; " + "continuing without SSO authentication." + ) + else: + message = ( + "The SSO session could not be renewed and its access token " + "has expired. Check your connection, then run 'cloudsmith auth'; " + "continuing without SSO authentication." + ) + click.secho(message, fg="yellow", err=True) opts.credential = credential diff --git a/cloudsmith_cli/cli/tests/test_decorators.py b/cloudsmith_cli/cli/tests/test_decorators.py index 9f6e5815..14950bce 100644 --- a/cloudsmith_cli/cli/tests/test_decorators.py +++ b/cloudsmith_cli/cli/tests/test_decorators.py @@ -1,7 +1,10 @@ +from unittest.mock import patch + import click import click.testing +import pytest -from ..decorators import report_retry +from ..decorators import report_retry, resolve_credentials def test_report_retry_writes_to_stderr(): @@ -14,3 +17,49 @@ def command(): assert result.stdout == '{"data": []}\n' assert "Request was throttled (429)" in result.stderr + + +def _credential_command(name="example"): + @click.command(name=name) + @resolve_credentials + def command(opts): + click.echo("command ran") + + return command + + +def test_rejected_sso_session_continues_without_early_exception(): + def reject(context): + context.keyring_refresh_failed = True + context.keyring_refresh_rejected = True + return None + + with patch( + "cloudsmith_cli.cli.decorators.CredentialProviderChain.resolve", + side_effect=reject, + ): + result = click.testing.CliRunner().invoke(_credential_command()) + + assert result.exit_code == 0 + assert "Your SSO session has expired" in result.stderr + assert "continuing without SSO authentication" in result.stderr + assert result.stdout == "command ran\n" + + +@pytest.mark.parametrize("command_name", ["authenticate", "login"]) +def test_auth_commands_skip_automatic_keyring_refresh(command_name): + def resolve(context): + assert context.skip_keyring_refresh is True + return None + + with patch( + "cloudsmith_cli.cli.decorators.CredentialProviderChain.resolve", + side_effect=resolve, + ): + result = click.testing.CliRunner().invoke( + _credential_command(name=command_name) + ) + + assert result.exit_code == 0 + assert result.stdout == "command ran\n" + assert result.stderr == "" diff --git a/cloudsmith_cli/core/credentials/models.py b/cloudsmith_cli/core/credentials/models.py index a0ca2020..aefabc24 100644 --- a/cloudsmith_cli/core/credentials/models.py +++ b/cloudsmith_cli/core/credentials/models.py @@ -27,6 +27,9 @@ class CredentialContext: profile: str | None = None debug: bool = False keyring_refresh_failed: bool = False + keyring_refresh_rejected: bool = False + keyring_refresh_unrenewable: bool = False + skip_keyring_refresh: bool = False oidc_audience: str | None = None org: str | None = None oidc_service_slug: str | None = None diff --git a/cloudsmith_cli/core/credentials/providers/keyring_provider.py b/cloudsmith_cli/core/credentials/providers/keyring_provider.py index a4c2f7a4..ee896b1a 100644 --- a/cloudsmith_cli/core/credentials/providers/keyring_provider.py +++ b/cloudsmith_cli/core/credentials/providers/keyring_provider.py @@ -5,36 +5,69 @@ import logging from ....core import keyring -from ...api.exceptions import ApiException -from ...sso import refresh_access_token +from ...sso import SsoRenewalStatus, access_token_is_valid, renew_sso_session from ..models import CredentialContext, CredentialResult from ..provider import CredentialProvider logger = logging.getLogger(__name__) -REFRESH_REJECTED_STATUSES = (400, 401, 403, 422) +def _credential(access_token): + return CredentialResult( + api_key=access_token, + source_name="keyring", + source_detail="SAML token from system keyring", + auth_type="bearer", + ) + + +def _access_token_from_renewal(context, renewal, held_access_token): + if renewal.status == SsoRenewalStatus.REJECTED: + context.keyring_refresh_failed = True + context.keyring_refresh_rejected = True + return None + if renewal.status == SsoRenewalStatus.MISSING: + context.keyring_refresh_failed = True + return held_access_token if access_token_is_valid(held_access_token) else None + if renewal.status == SsoRenewalStatus.FAILED: + context.keyring_refresh_failed = True + return None + if renewal.status == SsoRenewalStatus.UNRENEWABLE: + context.keyring_refresh_failed = True + context.keyring_refresh_unrenewable = True + if not access_token_is_valid(renewal.access_token): + return None + if renewal.status == SsoRenewalStatus.CURRENT and renewal.error: + context.keyring_refresh_failed = True + return renewal.access_token -def _handle_refresh_failure(context, wipe_tokens): - """Record a refresh failure and clear rejected tokens. - A definitive rejection means the stored tokens are dead. Remove the - profile's own entries so the CLI returns to a clean logged-out - state instead of retrying dead tokens on every command. When the - profile has no entries of its own, the rejected tokens came from - the legacy unscoped entries, so remove those. When no entry was - removed, stamp the attempt time to throttle the next refresh. - """ - tokens_removed = False - if wipe_tokens: - tokens_removed = keyring.delete_sso_tokens( - context.api_host, profile=context.profile, include_legacy=False - ) - if not tokens_removed: - tokens_removed = keyring.delete_sso_tokens(context.api_host) - if not tokens_removed: - keyring.update_refresh_attempted_at(context.api_host, profile=context.profile) +def _recover_from_unexpected_refresh_error(context, access_token): context.keyring_refresh_failed = True + keyring.update_refresh_attempted_at(context.api_host, profile=context.profile) + return access_token if access_token_is_valid(access_token) else None + + +def _refresh_access_token(context, access_token): + if context.skip_keyring_refresh or not keyring.should_refresh_access_token( + context.api_host, + access_token=access_token, + profile=context.profile, + ): + return access_token + + if not context.session: + logger.debug( + "Session unavailable; skipping token refresh, using existing token" + ) + return access_token + + renewal = renew_sso_session( + context.api_host, + context.session, + profile=context.profile, + ) + return _access_token_from_renewal(context, renewal, access_token) class KeyringProvider(CredentialProvider): @@ -54,52 +87,9 @@ def resolve(self, context: CredentialContext) -> CredentialResult | None: return None try: - if keyring.should_refresh_access_token(api_host, profile=profile): - if not context.session: - logger.debug( - "Session unavailable; skipping token refresh, using existing token" - ) - else: - refresh_token = keyring.get_refresh_token(api_host, profile=profile) - if not refresh_token: - logger.debug( - "No refresh token stored; using the existing access token" - ) - else: - new_access_token, new_refresh_token = refresh_access_token( - api_host, - access_token, - refresh_token, - session=context.session, - ) - if not new_access_token: - logger.debug("The refresh response has no access token") - _handle_refresh_failure(context, wipe_tokens=False) - return None - keyring.store_sso_tokens( - api_host, - new_access_token, - new_refresh_token, - profile=profile, - ) - access_token = new_access_token - except Exception as exc: # pylint: disable=broad-exception-caught - wipe_tokens = ( - isinstance(exc, ApiException) - and exc.status in REFRESH_REJECTED_STATUSES - ) - if wipe_tokens: - logger.debug( - "SSO refresh rejected; clearing stored SSO tokens", exc_info=True - ) - else: - logger.debug("Failed to refresh SAML token", exc_info=True) - _handle_refresh_failure(context, wipe_tokens=wipe_tokens) - return None + access_token = _refresh_access_token(context, access_token) + except Exception: # pylint: disable=broad-exception-caught + logger.debug("Failed to refresh SAML token", exc_info=True) + access_token = _recover_from_unexpected_refresh_error(context, access_token) - return CredentialResult( - api_key=access_token, - source_name="keyring", - source_detail="SAML token from system keyring", - auth_type="bearer", - ) + return _credential(access_token) if access_token else None diff --git a/cloudsmith_cli/core/tests/test_keyring_provider.py b/cloudsmith_cli/core/tests/test_keyring_provider.py index 6c1ec47d..cded6a22 100644 --- a/cloudsmith_cli/core/tests/test_keyring_provider.py +++ b/cloudsmith_cli/core/tests/test_keyring_provider.py @@ -1,14 +1,14 @@ """Tests for the keyring credential provider.""" import os -from unittest.mock import MagicMock, call, patch +from unittest.mock import MagicMock, patch import pytest from cloudsmith_cli.core import keyring -from cloudsmith_cli.core.api.exceptions import ApiException from cloudsmith_cli.core.credentials.models import CredentialContext from cloudsmith_cli.core.credentials.providers import KeyringProvider, keyring_provider +from cloudsmith_cli.core.sso import SsoRenewalResult @pytest.fixture(autouse=True) @@ -54,219 +54,108 @@ def test_returns_bearer_token(self): assert result.auth_type == "bearer" assert result.source_name == "keyring" - def test_returns_none_on_refresh_failure(self): + def test_uses_existing_token_when_session_is_unavailable(self): provider = KeyringProvider() - context = CredentialContext(session=MagicMock()) - env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) with ( - patch.dict(os.environ, env, clear=True), patch.object(keyring, "should_use_keyring", return_value=True), - patch.object(keyring, "get_access_token", return_value="old_token"), + patch.object(keyring, "get_access_token", return_value="sso-token"), patch.object(keyring, "should_refresh_access_token", return_value=True), - patch.object(keyring, "get_refresh_token", return_value="refresh_tok"), - patch.object( - keyring_provider, - "refresh_access_token", - side_effect=ApiException(status=401, detail="Unauthorized"), - ), - patch.object(keyring, "delete_sso_tokens", return_value=True), - patch.object(keyring, "update_refresh_attempted_at"), + patch.object(keyring_provider, "renew_sso_session") as renew_mock, ): - result = provider.resolve(context) - assert result is None - assert context.keyring_refresh_failed is True + result = provider.resolve(CredentialContext()) - def test_passes_profile_to_keyring(self): + assert result is not None + assert result.api_key == "sso-token" + renew_mock.assert_not_called() + + def test_uses_renewed_token_and_profile(self): provider = KeyringProvider() context = CredentialContext(session=MagicMock(), profile="staging") - env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) + renewal = SsoRenewalResult(status="renewed", access_token="new-token") with ( - patch.dict(os.environ, env, clear=True), patch.object(keyring, "should_use_keyring", return_value=True), patch.object( - keyring, "get_access_token", return_value="old_token" + keyring, "get_access_token", return_value="old-token" ) as get_access_mock, patch.object( keyring, "should_refresh_access_token", return_value=True ) as should_refresh_mock, patch.object( - keyring, "get_refresh_token", return_value="old_refresh" - ) as get_refresh_mock, - patch.object( - keyring_provider, - "refresh_access_token", - return_value=("new_token", "new_refresh"), - ), - patch.object(keyring, "store_sso_tokens") as store_mock, + keyring_provider, "renew_sso_session", return_value=renewal + ) as renew_mock, ): result = provider.resolve(context) assert result is not None - assert result.api_key == "new_token" + assert result.api_key == "new-token" get_access_mock.assert_called_once_with(context.api_host, profile="staging") - should_refresh_mock.assert_called_once_with(context.api_host, profile="staging") - get_refresh_mock.assert_called_once_with(context.api_host, profile="staging") - store_mock.assert_called_once_with( - context.api_host, "new_token", "new_refresh", profile="staging" + should_refresh_mock.assert_called_once_with( + context.api_host, access_token="old-token", profile="staging" ) - - @pytest.mark.parametrize("status", [400, 401, 403, 422]) - def test_wipes_tokens_when_refresh_is_rejected(self, status): - provider = KeyringProvider() - context = CredentialContext(session=MagicMock(), profile="staging") - env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) - with ( - patch.dict(os.environ, env, clear=True), - patch.object(keyring, "should_use_keyring", return_value=True), - patch.object(keyring, "get_access_token", return_value="stale_token"), - patch.object(keyring, "should_refresh_access_token", return_value=True), - patch.object(keyring, "get_refresh_token", return_value="stale_refresh"), - patch.object( - keyring_provider, - "refresh_access_token", - side_effect=ApiException(status=status, detail="Rejected"), - ), - patch.object(keyring, "delete_sso_tokens") as delete_mock, - patch.object(keyring, "update_refresh_attempted_at") as attempted_mock, - ): - result = provider.resolve(context) - - assert result is None - assert context.keyring_refresh_failed is True - delete_mock.assert_called_once_with( - context.api_host, profile="staging", include_legacy=False + renew_mock.assert_called_once_with( + context.api_host, context.session, profile="staging" ) - attempted_mock.assert_not_called() - def test_wipes_legacy_tokens_when_profile_has_none(self): + @pytest.mark.parametrize( + "error", + [ + RuntimeError("temporarily unavailable"), + ValueError("Cloudsmith did not return a new SSO access token."), + ], + ids=["transient-error", "missing-access-response"], + ) + def test_renewal_failure_uses_still_valid_token(self, error): provider = KeyringProvider() - context = CredentialContext(session=MagicMock(), profile="staging") - env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) - with ( - patch.dict(os.environ, env, clear=True), - patch.object(keyring, "should_use_keyring", return_value=True), - patch.object(keyring, "get_access_token", return_value="stale_token"), - patch.object(keyring, "should_refresh_access_token", return_value=True), - patch.object(keyring, "get_refresh_token", return_value="stale_refresh"), - patch.object( - keyring_provider, - "refresh_access_token", - side_effect=ApiException(status=401, detail="Rejected"), - ), - patch.object( - keyring, "delete_sso_tokens", side_effect=[False, True] - ) as delete_mock, - patch.object(keyring, "update_refresh_attempted_at") as attempted_mock, - ): - result = provider.resolve(context) - - assert result is None - assert context.keyring_refresh_failed is True - assert delete_mock.call_args_list == [ - call(context.api_host, profile="staging", include_legacy=False), - call(context.api_host), - ] - attempted_mock.assert_not_called() - - def test_stamps_attempt_when_wipe_removes_nothing(self): - provider = KeyringProvider() - context = CredentialContext(session=MagicMock(), profile="staging") - env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) + context = CredentialContext(session=MagicMock()) + renewal = SsoRenewalResult( + status="current", + access_token="old-token", + error=error, + ) with ( - patch.dict(os.environ, env, clear=True), patch.object(keyring, "should_use_keyring", return_value=True), - patch.object(keyring, "get_access_token", return_value="stale_token"), + patch.object(keyring, "get_access_token", return_value="old-token"), patch.object(keyring, "should_refresh_access_token", return_value=True), - patch.object(keyring, "get_refresh_token", return_value="stale_refresh"), - patch.object( - keyring_provider, - "refresh_access_token", - side_effect=ApiException(status=401, detail="Rejected"), - ), - patch.object(keyring, "delete_sso_tokens", return_value=False), - patch.object(keyring, "update_refresh_attempted_at") as attempted_mock, + patch.object(keyring_provider, "renew_sso_session", return_value=renewal), ): result = provider.resolve(context) - assert result is None + assert result is not None + assert result.api_key == "old-token" assert context.keyring_refresh_failed is True - attempted_mock.assert_called_once_with(context.api_host, profile="staging") - def test_skips_refresh_when_no_refresh_token_is_stored(self): + def test_unrenewable_session_uses_still_valid_token(self): provider = KeyringProvider() - context = CredentialContext(session=MagicMock(), profile="staging") - env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) + context = CredentialContext(session=MagicMock()) + renewal = SsoRenewalResult( + status="unrenewable", + access_token="old-token", + ) with ( - patch.dict(os.environ, env, clear=True), patch.object(keyring, "should_use_keyring", return_value=True), - patch.object(keyring, "get_access_token", return_value="sso_token"), + patch.object(keyring, "get_access_token", return_value="old-token"), patch.object(keyring, "should_refresh_access_token", return_value=True), - patch.object(keyring, "get_refresh_token", return_value=None), - patch.object(keyring_provider, "refresh_access_token") as refresh_mock, - patch.object(keyring, "delete_sso_tokens") as delete_mock, + patch.object(keyring_provider, "renew_sso_session", return_value=renewal), ): result = provider.resolve(context) assert result is not None - assert result.api_key == "sso_token" - assert context.keyring_refresh_failed is False - refresh_mock.assert_not_called() - delete_mock.assert_not_called() - - def test_keeps_tokens_on_transient_refresh_error(self): - provider = KeyringProvider() - context = CredentialContext(session=MagicMock(), profile="staging") - env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) - with ( - patch.dict(os.environ, env, clear=True), - patch.object(keyring, "should_use_keyring", return_value=True), - patch.object(keyring, "get_access_token", return_value="stale_token"), - patch.object(keyring, "should_refresh_access_token", return_value=True), - patch.object(keyring, "get_refresh_token", return_value="stale_refresh"), - patch.object( - keyring_provider, - "refresh_access_token", - side_effect=ApiException(status=503, detail="Service Unavailable"), - ), - patch.object(keyring, "delete_sso_tokens") as delete_mock, - patch.object(keyring, "update_refresh_attempted_at") as attempted_mock, - ): - result = provider.resolve(context) - - assert result is None + assert result.api_key == "old-token" assert context.keyring_refresh_failed is True - delete_mock.assert_not_called() - attempted_mock.assert_called_once_with(context.api_host, profile="staging") + assert context.keyring_refresh_unrenewable is True - def test_refresh_without_access_token_in_response_is_a_failure(self): + def test_rejected_dead_session_returns_no_credential(self): provider = KeyringProvider() - context = CredentialContext(session=MagicMock(), profile="staging") - env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) + context = CredentialContext(session=MagicMock()) + renewal = SsoRenewalResult(status="rejected") with ( - patch.dict(os.environ, env, clear=True), patch.object(keyring, "should_use_keyring", return_value=True), - patch.object(keyring, "get_access_token", return_value="stale_token"), + patch.object(keyring, "get_access_token", return_value="old-token"), patch.object(keyring, "should_refresh_access_token", return_value=True), - patch.object(keyring, "get_refresh_token", return_value="stale_refresh"), - patch.object( - keyring_provider, - "refresh_access_token", - return_value=(None, None), - ), - patch.object(keyring, "store_sso_tokens") as store_mock, - patch.object(keyring, "update_refresh_attempted_at") as attempted_mock, + patch.object(keyring_provider, "renew_sso_session", return_value=renewal), ): result = provider.resolve(context) assert result is None assert context.keyring_refresh_failed is True - store_mock.assert_not_called() - attempted_mock.assert_called_once_with(context.api_host, profile="staging") + assert context.keyring_refresh_rejected is True