awxkit: Support --conf.token/CONTROLLER_OAUTH_TOKEN#16384
Open
lalten wants to merge 4 commits intoansible:develfrom
Open
awxkit: Support --conf.token/CONTROLLER_OAUTH_TOKEN#16384lalten wants to merge 4 commits intoansible:develfrom
--conf.token/CONTROLLER_OAUTH_TOKEN#16384lalten wants to merge 4 commits intoansible:develfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds OAuth2 Bearer-token support to the CLI: config/CLI parsing now supplies a Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Config
participant Auth
participant Session
CLI->>Config: get_config('token')
Config-->>CLI: token
alt token present and non-empty
CLI->>Auth: authenticate()
Auth->>Session: set session.headers['Authorization'] = "Bearer {token}"
Session-->>Auth: header set
Auth-->>CLI: return (early)
else token absent or empty
CLI->>Auth: authenticate()
Auth->>Session: load_session() / login() (basic/session auth)
Session-->>Auth: session established
Auth-->>CLI: return
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
🧹 Nitpick comments (2)
awxkit/test/cli/test_authentication.py (2)
140-155: Assert session path is skipped in all token-auth tests
test_oauth_token_from_env_varandtest_oauth_token_precedence_over_basic_authshould also assertload_sessionis never called, to fully lock in the “Bearer token returns early” behavior.Proposed patch
def test_oauth_token_from_env_var(): @@ assert mock_connection.session.headers['Authorization'] == 'Bearer env_token_value' mock_connection.login.assert_not_called() + mock_root.load_session.assert_not_called() @@ def test_oauth_token_precedence_over_basic_auth(monkeypatch): @@ assert mock_connection.session.headers['Authorization'] == 'Bearer my_token' mock_connection.login.assert_not_called() + mock_root.load_session.assert_not_called()Also applies to: 157-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awxkit/test/cli/test_authentication.py` around lines 140 - 155, The tests for token-based auth need to assert that session loading is skipped; update test_oauth_token_from_env_var and test_oauth_token_precedence_over_basic_auth to verify load_session is never invoked. Mock or spy on the CLI.load_session (or the instance method used to restore sessions) before calling cli.authenticate(), then add an assertion like cli.load_session.assert_not_called() after authenticate() to lock in the “Bearer token returns early” behavior; reference the CLI.parse_args and CLI.authenticate calls already in the test to locate where to add the mock and assertion.
133-133: Fix Ruff RUF059: unused unpacked variable
mock_rootis unpacked but unused in two tests, which trips lint and adds noise.Proposed patch
- cli, mock_root, mock_connection = setup_token_auth(['awx', '--conf.token', 'cli_token_value']) + cli, _mock_root, mock_connection = setup_token_auth(['awx', '--conf.token', 'cli_token_value']) @@ - cli, mock_root, mock_connection = setup_token_auth(['awx', '--conf.token', 'my_token', '--conf.username', 'user', '--conf.password', 'pass']) + cli, _mock_root, mock_connection = setup_token_auth(['awx', '--conf.token', 'my_token', '--conf.username', 'user', '--conf.password', 'pass'])Also applies to: 159-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awxkit/test/cli/test_authentication.py` at line 133, The test unpacks a third-party helper return into cli, mock_root, mock_connection but never uses mock_root, triggering Ruff RUF059; update the unpacking in the tests that call setup_token_auth (e.g., where variables cli, mock_root, mock_connection are assigned) to ignore the unused value by renaming it to a throwaway identifier (for example cli, _mock_root, mock_connection or cli, _, mock_connection) or by only assigning the two used values from the tuple, and apply the same change to the other occurrence of the pattern in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@awxkit/test/cli/test_authentication.py`:
- Around line 140-155: The tests for token-based auth need to assert that
session loading is skipped; update test_oauth_token_from_env_var and
test_oauth_token_precedence_over_basic_auth to verify load_session is never
invoked. Mock or spy on the CLI.load_session (or the instance method used to
restore sessions) before calling cli.authenticate(), then add an assertion like
cli.load_session.assert_not_called() after authenticate() to lock in the “Bearer
token returns early” behavior; reference the CLI.parse_args and CLI.authenticate
calls already in the test to locate where to add the mock and assertion.
- Line 133: The test unpacks a third-party helper return into cli, mock_root,
mock_connection but never uses mock_root, triggering Ruff RUF059; update the
unpacking in the tests that call setup_token_auth (e.g., where variables cli,
mock_root, mock_connection are assigned) to ignore the unused value by renaming
it to a throwaway identifier (for example cli, _mock_root, mock_connection or
cli, _, mock_connection) or by only assigning the two used values from the
tuple, and apply the same change to the other occurrence of the pattern in the
file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0463233-939f-46bc-a51c-62f96fbccdbd
📒 Files selected for processing (1)
awxkit/test/cli/test_authentication.py
Document the new --conf.token flag, CONTROLLER_OAUTH_TOKEN and TOWER_OAUTH_TOKEN environment variables, and credential file token support in the authentication and usage documentation.
Author
|
ha, I just saw that this was already added very recently in #16281 and then removed in #16293... @stevensonmichel can you shine some light on what is the plan here? As far as I can see there is no other way than tokens to use the awx cli for SSO users, right? |
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.
SUMMARY
Add OAuth2 Bearer token authentication to the awx CLI.
The CLI currently only supports session-based login (
POST /api/login/with username+password) and HTTP Basic auth (viaAWXKIT_FORCE_BASIC_AUTH). Neither works for users who authenticate through social auth providers (e.g. GitHub SSO), since they have no local AWX password.AWX supports creating OAuth2 personal access tokens (User → Tokens → Add), and the API accepts them via
Authorization: Bearer <token>. This PR adds native CLI support for this auth method through:--conf.tokenCLI flagCONTROLLER_OAUTH_TOKEN/TOWER_OAUTH_TOKENenvironment variablestokenfield inAWXKIT_CREDENTIAL_FILEWhen a token is provided, it is sent as a Bearer token in the Authorization header, bypassing session and basic auth entirely.
ISSUE TYPE
COMPONENT NAME
STEPS TO REPRODUCE AND EXTRA INFO
n/a
Summary by CodeRabbit
New Features
Tests
Documentation