Implement token exchange based login (machine to machine)#329
Implement token exchange based login (machine to machine)#329
Conversation
WalkthroughThe changes add new command-line options Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant LoginFunction
participant OIDC
CLI->>LoginFunction: Provide credentials (token or username/password)
alt Token provided
LoginFunction->>OIDC: Call token_exchange_grant(token, connector_id)
OIDC-->>LoginFunction: Return authentication result
else No token provided
LoginFunction->>OIDC: Call password_grant(username, password)
OIDC-->>LoginFunction: Return authentication result
end
LoginFunction-->>CLI: Return login response
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_login.py (1)
25-28: Parameter type inconsistency with client_login.The
connector_idparameter is defined asstrhere, but asstr | Noneinclient_login.py. Consider making these consistent to avoid potential issues.- connector_id: str, + connector_id: str | None,packages/jumpstarter-cli-client/jumpstarter_cli_client/client_login.py (1)
103-109: Consider adding error handling for token exchange failures.The token exchange process might fail for various reasons (invalid token, network issues, etc.). Consider adding error handling to provide more detailed error messages to help users diagnose issues.
if token is not None: kwargs = {"connector_id": connector_id} if connector_id is not None else {} - tokens = await oidc.token_exchange_grant(token, **kwargs) + try: + tokens = await oidc.token_exchange_grant(token, **kwargs) + except Exception as e: + click.echo(f"Token exchange failed: {str(e)}") + # Optionally: fall back to other auth methods or exit + raise click.Abort() elif username is not None and password is not None: tokens = await oidc.password_grant(username, password) else:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_login.py(5 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py(2 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_login.py(3 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(3 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (12)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
281-284: New mock class implementation looks good.The addition of the
MockStorageMuxFlasherclass is straightforward and appropriately inherits from bothStorageMuxFlasherInterfaceandMockStorageMux. This implementation follows the pattern of other mock classes in the file and will be useful for testing.packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py (2)
17-20: LGTM: New connector_id option added correctly.The new command-line option is well documented with a helpful description that specifies it's Dex-specific.
40-54: Token exchange implementation looks good.The implementation follows the OAuth 2.0 Token Exchange standard (RFC 8693) and maintains a consistent pattern with the existing grant methods. It properly uses
run_syncfor the token fetching operation.packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_login.py (4)
2-2: Import updated correctly for the new OIDC option.The import has been updated to include the new
opt_connector_idfrom the common OIDC module.
14-14: Token option added correctly.The new
--tokenoption is well-defined with appropriate help text.
17-17: Connector ID option added correctly.The
opt_connector_idis properly included in the function decorators.
54-56: Token-based authentication logic looks good.The implementation prioritizes token-based authentication and correctly handles the optional connector_id parameter.
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_login.py (5)
3-3: Import updated correctly for the new OIDC option.The import has been updated to include the new
opt_connector_idfrom the common OIDC module.
32-32: Token option added correctly.The new
--tokenoption is well-defined with appropriate help text.
49-49: Connector ID option added correctly.The
opt_connector_idis properly included in the function decorators.
58-61: Parameter type is correctly set to optional.The
tokenandconnector_idparameters are properly defined as optional types.
103-105: Token-based authentication logic looks good.The implementation prioritizes token-based authentication and correctly handles the optional connector_id parameter.
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
Looks good, but let's document too.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_login.py (1)
28-28: Update type annotation forconnector_idparameterThe
connector_idparameter is typed asstr, but at line 55 it's checked forNone. The type annotation should be updated tostr | Noneto match how this parameter is used.- connector_id: str, + connector_id: str | None,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_login.py(5 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py(2 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_login.py(3 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(3 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py(2 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.py(2 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2.py(2 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py
- packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.py
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py
- packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2.py
- packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.py
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py
- packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
🔇 Additional comments (3)
packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_login.py (1)
54-57: LGTM: Authentication flow properly prioritizes token-based authThe implemented authentication flow correctly prioritizes token-based authentication when a token is provided. This approach aligns well with the PR objective of implementing token exchange based login for machine-to-machine communication, while maintaining fallback options for other authentication methods.
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_login.py (2)
58-61: LGTM: Token and connector_id parameters properly typedThe parameter types are correctly defined with
token: str | Noneandconnector_id: str | None, accurately representing that these parameters are optional.
103-106: LGTM: Authentication flow implementation is consistentThe token-based authentication flow is correctly implemented and consistent with the implementation in the exporter login function. The prioritization of token exchange over other authentication methods aligns with the PR objectives.
Done |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/source/config/oidc.md (2)
50-54: Enhance Clarity in CLI Options for Client LoginThe added block clearly demonstrates three separate authentication flows for clients:
- Default interactive login: without additional options, users are directed to the web-based flow.
- Non-interactive login: allowing direct specification of username and password.
- Machine-to-machine authentication: via the newly introduced
--token <token>option.While the content is accurate and aligns with the PR objective for token exchange based login, consider formatting these options as a bullet list or separate sub-sections to improve readability and reduce any potential ambiguity for CLI users.
60-60: Verify Consistency of Exporter Login DocumentationThe note on line 60 indicates that
jmp exporter loginnow accepts--username,--password, and--token, mirroring the client login options. This consistency is beneficial; ensure that the actual implementation and CLI help messages reflect this clearly. You might also consider adding an explicit CLI example for exporters using the token option to parallel the client login section.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/source/config/oidc.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores