[AAPRFE-955] feat(credentials): add OAuth2 client_credentials token plugin#185
Open
amasolov wants to merge 15 commits into
Open
[AAPRFE-955] feat(credentials): add OAuth2 client_credentials token plugin#185amasolov wants to merge 15 commits into
amasolov wants to merge 15 commits into
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new OAuth2 client-credentials credential plugin, registers it for discovery, updates spelling and docs configuration, and adds tests for discovery, token retrieval, and error handling. ChangesOAuth2 client-credentials plugin
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/awx_plugins/credentials/oauth2_client_credentials.py`:
- Around line 125-147: The code assumes resp.json() returns a mapping; defend
against non-object JSON by validating the parsed body is a dict before using
body.get(...) or indexing for ['access_token']. In the error path (where
resp.status_code != 200) call resp.json(), check isinstance(body, dict) and use
body.get('error_description', resp.text) only for dicts, otherwise set
error_detail to repr(body) or resp.text; similarly in the success path, parse =
resp.json(); if not isinstance(parse, dict) or 'access_token' not in parse,
raise the same ValueError with a clear message instead of letting
KeyError/TypeError leak (preserve the existing except-to-raise behavior around
access_token extraction).
🪄 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: Enterprise
Run ID: c8f9b075-3af0-49d2-83fa-5d713a438229
📒 Files selected for processing (5)
docs/spelling_wordlist.txtpyproject.tomlsrc/awx_plugins/credentials/oauth2_client_credentials.pytests/importable_test.pytests/oauth2_client_credentials_test.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/oauth2_client_credentials_test.py (2)
193-213: ⚡ Quick winAssert the parametrized
token_urlis actually usedThis test currently proves token parsing, but not provider URL forwarding. If backend URL handling regresses (e.g., hardcoded URL), this case can still pass.
Suggested patch
def test_works_with_various_providers( mocker: MockerFixture, token_url: str, ) -> None: """Verify the plugin works with different OAuth2 provider URLs.""" - mocker.patch.object( + mock_post = mocker.patch.object( oauth2_mod.requests, 'post', return_value=_FakeResponse( status_code=200, json_data={'access_token': FAKE_TOKEN}, ), ) @@ token = oauth2_mod.oauth2_client_credentials_backend( token_url=token_url, client_id=CLIENT_ID, client_secret=CLIENT_SECRET, ) assert token == FAKE_TOKEN + assert mock_post.call_args[0][0] == token_urlAs per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/oauth2_client_credentials_test.py` around lines 193 - 213, The test test_works_with_various_providers currently only validates token parsing; update it to assert that oauth2_mod.requests.post was invoked with the parametrized token_url so the backend actually forwards provider URLs. After patching oauth2_mod.requests.post (the mock used in the test), add an assertion on that mock (e.g., post_mock.assert_called_once_with(...) or check post_mock.call_args) to verify the first positional argument equals token_url and that expected form data/auth parameters used by oauth2_client_credentials_backend are present; reference the test name test_works_with_various_providers and the function oauth2_client_credentials_backend to locate where to add this assertion.
115-137: ⚡ Quick winLock in external-call contract assertions (
timeout, URL, andclient_secret)Given this backend calls an external token endpoint, this test should also guard against accidental removal of timeout/auth payload forwarding.
Suggested patch
call_kwargs = mock_post.call_args + assert call_kwargs[0][0] == TOKEN_URL + assert call_kwargs[1]['timeout'] == 30 assert call_kwargs[1]['data']['grant_type'] == 'client_credentials' assert call_kwargs[1]['data']['client_id'] == CLIENT_ID + assert call_kwargs[1]['data']['client_secret'] == CLIENT_SECRET assert call_kwargs[1]['data']['scope'] == ADO_SCOPEAs per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/oauth2_client_credentials_test.py` around lines 115 - 137, The test test_request_includes_grant_type is missing assertions that lock the external-call contract (timeout, URL and client_secret); update the test that mocks oauth2_mod.requests.post (mock_post) to also assert the request was made to TOKEN_URL, that call_kwargs[1] includes the forwarded client_secret value (CLIENT_SECRET) and that a timeout parameter is present and set (or at least non-None) so oauth2_client_credentials_backend continues to pass through timeout; locate these checks around the existing call_kwargs assertions and add assertions for URL, client_secret and timeout to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/oauth2_client_credentials_test.py`:
- Around line 193-213: The test test_works_with_various_providers currently only
validates token parsing; update it to assert that oauth2_mod.requests.post was
invoked with the parametrized token_url so the backend actually forwards
provider URLs. After patching oauth2_mod.requests.post (the mock used in the
test), add an assertion on that mock (e.g.,
post_mock.assert_called_once_with(...) or check post_mock.call_args) to verify
the first positional argument equals token_url and that expected form data/auth
parameters used by oauth2_client_credentials_backend are present; reference the
test name test_works_with_various_providers and the function
oauth2_client_credentials_backend to locate where to add this assertion.
- Around line 115-137: The test test_request_includes_grant_type is missing
assertions that lock the external-call contract (timeout, URL and
client_secret); update the test that mocks oauth2_mod.requests.post (mock_post)
to also assert the request was made to TOKEN_URL, that call_kwargs[1] includes
the forwarded client_secret value (CLIENT_SECRET) and that a timeout parameter
is present and set (or at least non-None) so oauth2_client_credentials_backend
continues to pass through timeout; locate these checks around the existing
call_kwargs assertions and add assertions for URL, client_secret and timeout to
prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 0e691348-15cc-44d3-bf89-e10bf100e018
📒 Files selected for processing (2)
src/awx_plugins/credentials/oauth2_client_credentials.pytests/oauth2_client_credentials_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/awx_plugins/credentials/oauth2_client_credentials.py
amasolov
force-pushed
the
feat/oauth2-client-credentials-plugin
branch
from
May 1, 2026 04:41
ca44827 to
877d8d2
Compare
amasolov
changed the title
amasolov
added a commit
to amasolov/eda-server
that referenced
this pull request
May 1, 2026
Add a new managed credential type that fetches OAuth2 access tokens using the client_credentials grant. This enables external credential lookups against any OAuth2 provider (Microsoft Entra/Azure AD, Keycloak, Okta, etc.) for use in rulebook activations and other EDA workflows. Changes: - Register oauth2_client_credentials_plugin in PLUGIN_TYPES - Add DefaultCredentialType.OAUTH2_CLIENT_CREDENTIALS enum value - Define OAUTH2_CLIENT_CREDENTIALS_INPUTS schema and seed via create_initial_data management command - Add credentials-oauth2-client-credentials extra to awx-plugins-core dependency - Add unit tests for plugin registration, dispatch, and initial data seeding Depends-On: ansible/awx-plugins#185 Signed-off-by: Alexey Masolov <[email protected]> Made-with: Cursor
amasolov
force-pushed
the
feat/oauth2-client-credentials-plugin
branch
from
May 1, 2026 05:36
3f44f95 to
0c2e324
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/awx_plugins/credentials/oauth2_client_credentials.py`:
- Around line 112-125: The current try/except around the requests.post call only
handles Timeout and ConnectionError but misses other transport failures; add a
final except requests.exceptions.RequestException as req_exc clause after the
existing handlers to normalize all remaining requests transport errors into a
ValueError (e.g., raise ValueError(f'Error requesting token from {token_url!s}')
from req_exc), keeping token_url, post_data and _REQUEST_TIMEOUT_SECONDS as used
in the existing requests.post call so every RequestException is wrapped and
propagated as a ValueError.
🪄 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: Enterprise
Run ID: 59dbf049-9c43-4935-a9a6-3ad2101adee6
📥 Commits
Reviewing files that changed from the base of the PR and between 856d793 and 3f44f9586f12ed059efb9293120796fb592bbdbc.
📒 Files selected for processing (4)
docs/conf.pydocs/spelling_wordlist.txtsrc/awx_plugins/credentials/oauth2_client_credentials.pytests/oauth2_client_credentials_test.py
✅ Files skipped from review due to trivial changes (1)
- docs/conf.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/spelling_wordlist.txt
- tests/oauth2_client_credentials_test.py
Add a new credential plugin that fetches a short-lived bearer token using the OAuth 2.0 client_credentials grant. This enables authentication with services that require OAuth2 tokens, such as Azure DevOps (via Microsoft Entra ID service principals), Keycloak, Okta, and any compliant OAuth2 identity provider. Closes ansible#184 Signed-off-by: Alexey Masolov <[email protected]> Signed-off-by: Alexey Masolov <[email protected]> Made-with: Cursor
for more information, see https://pre-commit.ci
Guard resp.json() usage against token endpoints that return valid JSON that is not an object (e.g. an array). Previously this would raise an AttributeError or TypeError instead of the documented ValueError. Signed-off-by: Alexey Masolov <[email protected]> Signed-off-by: Alexey Masolov <[email protected]> Made-with: Cursor
Extract _post_token_request, _extract_error_detail, and _extract_access_token helpers to reduce cognitive complexity, raise count, and try-body length. Replace magic number 30 with a named constant. Use http.HTTPStatus in tests to avoid WPS432 on status codes. Widen _FakeResponse.json_data to object to allow non-dict payloads in tests. Add noqa: ANN101 for self parameter annotations. Signed-off-by: Alexey Masolov <[email protected]> Signed-off-by: Alexey Masolov <[email protected]> Made-with: Cursor
for more information, see https://pre-commit.ci
Annotate resp.json() return as object to satisfy mypy misc check. Move noqa: ANN101 to the self parameter line where flake8 reports it. Signed-off-by: Alexey Masolov <[email protected]> Signed-off-by: Alexey Masolov <[email protected]> Made-with: Cursor
Add Okta and synchronisation to the spelling wordlist. Suppress Sphinx nitpick warning for requests.models.Response which is not in the intersphinx inventory. Signed-off-by: Alexey Masolov <[email protected]> Signed-off-by: Alexey Masolov <[email protected]> Made-with: Cursor
Add test for the case where the token endpoint returns HTTP 200 but the response body is not valid JSON, exercising the ValueError re-raise in _extract_access_token. Signed-off-by: Alexey Masolov <[email protected]> Signed-off-by: Alexey Masolov <[email protected]> Made-with: Cursor
Replace repeated oauth2_client_credentials_backend call arguments with a shared _COMMON_BACKEND_KWARGS dict to stay within the overused-expression limit. Signed-off-by: Alexey Masolov <[email protected]> Signed-off-by: Alexey Masolov <[email protected]> Made-with: Cursor
Replace the mutable module-level dict constant with a pytest fixture that wraps the backend call, and consolidate error-path test cases into parametrized groups. Signed-off-by: Alexey Masolov <[email protected]> Signed-off-by: Alexey Masolov <[email protected]> Made-with: Cursor
for more information, see https://pre-commit.ci
…licit-any Use typing.Protocol for the fixture type to avoid Callable[..., str] which triggers mypy explicit-any. Bundle error test params into a NamedTuple to stay within the 5-argument limit. Flip negated condition to satisfy WPS504. Signed-off-by: Alexey Masolov <[email protected]> Signed-off-by: Alexey Masolov <[email protected]> Made-with: Cursor Signed-off-by: Alexey Masolov <[email protected]> Made-with: Cursor
Add a catch-all handler for requests.exceptions.RequestException after the specific Timeout and ConnectionError handlers to normalise all transport failures (e.g. TooManyRedirects, InvalidURL) to ValueError, matching the documented error contract. Signed-off-by: Alexey Masolov <[email protected]> Signed-off-by: Alexey Masolov <[email protected]> Made-with: Cursor
Verify that test_works_with_various_providers checks the parametrized token_url is actually passed to requests.post, not just that a token is returned. Signed-off-by: Alexey Masolov <[email protected]> Signed-off-by: Alexey Masolov <[email protected]> Made-with: Cursor
amasolov
force-pushed
the
feat/oauth2-client-credentials-plugin
branch
from
July 2, 2026 00:49
ff69ced to
560dc1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/awx_plugins/credentials/oauth2_client_credentials.py (1)
112-117: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider enforcing an
httpsscheme ontoken_url.The static SSRF hint here is largely a false positive:
token_urloriginates from an admin-created credential, not untrusted request input. However,client_secretis sent in the POST body, so a misconfiguredhttp://endpoint would transmit the secret in plaintext. Validating the scheme before posting closes that gap.🔧 Example guard
try: + if not token_url.lower().startswith('https://'): + raise ValueError( + f'Token endpoint must use https: {token_url!s}', + ) return requests.post( token_url, data=post_data, timeout=_REQUEST_TIMEOUT_SECONDS, )🤖 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 `@src/awx_plugins/credentials/oauth2_client_credentials.py` around lines 112 - 117, The OAuth2 token request in the helper that calls requests.post with token_url should reject non-HTTPS endpoints before sending client_secret. Add a scheme validation in the token fetch path (around the code that builds post_data and performs the POST) so only https:// URLs are allowed, and raise a clear error for http:// or missing schemes. Keep the check close to the existing token_url handling in oauth2 client credentials flow so the secret is never posted to plaintext endpoints.
🤖 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 `@src/awx_plugins/credentials/oauth2_client_credentials.py`:
- Around line 147-151: The token parsing in oauth2_client_credentials.py still
accepts null or empty access_token values because the existing body check only
verifies the key exists. Update the access token validation in the token
response handling logic so the function that returns str(body['access_token'])
first rejects missing, None, or empty-string values by raising the same
ValueError, keeping the validation inside the token endpoint response path.
---
Nitpick comments:
In `@src/awx_plugins/credentials/oauth2_client_credentials.py`:
- Around line 112-117: The OAuth2 token request in the helper that calls
requests.post with token_url should reject non-HTTPS endpoints before sending
client_secret. Add a scheme validation in the token fetch path (around the code
that builds post_data and performs the POST) so only https:// URLs are allowed,
and raise a clear error for http:// or missing schemes. Keep the check close to
the existing token_url handling in oauth2 client credentials flow so the secret
is never posted to plaintext endpoints.
🪄 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: Enterprise
Run ID: 91c0b1de-e6f6-479b-b511-56c242d1011a
📒 Files selected for processing (6)
docs/conf.pydocs/spelling_wordlist.txtpyproject.tomlsrc/awx_plugins/credentials/oauth2_client_credentials.pytests/importable_test.pytests/oauth2_client_credentials_test.py
✅ Files skipped from review due to trivial changes (2)
- docs/conf.py
- docs/spelling_wordlist.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/importable_test.py
- pyproject.toml
- tests/oauth2_client_credentials_test.py
Address CodeRabbit review comments from the latest pass: - _extract_access_token now treats a null or empty-string access_token value the same as a missing one, instead of returning the literal string "None" as a bogus bearer token. - _post_token_request now rejects non-https token endpoints via a new _require_https helper before the client_secret is ever sent, closing a plaintext-credential-leak gap on misconfigured http:// endpoints. Signed-off-by: Alexey Masolov <[email protected]> Signed-off-by: Alexey Masolov <[email protected]> Made-with: Cursor
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.
Resolves: AAPRFE-955
Summary
Add a new credential plugin (
oauth2_client_credentials) that fetches a short-lived bearer token using the OAuth 2.0client_credentialsgrant. This enables authentication with services that require OAuth2 tokens without storing long-lived credentials.Closes #184
How it works
https://login.microsoftonline.com/<tenant>/oauth2/v2.0/token)499b84ac-1321-427f-aa17-267ca6975798/.defaultfor Azure DevOps).access_tokenfrom the response.Works with any compliant OAuth2 provider including Microsoft Entra ID, Keycloak, Okta, and Auth0.
Changes
src/awx_plugins/credentials/oauth2_client_credentials.pytests/oauth2_client_credentials_test.pypyproject.tomlawx_plugins.credentialsand optional dependency groupcredentials-oauth2-client-credentialstests/importable_test.pydocs/spelling_wordlist.txtPrimary use case: Azure DevOps project sync
Azure DevOps has deprecated PATs and SSH keys for service-to-service authentication in favour of Microsoft Entra ID service principals. This plugin enables:
x-token, scope:499b84ac-1321-427f-aa17-267ca6975798/.default).Testing
pytest+pytest-mock).Signed-off-by: Alexey Masolov [email protected]
Made with Cursor
Summary by CodeRabbit