Esteban PR feedback

Author: BartoszBlizniak

cloudsmith_cli/cli/commands/tokens.py

@@ -99,7 +99,7 @@ def request_api_key(ctx, opts, save_config=False):
 
         # Other errors - use the handler
         with handle_api_exceptions(ctx, opts=opts, context_msg=context_msg):
-            raise
+            raise exc
 
 
 @main.group(cls=command.AliasGroup, name="tokens")

cloudsmith_cli/cli/exceptions.py

@@ -7,7 +7,7 @@
 import click
 
 from ..core.api.exceptions import ApiException
-from ..core.keyring import get_access_token, should_use_keyring
+from ..core.keyring import get_access_token
 
 
 @contextlib.contextmanager
@@ -170,10 +170,7 @@ def get_401_error_hint(ctx, opts, exc):
             "you don't have the permission to perform this action."
         )
 
-    # Only check keyring if enabled to avoid keyring prompts
-    access_token = None
-    if should_use_keyring():
-        access_token = get_access_token(opts.api_host)
+    access_token = get_access_token(opts.api_host)
     if access_token:
         return "Since you have an SSO access token set, this probably means that it has expired. Try getting a new token with 'cloudsmith auth', then try again."
 

cloudsmith_cli/core/api/init.py

@@ -8,7 +8,6 @@
 
 from ...cli import saml
 from .. import keyring
-from ..keyring import should_use_keyring
 from ..rest import RestClient
 from .exceptions import ApiException
 
@@ -48,55 +47,49 @@ def initialise_api(
 
     # Use directly provided access token (e.g. from SSO callback),
     # or fall back to keyring lookup if enabled.
-    token_from_keyring = False
-    if not access_token and should_use_keyring():
+    if not access_token:
         access_token = keyring.get_access_token(config.host)
-        token_from_keyring = True
 
     if access_token:
         auth_header = config.headers.get("Authorization")
 
         # overwrite auth header if empty or is basic auth without username or password
         if not auth_header or auth_header == config.get_basic_auth_token():
-            # Only attempt refresh for tokens retrieved from keyring.
-            # Directly provided tokens (e.g. from SSO callback) are fresh
-            # and don't need a refresh cycle.
-            if token_from_keyring:
-                refresh_token = keyring.get_refresh_token(config.host)
-
-                try:
-                    if keyring.should_refresh_access_token(config.host):
-                        new_access_token, new_refresh_token = saml.refresh_access_token(
-                            config.host,
-                            access_token,
-                            refresh_token,
-                            session=saml.create_configured_session(config),
-                        )
-                        keyring.store_sso_tokens(
-                            config.host, new_access_token, new_refresh_token
-                        )
-                        # Use the new tokens
-                        access_token = new_access_token
-                except ApiException:
-                    keyring.update_refresh_attempted_at(config.host)
+            refresh_token = keyring.get_refresh_token(config.host)
+
+            try:
+                if keyring.should_refresh_access_token(config.host):
+                    new_access_token, new_refresh_token = saml.refresh_access_token(
+                        config.host,
+                        access_token,
+                        refresh_token,
+                        session=saml.create_configured_session(config),
+                    )
+                    keyring.store_sso_tokens(
+                        config.host, new_access_token, new_refresh_token
+                    )
+                    # Use the new tokens
+                    access_token = new_access_token
+            except ApiException:
+                keyring.update_refresh_attempted_at(config.host)
+
+                click.secho(
+                    "An error occurred when attempting to refresh your SSO access token. To refresh this session, run 'cloudsmith auth'",
+                    fg="yellow",
+                    err=True,
+                )
 
+                # Clear access_token to prevent using expired token
+                access_token = None
+
+                # Fall back to API key auth if available
+                if key:
                     click.secho(
-                        "An error occurred when attempting to refresh your SSO access token. To refresh this session, run 'cloudsmith auth'",
+                        "Falling back to API key authentication.",
                         fg="yellow",
                         err=True,
                     )
-
-                    # Clear access_token to prevent using expired token
-                    access_token = None
-
-                    # Fall back to API key auth if available
-                    if key:
-                        click.secho(
-                            "Falling back to API key authentication.",
-                            fg="yellow",
-                            err=True,
-                        )
-                        config.api_key["X-Api-Key"] = key
+                    config.api_key["X-Api-Key"] = key
 
             # Only use SSO token if refresh didn't fail
             if access_token:

cloudsmith_cli/core/download.py

@@ -11,7 +11,6 @@
 from . import keyring, ratelimits, utils
 from .api.exceptions import catch_raise_api_exception
 from .api.packages import get_packages_api, list_packages
-from .keyring import should_use_keyring
 from .rest import create_requests_session
 
 
@@ -40,9 +39,7 @@ def resolve_auth(
 
     # Only attempt keyring operations if keyring is enabled
     config = cloudsmith_api.Configuration()
-    access_token = None
-    if should_use_keyring():
-        access_token = keyring.get_access_token(config.host)
+    access_token = keyring.get_access_token(config.host)
     api_key = api_key_opt or getattr(opts, "api_key", None)
 
     if api_key:

cloudsmith_cli/core/keyring.py

@@ -11,9 +11,7 @@
 def should_use_keyring():
     """Check if keyring should be used based on CLOUDSMITH_NO_KEYRING env var."""
     env_value = os.environ.get("CLOUDSMITH_NO_KEYRING", "").strip().lower()
-    if env_value in ("1", "true", "yes"):
-        return False
-    return True
+    return env_value not in ("1", "true", "yes")
 
 
 ACCESS_TOKEN_REFRESH_ATTEMPTED_AT_KEY = (
@@ -45,6 +43,8 @@ def store_access_token(api_host, access_token):
 
 
 def get_access_token(api_host):
+    if not should_use_keyring():
+        return None
     key = ACCESS_TOKEN_KEY.format(api_host=api_host)
     return _get_value(key)
 
@@ -73,6 +73,9 @@ def get_refresh_attempted_at(api_host):
 
 
 def should_refresh_access_token(api_host):
+    if not should_use_keyring():
+        return False
+
     token_refreshed_at = get_refresh_attempted_at(api_host)
 
     if token_refreshed_at: