EDM-4975: Fix inventory env var fallback, OIDC-based username/password auth, and lazy jsonschema/PyYAML loading in ConfigLoader - #59
Conversation
…d auth, and lazy jsonschema/PyYAML loading in ConfigLoader Assisted-by: Cursor/Claude
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
WalkthroughChangesOIDC authentication
Configuration and packaging
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Consumer
participant oidc_password_grant
participant AuthConfig
participant OIDCProvider
Consumer->>oidc_password_grant: provide credentials and connection settings
oidc_password_grant->>AuthConfig: request authentication configuration
AuthConfig-->>oidc_password_grant: return OIDC provider
oidc_password_grant->>OIDCProvider: discover token endpoint
OIDCProvider-->>oidc_password_grant: return endpoint and client ID
oidc_password_grant->>OIDCProvider: submit password grant
OIDCProvider-->>oidc_password_grant: return bearer token
oidc_password_grant-->>Consumer: set bearer authorization
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 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: 7
🤖 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 `@plugins/inventory/flightctl.py`:
- Around line 487-491: Clear retained username and password immediately after
successful bearer-token caching in flightctl.py#487-491, api_module.py#183-195,
and imagebuilder_module.py#80-92, while preserving the bearer-header behavior.
Update the corresponding success-path tests in
tests/unit/plugins/inventory/test_flightctl.py#599-615,
tests/unit/plugins/module_utils/test_api_module.py#155-177, and
tests/unit/plugins/module_utils/test_imagebuilder_module.py#48-71 to assert both
credentials are cleared.
- Around line 475-482: Update _setup_connection_configuration so an omitted
verify_ssl option defaults to True and preserves False only when explicitly
configured, allowing _build_auth_headers to pass certificate validation to
oidc_password_grant. Add coverage for the omitted-option case and verify the
resulting Configuration value remains True.
In `@plugins/module_utils/api_module.py`:
- Around line 191-195: Prevent construction from continuing after OIDC
password-grant authentication fails: in plugins/module_utils/api_module.py lines
191-195 and plugins/module_utils/imagebuilder_module.py lines 88-92, raise
FlightctlException after fail_json returns without a bearer token so
Authorization is never set to Bearer None. Add coverage for non-raising
error_callback behavior in tests/unit/plugins/module_utils/test_api_module.py
lines 180-194 and tests/unit/plugins/module_utils/test_imagebuilder_module.py
lines 73-91, asserting FlightctlException is raised.
In `@plugins/module_utils/oidc_auth.py`:
- Around line 29-31: Fix the Flake8 E127 continuation indentation at
plugins/module_utils/oidc_auth.py:29-31 by reindenting the open_url call or
using parentheses; apply the same correction to the HTTPError continuations at
tests/unit/plugins/module_utils/test_oidc_auth.py:165-168 and
tests/unit/plugins/inventory/test_flightctl.py:620-622, and to the multi-context
with statement at
tests/unit/plugins/module_utils/test_imagebuilder_module.py:81-87. Preserve the
existing behavior while making all listed sites lint-clean.
- Around line 29-32: Validate all trust-boundary data before
oidc_password_grant() sends credentials: ensure http_get_json() returns a JSON
object, validate auth_config/provider/spec and token_data object shapes and
required string fields, and require discovery issuer/token_endpoint URLs to be
allowed HTTPS URLs matching the configured issuer where applicable. Perform
these checks before constructing or posting the password grant, rejecting
malformed, HTTP, or mismatched values.
In `@tests/unit/plugins/module_utils/test_config_loader.py`:
- Around line 31-73: Update the ConfigLoader tests in TestConfigLoaderParsing
and the dependency-error tests to mock filesystem access instead of creating
files with tmp_path. Patch the open operation used by ConfigLoader to return the
required YAML content for successful and dependency-error cases, and raise
FileNotFoundError for the missing-file case; preserve the existing assertions
and exception expectations.
In `@tests/unit/plugins/module_utils/test_oidc_auth.py`:
- Around line 13-17: New test helpers and test functions lack required type
annotations. In tests/unit/plugins/module_utils/test_oidc_auth.py lines 13-17
and 41-208, tests/unit/plugins/inventory/test_flightctl.py lines 535-539 and
563-650, tests/unit/plugins/module_utils/test_api_module.py lines 136-194, and
tests/unit/plugins/module_utils/test_imagebuilder_module.py lines 26-91,
annotate helper parameters and add -> None to every new test function or method,
using appropriate existing types for helper inputs and preserving test behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c52b7762-390e-4710-8d3e-90a0150d82ea
📒 Files selected for processing (11)
galaxy.ymlplugins/inventory/flightctl.pyplugins/module_utils/api_module.pyplugins/module_utils/config_loader.pyplugins/module_utils/imagebuilder_module.pyplugins/module_utils/oidc_auth.pytests/unit/plugins/inventory/test_flightctl.pytests/unit/plugins/module_utils/test_api_module.pytests/unit/plugins/module_utils/test_config_loader.pytests/unit/plugins/module_utils/test_imagebuilder_module.pytests/unit/plugins/module_utils/test_oidc_auth.py
| bearer_token, error_detail = oidc_password_grant( | ||
| host=getattr(config, 'host', None), | ||
| username=username, | ||
| password=password, | ||
| verify_ssl=getattr(config, 'verify_ssl', True), | ||
| ca_path=getattr(config, 'ssl_ca_cert', None), | ||
| request_timeout=getattr(config, 'request_timeout', None), | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)plugins/inventory/flightctl\.py$|test.*flightctl|flightctl'
echo "== relevant source =="
wc -l plugins/inventory/flightctl.py
sed -n '240,295p' plugins/inventory/flightctl.py
sed -n '440,505p' plugins/inventory/flightctl.py
echo "== open_url validate_certs usages =="
rg -n "verify_ssl|validate_certs|open_url|oidc_password_grant|_build_auth_headers|_setup_connection_configuration" plugins/inventory/flightctl.pyRepository: flightctl/flightctl-ansible
Length of output: 10304
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== option declarations / documentation around verify_ssl =="
rg -n -C 4 "verify_ssl|flightctl_verify_ssl|ssl_ca_cert|request_timeout|password:" plugins/inventory/flightctl.py
echo "== oidc auth helper =="
sed -n '1,260p' plugins/module_utils/oidc_auth.py
echo "== tests around verify_ssl/inventory options =="
rg -n -C 3 "verify_ssl|verify_ssl:*|test.*verify|oidc_password_grant|_setup_connection_configuration|_build_auth_headers" tests/unit/plugins/inventory/test_flightctl.py tests/integration/targets/inventory_flightctl plugins/inventory/flightctl.pyRepository: flightctl/flightctl-ansible
Length of output: 28823
Make TLS certificate validation the default for the password grant.
When verify_ssl is omitted and no config value overrides it, _setup_connection_configuration falls through to False and stores it on the Configuration object. _build_auth_headers then forwards False into oidc_password_grant(), which disables validate_certs for OIDC discovery and the password POST. Use True as the fallback; keep False only for explicit verify_ssl: false use unless the config file explicitly disables it. Add coverage for the omitted-option case.
[security_AND_privacy]
🤖 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 `@plugins/inventory/flightctl.py` around lines 475 - 482, Update
_setup_connection_configuration so an omitted verify_ssl option defaults to True
and preserves False only when explicitly configured, allowing
_build_auth_headers to pass certificate validation to oidc_password_grant. Add
coverage for the omitted-option case and verify the resulting Configuration
value remains True.
Source: Path instructions
| def http_get_json(url: str, verify_ssl: bool, ca_path: Optional[str], request_timeout: Optional[float]) -> Dict[str, Any]: | ||
| response = open_url(url, method='GET', validate_certs=verify_ssl, ca_path=ca_path, | ||
| timeout=request_timeout or 30) | ||
| return json.loads(response.read()) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
fd -a 'oidc_auth.py|urls.py' . | sed 's#^\./##'
echo
echo "oidc_auth outline:"
ast-grep outline plugins/module_utils/oidc_auth.py || true
echo
echo "Relevant oidc_auth sections:"
sed -n '1,180p' plugins/module_utils/oidc_auth.py | cat -n
echo
echo "Search open_url usages and redirects in urls.py if available:"
rg -n "open_url|follow_redirects|follow_redirect" plugins/module_utils/oidc_auth.py plugins/module_utils/urls.py --glob '*.py' || true
echo
echo "urls.py relevant snippet around redirect handling (first matching file):"
target=$(git ls-files | rg '(^|/)urls\.py$' | head -n 1 || true)
if [ -n "${target}" ]; then
echo "target=$target"
rg -n -C 6 "follow_redirects|307|308|preserve" "$target" || true
fiRepository: flightctl/flightctl-ansible
Length of output: 8365
🌐 Web query:
Ansible module_utils.urls open_url follow_redirects default urllib2 POST 307 308
💡 Result:
In Ansible's module_utils.urls, the open_url function and its associated redirect handler implement custom redirection logic that distinguishes between different follow_redirects configurations [1][2]. When follow_redirects is set to urllib2, Ansible delegates redirection behavior to the standard Python urllib2 (now urllib.request) library [1][3]. Because urllib2 is intended to mimic standard Python behavior, it does not inherently treat HTTP 307 or 308 status codes as automatic redirects for POST requests [3][4]. When follow_redirects is set to all or safe, Ansible's custom logic takes over to handle redirects, including 307 and 308 statuses [1][5]. Specifically, for 307 and 308 status codes, Ansible's implementation is designed to preserve the request payload and headers [1][5]. This allows POST requests to be redirected to the new location without being converted to GET requests, provided the chosen follow_redirects mode allows the redirection to proceed [1][3]. In summary: - The urllib2 mode defers to standard library behavior, which generally does not automatically follow 307/308 redirects for POST requests [3][4]. - The all and safe modes use Ansible's custom handler, which explicitly supports 307/308 redirects while preserving the request data and headers [1][5].
Citations:
- 1: https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/urls.py
- 2: https://github.com/ansible/ansible/blob/v2.0.2.0-1/lib/ansible/module_utils/urls.py
- 3: Required changes to support redirects on HTTP 307/308 ansible/ansible#36809
- 4: ansible.builtin.get_url does not respect 307 ansible/ansible#80785
- 5: https://github.com/ansible/ansible/blob/0239f706486ad777b146c6bcf56277064bcb2ae6/lib/ansible/module_utils/urls.py
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant tests:"
sed -n '1,260p' tests/unit/plugins/module_utils/test_oidc_auth.py | cat -n
echo
echo "Check installed Ansible availability:"
python3 - <<'PY'
import importlib.util
print("ansible.module_utils.urls:", importlib.util.find_spec("ansible.module_utils.urls") is not None)
PY
echo
echo "Parse return type usage with a small AST probe:"
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("plugins/module_utils/oidc_auth.py")
tree = ast.parse(path.read_text())
nodes = {}
for name, cls in ast.iter_fields(tree):
pass
for node in ast.walk(tree):
if isinstance(node, ast.FunctionDef):
for stmt in node.body:
if isinstance(stmt, ast.Return) and isinstance(stmt.value, ast.Call) and isinstance(stmt.value.func, ast.Name) and stmt.value.func.id == "open_url":
print(f"{node.name}: open_url call at line {node.lineno}, function annotations: {node.returns}")
PYRepository: flightctl/flightctl-ansible
Length of output: 10667
Validate OIDC discovery values before posting the password.
http_get_json() returns unvalidated json.loads() data. Auth config (auth_config.get()), provider spec (provider.get(), spec.get()), discovery (discovery.get('issuer'), discovery.get('token_endpoint')), and the token response (token_data.get()) are trust-boundary data that can be non-object, non-string, HTTP-based, or mismatched with the configured issuer. Add explicit JSON-object/field/URL allow-list checks before oidc_password_grant() sends the password.
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 31-31: continuation line over-indented for visual indent
(E127)
🪛 Ruff (0.16.0)
[warning] 29-29: Boolean-typed positional argument in function definition
(FBT001)
🤖 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 `@plugins/module_utils/oidc_auth.py` around lines 29 - 32, Validate all
trust-boundary data before oidc_password_grant() sends credentials: ensure
http_get_json() returns a JSON object, validate auth_config/provider/spec and
token_data object shapes and required string fields, and require discovery
issuer/token_endpoint URLs to be allowed HTTPS URLs matching the configured
issuer where applicable. Perform these checks before constructing or posting the
password grant, rejecting malformed, HTTP, or mismatched values.
Source: Path instructions
| def test_init_with_config_file_still_raises_when_jsonschema_missing(self, monkeypatch, tmp_path): | ||
| monkeypatch.setattr(config_loader, 'JSONSCHEMA_IMPORT_ERROR', ImportError('no module named jsonschema')) | ||
| config_file = tmp_path / "client.yaml" | ||
| config_file.write_text("authentication:\n access-token: abc\nservice:\n server: https://example.com\n") | ||
|
|
||
| with pytest.raises(ImportError, match="no module named jsonschema"): | ||
| ConfigLoader(config_file=str(config_file)) | ||
|
|
||
| def test_init_with_config_file_still_raises_when_pyyaml_missing(self, monkeypatch, tmp_path): | ||
| monkeypatch.setattr(config_loader, 'PYYAML_IMPORT_ERROR', ImportError('no module named yaml')) | ||
| config_file = tmp_path / "client.yaml" | ||
| config_file.write_text("authentication:\n access-token: abc\nservice:\n server: https://example.com\n") | ||
|
|
||
| with pytest.raises(ImportError, match="no module named yaml"): | ||
| ConfigLoader(config_file=str(config_file)) | ||
|
|
||
|
|
||
| class TestConfigLoaderParsing: | ||
| """Sanity checks that normal config file parsing still works when dependencies are present.""" | ||
|
|
||
| def test_parses_token_host_and_organization(self, tmp_path): | ||
| config_file = tmp_path / "client.yaml" | ||
| config_file.write_text( | ||
| "organization: my-org\n" | ||
| "authentication:\n" | ||
| " access-token: my-access-token\n" | ||
| "service:\n" | ||
| " server: https://flightctl.example.com\n" | ||
| " insecureSkipVerify: true\n" | ||
| ) | ||
|
|
||
| loader = ConfigLoader(config_file=str(config_file)) | ||
|
|
||
| assert loader.token == "my-access-token" | ||
| assert loader.organization == "my-org" | ||
| assert loader.host == "https://flightctl.example.com" | ||
| assert loader.verify_ssl is False | ||
|
|
||
| def test_missing_config_file_raises_clear_error(self, tmp_path): | ||
| missing_path = tmp_path / "does_not_exist.yaml" | ||
|
|
||
| with pytest.raises(Exception, match="was not found"): | ||
| ConfigLoader(config_file=str(missing_path)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Mock filesystem operations in these unit tests.
These tests create real files through tmp_path. Mock open to return YAML content or raise FileNotFoundError. This keeps the tests focused on ConfigLoader behavior.
As per path instructions, “Mock external dependencies (API calls, filesystem), not internal logic.”
🧰 Tools
🪛 Ruff (0.16.0)
[error] 64-64: Possible hardcoded password assigned to: "token"
(S105)
🤖 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 `@tests/unit/plugins/module_utils/test_config_loader.py` around lines 31 - 73,
Update the ConfigLoader tests in TestConfigLoaderParsing and the
dependency-error tests to mock filesystem access instead of creating files with
tmp_path. Patch the open operation used by ConfigLoader to return the required
YAML content for successful and dependency-error cases, and raise
FileNotFoundError for the missing-file case; preserve the existing assertions
and exception expectations.
Source: Path instructions
| def _fake_http_response(data): | ||
| """Build a stand-in for what ansible.module_utils.urls.open_url() returns.""" | ||
| response = MagicMock() | ||
| response.read.return_value = json.dumps(data).encode('utf-8') | ||
| return response |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add type annotations to the new test signatures.
The listed new helpers and test functions omit required signature annotations. Add parameter annotations where applicable and -> None return annotations to test functions.
tests/unit/plugins/module_utils/test_oidc_auth.py#L13-L17: annotate_fake_http_response.tests/unit/plugins/module_utils/test_oidc_auth.py#L41-L208: annotate the new test methods.tests/unit/plugins/inventory/test_flightctl.py#L535-L539: annotate_fake_http_response.tests/unit/plugins/inventory/test_flightctl.py#L563-L650: annotate the new test methods.tests/unit/plugins/module_utils/test_api_module.py#L136-L194: annotate the new test functions.tests/unit/plugins/module_utils/test_imagebuilder_module.py#L26-L91: annotate the helper and new test methods.
As per path instructions, “Use type hints for function signatures.”
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 15-15: use jsonify instead of json.dumps for JSON output
Context: json.dumps(data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.16.0)
[warning] 13-13: Missing return type annotation for private function _fake_http_response
(ANN202)
📍 Affects 4 files
tests/unit/plugins/module_utils/test_oidc_auth.py#L13-L17(this comment)tests/unit/plugins/module_utils/test_oidc_auth.py#L41-L208tests/unit/plugins/inventory/test_flightctl.py#L535-L539tests/unit/plugins/inventory/test_flightctl.py#L563-L650tests/unit/plugins/module_utils/test_api_module.py#L136-L194tests/unit/plugins/module_utils/test_imagebuilder_module.py#L26-L91
🤖 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 `@tests/unit/plugins/module_utils/test_oidc_auth.py` around lines 13 - 17, New
test helpers and test functions lack required type annotations. In
tests/unit/plugins/module_utils/test_oidc_auth.py lines 13-17 and 41-208,
tests/unit/plugins/inventory/test_flightctl.py lines 535-539 and 563-650,
tests/unit/plugins/module_utils/test_api_module.py lines 136-194, and
tests/unit/plugins/module_utils/test_imagebuilder_module.py lines 26-91,
annotate helper parameters and add -> None to every new test function or method,
using appropriate existing types for helper inputs and preserving test behavior.
Source: Path instructions
…ken exchange, and fix flake8 lint and check sanity Assisted-by: Cursor/Claude
1 participant
Summary
plugins/inventory/,plugins/module_utils/, and shared OIDC utilities.jsonschemaand PyYAML checks untilConfigLoaderloads a configuration file.galaxy.ymlbuild_ignorepatterns for CI files, tooling, changelogs, archives, and hidden files.Tests
ConfigLoadertests for deferred dependencies, YAML parsing, and missing files.API and compatibility
plugins/module_utils/oidc_auth.py.jsonschemaor PyYAML no longer affects configurations that do not load a configuration file.