Add Device Authorization Grant (RFC 8628) for headless/container environments - #976
Open
mickume wants to merge 2 commits into
Open
Add Device Authorization Grant (RFC 8628) for headless/container environments#976mickume wants to merge 2 commits into
mickume wants to merge 2 commits into
Conversation
…r environments Rebased from PR jumpstarter-dev#916 onto current upstream main. Resolves conflicts caused by upstream advancing past the original PR base; exceptions.py, exceptions_test.py, and shell_test.py changes were already absorbed upstream and are no longer part of this diff. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughThe CLI adds OAuth 2.0 Device Authorization Grant support. Users can enable it with ChangesOIDC device-flow support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Config.device_authorization_grant
participant OIDC discovery
participant OIDC provider
CLI->>Config.device_authorization_grant: start device authorization
Config.device_authorization_grant->>OIDC discovery: read device authorization endpoint
Config.device_authorization_grant->>OIDC provider: request device credentials
OIDC provider-->>Config.device_authorization_grant: verification URL and user code
Config.device_authorization_grant->>OIDC provider: poll token endpoint
OIDC provider-->>Config.device_authorization_grant: token or grant status
Config.device_authorization_grant-->>CLI: token data or error
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
python/packages/jumpstarter-cli/jumpstarter_cli/login.py (1)
345-346: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
--callback-portis silently ignored when the device flow is selected.If a user passes both
--device-flowand--callback-port, the command ignores--callback-port. Consider a warning so the user knows the port is unused.♻️ Proposed change
elif should_use_device_flow(device_flow): + if callback_port is not None: + click.echo("Warning: --callback-port is ignored when the device authorization grant is used.", err=True) tokens = await oidc.device_authorization_grant()🤖 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 345 - 346, Update the device-flow branch around should_use_device_flow and oidc.device_authorization_grant to warn when --callback-port is provided, since that option is unused in this flow; preserve the existing device authorization behavior.python/packages/jumpstarter-cli/jumpstarter_cli/login_test.py (1)
284-428: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParametrize the three device-flow selection tests.
The three tests differ only in the CLI flag and the environment variable. The
FakeOidcConfigclass, the fakefetch_auth_config, and the invocation are duplicated three times. A single parametrized test with a shared fixture reduces the duplication and keeps the cases aligned when the login flow changes.🤖 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_test.py` around lines 284 - 428, Parametrize the three device-flow selection tests into one test covering CLI flag, environment variable, and no-signal cases. Extract the shared auth configuration, fake fetch_auth_config, FakeOidcConfig, monkeypatch setup, and runner invocation into the parametrized test or fixtures, while retaining assertions that device_authorization_grant is selected only for the flag and environment-variable cases and authorization_code_grant is selected otherwise.
🤖 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/oidc_test.py`:
- Around line 49-51: Update test_flag_takes_priority_over_env to set
JMP_OIDC_DEVICE_FLOW to a non-"1" value instead of deleting it, while keeping
device_flow_flag=True, so the test verifies the explicit flag overrides the
environment setting.
In `@python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py`:
- Around line 252-261: Update the device authorization response handling around
response.json() and device_data["device_code"] to catch non-JSON parsing
failures and missing device_code values, converting both into descriptive
click.ClickException errors. Preserve the existing HTTP-status error handling
and only continue to interval/expires_in processing after valid device data is
obtained.
- Line 1: Replace the asyncio dependency in authentication_code_grant with
anyio: remove the asyncio import, add or reuse the anyio import, and change the
polling delay from asyncio.sleep(interval) to anyio.sleep(interval) while
preserving the existing device-flow loop behavior.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/login_test.py`:
- Around line 316-330: Update the affected login tests around the device-flow
invocations to accept the tmp_path fixture, construct the nonexistent
client-config path under tmp_path, and store each runner.invoke result. Assert
the result succeeds before checking device_flow_called and auth_code_called,
applying the same changes to the tests at all three referenced invocation
blocks.
---
Nitpick comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/login_test.py`:
- Around line 284-428: Parametrize the three device-flow selection tests into
one test covering CLI flag, environment variable, and no-signal cases. Extract
the shared auth configuration, fake fetch_auth_config, FakeOidcConfig,
monkeypatch setup, and runner invocation into the parametrized test or fixtures,
while retaining assertions that device_authorization_grant is selected only for
the flag and environment-variable cases and authorization_code_grant is selected
otherwise.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/login.py`:
- Around line 345-346: Update the device-flow branch around
should_use_device_flow and oidc.device_authorization_grant to warn when
--callback-port is provided, since that option is unused in this flow; preserve
the existing device authorization behavior.
🪄 Autofix
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: d138bfe8-8c12-4a8b-8b06-29174350edcb
📒 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
- oidc.py: use anyio.sleep instead of asyncio.sleep for consistency with the rest of the module's async backend - oidc.py: handle non-JSON device authorization responses and a missing device_code field with descriptive ClickExceptions instead of raw tracebacks - oidc_test.py: fix test_flag_takes_priority_over_env to actually verify flag precedence over a conflicting env value - login_test.py: use tmp_path instead of a shared /tmp path, and assert the CLI invocation succeeds before checking flow selection Also fixes a pre-existing bug uncovered by the login_test.py exit-code assertion: logging in with --client-config pointing at a new file hardcoded the config's alias to "default" and then tried to mark it as the current client by that alias, which always failed since the config was saved to the given path, not the default alias location. Path-based client configs are no longer treated as alias-addressable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py (1)
261-266: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate the JSON value and the
device_codevalue.If the endpoint returns a valid JSON array, string, or number, Line 262 raises
TypeError. This bypasses theClickExceptionhandling. Ifdevice_codeis empty or not a string, polling starts with an invalid credential.Validate that
device_datais an object and thatdevice_codeis a non-empty string before polling.Proposed fix
- try: - device_code = device_data["device_code"] - except KeyError as e: + if not isinstance(device_data, dict): + raise click.ClickException("Device authorization response must be a JSON object.") + + device_code = device_data.get("device_code") + if not isinstance(device_code, str) or not device_code: raise click.ClickException( "Device authorization response is missing the required 'device_code' field." - ) 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 - 266, Update the device authorization response handling around device_data and device_code to first validate that the decoded JSON value is an object, then ensure device_code is a non-empty string before polling. Convert invalid response shapes and values into the existing click.ClickException path, while preserving the current missing-field error behavior for valid objects without device_code.
🤖 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.
Duplicate comments:
In `@python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py`:
- Around line 261-266: Update the device authorization response handling around
device_data and device_code to first validate that the decoded JSON value is an
object, then ensure device_code is a non-empty string before polling. Convert
invalid response shapes and values into the existing click.ClickException path,
while preserving the current missing-field error behavior for valid objects
without device_code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ec9c3353-547e-4056-ba21-d7bacde8f9eb
📒 Files selected for processing (4)
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.py
🚧 Files skipped from review as they are similar to previous changes (3)
- python/packages/jumpstarter-cli/jumpstarter_cli/login_test.py
- python/packages/jumpstarter-cli/jumpstarter_cli/login.py
- python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc_test.py
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
1 participant
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.
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
Replaces #916, which could not be rebased cleanly due to divergence from
main(used directly as the PR branch). This PR is rebased onto current upstreammainfrom a dedicated feature branch. Note: theexceptions.py,exceptions_test.py, andshell_test.pychanges from #916 are no longer included, as they were already merged upstream separately.