feat(cli): OAuth 2.0 Device Authorization Grant, silent token refresh, and TLS fixes#656
feat(cli): OAuth 2.0 Device Authorization Grant, silent token refresh, and TLS fixes#656mickume wants to merge 16 commits intojumpstarter-dev:mainfrom
Conversation
…ixes #13) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ixes #13) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Silence shell token expiry warnings and success messages — only warn when refresh fails and the user needs to act. Auto-retry commands after successful re-authentication instead of telling the user to try again.
_try_refresh_token was not passing insecure_tls to the OIDC Config, causing silent TLS verification failures when using --insecure-tls. Also promote the "Token refresh failed" log to INFO so users can see why refresh fails at the default log level.
Replace nonlocal variables with mutable list containers in exceptions_test.py to work around ty's lack of nonlocal support. Add ty overrides for gpiod driver to suppress unresolvable import and attribute warnings from the Linux-only gpiod C extension.
📝 WalkthroughWalkthroughThis PR introduces OIDC device authorization flow support for CLI authentication, refactors token-expiration reauthentication with single-retry semantics, improves token recovery logging transparency, and enhances QEMU driver cloud-init media handling and binary selection. ChangesOIDC Device Flow & Exception Handling
QEMU Driver & Infrastructure Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py (1)
261-272: ⚡ Quick winUse
OAuthError.errorattribute instead of fragile string matchingThe string-based error dispatch is fragile. authlib raises
OAuthErrorwith a structured.errorattribute containing the RFC 8628 error code (e.g.,e.error == "authorization_pending"), which is safer and immune to message-format changes.Additionally, line 268's
or "expired" in error_msgadds only false-positive risk—"expired"is not an RFC 8628 device flow error code; only"expired_token"is. Using the structured.errorattribute with juste.error == "expired_token"avoids both the substring-matching fragility and the unnecessary fallback.♻️ Proposed refactor
+from authlib.integrations.base_client.errors import OAuthError client = self.client() try: token = await run_sync( lambda c=client: c.fetch_token( token_endpoint, grant_type=DEVICE_FLOW_GRANT_TYPE, device_code=device_code, ) ) return token - except Exception as e: - error_msg = str(e) - if "authorization_pending" in error_msg: + except OAuthError as e: + if e.error == "authorization_pending": continue - if "slow_down" in error_msg: + if e.error == "slow_down": interval += 5 continue - if "expired_token" in error_msg or "expired" in error_msg: + if e.error == "expired_token": raise click.ClickException("Device code expired. Please try again.") from e - if "access_denied" in error_msg: + if e.error == "access_denied": raise click.ClickException("Authorization was denied by the user.") from e raise click.ClickException(f"Device flow token request failed: {e}") from e + except Exception as e: + raise click.ClickException(f"Device flow token request failed: {e}") from e🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py` around lines 261 - 272, Replace fragile substring matching in the exception handler with structured checks against authlib.oauth2.rfc6749.errors.OAuthError.error: import OAuthError (or the correct authlib exception class), catch OAuthError (or check isinstance) in the except block and use e.error == "authorization_pending"/"slow_down"/"expired_token"/"access_denied" to control flow (increase interval for "slow_down", continue for "authorization_pending", raise a ClickException for "expired_token" and "access_denied"); remove the `"expired" in error_msg` fallback and for any other error re-raise a ClickException including the original exception for context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py`:
- Around line 232-239: Extract a shared helper (e.g.,
_invoke_and_map_exceptions(func, *args, login_func=None, **kwargs)) that runs
func(*args, **kwargs) and centralizes exception mapping: catch both
BaseExceptionGroup and Exception/KeyboardInterrupt, iterate leaf_exceptions(eg,
fix_tracebacks=False) for groups and single exceptions alike, call
_map_cli_exception on each leaf (instead of returning True on the first leaf),
and collect/make decisions from those mappings (call login_func if mapping
indicates reauth and return a boolean indicating whether reauth succeeded and
the caller should retry). Replace the duplicated try/except logic in
_handle_exception_group_with_reauth and the retry block so both use this new
helper, ensuring BaseExceptionGroup is handled on both initial and retry
attempts and reducing complexity to satisfy Ruff C901.
In `@python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py`:
- Around line 29-33: The current module-level warnings.filterwarnings call that
silences Unverified HTTPS warnings must be removed and the suppression made
conditional where the insecure_tls option is applied; delete the global
warnings.filterwarnings(...) for InsecureRequestWarning in
jumpstarter_cli_common/oidc.py and instead add a conditional
warnings.filterwarnings("ignore", category=InsecureRequestWarning, ...) inside
the code path that consumes insecure_tls (e.g., inside Config.client() or
wherever Config.client(insecure_tls=...) is invoked) so the warning is only
suppressed when insecure_tls=True; keep the authlib-related filter moved so all
warning filters run after imports.
- Around line 34-38: The E402 lint errors are caused by placing
warnings.filterwarnings(...) before the module imports; move both
warnings.filterwarnings(...) blocks so that all imports (e.g., OAuth2Session,
extract_compact, URL and JMP_OIDC_CALLBACK_PORT/JMP_OIDC_DEVICE_FLOW) appear
first, then add the two warnings.filterwarnings(...) calls after the import
section (they can remain where they are functionally, since the authlib warning
fires at runtime), and run make lint-fix to verify.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/login.py`:
- Around line 338-340: Silent reauth drops an explicit --device-flow choice:
login() respects the flag but relogin_client() recomputes flow from env/VS Code
detection causing headless users to fall back to browser auth; fix by persisting
the chosen flow in the client config (e.g., store a device_flow boolean on the
Client/Config object when login() runs) and update relogin_client() to read that
stored flag instead of recomputing via should_use_device_flow(), or add a
parameter to relogin_client(device_flow=...) and thread the device_flow
selection from login() through client initialization and any code paths that
call relogin_client(); update any helpers that call oidc.device_code_grant()
(references: login(), relogin_client(), should_use_device_flow(),
oidc.device_code_grant) to use the persisted/passed-in flag.
In `@python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py`:
- Around line 235-241: The loop that picks qemu_binary (in
jumpstarter_driver_qemu.driver.py) incorrectly treats the bare "qemu-kvm" as
present because "/" not in qemu_binary is true; change the selection logic to
check executability properly: for each candidate in ["/usr/libexec/qemu-kvm",
"/usr/bin/qemu-kvm", "qemu-kvm"] verify either Path(candidate).exists() and is
executable for absolute paths or use shutil.which(candidate) (or os.access on
the resolved path) for the bare name, only break when a valid executable is
found, and if none are valid set qemu_binary = f"qemu-system-{self.parent.arch}"
so the code falls back cleanly when qemu-kvm isn’t installed.
- Around line 378-380: The current assignment self._cidata_iso =
Path(self._cidata.name).parent / "cidata.iso" places the ISO in the shared temp
root causing races; change it to create the ISO under the driver's own temp
directory (e.g. use your driver temp attribute such as self._tmpdir or
self._temp_dir) so each VM gets a unique path, e.g. set self._cidata_iso =
Path(self._tmpdir) / "cidata.iso" (or the equivalent driver temp attribute
already in this class) and ensure subsequent code uses this new attribute.
---
Nitpick comments:
In `@python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py`:
- Around line 261-272: Replace fragile substring matching in the exception
handler with structured checks against
authlib.oauth2.rfc6749.errors.OAuthError.error: import OAuthError (or the
correct authlib exception class), catch OAuthError (or check isinstance) in the
except block and use e.error ==
"authorization_pending"/"slow_down"/"expired_token"/"access_denied" to control
flow (increase interval for "slow_down", continue for "authorization_pending",
raise a ClickException for "expired_token" and "access_denied"); remove the
`"expired" in error_msg` fallback and for any other error re-raise a
ClickException including the original exception for context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c168f8f7-15f1-4f7f-bc0e-87575bb1cb75
📒 Files selected for processing (10)
python/packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.pypython/packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions_test.pypython/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.pypython/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/login.pypython/packages/jumpstarter-cli/jumpstarter_cli/shell.pypython/packages/jumpstarter-cli/jumpstarter_cli/shell_test.pypython/packages/jumpstarter-driver-gpiod/pyproject.tomlpython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypython/packages/jumpstarter/jumpstarter/config/env.py
| warnings.filterwarnings( | ||
| "ignore", | ||
| message=r"Unverified HTTPS request is being made.*", | ||
| module=r"urllib3\..*", | ||
| ) |
There was a problem hiding this comment.
InsecureRequestWarning is suppressed globally, not conditionally on --insecure-tls
The comment says "when the user opts into --insecure-tls" but the filter is applied unconditionally at module import time. Any unverified HTTPS request made anywhere in the process — including from unrelated code paths — will silently drop its warning for every user, not just those who opted in.
Since the authlib comment above (lines 18-19) correctly explains why the warning fires at runtime not at import time, the right fix for both warning filters is to move them after all imports. For the InsecureRequestWarning specifically, the suppression should be conditional:
🛡️ Proposed fix: conditional suppression
Remove the module-level InsecureRequestWarning filter entirely, and add it where insecure_tls=True is first used, e.g., in Config.client():
def client(self, **kwargs):
+ if self.insecure_tls:
+ warnings.filterwarnings(
+ "ignore",
+ message=r"Unverified HTTPS request is being made.*",
+ module=r"urllib3\..*",
+ )
session = OAuth2Session(client_id=self.client_id, scope=self._scopes(), **kwargs)
session.verify = False if self.insecure_tls else (os.environ.get("SSL_CERT_FILE") or certifi.where())
return session🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py` around
lines 29 - 33, The current module-level warnings.filterwarnings call that
silences Unverified HTTPS warnings must be removed and the suppression made
conditional where the insecure_tls option is applied; delete the global
warnings.filterwarnings(...) for InsecureRequestWarning in
jumpstarter_cli_common/oidc.py and instead add a conditional
warnings.filterwarnings("ignore", category=InsecureRequestWarning, ...) inside
the code path that consumes insecure_tls (e.g., inside Config.client() or
wherever Config.client(insecure_tls=...) is invoked) so the warning is only
suppressed when insecure_tls=True; keep the authlib-related filter moved so all
warning filters run after imports.
| elif should_use_device_flow(device_flow): | ||
| tokens = await oidc.device_code_grant() | ||
| else: |
There was a problem hiding this comment.
Silent reauth forgets an explicit --device-flow choice.
login() honors --device-flow, but relogin_client() recomputes the flow only from env/VS Code detection. A user who logged in with jmp login --device-flow and did not also export JMP_OIDC_DEVICE_FLOW=1 will fall back to browser auth on token expiry, which breaks the headless workflow this PR is adding. Persist the selected auth flow in the client config, or otherwise thread it into relogin_client().
Also applies to: 388-391
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/login.py` around lines 338 -
340, Silent reauth drops an explicit --device-flow choice: login() respects the
flag but relogin_client() recomputes flow from env/VS Code detection causing
headless users to fall back to browser auth; fix by persisting the chosen flow
in the client config (e.g., store a device_flow boolean on the Client/Config
object when login() runs) and update relogin_client() to read that stored flag
instead of recomputing via should_use_device_flow(), or add a parameter to
relogin_client(device_flow=...) and thread the device_flow selection from
login() through client initialization and any code paths that call
relogin_client(); update any helpers that call oidc.device_code_grant()
(references: login(), relogin_client(), should_use_device_flow(),
oidc.device_code_grant) to use the persisted/passed-in flag.
| if self.parent.arch == platform.machine() and os.access("/dev/kvm", os.R_OK | os.W_OK): | ||
| # Try common locations for qemu-kvm | ||
| for qemu_binary in ["/usr/libexec/qemu-kvm", "/usr/bin/qemu-kvm", "qemu-kvm"]: | ||
| if Path(qemu_binary).exists() or "/" not in qemu_binary: | ||
| break | ||
| else: | ||
| qemu_binary = f"qemu-system-{self.parent.arch}" |
There was a problem hiding this comment.
Fall back cleanly when qemu-kvm is not installed.
This loop always breaks on the bare qemu-kvm candidate because "/" not in qemu_binary" is true, even if that wrapper is missing from PATH. On hosts with KVM access but only qemu-system-<arch>, on() now fails before boot.
Suggested fix
- if self.parent.arch == platform.machine() and os.access("/dev/kvm", os.R_OK | os.W_OK):
- # Try common locations for qemu-kvm
- for qemu_binary in ["/usr/libexec/qemu-kvm", "/usr/bin/qemu-kvm", "qemu-kvm"]:
- if Path(qemu_binary).exists() or "/" not in qemu_binary:
- break
- else:
- qemu_binary = f"qemu-system-{self.parent.arch}"
+ qemu_binary = f"qemu-system-{self.parent.arch}"
+ if self.parent.arch == platform.machine() and os.access("/dev/kvm", os.R_OK | os.W_OK):
+ for candidate in ["/usr/libexec/qemu-kvm", "/usr/bin/qemu-kvm", "qemu-kvm"]:
+ if ("/" in candidate and Path(candidate).exists()) or shutil.which(candidate):
+ qemu_binary = candidate
+ break🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py`
around lines 235 - 241, The loop that picks qemu_binary (in
jumpstarter_driver_qemu.driver.py) incorrectly treats the bare "qemu-kvm" as
present because "/" not in qemu_binary is true; change the selection logic to
check executability properly: for each candidate in ["/usr/libexec/qemu-kvm",
"/usr/bin/qemu-kvm", "qemu-kvm"] verify either Path(candidate).exists() and is
executable for absolute paths or use shutil.which(candidate) (or os.access on
the resolved path) for the bare name, only break when a valid executable is
found, and if none are valid set qemu_binary = f"qemu-system-{self.parent.arch}"
so the code falls back cleanly when qemu-kvm isn’t installed.
| # Create ISO image from cidata directory for cloud-init | ||
| # This is more compatible than vvfat which may not be available in qemu-kvm | ||
| self._cidata_iso = Path(self._cidata.name).parent / "cidata.iso" |
There was a problem hiding this comment.
Use a VM-scoped path for cidata.iso.
Path(self._cidata.name).parent / "cidata.iso" resolves to the shared temp root, so parallel QEMU instances race on the same file and off() from one VM can delete another VM's cloud-init media. Put the ISO under the driver's own temp directory instead.
Suggested fix
- self._cidata_iso = Path(self._cidata.name).parent / "cidata.iso"
+ self._cidata_iso = Path(self.parent._tmp_dir.name) / "cidata.iso"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py`
around lines 378 - 380, The current assignment self._cidata_iso =
Path(self._cidata.name).parent / "cidata.iso" places the ISO in the shared temp
root causing races; change it to create the ISO under the driver's own temp
directory (e.g. use your driver temp attribute such as self._tmpdir or
self._temp_dir) so each VM gets a unique path, e.g. set self._cidata_iso =
Path(self._tmpdir) / "cidata.iso" (or the equivalent driver temp attribute
already in this class) and ensure subsequent code uses this new attribute.
Move warnings.filterwarnings after imports to fix E402, extract helpers to reduce complexity below C901 threshold, and remove trailing whitespace. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
raballew
left a comment
raballew
left a comment
There was a problem hiding this comment.
Some inconsistencies but a good first second try.
| warnings.filterwarnings( | ||
| "ignore", | ||
| message=r"Unverified HTTPS request is being made.*", | ||
| module=r"urllib3\..*", | ||
| ) |
There was a problem hiding this comment.
The InsecureRequestWarning suppression runs unconditionally at module import time, which means any code that imports oidc silently hides legitimate TLS errors even when the user never passed --insecure-tls.
Consider moving the warnings.filterwarnings call into Config.client() or Config.__init__, gated on self.insecure_tls, so the warning is only suppressed when the user explicitly opted in.
| import ssl | ||
| from unittest.mock import AsyncMock, MagicMock, patch | ||
|
|
||
| from jumpstarter_cli_common.oidc import Config, _get_ssl_context | ||
| import click | ||
| import pytest | ||
|
|
||
| from jumpstarter_cli_common.oidc import ( | ||
| DEVICE_FLOW_GRANT_TYPE, | ||
| DEVICE_FLOW_POLL_INTERVAL, | ||
| Config, | ||
| _get_ssl_context, | ||
| should_use_device_flow, | ||
| ) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def anyio_backend(): | ||
| return "asyncio" | ||
|
|
||
|
|
||
| class TestConfigInsecureTls: |
There was a problem hiding this comment.
No tests covering the expired_token error path or the deadline-exceeded timeout path in device_code_grant. Both are critical failure modes.
| @@ -23,3 +38,253 @@ class TestGetSslContext: | |||
| def test_returns_ssl_context(self) -> None: | |||
There was a problem hiding this comment.
There is no test verifying the interval actually increases. A regression here would silently violate the RFC.
| except Exception as e: | ||
| error_msg = str(e) | ||
| if "authorization_pending" in error_msg: | ||
| continue | ||
| if "slow_down" in error_msg: | ||
| interval += 5 | ||
| continue | ||
| if "expired_token" in error_msg or "expired" in error_msg: | ||
| raise click.ClickException("Device code expired. Please try again.") from e | ||
| if "access_denied" in error_msg: | ||
| raise click.ClickException("Authorization was denied by the user.") from e | ||
| raise click.ClickException(f"Device flow token request failed: {e}") from e |
There was a problem hiding this comment.
The authlib library typically provides structured OAuthError with an .error attribute. Consider checking for specific exception types and using structured attributes, falling back to string matching only for unknown types.
| while time.monotonic() < deadline: | ||
| await anyio.sleep(interval) | ||
|
|
||
| client = self.client() |
There was a problem hiding this comment.
The device flow polling loop creates a new self.client() (OAuth2Session wrapping a requests.Session with a connection pool) on every iteration.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
When refresh_token_grant fails in relogin_client, a bare except Exception: pass silently swallows the error with no logging. This makes debugging refresh failures during re-authentication very difficult.
| lambda: client.fetch_token(config["token_endpoint"], authorization_response=authorization_response) | ||
| ) | ||
|
|
||
| async def device_code_grant(self): |
There was a problem hiding this comment.
This is a lot of responsibility in one class.
Consider extracting grant-specific logic into standalone functions or strategy classes. For example, device_code_grant could be a standalone function that receives a Config and returns tokens.
| "user_code": "ABCD-1234", | ||
| "verification_uri": "https://auth.example.com/device", | ||
| "expires_in": 600, | ||
| "interval": 0, |
There was a problem hiding this comment.
This is essentially dead code. Using a non-zero interval (e.g., 5) and verifying mock sleep was called with the correct value would make these tests more meaningful.
2 participants
Summary
--device-flow/--no-device-flowCLI option tojmp login, enabling headless authentication via the RFC 8628 device code flow. Auto-detected when running inside VS Code Dev Spaces (VSCODE_INJECTION=1) or whenJMP_OIDC_DEVICE_FLOW=1is set.logger.debug/logger.info. Token refresh also works correctly with--insecure-tlsnow by passing the TLS flag to the OIDC config.handle_exceptions_with_reauthenticationnow silently re-authenticates and retries the wrapped function once, instead of asking the user to manually retry.urllib3.InsecureRequestWarning(when user already opted into--insecure-tls) and transientauthlib.josedeprecation warnings.Test plan
jmp login --device-flowprompts with a verification URI and polls until the user completes browser authVSCODE_INJECTION=1)jmp loginwithout--device-flowstill uses the authorization code grant--insecure-tlsno longer emitsInsecureRequestWarningon every requestoidc_test.py,exceptions_test.py,shell_test.py)🤖 Generated with Claude Code