Add Device Authorization Grant (RFC 8628) for headless/container environments - #916
Add Device Authorization Grant (RFC 8628) for headless/container environments#916mickume wants to merge 5 commits into
Conversation
…eadless/container environments Add device_authorization_grant() method to the OIDC Config class implementing RFC 8628 Device Authorization Grant flow. This enables login from containerized IDEs (e.g., OpenShift Dev Spaces) where localhost callbacks are unreachable. Changes: - Add JMP_OIDC_DEVICE_FLOW env var constant to env.py - Add device_authorization_grant() to Config in oidc.py with full RFC 8628 compliance: device code request, verification URI display, token polling with authorization_pending/slow_down/expired_token/access_denied handling - Add --device-flow CLI flag to opt_oidc decorator - Add should_use_device_flow() helper for flag + env var detection - Wire device flow selection in login() and relogin_client() in login.py - Add comprehensive tests for device flow grant, polling, error handling, flow selection logic, and env var auto-detection
📝 WalkthroughWalkthroughAdds OAuth 2.0 device authorization selected by CLI flag or environment variable. Login and relogin use device flow when enabled, while expired-token handling now performs one automatic reauthentication retry. ChangesOIDC device authorization
Automatic reauthentication retry
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Config
participant OIDCProvider
CLI->>Config: select device authorization grant
Config->>OIDCProvider: request device code
OIDCProvider-->>Config: verification URI and user code
Config->>CLI: display verification details
Config->>OIDCProvider: poll token endpoint
OIDCProvider-->>Config: return token or device-flow error
Config-->>CLI: complete login
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py (1)
227-274: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding request timeouts on the device-flow HTTP calls.
Neither the device-authorization POST (Line 229) nor the token-poll POST (Line 266) sets a timeout. The overall
deadlineloop bounds the polling cadence, but an individual hung connection can still block past the intended expiry. Passing anaiohttp.ClientTimeoutto the session or per-requesttimeout=would harden this against a stalled IdP.🤖 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 227 - 274, Update the OIDC device-flow HTTP calls in the session around the device authorization POST and token-poll POST to apply an aiohttp.ClientTimeout, either on ClientSession or per request. Ensure both requests can terminate when the identity provider connection stalls, while preserving the existing polling deadline behavior.
🤖 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.
Nitpick comments:
In `@python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py`:
- Around line 227-274: Update the OIDC device-flow HTTP calls in the session
around the device authorization POST and token-poll POST to apply an
aiohttp.ClientTimeout, either on ClientSession or per request. Ensure both
requests can terminate when the identity provider connection stalls, while
preserving the existing polling deadline behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e55cd38-afb4-4c13-b317-fce6d017268f
📒 Files selected for processing (5)
python/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/login_test.pypython/packages/jumpstarter/jumpstarter/config/env.py
…ixes #18) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@mickume The e2e failures is unrelated, I wonder if there is a way to test this in E2E with our dex setup, or modified dex setup. |
|
@mickume: we also need to update the docs, explaining how to use this feature for login |
…ehavior The previous fix correctly implemented retry-after-reauth in the decorator but left shell_test.py::test_expired_token_triggers_reauth asserting the old "Please try again now" error message. Update the test to simulate successful re-authentication (login_mock updates the token) and verify that the retried command succeeds without raising an exception. - fix(#17, nightshift): suppress noisy warnings and retry transparently on token refresh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 261-301: Update _try_reauth_or_handle so _ReauthSucceeded is
always caught: return True when allow_reauth is enabled, and raise
ClickExceptionRed("Unexpected re-auth signal") from None when it is disabled.
Preserve the existing handler invocation behavior and ensure BaseExceptionGroup
retries surface the normal ClickExceptionRed instead of leaking the internal
signal.
🪄 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 Plus
Run ID: 3ea4cae8-3895-4587-b976-f5a83676d1ee
📒 Files selected for processing (5)
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/shell_test.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py
- python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc_test.py
| def _try_reauth_or_handle(handler, exc, login_func, *, allow_reauth: bool) -> bool: | ||
| """Attempt re-auth via *handler*; return True if re-auth succeeded. | ||
|
|
||
| When *allow_reauth* is False the handler is called without catching | ||
| ``_ReauthSucceeded`` — any re-auth signal propagates as a normal error. | ||
| """ | ||
| if allow_reauth: | ||
| try: | ||
| handler(exc, login_func) | ||
| except _ReauthSucceeded: | ||
| return True | ||
| else: | ||
| handler(exc, login_func) | ||
| return False | ||
|
|
||
|
|
||
| def _call_with_exception_handling(func, args, kwargs, login_func, *, allow_reauth: bool): | ||
| """Call *func* and handle exceptions, optionally allowing re-authentication. | ||
|
|
||
| Returns ``(result, needs_retry)`` where *needs_retry* is ``True`` when | ||
| re-authentication succeeded and the caller should retry the call. | ||
| """ | ||
| try: | ||
| return func(*args, **kwargs), False | ||
| except _ReauthSucceeded: | ||
| raise ClickExceptionRed("Unexpected re-auth signal") from None | ||
| except BaseExceptionGroup as eg: | ||
| if _try_reauth_or_handle(_handle_exception_group_with_reauth, eg, login_func, allow_reauth=allow_reauth): | ||
| return None, True | ||
| except (ConnectionError, JumpstarterException, click.ClickException) as e: | ||
| if isinstance(e, ConnectionError) and not allow_reauth: | ||
| raise ClickExceptionRed(str(e)) from None | ||
| if _try_reauth_or_handle(_handle_single_exception_with_reauth, e, login_func, allow_reauth=allow_reauth): | ||
| return None, True | ||
| except Exception as e: | ||
| _raise_if_mappable(e) | ||
| raise | ||
| except KeyboardInterrupt as e: | ||
| _raise_if_mappable(e) | ||
| raise | ||
| return None, False # pragma: no cover — handlers above always raise or return |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
_ReauthSucceeded can leak out uncaught when the retry hits a BaseExceptionGroup.
_try_reauth_or_handle's allow_reauth=False branch calls handler(exc, login_func) without catching _ReauthSucceeded (Line 272-273). For the single-exception path this is safe only because _call_with_exception_handling short-circuits ConnectionError before allow_reauth=False ever reaches the handler (Line 291-292: if isinstance(e, ConnectionError) and not allow_reauth: raise ClickExceptionRed(...)). No equivalent guard exists for the BaseExceptionGroup branch (Line 287-289): if the retry (allow_reauth=False) raises a BaseExceptionGroup whose leaf is an "expired" ConnectionError, _handle_exception_group_with_reauth → _handle_connection_error_with_reauth (Line 218-233) still calls login_func again and raises _ReauthSucceeded(), which now propagates uncaught out of wrapped() as a raw internal exception instead of a click.ClickException. This also breaks the "retries the original call exactly once... surfaced normally" guarantee documented on Line 308-310, since a leaked _ReauthSucceeded is not a normal, user-facing error.
The cleanest fix is to always catch _ReauthSucceeded in _try_reauth_or_handle and convert it to a proper error when re-auth isn't allowed, rather than duplicating the ConnectionError-specific short-circuit for every call site.
🐛 Proposed fix
def _try_reauth_or_handle(handler, exc, login_func, *, allow_reauth: bool) -> bool:
"""Attempt re-auth via *handler*; return True if re-auth succeeded.
- When *allow_reauth* is False the handler is called without catching
- ``_ReauthSucceeded`` — any re-auth signal propagates as a normal error.
+ When *allow_reauth* is False, a ``_ReauthSucceeded`` signal raised by the
+ handler is converted into a ``ClickExceptionRed`` instead of retrying
+ again, guaranteeing at most one re-auth attempt regardless of whether the
+ triggering exception arrived standalone or inside a ``BaseExceptionGroup``.
"""
- if allow_reauth:
- try:
- handler(exc, login_func)
- except _ReauthSucceeded:
- return True
- else:
- handler(exc, login_func)
- return False
+ try:
+ handler(exc, login_func)
+ except _ReauthSucceeded:
+ if allow_reauth:
+ return True
+ raise ClickExceptionRed(str(exc)) from None
+ return FalseConsider adding a test that raises a BaseExceptionGroup wrapping an expired-token ConnectionError on the retried call, to lock in this fix. As per coding guidelines, "Provide comprehensive package test coverage, prioritizing end-to-end tests that start a server and client; use mocks when system tools, services, or platform compatibility make end-to-end testing impractical."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _try_reauth_or_handle(handler, exc, login_func, *, allow_reauth: bool) -> bool: | |
| """Attempt re-auth via *handler*; return True if re-auth succeeded. | |
| When *allow_reauth* is False the handler is called without catching | |
| ``_ReauthSucceeded`` — any re-auth signal propagates as a normal error. | |
| """ | |
| if allow_reauth: | |
| try: | |
| handler(exc, login_func) | |
| except _ReauthSucceeded: | |
| return True | |
| else: | |
| handler(exc, login_func) | |
| return False | |
| def _call_with_exception_handling(func, args, kwargs, login_func, *, allow_reauth: bool): | |
| """Call *func* and handle exceptions, optionally allowing re-authentication. | |
| Returns ``(result, needs_retry)`` where *needs_retry* is ``True`` when | |
| re-authentication succeeded and the caller should retry the call. | |
| """ | |
| try: | |
| return func(*args, **kwargs), False | |
| except _ReauthSucceeded: | |
| raise ClickExceptionRed("Unexpected re-auth signal") from None | |
| except BaseExceptionGroup as eg: | |
| if _try_reauth_or_handle(_handle_exception_group_with_reauth, eg, login_func, allow_reauth=allow_reauth): | |
| return None, True | |
| except (ConnectionError, JumpstarterException, click.ClickException) as e: | |
| if isinstance(e, ConnectionError) and not allow_reauth: | |
| raise ClickExceptionRed(str(e)) from None | |
| if _try_reauth_or_handle(_handle_single_exception_with_reauth, e, login_func, allow_reauth=allow_reauth): | |
| return None, True | |
| except Exception as e: | |
| _raise_if_mappable(e) | |
| raise | |
| except KeyboardInterrupt as e: | |
| _raise_if_mappable(e) | |
| raise | |
| return None, False # pragma: no cover — handlers above always raise or return | |
| def _try_reauth_or_handle(handler, exc, login_func, *, allow_reauth: bool) -> bool: | |
| """Attempt re-auth via *handler*; return True if re-auth succeeded. | |
| When *allow_reauth* is False, a ``_ReauthSucceeded`` signal raised by the | |
| handler is converted into a ``ClickExceptionRed`` instead of retrying | |
| again, guaranteeing at most one re-auth attempt regardless of whether the | |
| triggering exception arrived standalone or inside a ``BaseExceptionGroup``. | |
| """ | |
| try: | |
| handler(exc, login_func) | |
| except _ReauthSucceeded: | |
| if allow_reauth: | |
| return True | |
| raise ClickExceptionRed(str(exc)) from None | |
| return False | |
| def _call_with_exception_handling(func, args, kwargs, login_func, *, allow_reauth: bool): | |
| """Call *func* and handle exceptions, optionally allowing re-authentication. | |
| Returns ``(result, needs_retry)`` where *needs_retry* is ``True`` when | |
| re-authentication succeeded and the caller should retry the call. | |
| """ | |
| try: | |
| return func(*args, **kwargs), False | |
| except _ReauthSucceeded: | |
| raise ClickExceptionRed("Unexpected re-auth signal") from None | |
| except BaseExceptionGroup as eg: | |
| if _try_reauth_or_handle(_handle_exception_group_with_reauth, eg, login_func, allow_reauth=allow_reauth): | |
| return None, True | |
| except (ConnectionError, JumpstarterException, click.ClickException) as e: | |
| if isinstance(e, ConnectionError) and not allow_reauth: | |
| raise ClickExceptionRed(str(e)) from None | |
| if _try_reauth_or_handle(_handle_single_exception_with_reauth, e, login_func, allow_reauth=allow_reauth): | |
| return None, True | |
| except Exception as e: | |
| _raise_if_mappable(e) | |
| raise | |
| except KeyboardInterrupt as e: | |
| _raise_if_mappable(e) | |
| raise | |
| return None, False # pragma: no cover — handlers above always raise or return |
🤖 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/exceptions.py`
around lines 261 - 301, Update _try_reauth_or_handle so _ReauthSucceeded is
always caught: return True when allow_reauth is enabled, and raise
ClickExceptionRed("Unexpected re-auth signal") from None when it is disabled.
Preserve the existing handler invocation behavior and ensure BaseExceptionGroup
retries surface the normal ClickExceptionRed instead of leaking the internal
signal.
Source: Coding guidelines
|
@mickume can you have an eye on the python linter? |
| # import chain (authlib._joserfc_helpers -> authlib.jose). The project already | ||
| # uses joserfc directly for JWT operations; authlib is only needed for | ||
| # OAuth2Session. The warning provides no actionable information to end users. | ||
| warnings.filterwarnings("ignore", category=DeprecationWarning, module=r"authlib\.") |
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
Thank you @mickume just a couple of nits, mostly the reorg of noqa imports, the other two are mostly nits/being overly protecting with bad server responses I'd say.
Some comments were AI assisted, but corrected, many removed by a human still in the loop :)
| verification_uri = device_data.get("verification_uri") | ||
| user_code = device_data.get("user_code") | ||
| click.echo( | ||
| f"To sign in, open the following URL in your browser:\n\n {verification_uri}\n\n" | ||
| f"Then enter the code: {user_code}\n" | ||
| ) |
There was a problem hiding this comment.
not really important, mostly a nit, because this means the server is having some sort of error in the response: If the server omits both verification_uri_complete and verification_uri, the user will see:
To sign in, open the following URL in your browser:
None
Then enter the code: None
This silently displays garbage instead of failing with a clear error. Consider validating these fields, e.g.:
verification_uri = device_data.get("verification_uri")
user_code = device_data.get("user_code")
if not verification_uri or not user_code:
raise click.ClickException(
"Device authorization response is missing required fields "
"(verification_uri / user_code). The identity provider may not "
"fully support RFC 8628."
)(Same concern applies to device_data["device_code"] on line 259 — a KeyError from a non-compliant server would surface as an unhelpful traceback, though this is lower priority since device_code is a REQUIRED field per RFC 8628 and the pattern is consistent with other grant methods.)
| raise click.ClickException( | ||
| "Device authorization timed out waiting for user approval. Please try again." | ||
| ) |
There was a problem hiding this comment.
This client-side polling timeout path is not covered by tests. The existing test_raises_on_expired_token covers the server responding with {"error": "expired_token"}, which is a different code path (line 301-303).
To cover this, you could add a test with expires_in: 0 (or mock time.monotonic) so the while loop exits immediately and this raise is reached:
@pytest.mark.asyncio
async def test_raises_on_polling_timeout(self) -> None:
config = Config(issuer="https://auth.example.com", client_id="test")
discovery = {
"token_endpoint": "https://auth.example.com/token",
"device_authorization_endpoint": "https://auth.example.com/device",
}
device_response_data = {
"device_code": "test-device-code",
"user_code": "ABCD-EFGH",
"verification_uri": "https://auth.example.com/device",
"interval": 0.01,
"expires_in": 0, # immediate expiry
}
# ... mock session that always returns authorization_pending ...
with pytest.raises(click.ClickException, match="timed out"):
await config.device_authorization_grant()| from authlib.integrations.requests_client import OAuth2Session # noqa: E402 | ||
| from joserfc.jws import extract_compact # noqa: E402 | ||
| from yarl import URL # noqa: E402 | ||
|
|
||
| from jumpstarter.config.env import JMP_OIDC_CALLBACK_PORT, JMP_OIDC_DEVICE_FLOW # noqa: E402 |
There was a problem hiding this comment.
Only the authlib import needs to be after the filterwarnings call. The joserfc, yarl, and jumpstarter.config.env imports are unrelated to the authlib warning and can stay in the normal import section, avoiding three unnecessary noqa: E402 markers:
from joserfc.jws import extract_compact
from yarl import URL
from jumpstarter.config.env import JMP_OIDC_CALLBACK_PORT, JMP_OIDC_DEVICE_FLOW
# Suppress AuthlibDeprecationWarning emitted during the authlib import chain.
warnings.filterwarnings("ignore", category=DeprecationWarning, module=r"authlib\.")
from authlib.integrations.requests_client import OAuth2Session # noqa: E402| # Suppress AuthlibDeprecationWarning emitted unconditionally during the authlib | ||
| # import chain (authlib._joserfc_helpers -> authlib.jose). The project already | ||
| # uses joserfc directly for JWT operations; authlib is only needed for | ||
| # OAuth2Session. The warning provides no actionable information to end users. | ||
| warnings.filterwarnings("ignore", category=DeprecationWarning, module=r"authlib\.") | ||
|
|
||
| from authlib.integrations.requests_client import OAuth2Session # noqa: E402 | ||
| from joserfc.jws import extract_compact # noqa: E402 | ||
| from yarl import URL # noqa: E402 | ||
|
|
||
| from jumpstarter.config.env import JMP_OIDC_CALLBACK_PORT, JMP_OIDC_DEVICE_FLOW # noqa: E402 |
There was a problem hiding this comment.
Nit: Only the authlib import needs to sit after filterwarnings. Move the unrelated imports back to the normal section to drop three unnecessary noqa: E402 markers:
| # Suppress AuthlibDeprecationWarning emitted unconditionally during the authlib | |
| # import chain (authlib._joserfc_helpers -> authlib.jose). The project already | |
| # uses joserfc directly for JWT operations; authlib is only needed for | |
| # OAuth2Session. The warning provides no actionable information to end users. | |
| warnings.filterwarnings("ignore", category=DeprecationWarning, module=r"authlib\.") | |
| from authlib.integrations.requests_client import OAuth2Session # noqa: E402 | |
| from joserfc.jws import extract_compact # noqa: E402 | |
| from yarl import URL # noqa: E402 | |
| from jumpstarter.config.env import JMP_OIDC_CALLBACK_PORT, JMP_OIDC_DEVICE_FLOW # noqa: E402 | |
| from joserfc.jws import extract_compact | |
| from yarl import URL | |
| from jumpstarter.config.env import JMP_OIDC_CALLBACK_PORT, JMP_OIDC_DEVICE_FLOW | |
| # Suppress AuthlibDeprecationWarning emitted unconditionally during the authlib | |
| # import chain (authlib._joserfc_helpers -> authlib.jose). The project already | |
| # uses joserfc directly for JWT operations; authlib is only needed for | |
| # OAuth2Session. The warning provides no actionable information to end users. | |
| warnings.filterwarnings("ignore", category=DeprecationWarning, module=r"authlib\.") | |
| from authlib.integrations.requests_client import OAuth2Session # noqa: E402 |
|
@mickume needs rebasing. |
3 participants
Add device_authorization_grant() method to the OIDC Config class implementing RFC 8628 Device Authorization Grant flow. This enables login from containerized IDEs (e.g., OpenShift Dev Spaces) where localhost callbacks are unreachable.
Changes:
compliance: device code request, verification URI display, token polling
with authorization_pending/slow_down/expired_token/access_denied handling
flow selection logic, and env var auto-detection